livepatch: Clear relocation targets on a module removal

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

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.

On ppc64le, we have a similar issue:

module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd]
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

Quoted 16 lines · Song Liu

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

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.

Quoted 5 lines

On ppc64le, we have a similar issue:

module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd]
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

Shouldn't there also be a fix for this powerpc issue?

Quoted 24 lines · Josh Poimboeuf

Quoted 21 lines · Song Liu

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

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.

On ppc64le, we have a similar issue:

module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd]
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

Quoted 1 line

Shouldn't there also be a fix for this powerpc issue?

There was a working version, but it was not very clean. We couldn't agree on the path forward for powerpc, so we are hoping to ship the fix to x86 (and s390?) first [1].

Quoted 25 lines · Song Liu

Quoted 24 lines · Josh Poimboeuf

Quoted 21 lines · Song Liu

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

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.

On ppc64le, we have a similar issue:

module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd]
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

Quoted 5 lines

Quoted 1 line

Shouldn't there also be a fix for this powerpc issue?

There was a working version, but it was not very clean. We couldn't agree on the path forward for powerpc, so we are hoping to ship the fix to x86 (and s390?) first [1].

Sorry for coming in late, I was on leave so I missed a lot of the discussions on previous versions. The decision to leave powerpc broken wasn't clear from reading the commit message. The bug is mentioned, and the fix is implied, but surprisingly there's no fix.

I agree that the powerpc fix should be in a separate patch, but I still don't feel comfortable merging the x86 fix without the corresponding powerpc fix.

powerpc is a major arch and not a second-class citizen. If we don't fix it now then it'll probably never get fixed until it blows up in the real world.

For powerpc, instead of clearing, how about just "fixing" the warning site, something like so (untested)?

arch/powerpc/kernel/module_64.c +16 -4

index 1096d6b3a62c..1a12463ba674 100644
@@ -499,9 +499,11 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
 
 /* We expect a noop next: if it is, replace it with instruction to
    restore r2. */
-static int restore_r2(const char *name, u32 *instruction, struct module *me)
+static int restore_r2(const char *name, u32 *instruction, struct module *me,
+                     bool klp_sym)
 {
        u32 *prev_insn = instruction - 1;
+       u32 insn_val = *instruction;
 
        if (is_mprofile_ftrace_call(name))
                return 1;

@@ -514,9 +516,18 @@ static int restore_r2(const char *name, u32 *instruction, struct module *me)
        if (!instr_is_relative_link_branch(ppc_inst(*prev_insn)))
                return 1;
 
-       if (*instruction != PPC_RAW_NOP()) {
+       /*
+        * For a livepatch relocation, the restore r2 instruction might have
+        * been previously written if the relocation references a symbol in a
+        * module which was unloaded and is now being reloaded.  In that case,
+        * skip the warning and instruction write.
+        */
+       if (klp_sym && insn_val == PPC_INST_LD_TOC)
+               return 0;
+
+       if (insn_val != PPC_RAW_NOP()) {
                pr_err("%s: Expected nop after call, got %08x at %pS\n",
-                       me->name, *instruction, instruction);
+                       me->name, insn_val, instruction);
                return 0;
        }
 

@@ -649,7 +660,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
                                if (!value)
                                        return -ENOENT;
                                if (!restore_r2(strtab + sym->st_name,
-                                                       (u32 *)location + 1, me))
+                                               (u32 *)location + 1, me,
+                                               sym->st_shndx == SHN_LIVEPATCH))
                                        return -ENOEXEC;
                        } else
                                value += local_entry_offset(sym);

Quoted 15 lines

Quoted 5 lines

Quoted 1 line

Shouldn't there also be a fix for this powerpc issue?

There was a working version, but it was not very clean. We couldn't agree on the path forward for powerpc, so we are hoping to ship the fix to x86 (and s390?) first [1].

Sorry for coming in late, I was on leave so I missed a lot of the discussions on previous versions. The decision to leave powerpc broken wasn't clear from reading the commit message. The bug is mentioned, and the fix is implied, but surprisingly there's no fix.

I agree that the powerpc fix should be in a separate patch, but I still don't feel comfortable merging the x86 fix without the corresponding powerpc fix.

Quoted 6 lines

powerpc is a major arch and not a second-class citizen. If we don't fix it now then it'll probably never get fixed until it blows up in the real world.

For powerpc, instead of clearing, how about just "fixing" the warning site, something like so (untested)?

