Re: [PATCH net-next v11 08/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 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?

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?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106031615.1047052-1-achender%40kernel.org