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