This version looks reasonable to me.

Quoted 25 lines · Song Liu

Quoted 24 lines · Josh Poimboeuf

Quoted 21 lines · Song Liu

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

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.

On ppc64le, we have a similar issue:

module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd]
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

Quoted 5 lines

Quoted 1 line

Shouldn't there also be a fix for this powerpc issue?

There was a working version, but it was not very clean. We couldn't agree on the path forward for powerpc, so we are hoping to ship the fix to x86 (and s390?) first [1].

Sorry for coming in late, I was on leave so I missed a lot of the discussions on previous versions. The decision to leave powerpc broken wasn't clear from reading the commit message. The bug is mentioned, and the fix is implied, but surprisingly there's no fix.

I agree that the powerpc fix should be in a separate patch, but I still don't feel comfortable merging the x86 fix without the corresponding powerpc fix.

powerpc is a major arch and not a second-class citizen. If we don't fix it now then it'll probably never get fixed until it blows up in the real world.

For powerpc, instead of clearing, how about just "fixing" the warning site, something like so (untested)?

arch/powerpc/kernel/module_64.c +3 -1

index 1096d6b3a62c..1a12463ba674 100644
@@ -499,9 +499,11 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
 
 /* We expect a noop next: if it is, replace it with instruction to
    restore r2. */
-static int restore_r2(const char *name, u32 *instruction, struct module *me)
+static int restore_r2(const char *name, u32 *instruction, struct module *me,
+		      bool klp_sym)
 {
 	u32 *prev_insn = instruction - 1;
+	u32 insn_val = *instruction;
 
 	if (is_mprofile_ftrace_call(name))
 		return 1;

@@ -514,9 +516,18 @@ static int restore_r2(const char *name, u32 *instruction, struct module *me)
 	if (!instr_is_relative_link_branch(ppc_inst(*prev_insn)))
 		return 1;

arch/powerpc/kernel/module_64.c +3 -1

+
+	if (insn_val != PPC_RAW_NOP()) {
 		pr_err("%s: Expected nop after call, got %08x at %pS\n",
-			me->name, *instruction, instruction);
+			me->name, insn_val, instruction);
 		return 0;
 	}
 

arch/powerpc/kernel/module_64.c static int restore_r2(const char *name, u32 *instruction, struct module *me) · line 526

Quoted 8 lines

arch/powerpc/kernel/module_64.c +8 -0

+	/*
+	 * For a livepatch relocation, the restore r2 instruction might have
+	 * been previously written if the relocation references a symbol in a
+	 * module which was unloaded and is now being reloaded.  In that case,
+	 * skip the warning and instruction write.
+	 */
+	if (klp_sym && insn_val == PPC_INST_LD_TOC)
+		return 0;

Hi Josh,

Nit: shouldn't this return 1?

And if you're willing to entertain a small refactor, wouldn't restore_r2() be clearer if it returned -ESOMETHING on error?

Maybe converting to a boolean could work, but then I'd suggest a name that clearly implies success/fail given true/false return. Maybe replace_nop_with_ld_toc() or replace_nop_to_restore_r2() ... still -ESOMETHING is more intuitive to me as there are cases like this where the function safely returns w/o replacing anything.

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

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.

On ppc64le, we have a similar issue:

module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd]
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

He also proposed three different solutions. We could remove the error check in apply_relocate_add() introduced by commit eda9cec4c9a1 ("x86/module: Detect and skip invalid relocations"). However the check is useful for detecting corrupted modules.

We could also deny the patched modules to be removed. If it proved to be a major drawback for users, we could still implement a different approach. The solution would also complicate the existing code a lot.

We thus decided to reverse the relocation patching (clear all relocation targets on x86_64). The solution is not
universal and is too much arch-specific, but it may prove to be simpler in the end.

Signed-off-by Miroslav Benes <[email protected]>

Signed-off-by Song Liu <[email protected]>

Acked-by Miroslav Benes <[email protected]>

Co-developed-by Song Liu <[email protected]>

Reported-by Josh Poimboeuf <[email protected]>

arch/x86/kernel/module.c +3 -2

