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:
- If old_func is stack_only, that's effectively the same as !old_func, in which case we don't need a nop.
Correct.
- 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