Re: [PATCH net-next v12 05/12] enic: add admin CQ service with MSI-X interrupt and workqueue polling

[Jakub Kicinski <kuba@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.
---
Now I have the confirmation. Let me generate the report.

commit 91d5187517ef604e2669cd4fde3a2c606489afe1
Author: Satish Kharat <satishkh@cisco.com>

enic: add admin CQ service with MSI-X interrupt and workqueue polling

This adds an MSI-X driven CQ service for the admin channel with a
workqueue-based poller, a bounded admin_msg backlog, and a deferred
dispatch work_struct for received admin messages.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
> index b2be42092106..9358165263d9 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c

[ ... ]

> @@ -94,6 +96,266 @@ static void enic_admin_rq_drain(struct enic *enic)
>  	vnic_rq_clean(&enic->admin_rq, enic_admin_rq_buf_clean);
>  }
>  
> +static unsigned int enic_admin_cq_color(void *cq_desc, unsigned int desc_size)
> +{

[ ... ]

> +unsigned int enic_admin_rq_cq_service(struct enic *enic)
> +{
> +	struct vnic_cq *cq = &enic->admin_cq[1];
> +	struct vnic_rq *rq = &enic->admin_rq;
> +	struct cq_enet_rq_desc *rq_desc;
> +	struct vnic_rq_buf *buf;
> +	u16 bwf, bytes_written;
> +	unsigned int work = 0;
> +	void *desc;
> +
[ ... ]
> +		rq_desc = desc;
> +		bwf = le16_to_cpu(rq_desc->bytes_written_flags);
> +		bytes_written = bwf & CQ_ENET_RQ_DESC_BYTES_WRITTEN_MASK;
> +		if (bytes_written > buf->len)
> +			goto next_desc;
> +
> +		dma_sync_single_for_cpu(&enic->pdev->dev,
> +					buf->dma_addr, buf->len,
> +					DMA_FROM_DEVICE);
[ ... ]
> +		enic_admin_msg_enqueue(enic, buf->os_buf, bytes_written);

[Medium]
Can this path leak uninitialised heap contents to downstream admin
message consumers?

The admin RQ buffers are allocated with plain kmalloc() in
enic_admin_rq_post_one():

    buf = kmalloc(ENIC_ADMIN_BUF_SIZE, gfp);

so the buffer contents are uninitialised heap.  The only bound applied
here is bytes_written > buf->len, but there is no check that hardware
actually DMA'd bytes_written bytes.  enic_admin_msg_enqueue() then does:

    memcpy(msg->data, buf, len);

A firmware or peer that DMAs a short payload but reports a larger
bytes_written value would cause the tail region to be copied verbatim
from stale heap into the queued admin_msg.

The commit comment above this block already flags a "buggy or hostile
VF" as part of the threat model.  Would switching enic_admin_rq_post_one()
to kzalloc(), or zeroing [bytes_written, buf->len) after
dma_sync_single_for_cpu() and before enqueue, be preferable?

The admin_rq_handler is NULL at this point in the series, but later
patches ("enic: add MBOX PF handlers for VF register and capability"
and "enic: add MBOX VF handlers for capability, register and link
state") process payload bytes, so any stale content would then reach
handler logic.

[ ... ]

> +static void enic_admin_poll_work_handler(struct work_struct *work)
> +{
> +	struct enic *enic = container_of(work, struct enic, admin_poll_work);
> +	unsigned int credits;
> +	unsigned int rq_work;
> +
> +	/* Snapshot the pending credit count before draining so we acknowledge
> +	 * exactly what the hardware reported for this interrupt.  Credits that
> +	 * accrue while enic_admin_rq_cq_service() runs are left for the next
> +	 * interrupt, which is harmless on this low-rate control path.
> +	 */
> +	credits = vnic_intr_credits(&enic->admin_intr);
> +
> +	rq_work = enic_admin_rq_cq_service(enic);
> +
> +	if (rq_work > 0)
> +		schedule_work(&enic->admin_msg_work);
> +
> +	/* Acknowledge the snapshotted credits and unmask the vector.  Unlike
> +	 * the NAPI data path, the admin channel is not re-polled, so the vector
> +	 * must be re-armed here to receive the next completion.  The unmask is
> +	 * applied through the interrupt mask register independently of the
> +	 * credit count, so returning zero credits on a spurious wakeup still
> +	 * re-arms the vector.
> +	 */
> +	vnic_intr_return_credits(&enic->admin_intr,
> +				 credits,
> +				 1 /* unmask */, 0);
> +}

[Low]
The commit message states:

    At least one credit is returned so the write always
    carries a non-zero acknowledgment and re-arms the vector, since the
    admin channel is not re-polled like the NAPI data path.

Does the code actually enforce a floor of one credit?

credits comes directly from vnic_intr_credits() and is passed verbatim
to vnic_intr_return_credits() with no max(credits, 1U) clamp.  On a
spurious wakeup credits can be zero.  The in-function comment right
below correctly notes:

    returning zero credits on a spurious wakeup still
    re-arms the vector.

Should the commit message be reworded to match the actual behaviour
(the UNMASK bit re-arms the vector independently of the credit count),
or should a max(credits, 1U) be added to match the message?

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