Re: [PATCH v9] livepatch: Clear relocation targets on a module removal

[Josh Poimboeuf <jpoimboe@kernel.org>]

On Fri, Jan 20, 2023 at 11:41:02AM -0800, Song Liu wrote:
> > >   The livepatch module has a relocation which references a symbol
> > >   in the _previous_ loading of nfsd. When apply_relocate_add()
> > >   tries to replace the old relocation with a new one, it sees that
> > >   the previous one is nonzero and it errors out.
> >
> > Should we add a selftest to make sure this problem doesn't come back?
> 
> IIRC, a selftest for this issue is not easy without Joe's klp-convert work.
> At the moment I use kpatch-build for testing.

Ah right, I remember that now.

> How about:
> 
> Signed-off-by: Song Liu <song@kernel.org>
> Originally-by: Miroslav Benes <mbenes@suse.cz>
> Acked-by: Miroslav Benes <mbenes@suse.cz>
> Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>

Yes, but the ordering looks off, I think it should be more like:

Reported-by: Josh Poimboeuf <jpoimboe@redhat.com>
Originally-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Song Liu <song@kernel.org>
Acked-by: Miroslav Benes <mbenes@suse.cz>

And then make sure 'From:' is you.

BTW, this patch affects both livepatch and x86, so the subject prefix
should have "x86" added, something like:

  livepatch,x86: Clear relocations on module removal

> > This code really needs to be removed anyway, it's been dead for at least
> > 15 years.
> 
> Shall we remove it now? Within the same patch? Or with a preparation
> patch?
> 

A preparatory patch sounds good.

> > > +                                    (int)ELF64_R_TYPE(rel[i].r_info), loc, val);
> > > +                             return -ENOEXEC;
> > > +                     }
> > > +                     write(loc, &val, write_size);
> > > +             } else {
> > > +                     if (memcmp(loc, &val, write_size)) {
> > > +                             pr_warn("x86/modules: Clearing invalid relocation target, existing value does not match expected value for type %d, loc %p, val %Lx\n",
> > > +                                     (int)ELF64_R_TYPE(rel[i].r_info), loc, val);
> > > +                     }
> > > +                     write(loc, &zero, write_size);
> >
> > If the value doesn't match then something has gone badly wrong.  Why go
> > ahead with the clearing in that case?
> 
> We can pr_err() then return -ENOEXEC (?). But I guess we need to
> handle the error case in:
>   klp_cleanup_module_patches_limited()
>   klp_module_coming()
>   klp_module_going()
> and all the functions that call klp_module_going().
> 
> This seems a big overkill to me...
> 
> Or do you mean we just skip the write()?

At the very least, skip the write.

But I really think it should just break out of the loop and return an
error, there's no point in trying to continue clearing the rest of the
relocations if one of them failed.

It's probably fine for the callers to ignore the error, the module's
going to get unloaded regardless.

-- 
Josh