Re: [PATCH net-next v5 12/12] net/rds: hold a connection reference from struct rds_incoming
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-23 07:11:43
Also in:
linux-rdma
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider. Critical: 0 · High: 0 · Medium: 1 · Low: 1 If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough). - [Medium] Making every socket-queued `rds_incoming` own a connection reference (`rds_inc_init()`/`rds_inc_path_init()`,… - [Low] The patch fixes a real, user-visible use-after-free (`rds6_inc_info_copy()` reading `inc->i_conn` after… -- net/rds: hold a connection reference from struct rds_incoming Every site that stores a connection pointer in rds_incoming->i_conn now takes a connection reference, and the reference is dropped from rds_inc_put() and from a new rds_message_free() helper. This closes the reported use-after-free where an inc queued on a socket outlives the connection it points at. Closes: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/ (local) Two questions below, one about the commit trailers and one about the lifetime this introduces. The changelog documents a confirmed KASAN slab-use-after-free and carries Reported-by:/Closes: for it:
Reported-by: Chengfeng Ye [off-list ref]
Closes: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/ (local)[Severity: Low] Should this also carry a Fixes: tag naming the commit that introduced the bare receive-side assignment in rds_inc_init()? Other bug fixes in this series do carry one, for example "net/rds: guard every work-requeueing site with rds_destroy_pending()" with Fixes: ebeeb1ad9b8a and "net/rds: make rds_destroy_pending() cover single-connection destroy" with Fixes: cdc306a5c9cd3, so the omission looks inconsistent within the series. The explanation for why this is not stable material could stay in the changelog next to the tag.
quoted hunk ↗ jump to hunk
diff --git a/net/rds/recv.c b/net/rds/recv.c index 6204e577a90a..1fcd4483d3be 100644 --- a/net/rds/recv.c +++ b/net/rds/recv.c@@ -46,6 +46,7 @@ void rds_inc_init(struct rds_incoming *inc, struct rds_connection *conn, { refcount_set(&inc->i_refcount, 1); INIT_LIST_HEAD(&inc->i_item); + rds_conn_get(conn); /* put in rds_inc_put() */ inc->i_conn = conn; inc->i_conn_path = NULL; inc->i_saddr = *saddr;@@ -61,6 +62,7 @@ void rds_inc_path_init(struct rds_incoming *inc, struct rds_conn_path *cp, { refcount_set(&inc->i_refcount, 1); INIT_LIST_HEAD(&inc->i_item); + rds_conn_get(cp->cp_conn); /* put in rds_inc_put() */ inc->i_conn = cp->cp_conn; inc->i_conn_path = cp; inc->i_saddr = *saddr;
[Severity: Medium]
With a reference now attached to every received inc, does this hand an
unprivileged reader control over how long rds_conn_wait_conns_freed()
blocks?
The reference taken here is released only from rds_inc_put(), which runs
from rds_recvmsg() or from rds_clear_recv_queue() on close, so an
application that simply never reads a queued datagram keeps the
connection allocated.
rds_conn_wait_conns_freed() then has no way out:
net/rds/connection.c:rds_conn_wait_conns_freed() {
while (!wait_event_timeout(rds_conn_freed_waitq,
!atomic_read(&trans->t_conn_count),
msecs_to_jiffies(RDS_CONN_FREE_POLL_MS))) {
...
if (time_after_eq(jiffies, warn_at)) {
pr_warn("RDS/%s: still waiting for %d connection(s) to be freed before unload\n",
...
}
That wait is uninterruptible and is reached from module unload paths,
rds_tcp_exit() -> rds_conn_wait_conns_freed(), rds_ib_exit() and
rds_loop_exit(), so the unloading task sits in D state until the
application reads or closes, and the module stays in MODULE_STATE_GOING
in the meantime.
A bound socket holds a module reference on its own transport through
rds_trans_get_preferred(), but rds_find_bound() does not filter on
transport. Can a datagram that arrived over, say, a TCP connection sit
unread on a socket whose rs_transport is a different module, so that
rmmod rds_tcp hangs rather than failing with -EBUSY?
Would it make sense to bound the wait, discard or quarantine the incs
still queued at teardown, or hold a module reference for queued incs so
unload fails instead of blocking?
quoted hunk ↗ jump to hunk
memset(inc->i_rx_lat_trace, 0, sizeof(inc->i_rx_lat_trace)); }@@ -81,9 +83,19 @@ void rds_inc_put(struct rds_incoming *inc) { rdsdebug("put inc %p ref %d\n", inc, refcount_read(&inc->i_refcount)); if (refcount_dec_and_test(&inc->i_refcount)) { + struct rds_connection *conn = inc->i_conn; + BUG_ON(!list_empty(&inc->i_item)); - inc->i_conn->c_trans->inc_free(inc); + /* inc_free() can free the memory @inc lives in, so the + * connection reference has to be dropped through the + * copy taken above. + */ + conn->c_trans->inc_free(inc); + /* get in rds_inc_init(), rds_inc_path_init() or + * rds_recv_incoming() + */ + rds_conn_put(conn); } } EXPORT_SYMBOL_GPL(rds_inc_put);
[ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919061149.250658-1-achender%40kernel.org