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

[Allison Henderson]

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: stable at vger.kernel.org
> Signed-off-by: Chengfeng Ye <nicoyip.dev at gmail.com>
> ---
>  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;