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

[Allison Henderson <achender@kernel.org>]

On Tue, 2026-11-10 at 13:57 +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> 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] With this patch, every rds_incoming holds a reference on its
>   connection.
> 
> --
> 
> net/rds: hold a connection reference from struct rds_incoming
> 
> This patch makes every holder of rds_incoming->i_conn own a connection
> reference. The references are taken in rds_inc_init(), rds_inc_path_init(),
> rds_recv_incoming() and the three m_inc assignment sites in send.c. They
> are dropped from rds_inc_put() and from a new rds_message_free() helper,
> which fixes a use-after-free when a transport unload destroys a connection
> that queued incs still point at.
> 
> Closes: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/
> 
> [ ... ]
> 
> > diff --git a/net/rds/recv.c b/net/rds/recv.c
> > index 00ab60f2d9969..e4cae82c16818 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;
> 
> [Severity: Medium]
> With this reference, an unread inc on a socket receive queue keeps its
> connection, and so the transport's t_conn_count, elevated until the
> application reads the datagram or closes the socket. Can that block
> rmmod of a transport forever?

Yes, but the the alternative to waiting is an unclean unload.  Without the
wait, the same sequence described above unloads rds_tcp with the inc still
queued.  Then the next rds_inc_put() calls conn->c_trans->inc_free, and for
an IB inc dereferences the freed connection which turns into a uaf.  So
waiting avoids the uaf and as well as a working unload.

Also the wait cannot be made interruptible/abortable in any way useful.
By the time rds_tcp_exit() runs, delete_module() has moved the module to
MODULE_STATE_GOING and the exit function has no way to fail. An early return
on a signal would unload over the live references, which is a UAF again.
If we purged the incs from the socket receive queues, it silently discards
datagrams that have been acknowledged to the peer from sockets nobody closed. 
Not discarding queued data is the one rule the teardown in this series follows
throughout, so I would rather keep it than add a drop path for a case that
today crashes.

Finally who controls the wait is not new.  The app owning the socket ends it by
reading or closing, or being killed by an admin. An unprivileged socket can
already keep the transport loaded for as long as it wants, and an rmmod simply
returns -EBUSY until the socket closes. The cross-transport case differs only in
that an inc holds no module reference, so the same hold shows up as a wait rather
than an immediate -EBUSY.

In any case, this complaint has come up a few times in prior reviews, and we've
added commentary around it as well as a warning that prints every 10 seconds.
I will send a v12 for the other sashiko reviews on patches 4 and 11, but I wanted
to explain why this review wouldnt be in it.

Thanks for the reviews!
Allison

> 
> rds_tcp_exit() and rds_ib_exit() both call rds_conn_wait_conns_freed().
> That loop exits only when t_conn_count reaches zero:
> 
> 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))) {
>     ...
> }
> 
> The loop never checks for signals, so nobody can kill the rmmod task, and
> the module stays in MODULE_STATE_GOING. The task wakes every 100ms, so the
> hung-task detector doesn't report it either. The only sign is the pr_warn
> every 10 seconds.
> 
> rds_find_bound() matches only on address, port and scope_id. It doesn't
> compare rs->rs_transport with conn->c_trans. A socket bound with one
> transport (through SO_RDS_TRANSPORT or rds_trans_get_preferred()) can
> therefore queue incs that arrived over another transport's connection,
> without holding a module reference on that transport:
> 
>   unprivileged process: binds an RDS socket using IB, never reads it
>   remote peer: connects to the rds_tcp listener and sends to that port
>     rds_recv_incoming()->rds_find_bound() matches the IB-bound socket
>     the inc is queued on rs_recv_queue, holding a TCP conn reference
>   admin: rmmod rds_tcp
>     rds_tcp_exit()->rds_conn_wait_conns_freed() never returns
> 
> Before this patch, only the hash-table reference and short-lived
> references pinned a connection, so the wait added earlier in the series
> always finished.
> 
> The commit message says this is intended:
> 
>   "It also means the transport cannot unload while such a datagram is
>    unread ... That is the intended behaviour"
> 
> However, it lets an unprivileged application decide how long a privileged
> unload stays stuck. Could the unload path instead purge the departing
> transport's incs from socket receive queues? Or could the wait be made
> interruptible or abortable?
> 
> [ ... ]
>