On Tue, Jan 24, 2023 at 7:38 PM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
>
> When a module with a livepatched function is unloaded and then reloaded,
> klp attempts to dynamically re-patch it. On ppc64, that fails with the
> following error:
>
> 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'
>
> The error happens because the restore r2 instruction had already
> previously been written into the klp module's replacement function when
> the original function was patched the first time. So the instruction
> wasn't a nop as expected.
>
> When the restore r2 instruction has already been patched in, detect that
> and skip the warning and the instruction write.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
> arch/powerpc/kernel/module_64.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 016e79bba531..bf1da99fff74 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -502,6 +502,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
> static int restore_r2(const char *name, u32 *instruction, struct module *me)
> {
> u32 *prev_insn = instruction - 1;
> + u32 insn_val = *instruction;
>
> if (is_mprofile_ftrace_call(name))
> return 0;
> @@ -514,9 +515,18 @@ static int restore_r2(const char *name, u32 *instruction, struct module *me)
> if (!instr_is_relative_link_branch(ppc_inst(*prev_insn)))
> return 0;
>
> - if (*instruction != PPC_RAW_NOP()) {
> + /*
> + * For livepatch, the restore r2 instruction might have already been
> + * written previously, if the referenced symbol is in a previously
> + * unloaded module which is now being loaded again. In that case, skip
> + * the warning and the instruction write.
> + */
> + if (insn_val == PPC_INST_LD_TOC)
> + return 0;
Do we need "sym->st_shndx == SHN_LIVEPATCH" here?
Thanks,
Song
> +
> + 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 -ENOEXEC;
> }
>
> --
> 2.39.0
>
