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

On Tue 2021-12-14 09:47:59, Miroslav Benes wrote:

On Mon, 13 Dec 2021, Josh Poimboeuf wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

 		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.

quoted

quoted

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

quoted

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