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