Re: [PATCH v2 1/2] livepatch: Allow user to specify functions to search for on a stack

[Petr Mladek <pmladek@suse.com>]

On Tue 2021-12-14 09:47:59, Miroslav Benes wrote:
> On Mon, 13 Dec 2021, Josh Poimboeuf wrote:
> > On Fri, Dec 10, 2021 at 01:44:48PM +0100, Miroslav Benes wrote:
> > 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.

Yup. It is not that easy because nops are dynamically allocated and
are freed after the transition is completed.

Well, stack_only has the same effect as nop from the livepatching POV.
Both are checked on stack and both do not redirect the code. The only
difference is that stack_only struct klp_func is static. It need not
be allocated and need not be freed.


> 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.

This is probably misunderstanding. I proposed to do not register the
ftrace handler for stack_only entries. But it would work only when
there is not already registered ftrace handler from another livepatch.
So, I agree that it is a bad idea.

Better solution seems to handle stack_only entries the same way as
nops except for the allocation/freeing.


> > > --- 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.

I am confused. My understanding is that .cold child is explicitly
livepatched to the new .cold child like it is done in the selftest:

static struct klp_func funcs_stack_only[] = {
	{
		.old_name = "child_function",
		.new_func = livepatch_child_function,
	}, {

We should not need anything special to check it on stack.
We only need to make sure that we check all .stack_only functions of
the to-be-disabled livepatch.

Best Regards,
Petr