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

[Larysa Zaremba <larysa.zaremba@intel.com>]

On Thu, Apr 09, 2026 at 11:07:02AM +0200, Paolo Abeni wrote:
> 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?
> ---
> 

Yes, there is a race condition risk that is easy to fix.

> > +/**
> > + * 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?
> ---
>

This should create only false-negatives, which is not a problem, timeout time is 
much longer than libie_xn_check_async_timeout() calling period.

> 
> > +		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?
> ---

Yes, I think that in trying to avoid unnecessary copying, I shot myself in the 
foot, will fix.

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

There are also a few AI's comments that will result in fixes to stable.

> 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
>