Re: [PATCH net-next v5 09/15] idpf: refactor idpf to use libie control queues

[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=9
---
--- Patch [9]: [PATCH net-next v5 09/15] idpf: refactor idpf to use libie control queues ---
commit 06a0b55dccd2a8bd004309d725e04b4253d24231
Author: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
Subject: idpf: refactor idpf to use libie control queues

This commit refactors the idpf driver to use the libie control queue
APIs for initializing, configuring, and managing control queue
transactions. It replaces the local control queue implementation with
libie equivalents, which introduces DMA mapping of send buffers rather
than performing memory copies.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c

[ ... ]

> -static int idpf_mb_clean(struct idpf_adapter *adapter,
> -			 struct idpf_ctlq_info *asq)
> +static void idpf_mb_clean(struct idpf_adapter *adapter,
> +			  struct libie_ctlq_info *asq, bool deinit)
>  {
> -	u16 i, num_q_msg = IDPF_DFLT_MBX_Q_LEN;
> -	struct idpf_ctlq_msg **q_msg;
> -	struct idpf_dma_mem *dma_mem;
> -	int err;
> -
> -	q_msg = kzalloc_objs(struct idpf_ctlq_msg *, num_q_msg, GFP_ATOMIC);
> -	if (!q_msg)
> -		return -ENOMEM;
> -
> -	err = idpf_ctlq_clean_sq(asq, &num_q_msg, q_msg);
> -	if (err)
> -		goto err_kfree;
> -
> -	for (i = 0; i < num_q_msg; i++) {
> -		if (!q_msg[i])
> -			continue;
> -		dma_mem = q_msg[i]->ctx.indirect.payload;
> -		if (dma_mem)
> -			dma_free_coherent(&adapter->pdev->dev, dma_mem->size,
> -					  dma_mem->va, dma_mem->pa);
> -		kfree(q_msg[i]);
> -		kfree(dma_mem);
> -	}
> -
> -err_kfree:
> -	kfree(q_msg);
> +	struct libie_ctlq_xn_clean_params clean_params = {
> +		.ctlq		= asq,
> +		.rel_tx_buf	= kfree,
> +		.num_msgs	= IDPF_DFLT_MBX_Q_LEN,
> +		.force		= deinit,
> +	};
>  
> -	return err;
> +	libie_ctlq_xn_send_clean(&clean_params);
>  }

[Severity: Medium]
Does setting force = deinit here cause a memory leak when encountering
zero-length messages?

If a zero-length payload message (like a VF reset message) is enqueued
followed by a non-zero length message, libie_ctlq_xn_send_clean evaluates
the loop break condition on the zero-length payload. If the condition
!(params->force && msg->data_len) evaluates to true due to data_len being 0,
the cleanup loop terminates early.

Could this cause any subsequent pending messages in the queue to be skipped,
leaking their DMA mappings during a teardown path?

[ ... ]

> +void idpf_send_vf_reset_msg(struct idpf_adapter *adapter)
> +{
> +	struct libie_ctlq_info *ctlq = adapter->asq;
> +
> +	/* Forcefully claim send queue slot */
> +	idpf_mb_clean(adapter, ctlq, true);

[Severity: High]
Could this forceful cleanup unmap DMA buffers that the hardware is still
actively reading?

During driver unload or VF reset, the hardware is still active since the
reset message hasn't been sent yet. Calling idpf_mb_clean with deinit=true
bypasses the hardware Done check for non-empty messages and immediately
unmaps their DMA buffers.

Can this lead to a DMA use-after-free and trigger IOMMU faults if the
hardware attempts to read the unmapped buffers?

> +
> +	scoped_guard(spinlock, &ctlq->lock) {
> +		*ctlq->tx_msg[ctlq->next_to_use] = (struct libie_ctlq_msg) {
> +			.opcode = LIBIE_CTLQ_SEND_MSG_TO_CP,
> +			.chnl_opcode = VIRTCHNL2_OP_RESET_VF,
> +		};
> +
> +		libie_ctlq_send(adapter->asq, 1);
> +	}
> +}