Re: [PATCH net-next v4 04/10] enic: add admin CQ service with MSI-X interrupt and NAPI 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.
---
enic: add admin CQ service with MSI-X interrupt and NAPI polling

Add completion queue service for the admin channel WQ and RQ, driven
by an MSI-X interrupt and NAPI polling.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
> index a8fcd5f116d17..345d194c6eebe 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
[ ... ]
> @@ -83,6 +85,207 @@ static void enic_admin_rq_drain(struct enic *enic)
>  	vnic_rq_clean(&enic->admin_rq, enic_admin_rq_buf_clean);
>  }
>  
[ ... ]
> +unsigned int enic_admin_rq_cq_service(struct enic *enic, unsigned int budget)
> +{
> +	struct vnic_cq *cq = &enic->admin_cq[1];
> +	struct vnic_rq *rq = &enic->admin_rq;
> +	struct vnic_rq_buf *buf;
> +	unsigned int work = 0;
> +	void *desc;
> +
> +	desc = vnic_cq_to_clean(cq);
> +	while (work < budget &&
> +	       enic_admin_cq_color(desc, cq->ring.desc_size) !=
> +	       cq->last_color) {
> +		/* Ensure CQ descriptor fields are read after
> +		 * the color/valid check.
> +		 */
> +		rmb();
> +		buf = rq->to_clean;
> +
> +		dma_sync_single_for_cpu(&enic->pdev->dev,
> +					buf->dma_addr, buf->len,
> +					DMA_FROM_DEVICE);
> +
> +		enic_admin_msg_enqueue(enic, buf->os_buf, buf->len);

Does this code expose uninitialized kernel heap memory?

The driver reads the completion queue descriptor but does not decode it to
determine the actual number of bytes written by the hardware. Instead, it
passes the full buf->len (2048 bytes) to enic_admin_msg_enqueue().

Since enic_admin_rq_post_one() allocates the receive buffers using kmalloc()
without __GFP_ZERO, passing the full 2048 bytes copies uninitialized kernel
memory following the actual packet payload.

> +
> +		enic_admin_rq_buf_clean(rq, rq->to_clean);
> +		rq->to_clean = rq->to_clean->next;
> +		rq->ring.desc_avail++;
> +
> +		vnic_cq_inc_to_clean(cq);
> +		work++;
> +		desc = vnic_cq_to_clean(cq);
> +	}
> +
> +	enic_admin_rq_fill(enic, GFP_ATOMIC);

Can this lead to a permanent receive queue stall?

Every processed receive buffer is unconditionally unmapped and freed via
enic_admin_rq_buf_clean() above. If enic_admin_rq_fill() fails due to memory
pressure, it silently returns an error and no replacement buffers are posted.

Because the original buffers were destroyed rather than recycled, and there
is no deferred retry mechanism, the descriptors are permanently lost. Repeated
transient failures will empty the RQ and permanently deadlock the admin channel.

[ ... ]
> @@ -128,23 +331,8 @@ static int enic_admin_alloc_resources(struct enic *enic)
>  	if (err)
>  		goto free_cq0;
>  
> -	/* PFs have dedicated SRIOV_INTR resources for admin channel.
> -	 * VFs lack SRIOV_INTR; use a regular INTR_CTRL slot instead.
> -	 */
> -	if (vnic_dev_get_res_count(enic->vdev, RES_TYPE_SRIOV_INTR) >= 1)
> -		err = vnic_intr_alloc_with_type(enic->vdev,
> -						&enic->admin_intr, 0,
> -						RES_TYPE_SRIOV_INTR);
> -	else
> -		err = vnic_intr_alloc(enic->vdev, &enic->admin_intr,
> -				      enic->intr_count);
> -	if (err)
> -		goto free_cq1;
> -
>  	return 0;

Will this removal break the admin channel on Physical Functions (PFs)?

Because PFs frequently have all their standard INTR_CTRL slots consumed by
regular networking queues, they rely on the dedicated SRIOV_INTR slot for
the admin channel.

Removing this fallback means enic_admin_setup_intr() might fail its bounds
check or vnic_intr_alloc() might fail, breaking initialization.

[ ... ]
> @@ -165,10 +353,32 @@ static void enic_admin_free_resources(struct enic *enic)
>  
>  static void enic_admin_init_resources(struct enic *enic)
>  {
> +	unsigned int intr_offset = enic->admin_intr_index;
> +
>  	vnic_wq_init(&enic->admin_wq, 0, 0, 0);
>  	vnic_rq_init(&enic->admin_rq, 1, 0, 0);
> -	vnic_cq_init(&enic->admin_cq[0], 0, 1, 0, 0, 1, 0, 1, 0, 0, 0);
> -	vnic_cq_init(&enic->admin_cq[1], 0, 1, 0, 0, 1, 0, 1, 0, 0, 0);
> +	vnic_cq_init(&enic->admin_cq[0],
> +		     0 /* flow_control_enable */,
> +		     1 /* color_enable */,
> +		     0 /* cq_head */,
> +		     0 /* cq_tail */,
> +		     1 /* cq_tail_color */,
> +		     1 /* interrupt_enable */,

Could setting interrupt_enable to 1 on the transmit completion queue (admin_cq[0])
cause an interrupt storm?

When a transmit completion occurs, it asserts the admin MSI-X interrupt and
schedules the NAPI poll routine (enic_admin_napi_poll). However, the NAPI
poll routine only services the receive queue (admin_cq[1]) and never clears
the event for admin_cq[0].

Since NAPI unmasks the interrupt upon completion, the uncleared transmit
completion would immediately re-trigger the interrupt.

[ ... ]
> @@ -187,12 +397,24 @@ int enic_admin_channel_open(struct enic *enic)
>  		return err;
>  	}
>  
> +	err = enic_admin_setup_intr(enic);
> +	if (err) {
> +		netdev_err(enic->netdev,
> +			   "Admin channel requires MSI-X, SR-IOV unavailable: %d\n",
> +			   err);
> +		goto free_resources;
> +	}
> +
> +	spin_lock_init(&enic->admin_msg_lock);
> +	INIT_LIST_HEAD(&enic->admin_msg_list);
> +	INIT_WORK(&enic->admin_msg_work, enic_admin_msg_work_handler);

Can a race condition occur here?

enic_admin_setup_intr() calls request_irq(), which registers the interrupt
handler and enables the IRQ line. If an interrupt fires immediately upon
registration, the ISR will schedule NAPI, which calls enic_admin_rq_cq_service().

NAPI would then attempt to acquire the uninitialized spinlock, modify the
uninitialized list, and schedule the uninitialized workqueue. Should the data
structures be initialized before requesting the interrupt?

[ ... ]
>  	return 0;
>  
>  disable_queues:
> +	enic_admin_teardown_intr(enic);
>  	vnic_wq_disable(&enic->admin_wq);
>  	vnic_rq_disable(&enic->admin_rq);
>  	enic_admin_qp_type_set(enic, 0);
>  	enic_admin_rq_drain(enic);
> +free_resources:
>  	enic_admin_free_resources(enic);
>  	return err;
>  }

Does the disable_queues error path leak queued messages?

If initialization fails after enic_admin_setup_intr() is called (for example,
during enic_admin_rq_fill()), the code branches to disable_queues. By this time,
the MSI-X interrupt and NAPI have been enabled.

If an interrupt fired and scheduled NAPI, messages could be added to
admin_msg_list and admin_msg_work could be scheduled. The error path does not
call cancel_work_sync(&enic->admin_msg_work) or enic_admin_msg_drain(enic),
which are required to stop the deferred work.