Message-ID: <20211215184707.e3iagidkvnpx2fb4@treble>
Date:   Wed, 15 Dec 2021 10:47:07 -0800
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Miroslav Benes <mbenes@...e.cz>, jikos@...nel.org,
        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 Wed, Dec 15, 2021 at 03:37:26PM +0100, Petr Mladek wrote:
> > Hm, this is different than how I understand it.
> > 
> > In the past I referred to the "parent" as the function which jumps to
> > the cold ("child") function.  So maybe we're getting confused by
> > different terminology.  But here I'll go with the naming from your
> > example.
> 
> I think that I was primary confused by the selftest where "child"
> function is livepatched and "parent" is defined as stack_only.

Ah, I guess I didn't look too closely at the selftest.

> Miroslav told me yesterday that the function that jumps into
> the .cold child needs to get livepatched. It makes sense
> because .cold child does not have well defined functionality.
> It depends on the compiler what code is put there.
> Hence I added one more level...
> 
> > If parent_func() is stack_only, that could create some false positive
> > scenarios where patching stalls unnecessarily.
> 
> Yes, it won't be optimal.
> 
> 
> > Also, wouldn't all of child_func()'s callers have to be made
> > stack_only?
> 
> Well, we already do this when handling compiler optimizations,
> for example, inlining.
> 
> 
> > How would you definitively find all the callers?
> 
> Good question. The best solution would be to get support from
> the compiler like we already get for another optimizations.
> 
> We always have these problems how to find functions that need
> special handling for livepatching.

But inlining and isra are inherently different from this.  They affect
static functions, which the compiler knows all the callers and is free
to (and does) tweak the ABI.  The compiler (and the patch author)
definitively know all the callers.

If child_func() happens to be a global function, it could be called from
anywhere.  Even from another klp_object.  If there are a lot of callers
across the kernel, there would be a proliferation of corresponding
stack_only funcs.

It could also be called from a function pointer, which levels up the
difficulty.

> > Instead I was thinking child_func.cold() should be stack_only.
> > 
> > e.g.:
> > 
> > static struct klp_func funcs[] = {
> > 	{
> > 		.old_name = "child_func",
> > 		.new_func = livepatch_child_func,
> > 	},
> > 	{
> > 		.old_name = "child_func.cold",
> > 		.new_name = "livepatch_child_func.cold",
> > 		.stack_only = true,
> > 	},
> > 
> > Any reason why that wouldn't work?
> 
> Yes, it should work in the given example. I am just curious how this
> would work in practice:
> 
> 
>   1. The compiler might optimize the new code another way and there
>      need not be 1:1 relation.
>
>      We might need another set of stack_only functions checked when
>      the livepatch is enabled. And another set of functions checked
>      when the livepatch gets disabled.

Regardless I'm thinking the above approach should be flexible enough.

If the patched child_func no longer has .cold, set 'new_name' to NULL in
the stack_only entry.

If the original child_func doesn't have .cold, but patched child_func
does, set 'old_name' to NULL in the stack_only entry.

If there were ever more than one of such "sub-functions" (which I
believe currently doesn't happen), the author could create multiple
stack_only entries.

>   2. The names of "child_func.cold" functions are generated by
>      the compiler. I mean that the names are "strange" ;-)
> 
>      It is likely easier with the kPatch approach that creates glue
>      around already compiled symbols. It is more tricky when preparing
>      the livepatch from sources. Well, it is doable.

kpatch-build has checks for symbols with ".cold" substring.  I'm
thinking it would be easy enough for you to do something similar since
you're already checking for other compiler optimizations.

> BTW: livepatch_child_func.cold function must be checked on the stack
>      also when the livepatch is replaced by another livepatch.
> 
>      I mean that we need to check two sets of stack only functions
>      when replacing one livepatch with another one:
> 
> 	+ "new_name" functions from to-be-replaced livepatch (like when disabling)
> 	+ "old_name" functions from new livepatch (like when enabling)

Urgh, this is starting to give me a headache.

Could we put the cold funcs in a klp_ops func_stack to make this work
automatically?

Alternatively we could link the .cold functions to their non-cold
counterparts somehow.  So when checking a function's stack, also check
it's linked counterpart.  It could even be part of the original
function's klp_func struct somehow, rather than having a dedicated
klp_func struct for the stack_only thing.

Or we could just give up trying to abstract this entirely, and go back
to Peter's suggestion to just always look for a ".cold" version of every
function in klp_check_stack_func() :-)

I dunno...

> Note that I do not have any strong opinion about any approach at the
> moment. I primary want to be sure that I understand the problem correctly :-)

Same here.

-- 
Josh

Powered by blists - more mailing lists