Re: [PATCH net-next v3] rds: synchronize info callbacks with module unload

On Sat, Aug 1, 2026 at 7:09=E2=80=AFAM Kuniyuki Iwashima <[email protected]=
> wrote:
>
> From: Chengfeng Ye <[email protected]>
> Date: Sat,  1 Aug 2026 00:09:42 +0800
> > rds_info_getsockopt() reads a callback from rds_info_funcs and invokes =
it
> > without protecting the callback's lifetime.  Transport modules register
> > functions stored in this array.  For example, rds_tcp.ko registers
> > rds_tcp_tc_info() for RDS_INFO_TCP_SOCKETS.
> >
> > This permits the following interleaving:
> >
> >   CPU0                             CPU1
> >   rds_info_getsockopt()
> >     func =3D rds_tcp_tc_info
> >                                    rmmod rds_tcp
> >                                      rds_tcp_exit()
> >                                        rds_info_deregister_func()
> >                                          rds_info_funcs[offset] =3D NUL=
L
> >                                      free rds_tcp module text
> >     func()
> >
> > The reader can therefore branch to an address in unloaded module text.
> >
> > Protect callback invocation with SRCU.  Enter the SRCU read-side critic=
al
> > section before loading the callback and leave it only after the callbac=
k
> > returns.  Clear the callback with release semantics and call
> > synchronize_srcu() before deregistration returns, preventing module unl=
oad
> > from freeing its text while an old reader is still executing it.  SRCU =
is
> > required because callbacks such as RDS_INFO_COUNTERS can sleep.
> >
> > Keep the callback array unannotated and use acquire and release operati=
ons
> > for publication so sparse does not have to apply __rcu through the
> > function-pointer typedef.  Replace the two callback-slot BUG_ON() check=
s
> > with WARN_ON_ONCE() and return without changing the slot on mismatch.
> >
> > Link: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.d=
[email protected]/
> > Suggested-by: Allison Henderson <[email protected]>
> > Reviewed-by: Allison Henderson <[email protected]>
> > Signed-off-by: Chengfeng Ye <[email protected]>
> > ---
> >
> > Changes in v3:
> > - Use ordinary loads for callback-slot validation under rds_info_lock;
> >   retain acquire semantics only for the lockless reader.
> > - Drop the Fixes tag because this patch targets net-next.
> > - Add Allison Henderson's Reviewed-by tag.
> >
> > Link: https://lore.kernel.org/netdev/20260727174826.135662-1-nicoyip.de=
[email protected]/ [v2]
> >
> > Changes in v2:
> > - Split the info callback/module-unload race from the independent
> >   inc->i_conn lifetime bug.
> > - Keep the callback array unannotated and use smp_load_acquire() and
> >   smp_store_release() to resolve the sparse errors.
> > - Replace the two touched callback-slot BUG_ON() checks with
> >   WARN_ON_ONCE() error paths.
> > - Rewrite the commit message for the callback race and omit the unrelat=
ed
> >   inc->i_conn KASAN report.
> >
> > Link: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.d=
[email protected]/ [v1]
> >
> >  net/rds/info.c | 26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/rds/info.c b/net/rds/info.c
> > index 21b32eb16559..385fefbfa07f 100644
> > --- a/net/rds/info.c
> > +++ b/net/rds/info.c
> > @@ -32,6 +32,7 @@
> >   */
> >  #include <linux/percpu.h>
> >  #include <linux/seq_file.h>
> > +#include <linux/srcu.h>
> >  #include <linux/slab.h>
> >  #include <linux/proc_fs.h>
> >  #include <linux/export.h>
> > @@ -68,6 +69,7 @@ struct rds_info_iterator {
> >       unsigned long offset;
> >  };
> >
> > +DEFINE_STATIC_SRCU(rds_info_srcu);
> >  static DEFINE_SPINLOCK(rds_info_lock);
> >  static rds_info_func rds_info_funcs[RDS_INFO_LAST - RDS_INFO_FIRST + 1=
];
> >
> > @@ -78,8 +80,12 @@ void rds_info_register_func(int optname, rds_info_fu=
nc func)
> >       BUG_ON(optname < RDS_INFO_FIRST || optname > RDS_INFO_LAST);
> >
> >       spin_lock(&rds_info_lock);
> > -     BUG_ON(rds_info_funcs[offset]);
> > -     rds_info_funcs[offset] =3D func;
> > +     if (WARN_ON_ONCE(rds_info_funcs[offset])) {
> > +             spin_unlock(&rds_info_lock);
> > +             return;
> > +     }
> > +     /* Pair with lockless callback lookup. */
> > +     smp_store_release(&rds_info_funcs[offset], func);
>
> Why is smp_store_release() necessary here ? and same for
> smp_load_acquire() on the reader.
>
> I think just WRITE_ONCE() / READ_ONCE() should be enough.
>
> It's just a function pointer, not struct, and the reader
> does not require any partial ordering, no ?
>
>
> >       spin_unlock(&rds_info_lock);
> >  }
> >  EXPORT_SYMBOL_GPL(rds_info_register_func);
> > @@ -91,9 +97,14 @@ void rds_info_deregister_func(int optname, rds_info_=
func func)
> >       BUG_ON(optname < RDS_INFO_FIRST || optname > RDS_INFO_LAST);
> >
> >       spin_lock(&rds_info_lock);
> > -     BUG_ON(rds_info_funcs[offset] !=3D func);
> > -     rds_info_funcs[offset] =3D NULL;
> > +     if (WARN_ON_ONCE(rds_info_funcs[offset] !=3D func)) {
> > +             spin_unlock(&rds_info_lock);
> > +             return;
> > +     }
> > +     /* Hide the callback before waiting for old readers. */
> > +     smp_store_release(&rds_info_funcs[offset], NULL);
> >       spin_unlock(&rds_info_lock);
> > +     synchronize_srcu(&rds_info_srcu);
> >  }
> >  EXPORT_SYMBOL_GPL(rds_info_deregister_func);
> >
> > @@ -165,6 +176,7 @@ int rds_info_getsockopt(struct socket *sock, int op=
tname, sockopt_t *opt)
> >       int npages =3D 0;
> >       int ret;
> >       int len;
> > +     int srcu_idx;
>
> nit: please keep the reverse xmas tree order.

Hi,

Indeed WRITE/READ_ONCE should suffice for this code, maybe we don't
need that smp store/load despite it looks more secure. I will make a
v4 patch to address it as well as the reverse xmas tree order problem.

Best Regards,
Chengfeng