Re: [PATCH net-next v4 03/10] enic: add admin RQ buffer management

[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.
---
enic: add admin RQ buffer management

This commit adds buffer management for the enic driver's admin receive queue,
allocating DMA buffers on open and draining them on close.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
> index d1abe6a50095d..a8fcd5f116d17 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
[ ... ]
> @@ -151,6 +212,7 @@ int enic_admin_channel_open(struct enic *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);
>  	enic_admin_free_resources(enic);
>  	return err;
>  }

Could this sequence allow the hardware to DMA into freed memory?

Here and in enic_admin_channel_close() below, vnic_rq_disable() is called
before enic_admin_qp_type_set(enic, 0), meaning the firmware might still be
actively routing admin messages to the queue.

Additionally, the return value of vnic_rq_disable() is ignored. If the disable
operation fails or times out, the hardware queue will remain active.

Then enic_admin_rq_drain() unconditionally unmaps the DMA addresses and frees
the buffers. If the queue is still active, incoming messages could be written
directly into the freed memory.

Would it be safer to stop the source of the messages first, and verify the
queue is successfully disabled before freeing the buffers?

[ ... ]
> @@ -166,7 +228,7 @@ void enic_admin_channel_close(struct enic *enic)
>  	enic_admin_qp_type_set(enic, 0);
>  
>  	vnic_wq_clean(&enic->admin_wq, enic_admin_wq_buf_clean);
> -	vnic_rq_clean(&enic->admin_rq, enic_admin_rq_buf_clean);
> +	enic_admin_rq_drain(enic);
>  	vnic_cq_clean(&enic->admin_cq[0]);
>  	vnic_cq_clean(&enic->admin_cq[1]);
>  	vnic_intr_clean(&enic->admin_intr);