Message-ID: <CADBMgpzZz_Nn63DLwEbdurwkEUK7RsFXPhMf7Fw3WwOdpp67BA@mail.gmail.com>
Date: Thu, 22 May 2025 16:35:02 -0700
From: Dylan Hatch <dylanbhatch@...gle.com>
To: Will Deacon <will@...nel.org>
Cc: Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman <eddyz87@...il.com>, 
	Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, 
	Jiri Olsa <jolsa@...nel.org>, Puranjay Mohan <puranjay@...nel.org>, 
	Xu Kuohai <xukuohai@...weicloud.com>, Philippe Mathieu-Daudé <philmd@...aro.org>, 
	Catalin Marinas <catalin.marinas@....com>, "Mike Rapoport (Microsoft)" <rppt@...nel.org>, Arnd Bergmann <arnd@...db.de>, 
	Geert Uytterhoeven <geert@...ux-m68k.org>, Luis Chamberlain <mcgrof@...nel.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, Song Liu <song@...nel.org>, 
	Ard Biesheuvel <ardb@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	Roman Gushchin <roman.gushchin@...ux.dev>
Subject: Re: [PATCH v2 2/2] arm64/module: Use text-poke API for late relocations.

Hi Will,

I've sent a v4 of these patches that should address your comments,
feel free to have a look.

On Fri, May 9, 2025 at 9:15 AM Will Deacon <will@...nel.org> wrote:
>
> Hi Dylan,
>
> On Sat, Apr 12, 2025 at 01:09:39AM +0000, Dylan Hatch wrote:
> > To enable late module patching, livepatch modules need to be able to
> > apply some of their relocations well after being loaded. In this
> > scenario, use the text-poking API to allow this, even with
> > STRICT_MODULE_RWX.
> >
> > This patch is largely based off commit 88fc078a7a8f6 ("x86/module: Use
> > text_poke() for late relocations").
> >
> > Signed-off-by: Dylan Hatch <dylanbhatch@...gle.com>
> > ---
> >  arch/arm64/kernel/module.c | 129 ++++++++++++++++++++++++-------------
> >  1 file changed, 83 insertions(+), 46 deletions(-)
>
> On its own, this isn't gaining us an awful lot upstream as we don't have
> livepatch support (arm64 does not select HAVE_LIVEPATCH), however I'm
> not against incremental changes towards enabling that. Are you planning
> to work on follow-up changes for the rest of the support?

As Song mentioned, hopefully
https://lore.kernel.org/linux-arm-kernel/20250320171559.3423224-1-song@kernel.org/
in combination with this patch should be enough for initial support of
livepatch on arm64. I'll need to follow up with Weinan on next steps
for the SFrame approach.

>
> > diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> > index 06bb680bfe975..0703502d9dc37 100644
> > --- a/arch/arm64/kernel/module.c
> > +++ b/arch/arm64/kernel/module.c
> > @@ -18,11 +18,13 @@
> >  #include <linux/moduleloader.h>
> >  #include <linux/random.h>
> >  #include <linux/scs.h>
> > +#include <linux/memory.h>
> >
> >  #include <asm/alternative.h>
> >  #include <asm/insn.h>
> >  #include <asm/scs.h>
> >  #include <asm/sections.h>
> > +#include <asm/text-patching.h>
> >
> >  enum aarch64_reloc_op {
> >       RELOC_OP_NONE,
> > @@ -48,7 +50,8 @@ static u64 do_reloc(enum aarch64_reloc_op reloc_op, __le32 *place, u64 val)
> >       return 0;
> >  }
> >
> > -static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
> > +static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len,
> > +                   void *(*write)(void *dest, const void *src, size_t len))
> >  {
>
> I think it's a bit grotty indirecting the write when in reality we either
> need a straight-forward assignment (like we have today) or a call to
> an instruction-patching helper.
>
> In particular, when you get to functions such as:
>
> >  static int reloc_insn_movw(enum aarch64_reloc_op op, __le32 *place, u64 val,
> > -                        int lsb, enum aarch64_insn_movw_imm_type imm_type)
> > +                        int lsb, enum aarch64_insn_movw_imm_type imm_type,
> > +                        void *(*write)(void *dest, const void *src, size_t len))
> >  {
> >       u64 imm;
> >       s64 sval;
> >       u32 insn = le32_to_cpu(*place);
> > +     __le32 le_insn;
> >
> >       sval = do_reloc(op, place, val);
> >       imm = sval >> lsb;
> > @@ -145,7 +150,8 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, __le32 *place, u64 val,
> >
> >       /* Update the instruction with the new encoding. */
> >       insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_16, insn, imm);
> > -     *place = cpu_to_le32(insn);
> > +     le_insn = cpu_to_le32(insn);
> > +     write(place, &le_insn, sizeof(le_insn));
>
> we're forced into passing &le_insn because we need the same function
> prototype as memcpy().
>
> Instead, how about we pass the 'struct module *mod' pointer down from
> apply_relocate_add()? That's already done for reloc_insn_adrp() and it
> would mean that the above could be written as:
>
>
> static int reloc_insn_movw(struct module *mod, enum aarch64_reloc_op op,
>                            __le32 *place, u64 val, int lsb,
>                            enum aarch64_insn_movw_imm_type imm_type)
> {
>         ...
>
>         insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_16, insn, imm);
>         insn = cpu_to_le32(insn);
>
>         if (mod->state != MODULE_STATE_UNFORMED)
>                 aarch64_insn_set(place, insn, sizeof(insn));
>         else
>                 *place = insn;
>
>
> meaning we can also drop the first patch, because I don't think we need
> a text_poke() helper.

Dropped the first patch in v4 and switched to the method suggested
here, so we use a normal assignment for the non-late case. Though, it
does seem a little repetitive, with 6 different sites doing this
module state check. If having a straightforward assignment directly in
the reloc_* functions isn't too important, I wonder if we can make
local memset/memcpy-like helpers to contain this check? Like:

static void *write_insn(void *place, u32 insn, struct module *me);
static void *write_data(void *place, s64 *sval, struct module *me);

>
> > +int apply_relocate_add(Elf64_Shdr *sechdrs,
> > +                    const char *strtab,
> > +                    unsigned int symindex,
> > +                    unsigned int relsec,
> > +                    struct module *me)
> > +{
> > +     int ret;
> > +     bool early = me->state == MODULE_STATE_UNFORMED;
> > +     void *(*write)(void *, const void *, size_t) = memcpy;
> > +
> > +     if (!early) {
> > +             write = text_poke;
> > +             mutex_lock(&text_mutex);
> > +     }
> > +
> > +     ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
> > +                                write);
> > +
> > +     if (!early)
> > +             mutex_unlock(&text_mutex);
>
> Why is the responsibility of the arch code to take the 'text_mutex' here?
> The core code should be able to do that when the module state is !=
> MODULE_STATE_UNFORMED.
>

Moved the locking to kernel/livepatch/core.c in v4, since other call
sites to apply_relocate_add() don't attempt late relocations. Since
the locking was already being done in the x86 module code I had to
remove this. It also made sense to me to split out the text_mutex
changes into a separate patch because they only touch module/x86 and
livepatch code, so that's how I did it in v4. But they can just as
easily be squashed into one patch.

Thanks,
Dylan

Powered by blists - more mailing lists