Re: [PATCH] LoongArch: Fix calling smp_processor_id() in preemptible code

[Xi Ruoyao]

On Thu, 2026-02-26 at 17:49 +0800, Huacai Chen wrote:
> On Thu, Feb 26, 2026 at 5:32 PM Xi Ruoyao <xry111@xxxxxxxxxxx> wrote:
> > 
> > On Thu, 2026-02-26 at 17:08 +0800, Huacai Chen wrote:
> > > Hi, Ruoyao,
> > > 
> > > On Wed, Feb 25, 2026 at 10:15 PM Xi Ruoyao <xry111@xxxxxxxxxxx> wrote:
> > > > 
> > > > Fix the warning recently exploited by commit 4ab17e762b34 ("LoongArch:
> > > > BPF: Use BPF prog pack allocator"):
> > > > 
> > > >     BUG: using smp_processor_id() in preemptible [00000000] code: systemd/1
> > > >     caller is larch_insn_text_copy+0x40/0xf0
> > > > 
> > > > Simply changing it to raw_smp_processor_id() is not enough: if preempt
> > > > and CPU hotplug happens after raw_smp_processor_id() but before
> > > > stop_machine(), the CPU where raw_smp_processor_id() has run may be
> > > > offline when stop_machine() and no CPU will run copy_to_kernel_nofault()
> > > > in text_copy_cb().  Thus guard the larch_insn_text_copy() calls with
> > > > cpus_read_lock() and change stop_machine() to stop_machine_cpuslocked()
> > > > to prevent this.
> > > > 
> > > > Fixes: 9fbd18cf4c69 ("LoongArch: BPF: Add dynamic code modification support")
> > > > Signed-off-by: Xi Ruoyao <xry111@xxxxxxxxxxx>
> > > > ---
> > > > 
> > > > P.S. I've considered moving the locks inside larch_insn_text_copy() but
> > > > doing so seems not an easy hack.  In bpf_arch_text_poke() obviously the
> > > > memcpy() call must be guarded by text_mutex, so we have to leave the
> > > > acquire of text_mutex out of larch_insn_text_copy.  But in the entire
> > > > kernel the acquire of mutexes is always after cpus_read_lock(), so we
> > > > cannot put cpus_read_lock() into larch_insn_text_copy() while leaving
> > > > the text_mutex acquire out (or we risk a deadlock due to inconsistent
> > > > lock acquire order).  So I'd just submit this as-is to fix the bug and
> > > > leave the posssible refactor for others.
> > > > 
> > > >  arch/loongarch/kernel/inst.c | 4 ++--
> > > >  arch/loongarch/net/bpf_jit.c | 6 ++++++
> > > >  2 files changed, 8 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/loongarch/kernel/inst.c b/arch/loongarch/kernel/inst.c
> > > > index bf037f0c6b26..db236e2b34a7 100644
> > > > --- a/arch/loongarch/kernel/inst.c
> > > > +++ b/arch/loongarch/kernel/inst.c
> > > > @@ -263,14 +263,14 @@ int larch_insn_text_copy(void *dst, void *src, size_t len)
> > > >                 .dst = dst,
> > > >                 .src = src,
> > > >                 .len = len,
> > > > -               .cpu = smp_processor_id(),
> > > > +               .cpu = raw_smp_processor_id(),
> > > I think we should keep the old function here, because it can help
> > > others to catch new bugs if the code is refactored in future.
> > 
> > Hmm, IIUC using smp_processor_id will still trigger the warning as we've
> > only acquired the cpu hotplug lock, instead of disabling the preemption
> > completely.
> > 
> > How about adding a lockdep_assert_cpus_held() in this function with an
> > explanatory comment?
> Does this context really need preemption disabled?

I don't think we really need to disable preemption here.  If a
preemption happens here we'll just invoke stop_machine from a different
cpu, and it will still work as long as copy.cpu doesn't get hot removed
(this is guaranteed by the cpus read lock).

If my understanding is incorrect maybe we'll need to use a lottery
algorithm like RISC-V.  I think invoking stop_machine with preemption
disabled will immediately cause a deadlock?

> Why lockdep_assert_cpus_held() can silence the warning?

It cannot, we still need to use raw_smp_processor_id.  It just indicates
this function needs to hold the cpus read lock so people don't add new
bugs.
> 

-- 
Xi Ruoyao <xry111@xxxxxxxxxxx>