Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 19 Jan 2023 22:42:15 -0800
From:   Josh Poimboeuf <jpoimboe@kernel.org>
To:     Song Liu <song@kernel.org>
Cc:     linux-kernel@vger.kernel.org, linux-modules@vger.kernel.org,
        live-patching@vger.kernel.org, x86@kernel.org, jikos@kernel.org,
        pmladek@suse.com, joe.lawrence@redhat.com,
        Miroslav Benes <mbenes@suse.cz>,
        Josh Poimboeuf <jpoimboe@redhat.com>
Subject: Re: [PATCH v9] livepatch: Clear relocation targets on a module
 removal
Message-ID: <20230120064215.cdyfbjlas5noxam6@treble>
References: <20230118204728.1876249-1-song@kernel.org>
 <20230118220812.dvztwhlmliypefha@treble>
 <CAPhsuW6FyHLeG3XMMMJiNnhwzW3dPXKrj3ksyB-C_iK1PNk71Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAPhsuW6FyHLeG3XMMMJiNnhwzW3dPXKrj3ksyB-C_iK1PNk71Q@mail.gmail.com>
Precedence: bulk

On Thu, Jan 19, 2023 at 11:06:35AM -0800, Song Liu wrote:
> On Wed, Jan 18, 2023 at 2:08 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> >
> > On Wed, Jan 18, 2023 at 12:47:28PM -0800, Song Liu wrote:
> > > From: Miroslav Benes <mbenes@suse.cz>
> > >
> > > 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'
> >
> > 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)?


diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 1096d6b3a62c..1a12463ba674 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -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);