Re: [PATCH] rds: synchronize info callbacks with connection teardown

On Thu, Jul 23, 2026 at 12:20 PM Allison Henderson <[email protected]> wrote:
>
> On Tue, 2026-07-21 at 02:49 +0800, Chengfeng Ye wrote:
> > rds_info_getsockopt() reads an info callback without holding
> > rds_info_lock across its invocation.  At the same time, rds_exit()
> > destroys all connections before unregistering the socket info callbacks.
> >
> > This permits the following interleaving:
> >
> >   CPU0                              CPU1
> >   rds_info_getsockopt()
> >     func = rds6_sock_inc_info
> >                                     rds_exit()
> >                                       rds_conn_exit()
> >                                         rds_conn_destroy()
> >                                           kmem_cache_free(conn)
> >     func()
> >       rds6_inc_info_copy()
> >         inc->i_conn->c_tos
> >
> > The receive queue lock keeps the incoming message on its socket queue,
> > but it does not keep inc->i_conn alive.  KASAN reports:
> >
> >   BUG: KASAN: slab-use-after-free in rds6_inc_info_copy+0x459/0x530 [rds]
> >   Read of size 1 at addr ffff888106031c50 by task poc/101
> >   Call Trace:
> >    rds6_inc_info_copy+0x459/0x530 [rds]
> >    rds6_sock_inc_info+0x2b9/0x3c0 [rds]
> >    rds_info_getsockopt+0x19d/0x380 [rds]
> >    do_sock_getsockopt+0x2ac/0x480
> >    __sys_getsockopt+0x128/0x210
> >   Allocated by task 99:
> >    kmem_cache_alloc_noprof+0x11a/0x360
> >    __rds_conn_create+0x7e/0xd60 [rds]
> >    rds_conn_create_outgoing+0x61/0x80 [rds]
> >    rds_sendmsg+0xb83/0x1d00 [rds]
> >   Freed by task 102:
> >    kmem_cache_free+0x1b5/0x3d0
> >    rds_conn_destroy+0x484/0x600 [rds]
> >    rds_loop_exit_net+0x32/0x50 [rds]
> >    unregister_pernet_device+0x2c/0x50
> >    rds_conn_exit+0x13/0xa0 [rds]
> >    rds_exit+0x1a/0xc40 [rds]
> >    __do_sys_delete_module+0x30a/0x4d0
> >
> > Publish and clear callback pointers with SRCU, and keep the SRCU read-side
> > critical section across callback execution. SRCU permits callbacks such as
> > RDS_INFO_COUNTERS to sleep, unlike classic RCU. After clearing a slot,
> > synchronize_srcu() waits for every in-flight callback.
> > Move socket callback deregistration ahead of protocol and connection
> > teardown. This prevents callbacks from reaching a connection after it is
> > freed.
>
> Hi Chengfeng,
>
> Thanks for the report and the patch, this is a great start. The UAF
> analysis is correct, but there are a few bumps to smooth out before
> this can go in.  First, the build bots caught some sparse errors that
> need to be addressed:
>
> https://netdev-ctrl.bots.linux.dev/logs/build/1131097/14698942/build_allmodconfig_warn/stderr
>
> Sparse can't apply the __rcu notation through a function-pointer typedef,
> so all of the rcu_*() macros trip over it.  The simplest fix is to drop
> the __rcu annotation and just use smp_load_acquire/smp_store_release on
> a plain array in register/deregister. Also, checkpatch warns about the
> two BUG_ON()s you touched. We can clean those up by converting them to
> a WARN_ON_ONCE().
>
> On to the fix itself: this patch has two separate changes going on that
> I think would be better done as two separate patches.  Comments below:
>
>
> >
> > Fixes: 7d0a06586b26 ("net/rds: Fix info leak in rds6_inc_info_copy()")
> > Cc: [email protected]
> > Signed-off-by: Chengfeng Ye <[email protected]>
> > ---
> >  net/rds/af_rds.c | 12 ++++++------
> >  net/rds/info.c   | 20 ++++++++++++++------
> >  2 files changed, 20 insertions(+), 12 deletions(-)
> >
> > diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> > index d5defe9172e3..b3fe2bd6ea35 100644
> > --- a/net/rds/af_rds.c
> > +++ b/net/rds/af_rds.c
>
> First, the changes in af_rds.c changes don't quite work the way you might think.
> net/rds builds as three separate modules: rds.ko, rds_tcp.ko and rds_rdma.ko.
> rds_exit() only runs for "rmmod rds". rds_tcp_exit() only runs for "rmmod
> rds_tcp".
>
> "rmmod rds_tcp" deregisters RDS_INFO_TCP_SOCKETS, and then destroys and
> frees all the TCP connections.  The rds module stays loaded with
> RDS_INFO_RECV_MESSAGES still registered, so "rmmod rds_tcp" still reproduces
> the splat since rds_exit() never runs in that case.
>
> Ideally what we want here is a ref counting infrastructure that isn't upstream
> yet.  The good news is, I've been working on one, so I can push a tag for you to
> base a revised fix on.
>
>   https://github.com/allisonhenderson/rds_work/releases/tag/rds-conn-kref-base-v1
>
> The branch may still see some review churn, but I'll leave this tag here for
> you to base your fix on.  On top of it, the fix is just to make every holder
> of inc->i_conn own a reference.  Really what we're doing here is porting a
> small UEK patch:
>
>   https://github.com/oracle/linux-uek/commit/99b9a3715419
>
> Upstream's assignment sites differ a little from UEK's, but basically the
> idea is to use rds_conn_get() everywhere i_conn is assigned.
>
> This can target net-next since every remaining trigger needs CAP_SYS_MODULE
> or physical device removal.  So it isn't reachable by an unprivileged attacker.
>
> Once you've worked out the patch, I'll carry it with yourself as the author
> as part of that series so it lands in the right order.  Alternately if you'd
> rather not take on a port, that's fine too, just let me know and I would tag
> you for the Reported-by.
>
>
> > @@ -925,6 +925,12 @@ static void rds6_sock_info(struct socket *sock, unsigned int len,
> >  static void rds_exit(void)
> >  {
> >       sock_unregister(rds_family_ops.family);
> > +     rds_info_deregister_func(RDS_INFO_SOCKETS, rds_sock_info);
> > +     rds_info_deregister_func(RDS_INFO_RECV_MESSAGES, rds_sock_inc_info);
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +     rds_info_deregister_func(RDS6_INFO_SOCKETS, rds6_sock_info);
> > +     rds_info_deregister_func(RDS6_INFO_RECV_MESSAGES, rds6_sock_inc_info);
> > +#endif
> >       proto_unregister(&rds_proto);
> >       rds_conn_exit();
> >       rds_cong_exit();
> > @@ -933,12 +939,6 @@ static void rds_exit(void)
> >       rds_stats_exit();
> >       rds_page_exit();
> >       rds_bind_lock_destroy();
> > -     rds_info_deregister_func(RDS_INFO_SOCKETS, rds_sock_info);
> > -     rds_info_deregister_func(RDS_INFO_RECV_MESSAGES, rds_sock_inc_info);
> > -#if IS_ENABLED(CONFIG_IPV6)
> > -     rds_info_deregister_func(RDS6_INFO_SOCKETS, rds6_sock_info);
> > -     rds_info_deregister_func(RDS6_INFO_RECV_MESSAGES, rds6_sock_inc_info);
> > -#endif
> >  }
> >  module_exit(rds_exit);
> >
> > diff --git a/net/rds/info.c b/net/rds/info.c
> > index 21b32eb16559..7d6f3552d65b 100644
> > --- a/net/rds/info.c
> > +++ b/net/rds/info.c
>
> These SRCU conversions in info.c are actually fixing a completely separate bug
> independent of the connections.   A getsockopt(RDS_INFO_TCP_SOCKETS)
> racing against a "rmmod rds_tcp" could call the callback pointer of a freed
> module. Which this patch fixes. So please resend the info.c changes
> against net-next, with the build fixes, and a commit message describing
> that race.
>
> I think that about covers it.  Let me know if you get stuck or have questions
>
> Thanks again, this was a good find.
>
> Allison
>
> > @@ -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,8 +69,9 @@ 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];
> > +static rds_info_func __rcu rds_info_funcs[RDS_INFO_LAST - RDS_INFO_FIRST + 1];
> >
> >  void rds_info_register_func(int optname, rds_info_func func)
> >  {
> > @@ -78,8 +80,8 @@ void rds_info_register_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]);
> > -     rds_info_funcs[offset] = func;
> > +     BUG_ON(rcu_access_pointer(rds_info_funcs[offset]));
> > +     rcu_assign_pointer(rds_info_funcs[offset], func);
> >       spin_unlock(&rds_info_lock);
> >  }
> >  EXPORT_SYMBOL_GPL(rds_info_register_func);
> > @@ -91,9 +93,10 @@ 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] != func);
> > -     rds_info_funcs[offset] = NULL;
> > +     BUG_ON(rcu_access_pointer(rds_info_funcs[offset]) != func);
> > +     RCU_INIT_POINTER(rds_info_funcs[offset], NULL);
> >       spin_unlock(&rds_info_lock);
> > +     synchronize_srcu(&rds_info_srcu);
> >  }
> >  EXPORT_SYMBOL_GPL(rds_info_deregister_func);
> >
> > @@ -165,6 +168,7 @@ int rds_info_getsockopt(struct socket *sock, int optname, sockopt_t *opt)
> >       int npages = 0;
> >       int ret;
> >       int len;
> > +     int srcu_idx;
> >       int total;
> >
> >       len = opt->optlen;
> > @@ -214,8 +218,11 @@ int rds_info_getsockopt(struct socket *sock, int optname, sockopt_t *opt)
> >       rdsdebug("len %d nr_pages %lu\n", len, nr_pages);
> >
> >  call_func:
> > -     func = rds_info_funcs[optname - RDS_INFO_FIRST];
> > +     srcu_idx = srcu_read_lock(&rds_info_srcu);
> > +     func = srcu_dereference(rds_info_funcs[optname - RDS_INFO_FIRST],
> > +                             &rds_info_srcu);
> >       if (!func) {
> > +             srcu_read_unlock(&rds_info_srcu, srcu_idx);
> >               ret = -ENOPROTOOPT;
> >               goto out;
> >       }
> > @@ -225,6 +232,7 @@ int rds_info_getsockopt(struct socket *sock, int optname, sockopt_t *opt)
> >       iter.offset = offset0;
> >
> >       func(sock, len, &iter, &lens);
> > +     srcu_read_unlock(&rds_info_srcu, srcu_idx);
> >       BUG_ON(lens.each == 0);
> >
> >       total = lens.nr * lens.each;
>

Hi Allison,

Thank you for the detailed review and for confirming the bug.

I would prefer to leave the connection-reference port to you, since it
depends on the new refcounting infrastructure. Please feel free to carry
the fix in your series with:

Reported-by: Chengfeng Ye <[email protected]>

I will resend the info.c changes against net-next.

Best regards,
Chengfeng