Re: [PATCH net-next v2 2/2] net/rds: initialize i_conn_path in rds_inc_init()

[Paolo Abeni <pabeni@redhat.com>]

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.

> 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.

>  	/* 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.