-static int __apply_relocate_add(Elf64_Shdr *sechdrs,
+static int __write_relocate_add(Elf64_Shdr *sechdrs,
 		   const char *strtab,
 		   unsigned int symindex,
 		   unsigned int relsec,
 		   struct module *me,
-		   void *(*write)(void *dest, const void *src, size_t len))
+		   void *(*write)(void *dest, const void *src, size_t len),
+		   bool apply)
 {

arch/x86/kernel/module.c +1 -3

 			break;
 		case R_X86_64_PC64:
-			if (*(u64 *)loc != 0)
-				goto invalid_relocation;
 			val -= (u64)loc;
-			write(loc, &val, 8);
+			write_size = 8;

include/linux/moduleloader.h +6 -0

+void clear_relocate_add(Elf_Shdr *sechdrs,
+		   const char *strtab,
+		   unsigned int symindex,
+		   unsigned int relsec,
+		   struct module *me);
+#endif

Quoted 11 lines · Song Liu

From: Miroslav Benes <[email protected]>

Josh reported a bug:

When the object to be patched is a module, and that module is rmmod'ed and reloaded, it fails to load with:

module: x86/modules: Skipping invalid relocation target, existing value is nonzero for type 2, loc 00000000ba0302e9, val ffffffffa03e293c
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

Quoted 4 lines

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?

Quoted 6 lines

On ppc64le, we have a similar issue:

module_64: livepatch_nfsd: Expected nop after call, got e8410018 at e_show+0x60/0x548 [livepatch_nfsd]
livepatch: failed to initialize patch 'livepatch_nfsd' for module 'nfsd' (-8)
livepatch: patch 'livepatch_nfsd' failed for module 'nfsd', refusing to load module 'nfsd'

Just to clarify my previous comment, the reference to the powerpc issue should be removed because it'll be fixed in a separate patch.

Quoted 15 lines

He also proposed three different solutions. We could remove the error check in apply_relocate_add() introduced by commit eda9cec4c9a1 ("x86/module: Detect and skip invalid relocations"). However the check is useful for detecting corrupted modules.

We could also deny the patched modules to be removed. If it proved to be a major drawback for users, we could still implement a different approach. The solution would also complicate the existing code a lot.

We thus decided to reverse the relocation patching (clear all relocation targets on x86_64). The solution is not
universal and is too much arch-specific, but it may prove to be simpler in the end.

Quoted 5 lines

Signed-off-by Miroslav Benes <[email protected]>

Signed-off-by Song Liu <[email protected]>

Acked-by Miroslav Benes <[email protected]>

Co-developed-by Song Liu <[email protected]>

Reported-by Josh Poimboeuf <[email protected]>

According to submitting-patches.rst the Co-developed-by is supposed to be immediately before your Signed-off-by.

Also, other than the commit log, this patch is almost completely unrecognizable compared to Miroslav's original patch. Does it still make sense for him to be listed as the author?

In the -tip tree they sometimes use an Originally-by tag, which might be relevant here.

Quoted 11 lines

arch/x86/kernel/module.c +3 -2

-static int __apply_relocate_add(Elf64_Shdr *sechdrs,
+static int __write_relocate_add(Elf64_Shdr *sechdrs,
                 const char *strtab,
                 unsigned int symindex,
                 unsigned int relsec,
                 struct module *me,
-                void *(*write)(void *dest, const void *src, size_t len))
+                void *(*write)(void *dest, const void *src, size_t len),
+                bool apply)
 {

arch/x86/kernel/module.c int apply_relocate(Elf32_Shdr *sechdrs, · line 148

Quoted 10 lines

arch/x86/kernel/module.c +3 -1

      unsigned int i;
      Elf64_Rela *rel = (void *)sechdrs[relsec].sh_addr;
      Elf64_Sym *sym;
      void *loc;
      u64 val;
+     u64 zero = 0ULL;
 
-     DEBUGP("Applying relocate section %u to %u\n",
+     DEBUGP("%s relocate section %u to %u\n",
+            (apply) ? "Applying" : "Clearing",

"(apply)" has unnecessary parentheses.

Quoted 3 lines

             relsec, sechdrs[relsec].sh_info);
      for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {

+ int write_size = 4;

We already know we're writing, I suggest just calling it 'size' to be more consistent with kernel style.

Also, it can be an unsigned value like size_t.

Also, instead of initializing it here with a potentially bogus value which may need to be overwritten for a 64-bit reloc, it's better to explicitly set the size in each individual case below. That's makes the logic clearer and helps prevent future bugs if new 64-bit relocation types are added later.

arch/x86/kernel/module.c static int __apply_relocate_add(Elf64_Shdr *sechdrs, · line 188

Quoted 12 lines

              case R_X86_64_PC32:
              case R_X86_64_PLT32:

arch/x86/kernel/module.c +1 -4

-                     if (*(u32 *)loc != 0)
-                             goto invalid_relocation;
                      val -= (u64)loc;
-                     write(loc, &val, 4);
 #if 0
-                     if ((s64)val != *(s32 *)loc)
+                     if ((s64)val != *(s32 *)&val)
                              goto overflow;
 #endif

Why is the compiled-out code getting changed? Is it actually fixing a hypothetical bug?

This code really needs to be removed anyway, it's been dead for at least 15 years.

Quoted 8 lines

                      break;
              case R_X86_64_PC64:

arch/x86/kernel/module.c +1 -3

-                     if (*(u64 *)loc != 0)
-                             goto invalid_relocation;
                      val -= (u64)loc;
-                     write(loc, &val, 8);
+                     write_size = 8;

arch/x86/kernel/module.c static int __apply_relocate_add(Elf64_Shdr *sechdrs, · line 202

Quoted 10 lines

arch/x86/kernel/module.c +4 -0

                      break;
              default:
                      pr_err("%s: Unknown rela relocation: %llu\n",
                             me->name, ELF64_R_TYPE(rel[i].r_info));
                      return -ENOEXEC;
              }
+
+             if (apply) {
+                     if (memcmp(loc, &zero, write_size)) {
+                             pr_err("x86/modules: Skipping invalid relocation target, existing value is nonzero for type %d, loc %p, val %Lx\n",

It's not just "skipping", it's erroring out completely. Yes, it's is a pre-existing error message but we might as well improve it while touching it.

Maybe just remove the "Skipping", i.e. change "Skipping invalid ..." to "Invalid ..." ?

arch/x86/kernel/module.c static int __apply_relocate_add(Elf64_Shdr *sechdrs, · line 212

Quoted 11 lines

arch/x86/kernel/module.c +10 -0

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

arch/x86/kernel/module.c int apply_relocate_add(Elf64_Shdr *sechdrs, · line 271

Quoted 12 lines

arch/x86/kernel/module.c +11 -0

+#ifdef CONFIG_LIVEPATCH
+
+void clear_relocate_add(Elf64_Shdr *sechdrs,
+                     const char *strtab,
+                     unsigned int symindex,
+                     unsigned int relsec,
+                     struct module *me)
+{
+     write_relocate_add(sechdrs, strtab, symindex, relsec, me, false);
+}
+#endif

Superflous newline after the '#ifdef CONFIG_LIVEPATCH'.

include/linux/moduleloader.h int apply_relocate_add(Elf_Shdr *sechdrs, · line 78

Quoted 13 lines

arch/x86/kernel/module.c +1 -0

+
 #endif
 
 int module_finalize(const Elf_Ehdr *hdr,

include/linux/moduleloader.h +1 -0

index 7b4587a19189..0b54ec9856df 100644
@@ -75,6 +75,13 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
                     unsigned int symindex,
                     unsigned int relsec,
                     struct module *mod);
+#ifdef CONFIG_LIVEPATCH

This could use a comment explaining the purpose of this function:

/*
* Some architectures (namely x86_64 and ppc64) perform sanity checks when
* applying relocations. If a patched module gets unloaded and then later
* reloaded (and re-patched), klp re-applies relocations to the replacement
* function(s). Any leftover relocations from the previous loading of the
* patched module might trigger the sanity checks.
*
* To prevent that, when unloading a patched module, clear out any relocations
* that might trigger arch-specific sanity checks on a future module reload. */

include/linux/moduleloader.h int apply_relocate_add(Elf_Shdr *sechdrs, · line 84

Quoted 7 lines

include/linux/moduleloader.h +6 -0

+void clear_relocate_add(Elf_Shdr *sechdrs,
+                const char *strtab,
+                unsigned int symindex,
+                unsigned int relsec,
+                struct module *me);
+#endif

--
Josh

Quoted 6 lines

Quoted 4 lines

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.

How about:

Signed-off-by Song Liu <[email protected]>

Originally-by Miroslav Benes <[email protected]>

Acked-by Miroslav Benes <[email protected]>

Reported-by Josh Poimboeuf <[email protected]>

Quoted 2 lines

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?

Quoted 11 lines

arch/x86/kernel/module.c static int __apply_relocate_add(Elf64_Shdr *sechdrs, · line 212

Quoted 10 lines

arch/x86/kernel/module.c +10 -0

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

Quoted 2 lines

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()?