net/rds/info.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-)
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 = rds_tcp_tc_info
rmmod rds_tcp
rds_tcp_exit()
rds_info_deregister_func()
rds_info_funcs[offset] = NULL
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 critical
section before loading the callback and leave it only after the callback
returns. Clear the callback with release semantics and call
synchronize_srcu() before deregistration returns, preventing module unload
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 operations
for publication so sparse does not have to apply __rcu through the
function-pointer typedef. Replace the two callback-slot BUG_ON() checks
with WARN_ON_ONCE() and return without changing the slot on mismatch.
Link: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/
Suggested-by: Allison Henderson <achender@kernel.org>
Reviewed-by: Allison Henderson <achender@kernel.org>
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
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.dev@gmail.com/ [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 unrelated
inc->i_conn KASAN report.
Link: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/ [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_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;
+ 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);
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] != func);
- rds_info_funcs[offset] = NULL;
+ if (WARN_ON_ONCE(rds_info_funcs[offset] != 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 optname, sockopt_t *opt)
int npages = 0;
int ret;
int len;
+ int srcu_idx;
int total;
len = opt->optlen;
@@ -214,8 +226,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);
+ /* Pair with callback slot publication and removal. */
+ func = smp_load_acquire(&rds_info_funcs[optname - RDS_INFO_FIRST]);
if (!func) {
+ srcu_read_unlock(&rds_info_srcu, srcu_idx);
ret = -ENOPROTOOPT;
goto out;
}
@@ -225,6 +240,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;
--
2.43.0From: Chengfeng Ye <nicoyip.dev@gmail.com>
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 = rds_tcp_tc_info
> rmmod rds_tcp
> rds_tcp_exit()
> rds_info_deregister_func()
> rds_info_funcs[offset] = NULL
> 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 critical
> section before loading the callback and leave it only after the callback
> returns. Clear the callback with release semantics and call
> synchronize_srcu() before deregistration returns, preventing module unload
> 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 operations
> for publication so sparse does not have to apply __rcu through the
> function-pointer typedef. Replace the two callback-slot BUG_ON() checks
> with WARN_ON_ONCE() and return without changing the slot on mismatch.
>
> Link: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/
> Suggested-by: Allison Henderson <achender@kernel.org>
> Reviewed-by: Allison Henderson <achender@kernel.org>
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> ---
>
> 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.dev@gmail.com/ [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 unrelated
> inc->i_conn KASAN report.
>
> Link: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/ [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_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;
> + 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] != func);
> - rds_info_funcs[offset] = NULL;
> + if (WARN_ON_ONCE(rds_info_funcs[offset] != 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 optname, sockopt_t *opt)
> int npages = 0;
> int ret;
> int len;
> + int srcu_idx;
nit: please keep the reverse xmas tree order.
> int total;
>
> len = opt->optlen;
> @@ -214,8 +226,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);
> + /* Pair with callback slot publication and removal. */
> + func = smp_load_acquire(&rds_info_funcs[optname - RDS_INFO_FIRST]);
> if (!func) {
> + srcu_read_unlock(&rds_info_srcu, srcu_idx);
> ret = -ENOPROTOOPT;
> goto out;
> }
> @@ -225,6 +240,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;
> --
> 2.43.0
>
On Sat, Aug 1, 2026 at 7:09 AM Kuniyuki Iwashima <kuniyu@google.com> wrote:
>
> From: Chengfeng Ye <nicoyip.dev@gmail.com>
> 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 = rds_tcp_tc_info
> > rmmod rds_tcp
> > rds_tcp_exit()
> > rds_info_deregister_func()
> > rds_info_funcs[offset] = NULL
> > 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 critical
> > section before loading the callback and leave it only after the callback
> > returns. Clear the callback with release semantics and call
> > synchronize_srcu() before deregistration returns, preventing module unload
> > 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 operations
> > for publication so sparse does not have to apply __rcu through the
> > function-pointer typedef. Replace the two callback-slot BUG_ON() checks
> > with WARN_ON_ONCE() and return without changing the slot on mismatch.
> >
> > Link: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/
> > Suggested-by: Allison Henderson <achender@kernel.org>
> > Reviewed-by: Allison Henderson <achender@kernel.org>
> > Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> > ---
> >
> > 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.dev@gmail.com/ [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 unrelated
> > inc->i_conn KASAN report.
> >
> > Link: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/ [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_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;
> > + 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] != func);
> > - rds_info_funcs[offset] = NULL;
> > + if (WARN_ON_ONCE(rds_info_funcs[offset] != 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 optname, sockopt_t *opt)
> > int npages = 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
© 2016 - 2026 Red Hat, Inc.