Re: [net-next,v1,1/7] net/rds: new extension header: rdma bytes

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net/rds: new extension header: rdma bytes

This patch introduces a new extension header type RDS_EXTHDR_RDMA_BYTES
to allow RDMA initiators to exchange byte counts with their targets for
statistics and policy purposes. It also expands rds_message_add_extension()
to support multiple extensions per message.

> diff --git a/net/rds/rds.h b/net/rds/rds.h
> index 8a549fe687ac..cadfd7ec0ba9 100644
> --- a/net/rds/rds.h
> +++ b/net/rds/rds.h

[ ... ]

> @@ -258,6 +259,19 @@ struct rds_ext_header_rdma_dest {
>  	__be32			h_rdma_offset;
>  };
>
> +/*
> + * This extension header tells the peer about delivered RDMA byte count.
> + */
> +#define RDS_EXTHDR_RDMA_BYTES	4
> +
> +struct rds_ext_header_rdma_bytes {
> +	__be32		h_rdma_bytes;	/* byte count */
> +	u8		h_rflags;	/* direction of RDMA, write or read */
> +};

This struct has 5 bytes of actual data but sizeof() will return 8 bytes
due to alignment padding. The 3 padding bytes after h_rflags are not
initialized when this struct is used in rds_ib_xmit(). Since
rds_message_add_extension() copies sizeof(struct) bytes via memcpy(),
those uninitialized padding bytes get transmitted to the peer.

Should this struct be marked __packed, or should the declaration in
rds_ib_xmit() use a zeroing initializer like = {} to avoid leaking
kernel stack contents over the network?

> diff --git a/net/rds/ib_send.c b/net/rds/ib_send.c
> index f9d28ddd168d..8282ff61b0b3 100644
> --- a/net/rds/ib_send.c
> +++ b/net/rds/ib_send.c
> @@ -578,10 +578,27 @@ int rds_ib_xmit(struct rds_connection *conn, struct rds_message *rm,
>  		 * used by the peer to release use-once RDMA MRs. */
>  		if (rm->rdma.op_active) {
>  			struct rds_ext_header_rdma ext_hdr;
> +			struct rds_ext_header_rdma_bytes rdma_bytes_ext_hdr;
>
>  			ext_hdr.h_rdma_rkey = cpu_to_be32(rm->rdma.op_rkey);
>  			rds_message_add_extension(&rm->m_inc.i_hdr,
> -					RDS_EXTHDR_RDMA, &ext_hdr, sizeof(ext_hdr));
> +						  RDS_EXTHDR_RDMA, &ext_hdr);
> +
> +			/* prepare the rdma bytes ext header */
> +			rdma_bytes_ext_hdr.h_rflags = rm->rdma.op_write ?
> +				RDS_FLAG_RDMA_WR_BYTES : RDS_FLAG_RDMA_RD_BYTES;
> +			rdma_bytes_ext_hdr.h_rdma_bytes =
> +				cpu_to_be32(rm->rdma.op_bytes);

Here rdma_bytes_ext_hdr is declared on the stack without initialization.
Only h_rflags and h_rdma_bytes are assigned, leaving the 3 padding bytes
uninitialized. When rds_message_add_extension() calls memcpy() with
len = sizeof(struct rds_ext_header_rdma_bytes) = 8, those uninitialized
bytes are copied into the header and sent to the peer.

[ ... ]
-- 
pw-bot: cr