Re: [PATCH net-next 3/6] net/rds: hold connection references in lookup, sockets and c_passive

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider:

Critical: 0 · High: 2 · Medium: 3 · Low: 2

- [High] Transport per-connection nodes can stay linked into a stack-
  local temporary list after rds_conn_destroy() returns, and the…
- [High] The commit asserts that cm_id->context may stay reference-free
  because "connection destroy tears down the cm_id before the connection…
- [Medium] rds_sendmsg() now declares and enforces rs->rs_lock as the
  serialization for rs->rs_conn (plain load under rs_lock at…
- [Medium] The new reference model makes parent->c_passive own a
  reference that is only dropped when the *parent* is destroyed, but…
- [Medium] parent->c_passive becomes a concurrently written, RCU-consumed
  pointer with no RCU annotations.
- [Low] The patch closes a syzbot-reported KCSAN data race on rs->rs_conn
  (Reported-by/Closes tags present) and removes stale-pointer reuse of a…
- [Low] The new comment in rds_conn_lookup() asserts that an RCU
  traversal of rds_conn_hash may legitimately encounter a connection…

--

net/rds: hold connection references in lookup, sockets and c_passive

This hands out real references wherever a struct rds_connection pointer
used to escape bare: rds_conn_lookup() and __rds_conn_create() return a
held reference, the rs->rs_conn sendmsg cache owns one and is serialized
under rs_lock, and parent->c_passive owns one dropped when the parent is
destroyed.  The cm_id->context back-pointers are left reference-free by
design.

Link: https://syzkaller.appspot.com/bug?extid=879c1877016972360186

    Reported-by: syzbot+879c1877016972360186@syzkaller.appspotmail.com
    Closes: https://syzkaller.appspot.com/bug?extid=879c1877016972360186

[Severity: Low]
The changelog closes a reported KCSAN data race but carries no Fixes: tag
for the commit that introduced the unsynchronized rs_conn cache (git blame
points at 5c11559046c4 "RDS: send.c").

Should a Fixes: tag be added, or if the patch is intentionally not a stable
candidate because it depends on the earlier patches in this series, could
the changelog say so explicitly?

[ ... ]

[Severity: Low]
Can an RCU traversal of rds_conn_hash actually observe an entry whose
refcount has already reached zero?  rds_conn_destroy() unhashes the node
under rds_conn_lock, calls synchronize_rcu(), and only then drops the
initial reference, so a reader that can still reach the node appears to be
inside a grace period where the refcount is at least one.

If the state the comment describes were reachable, would the guard even
help?  rds_conn_destroy_fini() frees immediately at the final put:

net/rds/connection.c:rds_conn_destroy_fini() {
	...
	kfree(conn->c_path);
	kmem_cache_free(rds_conn_slab, conn);
}

and rds_conn_init() creates the cache without SLAB_TYPESAFE_BY_RCU:

	rds_conn_slab = KMEM_CACHE(rds_connection, 0);

so the c_faddr/c_laddr/c_trans comparisons above the guard would already be
reads of freed or recycled memory.

kref_get_unless_zero() itself is a harmless idiom here; could the comment
be reworded so a later reader does not conclude that zero-refcount entries
are safely traversable?

[Severity: Medium]
Nothing here tests c_destroy_in_prog on the passive child, and nothing
clears the parent's c_passive when the passive connection itself is
destroyed - rds_conn_destroy() only clears its own c_passive:

net/rds/connection.c:rds_conn_destroy() {
	...
	if (conn->c_destroy_in_prog) {
		spin_unlock_irq(&rds_conn_lock);
		return;
	}
	...
	passive = conn->c_passive;
	conn->c_passive = NULL;
}

Since a quiesced connection now stays alive until its last reference goes
away, can this hand an already-destroyed passive conn, with a fresh
reference, back to rds_ib_cm_handle_connect()?

The quiesce leaves the path in RDS_CONN_DOWN with ic->i_cm_id == NULL, so
the DOWN->CONNECTING transition succeeds and a new cm_id is installed,
rds_ib_setup_qp() runs and rdma_accept() is called.  A second
rds_conn_destroy() then returns early on the c_destroy_in_prog guard, so
that state is never torn down and rds_conn_destroy_fini() only calls
conn_free(), which kfree()s the rds_ib_connection with the QP and CQs still
armed.

The one single-conn destroy path for IB is the version-mismatch drop in
rds_ib_cm_connect_complete(); would refusing to return a conn with
c_destroy_in_prog set (or invalidating the parent's pointer when the child
is destroyed) close this?

Separately, even without reuse, does the new c_passive reference keep a
quiesced passive conn - its transport state and its per-path workqueue -
pinned for the whole lifetime of the parent?

[Severity: Medium]
The install of the passive conn is a plain store consumed by a reader that
holds only rcu_read_lock():

	parent->c_passive = conn;

Should this be rcu_assign_pointer(), with rcu_dereference() at the read
site above and a __rcu annotation on the field in struct rds_connection?

The initialising stores to c_trans, c_path and the transport data are
ordered only by the rds_conn_lock unlock, which gives nothing to a reader
that never acquires that lock.  On a weakly ordered CPU, can the RCU reader
in __rds_conn_create() see c_passive != NULL with those fields still stale,
and then pass that pointer to rds_ib_cm_handle_connect(), which
dereferences them?

[ ... ]

[Severity: Medium]
This adds a second concurrent writer of c_passive as a plain store.  Before
this patch c_passive was written once and never cleared, so the unlocked
read in __rds_conn_create() had no competing writer.

The reader takes rcu_read_lock() but not rds_conn_lock, so the two accesses
share no lock.  Is this pair reportable by KCSAN in the same way as the
rs_conn race the changelog fixes, and would WRITE_ONCE()/rcu_assign_pointer()
here plus rcu_dereference() at the read site be preferable?

[Severity: High]
Now that real holders exist (the socket rs_conn cache, an in-flight
sendmsg, c_passive, lookup callers), rds_conn_destroy() can return with the
connection still alive and trans->conn_free() deferred to the final put.

Do the transport teardown helpers still rely on conn_free() unlinking the
per-connection node before they return?  They keep the node linked into a
list head on their own stack:

net/rds/ib_rdma.c:rds_ib_destroy_nodev_conns() {
	LIST_HEAD(tmp_list);

	spin_lock_irq(&ib_nodev_conns_lock);
	list_splice(&ib_nodev_conns, &tmp_list);
	spin_unlock_irq(&ib_nodev_conns_lock);

	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
		rds_conn_destroy(ic->conn);
}

and the unlink happens much later, from the deferred free:

net/rds/ib_cm.c:rds_ib_conn_free() {
	...
	spin_lock_irq(lock_ptr);
	list_del(&ic->ib_node);
	spin_unlock_irq(lock_ptr);
}

For a node whose neighbour is the tmp_list head, does that list_del() write
ic->ib_node.prev->next and .next->prev into a stack frame that has already
returned?

The same shape appears in rds_tcp_destroy_conns() and rds_tcp_kill_sock(),
where the node moved to tmp_list is not marked t_tcp_node_detached, so
rds_tcp_conn_free() takes the list_del() branch:

net/rds/tcp.c:rds_tcp_conn_free() {
	...
	if (!tc->t_tcp_node_detached)
		list_del(&tc->t_tcp_node);
}

and in rds_loop_kill_conns() with rds_loop_conn_free()'s
list_del(&lc->loop_node).

The later series patch that waits for connections to be freed on transport
unload does not appear to cover these: rds_ib_exit() calls
rds_ib_destroy_nodev_conns() and only then
rds_conn_wait_conns_freed(&rds_ib_transport), and rds_tcp_exit() waits
after rds_tcp_destroy_conns() has returned; the netns paths
(rds_tcp_kill_sock, rds_loop_kill_conns) get no wait at all.

Would unlinking the node (or marking it detached, as rds_tcp_kill_sock
already does for the secondary nodes) before rds_conn_destroy() can return,
or using a list head that outlives the pending frees, avoid this?

[Severity: High]
Is anything enforcing the invariant in this comment, that destroy tears the
cm_id down before the conn can be freed?

The CM event handler picks up the back-pointer with no reference and locks
a mutex that lives inside the separately freed c_path array
(c_cm_lock is c_path[0].cp_cm_lock per net/rds/rds_single_path.h):

net/rds/rdma_transport.c:rds_rdma_cm_event_handler_cmn() {
	struct rds_connection *conn = cm_id->context;
	...
	if (conn)
		mutex_lock(&conn->c_cm_lock);
	...
	case RDMA_CM_EVENT_ESTABLISHED:
		... rds_ib_cm_connect_complete(conn, event);
	...
	if (conn)
		mutex_unlock(&conn->c_cm_lock);
}

and for an unsupported peer protocol version that ESTABLISHED handling
destroys the conn from inside the callback:

net/rds/ib_cm.c:rds_ib_cm_connect_complete() {
	...
	rds_conn_destroy(conn);
}

Since this hunk drops the caller reference at out:, can that destroy be the
final put, so rds_conn_destroy_fini() kfree()s c_path and the conn before
the handler's mutex_unlock(&conn->c_cm_lock) runs on freed memory?

On the same path, does destroy deadlock the rdma_cm event workqueue?

rds_conn_destroy()
  rds_conn_path_quiesce()
    rds_conn_path_drop(cp, true)
    flush_work(&cp->cp_down_w)
      rds_shutdown_worker() -> rds_conn_shutdown()
        mutex_lock(&cp->cp_cm_lock)   <- already held by this CM callback

Also, rds_ib_cm_handle_connect() does not re-check rds_destroy_pending(conn)
after taking c_cm_lock.  When a destroy's quiesce has already completed, the
path is back in RDS_CONN_DOWN, so can this still win the DOWN->CONNECTING
transition, install ic->i_cm_id and cm_id->context, set up the QP and
accept, after which the new rds_conn_put() here is the final put and frees
the conn and its rds_ib_connection with the cm_id and QP still armed?

Would a real reference for the CM back-pointer (kref_get_unless_zero()) or
an rds_destroy_pending() re-check under c_cm_lock be needed to make the
comment's claim hold?

[ ... ]

[Severity: Medium]
The changelog says "reads and updates of rs_conn are serialized by
rs_lock", but rds_ioctl(SIOCRDSSETTOS) still reads rs_conn and writes
rs_tos under an unrelated global lock:

net/rds/af_rds.c:rds_ioctl() {
	...
	spin_lock_bh(&rds_sock_lock);
	if (rs->rs_tos || rs->rs_conn) {
		spin_unlock_bh(&rds_sock_lock);
		return -EINVAL;
	}
	rs->rs_tos = tos;
	spin_unlock_bh(&rds_sock_lock);
}

The two paths share no lock.  Is the plain load of rs->rs_conn there still
racing with the plain store of rs->rs_conn here, on the same eight-byte
field that the KCSAN report names?

The ioctl's "TOS cannot change once a connection exists" test also looks
like a check-then-act across the two locks:

ioctl(SIOCRDSSETTOS)                sendmsg()
  reads rs->rs_conn == NULL
                                      rds_conn_create_outgoing(..., rs->rs_tos, ...)
  rs->rs_tos = tos;
                                      rs->rs_conn = conn;

Can that leave a cached connection whose c_tos no longer matches rs_tos, so
every later sendmsg fails the rs->rs_tos == conn->c_tos test above and
recreates or re-looks-up a connection?

[ ... ]

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