Re: [PATCH net-next v2 05/14] libie: add bookkeeping support for control queue messages

[Paolo Abeni <pabeni@redhat.com>]

On 4/3/26 9:49 PM, Tony Nguyen wrote:
> +static bool
> +libie_ctlq_xn_process_recv(struct libie_ctlq_xn_recv_params *params,
> +			   struct libie_ctlq_msg *ctlq_msg)
> +{
> +	struct libie_ctlq_xn_manager *xnm = params->xnm;
> +	struct libie_ctlq_xn *xn;
> +	u16 msg_cookie, xn_index;
> +	struct kvec *response;
> +	int status;
> +	u16 data;
> +
> +	data = ctlq_msg->sw_cookie;
> +	xn_index = FIELD_GET(LIBIE_CTLQ_XN_INDEX_M, data);
> +	msg_cookie = FIELD_GET(LIBIE_CTLQ_XN_COOKIE_M, data);
> +	status = ctlq_msg->chnl_retval ? -EFAULT : 0;
> +
> +	xn = &xnm->ring[xn_index];
> +	if (ctlq_msg->chnl_opcode != xn->virtchnl_opcode ||
> +	    msg_cookie != xn->cookie)
> +		return false;
> +
> +	spin_lock(&xn->xn_lock);

Sashiko says:

---
Because the cookie and opcode are checked before acquiring the lock, is
it possible for the transaction to time out, be returned to the free
list, and get reallocated for a new message before the lock is acquired?
If that happens, could the old delayed response falsely complete the
newly allocated transaction since the identifiers are not re-verified
inside the lock?
---

> +/**
> + * libie_xn_check_async_timeout - Check for asynchronous message timeouts
> + * @xnm: Xn transaction manager
> + *
> + * Call the corresponding callback to notify the caller about the timeout.
> + */
> +static void libie_xn_check_async_timeout(struct libie_ctlq_xn_manager *xnm)
> +{
> +	u32 idx;
> +
> +	for_each_clear_bit(idx, xnm->free_xns_bm, LIBIE_CTLQ_MAX_XN_ENTRIES) {

Sashiko says:

---
This iterates over the bitmap without holding the lock. Concurrently,
other paths modify this bitmap using non-atomic bitwise operations like
__clear_bit() and __set_bit() under the lock. Will this cause torn reads
or data races that might lead the timeout handler to skip valid
transactions or examine invalid ones?
---


> +		params->ctlq_msg->sw_cookie = cookie;
> +		params->ctlq_msg->send_mem = *dma_mem;
> +		params->ctlq_msg->data_len = buf_len;
> +		params->ctlq_msg->chnl_opcode = params->chnl_opcode;
> +		ret = libie_ctlq_send(params->ctlq, params->ctlq_msg, 1);
> +	}
> +
> +	if (ret && !libie_cp_can_send_onstack(buf_len))
> +		libie_cp_unmap_dma_mem(dev, dma_mem);

Sashiko says:

---
When libie_ctlq_send() fails here, the DMA memory is unmapped and the
buffer is freed by the caller. However, the software tracking ring at
tx_msg[next_to_use] still contains the populated send_mem details and a
non-zero data_len.

During driver teardown, libie_ctlq_xn_send_clean() is invoked with
params->force = true, which processes the ring without checking the
hardware completion bit. Could this cause the cleanup routine to process
the failed slot again, resulting in a double-free and double-unmap?
---

There are more remarks on the following patch, please have a look.

Also, it would be very helpful if you could help triaging such
(overwhelming amount of) feedback on future submissions, explicitly
commenting on the ML. Sashiko tends to be quite noise on device driver code.

Thanks,

Paolo