Re: [PATCH net-next v8 09/13] net/rds: hold a connection reference from struct rds_incoming

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message gives the wrong triggers for the use-after-
  free.
- [Low] The commit fixes a documented use-after-free and panic (KASAN
  slab-use-after-free in rds6_inc_info_copy, and the…
- [Low] In rds_inc_on_sock_queue() (net/rds/recv.c:505), the comment "/*
  XXX make sure this i_conn is reliable */" still sits right before…

--

net/rds: hold a connection reference from struct rds_incoming

This patch makes every assignment of rds_incoming->i_conn take a
connection reference. Received incs drop it in rds_inc_put(), and
messages drop it through a new rds_message_free() helper. The goal is to
keep a connection allocated for as long as incs or messages that point
at it remain queued on sockets.

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

> Embedded as the messages m_inc, an inc routinely outlives the connection
> it points at, by sitting in the socket's receive queue until the
> application reads it, while the connection is destroyed by netns
> teardown or module unload - and every dereference of i_conn after that
> point touches freed memory.

[Severity: Low]
Are netns teardown and core module unload the right triggers here?

rds_create() allocates user RDS sockets with kern == 0:

    sk = sk_alloc(net, AF_RDS, GFP_KERNEL, &rds_proto, kern);

So each socket holds a reference on its netns. rds_recv_incoming() also
already drops incs whose socket lives in a different netns from the
connection:

    if (!net_eq(sock_net(rds_rs_to_sk(rs)), rds_conn_net(conn))) {

Taken together, it doesn't look like rds_loop_exit_net() or
rds_tcp_exit_net() can destroy a connection that an inc on a live
socket's receive queue still points at.

For the first KASAN trace, rds_proto.owner and rds_proto_ops.owner are
both THIS_MODULE in af_rds.c. Every open RDS socket therefore pins
rds.ko, and delete_module() can't reach rds_exit() while task 101 is in
getsockopt() on one. Does that trace need a forced unload
(CONFIG_MODULE_FORCE_UNLOAD)?

The path that does look reachable without a forced unload is a
cross-transport one. rds_find_bound() looks up sockets by address, port
and scope_id only, and doesn't check rs_transport. A socket bound with
one transport (for example TCP chosen via SO_RDS_TRANSPORT on an IPoIB
address) can then have incs queued that arrived on an IB connection.

That socket pins only rds.ko and its own rs_transport module, so
rmmod rds_rdma succeeds:

    rds_ib_exit()
      rds_ib_destroy_nodev_conns()
        rds_conn_destroy()

A later close then reaches the freed connection:

    rds_release()->rds_clear_recv_queue()->rds_inc_put()->rds_ib_inc_free()

This matches the second trace.

Could the commit message describe this cross-transport unload case
instead of netns teardown or unloading rds.ko? The conclusion about
CAP_SYS_MODULE and stable still holds either way.

> Reported-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> Closes: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/

[Severity: Low]
This isn't a bug, but the trailers have no Fixes: tag. The underlying
defect is that rds_incoming->i_conn never held a connection reference,
and that goes back to the original RDS code.

The message explains why this isn't a stable candidate, so leaving out
Fixes: may be deliberate, to keep AUTOSEL from picking it up. If so,
could the commit message say that explicitly?

> diff --git a/net/rds/recv.c b/net/rds/recv.c
> index 6204e577a90ae..1fcd4483d3be4 100644
> --- a/net/rds/recv.c
> +++ b/net/rds/recv.c

[ ... ]

> @@ -406,8 +425,9 @@ void rds_recv_incoming(struct rds_connection *conn, struct in6_addr *saddr,
>  	 * rds_find_bound() uses a global (netns-agnostic) hash table.
>  	 * An RDS connection created in netns A can match a socket bound
>  	 * in the init netns, delivering inc cross-netns with inc->i_conn
> -	 * pointing into netns A.  When cleanup_net() then frees that conn,
> -	 * any subsequent dereference of inc->i_conn is a use-after-free.
> +	 * pointing into netns A.  Cross-netns delivery is wrong on its
> +	 * own, and the inc's connection reference would keep a
> +	 * connection of a dead netns around, with a stale c_net.
>  	 * Drop the inc if the receiving socket lives in a different netns.
>  	 */
>  	if (!net_eq(sock_net(rds_rs_to_sk(rs)), rds_conn_net(conn))) {

[Severity: Low]
This isn't a bug, but this comment was updated for the new reference
rules while a nearby one in the same file was left as is.
rds_inc_on_sock_queue() still has:

net/rds/recv.c:rds_inc_on_sock_queue() {
    ...
		if (drop) {
			/* XXX make sure this i_conn is reliable */
			rds_recv_rcvbuf_delta(rs, sk, inc->i_conn->c_lcong,
					      -be32_to_cpu(inc->i_hdr.h_len),
					      inc->i_hdr.h_dport);
    ...
}

With this patch, an inc linked on rs_recv_queue holds its own reference
on i_conn. That reference is taken in rds_inc_init(), rds_inc_path_init()
or rds_recv_incoming(), and dropped only in rds_inc_put() after
inc_free(). The kernel-doc for rds_conn_wait_conns_freed() also says
that incs now pin their connection.

Should this XXX comment be removed or reworded, since it now contradicts
that contract? It is still there at the end of the series.

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