On Mon, 13 Dec 2021, Josh Poimboeuf wrote:
> On Fri, Dec 10, 2021 at 01:44:48PM +0100, Miroslav Benes wrote:
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -29,6 +29,8 @@
> > * @new_func: pointer to the patched function code
> > * @old_sympos: a hint indicating which symbol position the old function
> > * can be found (optional)
> > + * @stack_only: only search for the function (specified by old_name) on any
> > + * task's stack
>
> This should probably make it clear that it doesn't actually patch the
> function. How about something like:
>
> * @stack_only: don't patch old_func; only make sure it's not on the stack
Definitely better, thanks.
> > * @old_func: pointer to the function being patched
> > * @kobj: kobject for sysfs resources
> > * @node: list node for klp_object func_list
> > @@ -66,6 +68,7 @@ struct klp_func {
> > * in kallsyms for the given object is used.
> > */
> > unsigned long old_sympos;
> > + bool stack_only;
> >
> > /* internal */
> > void *old_func;
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 335d988bd811..62ff4180dc9b 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -89,6 +89,10 @@ static struct klp_func *klp_find_func(struct klp_object *obj,
> > struct klp_func *func;
> >
> > klp_for_each_func(obj, func) {
> > + /* Do not create nop klp_func for stack_only function */
> > + if (func->stack_only)
> > + return func;
> > +
>
> This has me confused. Maybe I'm missing something.
>
> First, klp_find_func() is a surprising place for this behavior.
You are right, it is not a nice place.
> Second, if obj's first func happens to be stack_only, this will short
> circuit the rest of the list traversal and will effectively prevent nops
> for all the rest of the funcs, even if they're *not* stack_only.
Oh, of course.
> Third, I'm not sure this approach would even make sense. I was thinking
> there are two special cases to consider:
>
> 1) If old_func is stack_only, that's effectively the same as !old_func,
> in which case we don't need a nop.
Correct.
> 2) If old_func is *not* stack_only, but new_func *is* stack_only, that's
> effectively the same as (old_func && !new_func), in which case we
> *do* want a nop. Since new_func already exists, we can just convert
> it from stack_only to nop.
And I somehow missed this case.
> Does that make sense? Or am I missing something?
>
> For example:
>
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -536,9 +536,23 @@ static int klp_add_object_nops(struct klp_patch *patch,
> }
>
> klp_for_each_func(old_obj, old_func) {
> + if (old_func->stack_only) {
> + /* equivalent to !old_func; no nop needed */
> + continue;
> + }
Nicer.
> func = klp_find_func(obj, old_func);
> - if (func)
> + if (func) {
> + if (func->stack_only) {
> + /*
> + * equivalent to (old_func && !new_func);
> + * convert stack_only to nop:
> + */
> + func->stack_only = false;
> + func->nop = true;
> + }
> +
> continue;
> + }
>
> func = klp_alloc_func_nop(old_func, obj);
> if (!func)
I think that it cannot be that straightforward. We assume that nop
functions are allocated dynamically elsewhere in the code, so the
conversion here from a stack_only function to a nop would cause troubles.
I like the idea though. We would also need to set func->new_func for it
and there may be some more places to handle, which I am missing now.
If I understood it correctly, Petr elsewhere in the thread proposed to
ignore nop functions completely. They would be allocated, not used and
discarded once a replace live patch is applied. I did not like the idea,
because it seemed hacky. And it would probably suffer from similar issues
as the above.
> And while we're at it, we may want to rename "{func,obj}" to
> "new_{func,obj}" for those functions which have "old_{func,obj}". It
> would help prevent confusion between the two.
Yes, the naming here does not help.
> > if ((strcmp(old_func->old_name, func->old_name) == 0) &&
> > (old_func->old_sympos == func->old_sympos)) {
> > return func;
> > @@ -499,6 +503,17 @@ static struct klp_func *klp_alloc_func_nop(struct klp_func *old_func,
> > return func;
> > }
> >
> > +static bool klp_is_object_stack_only(struct klp_object *old_obj)
> > +{
> > + struct klp_func *old_func;
> > +
> > + klp_for_each_func(old_obj, old_func)
> > + if (!old_func->stack_only)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > static int klp_add_object_nops(struct klp_patch *patch,
> > struct klp_object *old_obj)
> > {
> > @@ -508,6 +523,13 @@ static int klp_add_object_nops(struct klp_patch *patch,
> > obj = klp_find_object(patch, old_obj);
> >
> > if (!obj) {
> > + /*
> > + * Do not create nop klp_object for old_obj which contains
> > + * stack_only functions only.
> > + */
> > + if (klp_is_object_stack_only(old_obj))
> > + return 0;
>
> This code is already pretty self explanatory and the comment isn't
> needed IMO.
Ok.
> > +
> > obj = klp_alloc_object_dynamic(old_obj->name, patch);
> > if (!obj)
> > return -ENOMEM;
> > @@ -723,8 +745,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> > /*
> > * NOPs get the address later. The patched module must be loaded,
> > * see klp_init_object_loaded().
> > + * stack_only functions do not get new_func addresses at all.
> > */
> > - if (!func->new_func && !func->nop)
> > + if (!func->new_func && !func->nop && !func->stack_only)
> > return -EINVAL;
>
> Same here, I'm not sure this comment really helps.
Hm, I think the original comment is useful and it would be strange to add
a new check without extending it. I can remove the hunk, no problem.
> >
> > if (strlen(func->old_name) >= KSYM_NAME_LEN)
> > @@ -804,6 +827,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> > if (func->nop)
> > func->new_func = func->old_func;
> >
> > + if (func->stack_only)
> > + continue;
> > +
> > ret = kallsyms_lookup_size_offset((unsigned long)func->new_func,
> > &func->new_size, NULL);
> > if (!ret) {
> > diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> > index fe316c021d73..221ed691cc7f 100644
> > --- a/kernel/livepatch/patch.c
> > +++ b/kernel/livepatch/patch.c
> > @@ -250,6 +250,9 @@ static void __klp_unpatch_object(struct klp_object *obj, bool nops_only)
> > if (nops_only && !func->nop)
> > continue;
> >
> > + if (func->stack_only)
> > + continue;
> > +
> > if (func->patched)
> > klp_unpatch_func(func);
> > }
> > @@ -273,6 +276,9 @@ int klp_patch_object(struct klp_object *obj)
> > return -EINVAL;
> >
> > klp_for_each_func(obj, func) {
> > + if (func->stack_only)
> > + continue;
> > +
>
> Instead of these stack_only checks, we might want to add a new
> klp_for_each_patchable_func() macro. It could also be used in
> klp_add_object_nops() to filter out old_func->stack_only.
Maybe. On the other hand, I like the explicit check here and there. Will
consider...
> > ret = klp_patch_func(func);
> > if (ret) {
> > klp_unpatch_object(obj);
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > index 5683ac0d2566..fa0630fcab1a 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -200,7 +200,10 @@ static int klp_check_stack_func(struct klp_func *func, unsigned long *entries,
> > for (i = 0; i < nr_entries; i++) {
> > address = entries[i];
> >
> > - if (klp_target_state == KLP_UNPATCHED) {
> > + if (func->stack_only) {
> > + func_addr = (unsigned long)func->old_func;
> > + func_size = func->old_size;
> > + } else if (klp_target_state == KLP_UNPATCHED) {
>
> Hm, what does this mean for the unpatching case? What if the new
> function's .cold child is on the stack when we're trying to unpatch?
Good question. I did not realize it worked both ways. Of course it does.
> Would it make sense to allow the user specify a 'new_func' for
> stack_only, which is a func to check on the stack when unpatching? Then
> new_func could point to the new .cold child. And then
> klp_check_stack_func() wouldn't need a special case.
Maybe. klp_check_stack_func() would still need a special case, no? It
would just move down to KLP_PATCHED case. Or, the check after
klp_find_ops() would have to be improved, but I would like the explicit
check for stack_only better.
Thanks for the review.
Miroslav
