Re: [PATCH net-next v5 12/12] net/rds: hold a connection reference from struct rds_incoming

[netdev-bot+sashiko@kernel.org]

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/

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 <nicoyip.dev@gmail.com>
>     Closes: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/

[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.

> 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?

>  	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