On Sun 2024-09-08 10:51:14, zhang warden wrote:
>
> Hi, Petr
> >
> > The 1st patch adds the pointer to struct klp_ops into struct
> > klp_func. We might check the state a similar way as klp_ftrace_handler().
> >
> > I had something like this in mind when I suggested to move the pointer:
> >
> > static ssize_t using_show(struct kobject *kobj,
> > struct kobj_attribute *attr, char *buf)
> > {
> > struct klp_func *func, *using_func;
> > struct klp_ops *ops;
> > int using;
> >
> > func = container_of(kobj, struct klp_func, kobj);
> >
> > rcu_read_lock();
> >
> > if (func->transition) {
> > using = -1;
> > goto out;
> > }
> >
> > # FIXME: This requires releasing struct klp_ops via call_rcu()
This would require adding "struct rcu_head" into "struct klp_ops",
like:
struct klp_ops {
struct list_head func_stack;
struct ftrace_ops fops;
struct rcu_head rcu;
};
and then freeing the structure using kfree_rcu():
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 90408500e5a3..f096dd9390d2 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -149,7 +149,7 @@ static void klp_unpatch_func(struct klp_func *func)
list_del_rcu(&func->stack_node);
list_del(&ops->node);
- kfree(ops);
+ kfree_rcu(ops, rcu);
} else {
list_del_rcu(&func->stack_node);
}
@@ -223,7 +223,7 @@ static int klp_patch_func(struct klp_func *func)
err:
list_del_rcu(&func->stack_node);
list_del(&ops->node);
- kfree(ops);
+ kfree_rcu(ops, rcu);
return ret;
}
With this the function should be safe against accessing an invalid
pointer.
> > ops = func->ops;
> > if (!ops) {
> > using = 0;
> > goto out;
> > }
> >
> > using_func = list_first_or_null_rcu(&ops->func_stack,
> > struct klp_func, stack_node);
> > if (func == using_func)
> > using = 1;
> > else
> > using = 0;
> >
> > out:
> > rcu_read_unlock();
> >
> > return sysfs_emit(buf, "%d\n", func->using);
> > }
But the function is still not correct according the order of reading.
A more correct solution would be something like:
static ssize_t using_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{
struct klp_func *func, *using_func;
struct klp_ops *ops;
int using;
func = container_of(kobj, struct klp_func, kobj);
rcu_read_lock();
/* This livepatch is used when the function is on top of the stack. */
ops = func->ops;
if (ops) {
using_func = list_first_or_null_rcu(&ops->func_stack,
struct klp_func, stack_node);
if (func == using_func)
using = 1;
else
using = 0;
}
/*
* The function stack gives the right information only when there
* is no transition in progress.
*
* Make sure that we see the updated ops->func_stack when
* func->transition is cleared. This matches with:
*
* The write barrier in __klp_enable_patch() between
* klp_init_transition() and klp_patch_object().
*
* The write barrier in __klp_disable_patch() between
* klp_init_transition() and klp_start_transition().
*
* The write barrier in klp_complete_transition()
* between klp_unpatch_objects() and func->transition = false.
*/
smp_rmb();
if (func->transition)
using = -1;
rcu_read_unlock();
return sysfs_emit(buf, "%d\n", func->using);
}
Now, the question is whether we want to maintain such a barrier. Any
lockless access and barrier adds a maintenance burden.
You might try to put the above into a patch see what others tell
about it.
Best Regards,
Petr
