Message-ID: <alpine.LSU.2.21.2112140857570.20187@pobox.suse.cz>
Date:   Tue, 14 Dec 2021 09:47:59 +0100 (CET)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
cc:     jikos@...nel.org, pmladek@...e.com, joe.lawrence@...hat.com,
        peterz@...radead.org, linux-kernel@...r.kernel.org,
        live-patching@...r.kernel.org, shuah@...nel.org,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v2 1/2] livepatch: Allow user to specify functions to
 search for on a stack

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

Powered by blists - more mailing lists