Re: [PATCH net-next v5 04/15] libie: add control queue support

[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.
https://sashiko.dev/#/patchset/20260715180042.1972010-2-anthony.l.nguyen@intel.com?part=4
---
--- Patch [4]: [PATCH net-next v5 04/15] libie: add control queue support ---
commit 1bdb26ce5f332eed97512271b4f8acb160c1f3d5
Author: Phani R Burra <phani.r.burra@intel.com>

libie: add control queue support

Libie will now support control queue setup and configuration APIs. These
are mainly used for mailbox communication between drivers and control
plane. Make use of the libeth_rx page pool support for managing controlq
buffers.

> diff --git a/drivers/net/ethernet/intel/libie/controlq.c b/drivers/net/ethernet/intel/libie/controlq.c
> new file mode 100644
> index 0000000000000..a3aa241f03b9a
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/libie/controlq.c
> @@ -96,30 +96,30 @@ int libie_ctlq_post_rx_buffs(struct libie_ctlq_info *ctlq)

[ ... ]

>  	while (num_to_post--) {
>  		dma_addr_t addr;
>  
>  		ctlq->descs[ntp] = (struct libie_ctlq_desc) {};
>  
>  		addr = libeth_rx_alloc(&fq, ntp);
>  		if (unlikely(addr == DMA_MAPPING_ERROR)) {
>  			ret = -ENOMEM;
>  			goto post_bufs;
>  		}
>  
>  		libie_ctlq_prep_rx_desc(&ctlq->descs[ntp], addr, fq.truesize);

[Severity: High]
Since libeth_rx_alloc() updates buf->truesize to reflect the actual space
available for DMA, should this use ctlq->rx_fqes[ntp].truesize instead of
the fixed fq.truesize? Using the fixed maximum size could allow the hardware
to DMA data beyond the allocated page pool buffer boundaries.

>  
>  		if (unlikely(++ntp == ctlq->ring_len))
>  			ntp = 0;
>  	}

[ ... ]

> @@ -478,13 +478,13 @@ u32 libie_ctlq_send_desc_avail(const struct libie_ctlq_info *ctlq)
>  {
>  	u32 ntu = ctlq->next_to_use, ntc = ctlq->next_to_clean;
>  
>  	return (ntc > ntu ? 0 : ctlq->ring_len) + ntc - ntu - 1;
>  }

[Severity: High]
Is there a missing API to reap TX completions? The available TX slots are
calculated based on next_to_clean, but there doesn't appear to be any
function provided to advance next_to_clean for TX queues. This would lead
to the TX queue eventually becoming permanently exhausted.

[ ... ]

> @@ -524,30 +524,30 @@ static void libie_ctlq_fill_rx_msg(struct libie_ctlq_msg *msg,
>  				   const struct libie_ctlq_desc *desc,
>  				    struct libeth_fqe *rx_buf)
>  {

[ ... ]

>  	if (likely(msg->data_len)) {
>  		if (unlikely(msg->data_len > LIBIE_CTLQ_MAX_BUF_LEN)) {
>  			msg->data_len = LIBIE_CTLQ_MAX_BUF_LEN;
>  			msg->chnl_retval = U32_MAX;
>  		}

[Severity: High]
Does this length validation need to check against the actual available space?
Checking against LIBIE_CTLQ_MAX_BUF_LEN instead of rx_buf->truesize could
lead to out-of-bounds CPU memory accesses when syncing and processing the
received data, as it might exceed what was actually allocated.

>  		msg->recv_mem = (struct kvec) {
>  			.iov_base = netmem_address(rx_buf->netmem) +
>  				    rx_buf->offset,
>  			.iov_len = msg->data_len,
>  		};

[Severity: High]
Can this virtual address calculation omit the page pool's p.offset?

In libeth_rx_alloc(), the DMA address correctly incorporates it:

	return page_pool_get_dma_addr_netmem(buf->netmem) + buf->offset +
	       fq->pp->p.offset;

If p.offset is non-zero in the page pool configuration, omitting it here
would result in reading from the wrong memory location, pointing prior to
the actual data payload.

>  		libeth_rx_sync_for_cpu(rx_buf, msg->data_len);
>  	} else {

[ ... ]

> @@ -584,20 +584,20 @@ u32 libie_ctlq_recv(struct libie_ctlq_info *ctlq, struct libie_ctlq_msg *msg,
>  		    u32 num_q_msg)
>  {
>  	u32 ntc, i;
>  
>  	ntc = ctlq->next_to_clean;
>  
>  	for (i = 0; i < num_q_msg; i++) {
>  		const struct libie_ctlq_desc *desc = &ctlq->descs[ntc];
>  		struct libeth_fqe *rx_buf = &ctlq->rx_fqes[ntc];

[Severity: High]
What happens if a caller attempts to use this function to reap completions
on a TX queue?

Since rx_fqes and tx_msg share a union in struct libie_ctlq_info:

	union {
		struct libeth_fqe		*rx_fqes;
		struct libie_ctlq_msg		**tx_msg;
	};

Blindly accessing rx_fqes without verifying the queue type would treat a
pointer to struct libie_ctlq_msg as a struct libeth_fqe, leading to
type confusion and wild pointer dereferences in libie_ctlq_fill_rx_msg().