Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

From: Petr Mladek
Date: Tue Sep 10 2024 - 04:03:41 EST


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