MIME-Version: 1.0
References: <cover.1674617130.git.jpoimboe@kernel.org> <2f6329ffd9674df6ff57e03edeb2ca54414770ab.1674617130.git.jpoimboe@kernel.org>
In-Reply-To: <2f6329ffd9674df6ff57e03edeb2ca54414770ab.1674617130.git.jpoimboe@kernel.org>
From:   Song Liu <song@kernel.org>
Date:   Tue, 24 Jan 2023 22:09:56 -0800
Message-ID: <CAPhsuW40jEiyp0ogsO6oH_frpFCmiioSHrMOKkwGcZ8_6w5dZA@mail.gmail.com>
Subject: Re: [PATCH 2/2] powerpc/module_64: Fix "expected nop" error on module re-patching
To:     Josh Poimboeuf <jpoimboe@kernel.org>
Cc:     Michael Ellerman <mpe@ellerman.id.au>,
        live-patching@vger.kernel.org, Nicholas Piggin <npiggin@gmail.com>,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jan 24, 2023 at 7:38 PM Josh Poimboeuf <jpoimboe@kernel.org> 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@kernel.org>
> ---
>  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
>