Hi Petr,
On Tue, May 27, 2025 at 7:46 AM Petr Mladek <pmla...@suse.com> wrote:
>
> As this patch suggests, the "text_mutex" has been used to
> sychronize apply_relocate_add() only on x86_64 so far.
>
> s390x seems to rely on "s390_kernel_write_lock" taken by:
>
> + apply_relocate_add()
> + s390_kernel_write()
> + __s390_kernel_write()
>
> And powerpc seems to rely on "pte" locking taken by
>
> + apply_relocate_add()
> + patch_instruction()
> + patch_mem()
> + __do_patch_mem_mm()
> + get_locked_pte()
>
Reading through these implementations, I see that the serialization
happens only at the level of each individual write action. This is
equivalent to the patch_lock already used by aarch64_insn_copy() and
aarch64_insn_set(). I see now that this same serialization is achieved
by x86 using text_mutex, and that text_poke uses
'lockdep_assert_held(&text_mutex);' instead of grabbing the lock
itself, which is why only the x86 apply_relocate_add() currently takes
this mutex.
> I see two possibilities:
>
> 1. Either this change makes a false feeling that "text_mutex"
> sychronizes apply_relocate_add() on all architextures.
>
> This does not seems to be the case on, for example, s390
> and powerpc.
>
> => The code is misleading and could lead to troubles.
>
My original intent with this change was to give the late relocations
on arm64 the correct synchronization with respect to other
text-patching code. From what you've shown above, it looks like the
[PATCH 2/2] should work fine without this change since the arm64
patching code already takes patch_lock.
>
> 2. Or it actually provides some sychronization on all
> architectures, for example, against kprobe code.
>
> In this case, it might actually fix an existing race.
> It should be described in the commit message
> and nominated for backporting to stable.
>
I hadn't really considered this. From what I can tell, kprobe is the
only non-arch-specific code that takes this mutex when touching kernel
text. Though my understanding of kprobe is very limited, I think there
could be a risk due to the late relocations for livepatch:
Suppose I apply a livepatch 'L' that touches some module 'M', but M
isn't currently loaded. Between check_kprobe_address_safe() and
__register_kprobe(), I don't see any check that would fail for a probe
'P' registered on a function inside L. So it seems to me that it's
possible for prepare_kprobe() on L to race with apply_relocate_add()
for L if P is registered while M is being loaded.
Perhaps more importantly, is it ever safe to kprobe an instruction
that hasn't yet received relocation? This would probably only be
possible in the case of late relocations for a livepatch, so maybe
this scenario was overlooked. I wonder if check_kprobe_address_safe()
can check for this case and cause the kprobe to fail, preventing the
above race condition from ever being possible.
In any case, synchronizing against kprobe wasn't the original intent
of this patch series, so in my opinion it makes sense to resend it as
a standalone patch (if it is to be resent at all).
>
> I am sorry if this has already been discussed. But I have been
> in Cc only for v3 and v4. And there is no changelog in
> the cover letter.
>
This patch was added to the series in v3, which is how you got added
to CC. Sorry about not adding a changelog, I'm still learning the best
practices for sending patches.
> > +
> > + if (apply)
> > + ret = apply_relocate_add(sechdrs, strtab, symndx, secndx,
> > pmod);
> > + else
> > + clear_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> > +
> > + if (!early)
> > + mutex_unlock(&text_mutex);
> > + return ret;
> > }
>
> Best Regards,
> Petr
Thanks,
Dylan
On Tue, May 27, 2025 at 7:46 AM Petr Mladek <pmla...@suse.com> wrote:
>
> On Thu 2025-05-22 20:52:04, Dylan Hatch wrote:
> > Late module relocations are an issue on any arch that supports
> > livepatch, so move the text_mutex locking to the livepatch core code.
> >
> > Signed-off-by: Dylan Hatch <dylanbha...@google.com>
> > Acked-by: Song Liu <s...@kernel.org>
> > ---
> > arch/x86/kernel/module.c | 8 ++------
> > kernel/livepatch/core.c | 18 +++++++++++++-----
> > 2 files changed, 15 insertions(+), 11 deletions(-)
> >
> > --- a/arch/x86/kernel/module.c
> > +++ b/arch/x86/kernel/module.c
> > @@ -197,18 +197,14 @@ static int write_relocate_add(Elf64_Shdr *sechdrs,
> > bool early = me->state == MODULE_STATE_UNFORMED;
> > void *(*write)(void *, const void *, size_t) = memcpy;
> >
> > - if (!early) {
> > + if (!early)
> > write = text_poke;
> > - mutex_lock(&text_mutex);
> > - }
> >
> > ret = __write_relocate_add(sechdrs, strtab, symindex, relsec, me,
> > write, apply);
> >
> > - if (!early) {
> > + if (!early)
> > text_poke_sync();
> > - mutex_unlock(&text_mutex);
> > - }
> >
> > return ret;
> > }
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 0e73fac55f8eb..9968441f73510 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -319,12 +320,19 @@ static int klp_write_section_relocs(struct module
> > *pmod, Elf_Shdr *sechdrs,
> > sec, sec_objname);
> > if (ret)
> > return ret;
> > -
> > - return apply_relocate_add(sechdrs, strtab, symndx, secndx,
> > pmod);
> > }
> >
> > - clear_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> > - return 0;
> > + if (!early)
> > + mutex_lock(&text_mutex);
>
> I understand why you do this but it opens some questions.
>
> As this patch suggests, the "text_mutex" has been used to
> sychronize apply_relocate_add() only on x86_64 so far.
>
> s390x seems to rely on "s390_kernel_write_lock" taken by:
>
> + apply_relocate_add()
> + s390_kernel_write()
> + __s390_kernel_write()
>
> And powerpc seems to rely on "pte" locking taken by
>
> + apply_relocate_add()
> + patch_instruction()
> + patch_mem()
> + __do_patch_mem_mm()
> + get_locked_pte()
>
> I see two possibilities:
>
> 1. Either this change makes a false feeling that "text_mutex"
> sychronizes apply_relocate_add() on all architextures.
>
> This does not seems to be the case on, for example, s390
> and powerpc.
>
> => The code is misleading and could lead to troubles.
>
>
> 2. Or it actually provides some sychronization on all
> architectures, for example, against kprobe code.
>
> In this case, it might actually fix an existing race.
> It should be described in the commit message
> and nominated for backporting to stable.
>
>
> I am sorry if this has already been discussed. But I have been
> in Cc only for v3 and v4. And there is no changelog in
> the cover letter.
>
> > +
> > + if (apply)
> > + ret = apply_relocate_add(sechdrs, strtab, symndx, secndx,
> > pmod);
> > + else
> > + clear_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> > +
> > + if (!early)
> > + mutex_unlock(&text_mutex);
> > + return ret;
> > }
>
> Best Regards,
> Petr
