Re: [PATCH net-next v2 2/2] net/rds: initialize i_conn_path in rds_inc_init()
From: Paolo Abeni <pabeni@redhat.com>
Date: 2026-08-13 09:44:39
Also in:
linux-rdma
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://netdev-ai.bots.linux.dev/sashiko/ --- net/rds: initialize i_conn_path in rds_inc_init() This sets inc->i_conn_path to NULL in rds_inc_init() so the field is no longer left holding stale slab contents, and switches rds_loop_xmit() to rds_inc_path_init() so the loopback path keeps a live path pointer in the message's embedded inc.
quoted hunk ↗ jump to hunk
diff --git a/net/rds/loop.c b/net/rds/loop.c index ac9295a766b1..e6b0750bbeda 100644 --- a/net/rds/loop.c +++ b/net/rds/loop.c@@ -89,7 +89,11 @@ static int rds_loop_xmit(struct rds_connection *conn, struct rds_message *rm, BUG_ON(hdr_off || sg || off); - rds_inc_init(&rm->m_inc, conn, &conn->c_laddr); + /* rds_send_queue_rm() stored the connection path in this embedded + * inc; use the path init so the re-initialization keeps the field + * valid instead of discarding it. + */ + rds_inc_path_init(&rm->m_inc, &conn->c_path[0], &conn->c_laddr);
[Severity: Medium] Does this swap silently drop the i_rx_lat_trace[] zeroing for loopback incs? rds_inc_init() in net/rds/recv.c ends with: inc->i_usercopy.rx_tstamp = ktime_set(0, 0); memset(inc->i_rx_lat_trace, 0, sizeof(inc->i_rx_lat_trace)); while rds_inc_path_init() stops at: inc->i_usercopy.rx_tstamp = ktime_set(0, 0); } EXPORT_SYMBOL_GPL(rds_inc_path_init); So after this change nothing in rds_loop_xmit() clears the array, and the RDS_MSG_RX_HDR and RDS_MSG_RX_START slots (written only by net/rds/ib_recv.c and net/rds/tcp_recv.c, never on the loopback path) are left to whatever rds_message_alloc()'s kzalloc() left behind rather than being explicitly initialized. The array is handed to userspace as RDS_CMSG_RXPATH_LATENCY deltas in rds_cmsg_recv(): inc->i_rx_lat_trace[RDS_MSG_RX_CMSG] = local_clock(); ... t.rx_trace[i] = inc->i_rx_lat_trace[j + 1] - inc->i_rx_lat_trace[j]; Would it be preferable to move the memset() into rds_inc_path_init(), or have rds_inc_path_init() call rds_inc_init() and then assign i_conn_path? That would also cover the other rds_inc_path_init() caller in rds_tcp_data_recv(), where tinc comes from a recycled, non-zeroing kmem_cache and therefore keeps the same stale-field property this patch sets out to remove.
quoted hunk ↗ jump to hunk
/* For the embedded inc. Matching put is in loop_inc_free() */ rds_message_addref(rm);diff --git a/net/rds/recv.c b/net/rds/recv.c index cf3884d87931..f1513dfb2716 100644 --- a/net/rds/recv.c +++ b/net/rds/recv.c@@ -47,6 +47,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); inc->i_conn = conn; + inc->i_conn_path = NULL; inc->i_saddr = *saddr; inc->i_usercopy.rdma_cookie = 0; inc->i_usercopy.rx_tstamp = ktime_set(0, 0);
[Severity: Medium]
Related to the above, the commit message says:
"and so the two init helpers
(rds_inc_init/rds_inc_path_init) leave the structure in an
equivalent, fully-initialized state."
Does that hold after this patch? rds_inc_init() gains i_conn_path = NULL
here, but rds_inc_path_init() still lacks the
memset(inc->i_rx_lat_trace, 0, sizeof(inc->i_rx_lat_trace)) that
rds_inc_init() performs, so the two helpers leave i_rx_lat_trace[] in
different states. Could the changelog be adjusted, or the missing
memset() added to rds_inc_path_init() so the claim is accurate?
--
This is an AI-generated review.