On Tue 2024-09-24 13:27:58, Petr Mladek wrote:
> On Fri 2024-09-20 17:04:03, Wardenjohn wrote:
> > This feature can provide livepatch patch order information.
> > With the order of sysfs interface of one klp_patch, we can
> > use patch order to find out which function of the patch is
> > now activate.
> >
> > After the discussion, we decided that patch-level sysfs
> > interface is the only accaptable way to introduce this
> > information.
> >
> > This feature is like:
> > cat /sys/kernel/livepatch/livepatch_1/order -> 1
> > means this livepatch_1 module is the 1st klp patch applied.
> >
> > cat /sys/kernel/livepatch/livepatch_module/order -> N
> > means this lviepatch_module is the Nth klp patch applied
> > to the system.
> >
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -46,6 +46,15 @@ EXPORT_SYMBOL(klp_sched_try_switch_key);
> >
> > #endif /* CONFIG_PREEMPT_DYNAMIC && CONFIG_HAVE_PREEMPT_DYNAMIC_CALL */
> >
> > +static inline int klp_get_patch_order(struct klp_patch *patch)
> > +{
> > + int order = 0;
> > +
> > + klp_for_each_patch(patch)
> > + order = order + 1;
> > + return order;
> > +}
>
> This does not work well. It uses the order on the stack when
> the livepatch is being loaded. It is not updated when any livepatch gets
> removed. It might create wrong values.
>
> I have even tried to reproduce this:
>
> # modprobe livepatch-sample
> # modprobe livepatch-shadow-fix1
> # cat /sys/kernel/livepatch/livepatch_sample/order
> 1
> # cat /sys/kernel/livepatch/livepatch_shadow_fix1/order
> 2
>
> # echo 0 >/sys/kernel/livepatch/livepatch_sample/enabled
> # rmmod livepatch_sample
> # cat /sys/kernel/livepatch/livepatch_shadow_fix1/order
> 2
>
> # modprobe livepatch-sample
> # cat /sys/kernel/livepatch/livepatch_shadow_fix1/order
> 2
> # cat /sys/kernel/livepatch/livepatch_sample/order
> 2
>
> BANG: The livepatches have the same order.
>
> I suggest to replace this with a global load counter. Something like:
Wait! Miroslav asked me on chat about a possibility to use klp_mutex
in the sysfs order_show() callback. It is a good point.
If I get it correctly then we actually could take klp_mutex in
the sysfs callbacks associated with struct klp_patch, similar
to enabled_store().
It should be safe because the "final" kobject_put(&patch->kobj) is
called without klp_mutex, see klp_free_patch_finish(). It was
explicitly done this way to allow taking klp_mutex in
enabled_store().
Note that it was not safe to take klp_mutex in the sysfs callback
associated with struct klp_func. The "final" kobject_put(&func->kobj)
is called under klp_mutex, see __klp_free_funcs().
Back to the order_show(). We could take klp_mutex there => we do not
need to store the order in struct klp_patch. Instead, we could do
something like:
static ssize_t order_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{
struct klp_patch *this_patch, *patch;
int order;
this_patch = container_of(kobj, struct klp_patch, kobj);
mutex_lock(&klp_mutex);
order = 0;
klp_for_each_patch(patch) {
order++;
if (patch == this_patch)
break;
}
mutex_unlock(&klp_mutex);
return sysfs_emit(buf, "%d\n", order);
}
BTW: I would prefer to rename the attribute from "order" to "stack_order".
IMHO, it would make the meaning more clear.
Best Regards,
Petr
