Message-ID: <CAMB2axOLh_sJL3FnPFPWr+s3dTjwGn4y+63vfbC3abT8+z2h-A@mail.gmail.com>
Date: Fri, 21 Nov 2025 13:54:26 -0800
From: Amery Hung <ameryhung@...il.com>
To: bot+bpf-ci@...nel.org
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, alexei.starovoitov@...il.com, 
	andrii@...nel.org, daniel@...earbox.net, memxor@...il.com, 
	david.laight.linux@...il.com, dave@...olabs.net, paulmck@...nel.org, 
	josh@...htriplett.org, kernel-team@...a.com, ast@...nel.org, 
	martin.lau@...nel.org, eddyz87@...il.com, yonghong.song@...ux.dev, 
	clm@...a.com, ihor.solodrai@...ux.dev
Subject: Re: [PATCH bpf-next v2 1/2] rqspinlock: Annotate rqspinlock lock
 acquiring functions with __must_check

On Fri, Nov 21, 2025 at 1:27 PM <bot+bpf-ci@...nel.org> wrote:
>
> > diff --git a/include/asm-generic/rqspinlock.h b/include/asm-generic/rqspinlock.h
> > index 6d4244d64..855c09435 100644
> > --- a/include/asm-generic/rqspinlock.h
> > +++ b/include/asm-generic/rqspinlock.h
>
> [ ... ]
>
> > @@ -171,7 +171,7 @@ static __always_inline void release_held_lock_entry(void)
> >   * * -EDEADLK        - Lock acquisition failed because of AA/ABBA deadlock.
> >   * * -ETIMEDOUT - Lock acquisition failed because of timeout.
> >   */
> > -static __always_inline int res_spin_lock(rqspinlock_t *lock)
> > +static __always_inline __must_check int res_spin_lock(rqspinlock_t *lock)
> >  {
> >       int val = 0;
> >
> > @@ -223,27 +223,36 @@ static __always_inline void res_spin_unlock(rqspinlock_t *lock)
> >  #define raw_res_spin_lock_init(lock) ({ *(lock) = (rqspinlock_t){0}; })
> >  #endif
> >
> > -#define raw_res_spin_lock(lock)                    \
> > -     ({                                         \
> > -             int __ret;                         \
> > -             preempt_disable();                 \
> > -             __ret = res_spin_lock(lock);       \
> > -             if (__ret)                         \
> > -                     preempt_enable();          \
> > -             __ret;                             \
> > -     })
> > +static __always_inline __must_check int raw_res_spin_lock(rqspinlock_t *lock)
> > +{
> > +     int ret;
> > +
> > +     preempt_disable();
> > +     ret = res_spin_lock(lock);
> > +     if (ret)
> > +             preempt_enable();
> > +
> > +     return ret;
> > +}
> >
> >  #define raw_res_spin_unlock(lock) ({ res_spin_unlock(lock); preempt_enable(); })
> >
> > -#define raw_res_spin_lock_irqsave(lock, flags)    \
> > -     ({                                        \
> > -             int __ret;                        \
> > -             local_irq_save(flags);            \
> > -             __ret = raw_res_spin_lock(lock);  \
> > -             if (__ret)                        \
> > -                     local_irq_restore(flags); \
> > -             __ret;                            \
> > -     })
> > +static __always_inline __must_check int
> > +__raw_res_spin_lock_irqsave(rqspinlock_t *lock, unsigned long *flags)
> > +{
> > +     unsigned long __flags;
> > +     int ret;
> > +
> > +     local_irq_save(__flags);
> > +     ret = raw_res_spin_lock(lock);
> > +     if (ret)
> > +             local_irq_restore(__flags);
> > +
> > +     *flags = __flags;
> > +     return ret;
> > +}
> > +
> > +#define raw_res_spin_lock_irqsave(lock, flags) __raw_res_spin_lock_irqsave(lock, &flags)
>
> The __must_check annotations will cause compiler warnings in
> kernel/locking/locktorture.c where torture_raw_res_spin_write_lock()
> and torture_raw_res_spin_write_lock_irq() ignore lock acquisition
> failures.

This should be fine. I compiled with gcc and clang and they all seem
to not propagate the check more than one level.

>
> When raw_res_spin_lock() returns an error (deadlock or timeout), the
> torture test functions still return 0 to indicate success. The torture
> framework then calls the unlock functions, but the lock was never
> acquired. In res_spin_unlock(), this causes an out-of-bounds array
> access when computing locks[cnt - 1] with cnt=0, potentially corrupting
> per-CPU memory.
>
> Should the torture test handle lock acquisition failures, or use
> different lock types that cannot fail?
>

Deadlock should not happen in this case, but for the correctness of
the code I can send another patch to address it if people find that
necessary.

Perhaps something like this:

@@ -931,7 +931,11 @@ static int lock_torture_writer(void *arg)
                if (!skip_main_lock) {
                        if (acq_writer_lim > 0)
                                j = jiffies;
-                       cxt.cur_ops->writelock(tid);
+                       err = cxt.cur_ops->writelock(tid);
+                       if (WARN_ON_ONCE(err)) {
+                               lwsp->n_lock_fail++;
+                               goto nested_unlock;
+                       }
                        if (WARN_ON_ONCE(lock_is_write_held))
                                lwsp->n_lock_fail++;
                        lock_is_write_held = true;
@@ -951,6 +955,7 @@ static int lock_torture_writer(void *arg)
                        WRITE_ONCE(last_lock_release, jiffies);
                        cxt.cur_ops->writeunlock(tid);
                }
+nested_unlock:
                if (cxt.cur_ops->nested_unlock)
                        cxt.cur_ops->nested_unlock(tid, lockset_mask);

>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19583558278

Powered by blists - more mailing lists