On Fri, Jan 20, 2023 at 11:41:02AM -0800, Song Liu wrote:

quoted

quoted

  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 [off-list ref]
Acked-by: Miroslav Benes <mbenes@suse.cz>
Reported-by: Josh Poimboeuf <redacted>

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

Reported-by: Josh Poimboeuf <redacted>
Originally-by: Miroslav Benes [off-list ref]
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

quoted

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.

quoted

quoted

+                                    (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