Re: [PATCH net-next 6/6] 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: 1 · Medium: 1 · Low: 0

- [High] Module-unload barrier defeated: incs/messages parked on a socket
  now hold a `struct rds_connection` reference for a userspace-…
- [Medium] The commit message understates the cost and the residual
  exposure of the new reference.

--

net/rds: hold a connection reference from struct rds_incoming

This makes every holder of struct rds_incoming->i_conn own a connection
reference: the six assignment sites take one, and rds_inc_put() and the
new rds_message_free() helper drop it.  The stated goal is to stop
dereferences of i_conn from touching a connection that was already freed
by device removal, netns teardown or module unload.

Closes: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/

First, a question about the commit message itself:

>     This keeps a connection allocated for as long as messages that arrived
>     over it are queued on sockets, which is longer than before but costs
>     only the connection's memory: rds_conn_destroy() still quiesces the
>     connection synchronously, so a lingering inc holds nothing running.

[Severity: Medium]
Is "costs only the connection's memory" accurate?  A lingering inc keeps
trans->t_conn_count non-zero, and that counter only reaches zero from
rds_conn_destroy_fini():

net/rds/connection.c:rds_conn_destroy_fini() {
	for (i = 0; i < npaths; i++)
		rds_conn_path_free(&conn->c_path[i]);
	...
	if (!atomic_dec_return(&trans->t_conn_count))
		wake_up_all(&rds_conn_freed_waitq);
}

so unloading a transport while a datagram sits unread stalls for
RDS_CONN_FREE_TIMEOUT_MS and then warns:

net/rds/connection.c:rds_conn_wait_conns_freed() {
	if (!wait_event_timeout(rds_conn_freed_waitq,
				!atomic_read(&trans->t_conn_count),
				msecs_to_jiffies(RDS_CONN_FREE_TIMEOUT_MS)))
		WARN(1, "RDS/%s: %d connection(s) not freed\n",
		     trans->t_name, atomic_read(&trans->t_conn_count));
}

Does "holds nothing running" also hold?  destroy_workqueue() and the
transport's per-connection state are released only at the final put:

net/rds/connection.c:rds_conn_path_free() {
	if (cp->cp_wq != rds_wq) {
		destroy_workqueue(cp->cp_wq);
		cp->cp_wq = NULL;
	}

	cp->cp_conn->c_trans->conn_free(cp->cp_transport_data);
}

so the per-path workqueues and, for IB, the connection's ic with its
receive caches stay allocated for as long as the new reference is held.
Could the message mention the 10 second unload stall plus WARN, and that
the per-path workqueues and transport state are kept alive too?

> diff --git a/net/rds/message.c b/net/rds/message.c
> index f25f2592586f7..29e95028e61e4 100644
> --- a/net/rds/message.c
> +++ b/net/rds/message.c
> @@ -182,6 +182,18 @@ static void rds_message_purge(struct rds_message *rm)
>  		kref_put(&rm->atomic.op_rdma_mr->r_kref, __rds_put_mr_final);
>  }
>  
> +static void rds_message_free(struct rds_message *rm)
> +{
> +	/* get in rds_send_queue_rm(), rds_send_probe() or the congestion
> +	 * map path of rds_send_xmit().  Messages that were never queued on
> +	 * a connection have no reference to drop.
> +	 */
> +	if (rm->m_inc.i_conn)
> +		rds_conn_put(rm->m_inc.i_conn);
> +
> +	kfree(rm);
> +}
> +

[ ... ]

> diff --git a/net/rds/recv.c b/net/rds/recv.c
> index 6204e577a90ae..b031c0b43af83 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;
> @@ -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);

[Severity: High]
Does the new reference outlive the transport unload barrier in the module
unload case this patch targets?

The get taken in rds_inc_init()/rds_inc_path_init() is dropped only from
rds_inc_put(), i.e. when userspace calls recvmsg() or close(), so the
hold time is chosen by the application.  The unload barrier, however, is
bounded:

net/rds/connection.c:rds_conn_wait_conns_freed() {
	if (!wait_event_timeout(rds_conn_freed_waitq,
				!atomic_read(&trans->t_conn_count),
				msecs_to_jiffies(RDS_CONN_FREE_TIMEOUT_MS)))
		WARN(1, "RDS/%s: %d connection(s) not freed\n",
		     trans->t_name, atomic_read(&trans->t_conn_count));
}

and its own comment states the assumption this patch appears to break -
"anything still holding a connection reference at that point is a
short-lived lookup-style holder".

Is there a configuration where the socket does not pin the transport of
the conn it holds?  rds_find_bound() matches on daddr, dport and
bound_if only, and rds_recv_incoming() re-points the inc at the arriving
conn, so a socket bound with one transport can end up queueing an inc
whose i_conn belongs to the other transport (for example an inbound
RDS/TCP conn to an IPoIB address on a host whose sockets prefer RDS/IB).
The socket's module reference from rds_set_transport() covers only its
own transport, so rmmod of the other one is not blocked, hits the
timeout and unloads anyway.

Once that happens, doesn't the later recvmsg() or close() run these two
calls against a module that is gone?

rds_inc_put()
    conn->c_trans->inc_free(inc);
    rds_conn_put(conn)
        rds_conn_destroy_fini()
            rds_conn_path_free()
                cp->cp_conn->c_trans->conn_free(cp->cp_transport_data);
            atomic_dec_return(&trans->t_conn_count);

By that point rds_tcp_exit() has already run rds_trans_unregister() and
kmem_cache_destroy(rds_tcp_conn_slab), and rds_ib_exit() has run
rds_ib_recv_exit(), which destroys rds_ib_incoming_slab and
rds_ib_frag_slab that the queued inc and its fragments came from.
rds_recvmsg() dereferences inc->i_conn->c_trans->inc_copy_to_user the
same way.

Would the barrier need to become unbounded, or the socket-held
references dropped or detached at quiesce time, so that the reachable
dereference is not simply moved from freed conn memory to freed module
text after the timeout?

The same question applies to the send side: a message parked on a socket
send queue drops its conn reference only from rds_message_free() at
close, so it extends the connection lifetime past the unload wait too.

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