On Thu 2024-09-05 12:23:20, Miroslav Benes wrote:
> Hi,
>
> On Wed, 28 Aug 2024, Wardenjohn wrote:
>
> > One system may contains more than one livepatch module. We can see
> > which patch is enabled. If some patches applied to one system
> > modifing the same function, livepatch will use the function enabled
> > on top of the function stack. However, we can not excatly know
> > which function of which patch is now enabling.
> >
> > This patch introduce one sysfs attribute of "using" to klp_func.
> > For example, if there are serval patches make changes to function
> > "meminfo_proc_show", the attribute "enabled" of all the patch is 1.
> > With this attribute, we can easily know the version enabling belongs
> > to which patch.
> >
> > The "using" is set as three state. 0 is disabled, it means that this
> > version of function is not used. 1 is running, it means that this
> > version of function is now running. -1 is unknown, it means that
> > this version of function is under transition, some task is still
> > chaning their running version of this function.
> >
> > cat /sys/kernel/livepatch/<patch1>/<object1>/<function1,sympos>/using -> 0
> > means that the function1 of patch1 is disabled.
> >
> > cat /sys/kernel/livepatch/<patchN>/<object1>/<function1,sympos>/using -> 1
> > means that the function1 of patchN is enabled.
> >
> > cat /sys/kernel/livepatch/<patchN>/<object1>/<function1,sympos>/using -> -1
> > means that the function1 of patchN is under transition and unknown.
> >
> > Signed-off-by: Wardenjohn <zhangwarden@xxxxxxxxx>
>
> I am not a fan. Josh wrote most of my objections already so I will not
> repeat them. I understand that the attribute might be useful but the
> amount of code it adds to sensitive functions like
> klp_complete_transition() is no fun.
>
> Would it be possible to just use klp_transition_patch and implement the
> logic just in using_show()? I have not thought through it completely but
> klp_transition_patch is also an indicator that there is a transition going
> on. It is set to NULL only after all func->transition are false. So if you
> check that, you can assign -1 in using_show() immediately and then just
> look at the top of func_stack.
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()
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);
}
It is racy and tricky. We probably should add some memory barriers.
And maybe even the ordering of reads should be different.
We could not take klp_mutex because it might cause a deadlock when
the sysfs file gets removed. kobject_put(&func->kobj) is called
by __klp_free_funcs() under klp_mutex.
It would be easier if we could take klp_mutex. But it would require
decrementing the kobject refcout without of klp_mutex. It might
be complicated.
I am afraid that this approach is not worth the effort and
is is not the way to go.
Best Regards,
Petr
