Re: [PATCH net-next v4 06/10] enic: add MBOX core send and receive for admin channel

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: enic: add MBOX core send and receive for admin channel

This commit implements the mailbox protocol engine used for PF-VF
communication over the admin channel.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
> index 345d194c6eebe..c96268adc173f 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
> @@ -19,6 +19,7 @@
>  #include "cq_enet_desc.h"
>  #include "wq_enet_desc.h"
>  #include "rq_enet_desc.h"
> +#include "enic_mbox.h"
>  
>  /* No-op: admin WQ buffers are freed inline after completion polling */
>  static void enic_admin_wq_buf_clean(struct vnic_wq *wq,
>  				    struct vnic_wq_buf *buf)
>  {
>  }

If an MBOX send times out, the timeout path intentionally leaves the buffer
and DMA mapping pending to prevent a hardware use-after-free. Because this
cleanup function is a no-op, are those pending descriptors and mappings
permanently leaked when the admin channel is later closed or the driver is
removed?

[ ... ]
> @@ -156,7 +157,26 @@ unsigned int enic_admin_rq_cq_service(struct enic *enic, unsigned int budget)
>  					buf->dma_addr, buf->len,
>  					DMA_FROM_DEVICE);
>  
> -		enic_admin_msg_enqueue(enic, buf->os_buf, buf->len);
> +		if (enic->admin_rq_handler) {
> +			struct cq_enet_rq_desc *rq_desc = desc;
> +			u16 sender_vlan;

Does this sequence process the CQ completion unconditionally without checking
the hardware descriptor for errors?

Standard data paths typically verify flags like packet_error or fcs_ok before
accepting the packet. If corrupted administrative messages are received here,
will they be processed as valid?

> +
> +			/* Firmware sets the CQ VLAN field to identify the
> +			 * sender: 0 = PF, 1-based = VF index.  Overwrite
> +			 * the untrusted src_vnic_id in the MBOX header with
> +			 * the hardware-verified value.
> +			 */
> +			sender_vlan = le16_to_cpu(rq_desc->vlan);
> +			if (buf->len >= sizeof(struct enic_mbox_hdr)) {
> +				struct enic_mbox_hdr *hdr = buf->os_buf;
> +
> +				hdr->src_vnic_id = (sender_vlan == 0) ?
> +					cpu_to_le16(ENIC_MBOX_DST_PF) :
> +					cpu_to_le16(sender_vlan - 1);
> +			}
> +
> +			enic_admin_msg_enqueue(enic, buf->os_buf, buf->len);
> +		}

Is buf->len the original static allocation size (2048 bytes) rather than the
actual number of bytes written by the hardware?

If the hardware wrote fewer bytes than the full buffer capacity, it looks
like passing buf->len will enqueue uninitialized heap memory. This would
also bypass the bounds checks in enic_mbox_recv_handler(), as the length
will appear to always be 2048 regardless of the actual packet size.

[ ... ]
> diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> new file mode 100644
> index 0000000000000..d144c86d9ef84
> --- /dev/null
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
[ ... ]
> +	vnic_wq_doorbell(wq);
> +
> +	timeout = jiffies + usecs_to_jiffies(ENIC_MBOX_POLL_TIMEOUT_US);
> +	err = -ETIMEDOUT;
> +	while (time_before(jiffies, timeout)) {
> +		if (enic_admin_wq_cq_service(enic)) {
> +			err = 0;
> +			break;
> +		}
> +		usleep_range(ENIC_MBOX_POLL_INTERVAL_US,
> +			     ENIC_MBOX_POLL_INTERVAL_US + 50);
> +	}

Could this loop falsely report a timeout if the thread experiences a long
scheduling delay during usleep_range()?

If the thread wakes up after the timeout has passed, it will exit the loop
immediately without checking one last time. If the hardware actually
completed the operation during the sleep window, the completion would be
missed, incorrectly disabling the channel.

[ ... ]