Re: [PATCH net-next 1/2] idpf: add flow-based XDP fallback for FWs without Tx FIFO support

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Sashiko-Bot <sashiko-bot@kernel.org>
Date: Wed, 30 Sep 2026 23:13:30 +0000

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] idpf: data race on pending_mask and last_ntu in XDP Tx finalize
> - [High] idpf: out-of-bounds write in idpf_xdpsq_poll_fb due to unvalidated completion tag
> - [Medium] idpf: mixing goto-based cleanup and __free() in idpf_xdpsqs_get

I think this already appeared earlier and I replied to these a while ago?

> --
> 
> commit 512d4db75f1d8f6ab37619b2712b334a77a2df35
> Author: Alexander Lobakin <aleksander.lobakin@intel.com>
> 
> idpf: add flow-based XDP fallback for FWs without Tx FIFO support
> 
> This commit introduces a static-key guarded flow-based XDP fallback for
> firmware that lacks queue-based scheduling support. It generates a
> completion for each Tx descriptor and serializes them using a dynamically
> allocated bitmap.
> 
>> diff --git a/drivers/net/ethernet/intel/idpf/xdp.c b/drivers/net/ethernet/intel/idpf/xdp.c
>> index cbccd45467681..c771663677a52 100644
>> --- a/drivers/net/ethernet/intel/idpf/xdp.c
>> +++ b/drivers/net/ethernet/intel/idpf/xdp.c
>> @@ -149,10 +151,12 @@ void idpf_xdp_copy_prog_to_rqs(const struct idpf_q_vec_rsrc *rsrc,
>>  
>>  static void idpf_xdp_tx_timer(struct work_struct *work);
>>  
>> -int idpf_xdpsqs_get(const struct idpf_vport *vport)
>> +int idpf_xdpsqs_get(struct idpf_vport *vport)
>>  {
>>  	struct libeth_xdpsq_timer **timers __free(kvfree) = NULL;
>> -	struct net_device *dev;
>> +	unsigned long **masks __free(kvfree) = NULL;
> 
> [ ... ]
> 
>> +	masks = kvzalloc_objs(*masks, vport->num_xdp_txq);
>> +	if (!masks)
>> +		goto err_masks;
> 
> [ ... ]
> 
>> +err_masks:
> 
> [Severity: Medium]
> Is it safe to mix goto statements with __free() scope-based cleanups in
> idpf_xdpsqs_get?

Safe. __free() can be used only to automatically cleanup pointers that
are on the stack. Here we have 2 dynamically-allocated arrays of
pointers, then can't be cleaned up that way.

> 
> The cleanup subsystem guidelines recommend that functions should either use
> scope-based cleanups entirely or rely on goto-based cleanups, but never
> mix both. Mixing them can lead to confusing ownership semantics and
> double-free or resource leak bugs.
> 
>> +	vport->xdpsq_fb = false;
>> +
>> +	for (u32 i = 0; i < vport->num_xdp_txq; i++)
>> +		kfree(timers[i]);
>> +
>> +	return -ENOMEM;
>>  }
> 
> [ ... ]
> 
>> @@ -250,6 +308,65 @@ static int idpf_xdp_parse_cqe(const struct idpf_splitq_4b_tx_compl_desc *desc,
>>  	return upper_16_bits(val);
>>  }
>>  
>> +static u32 idpf_xdpsq_poll_fb(struct idpf_tx_queue *xdpsq, u32 budget)
>> +{
> 
> [ ... ]
> 
>> +	for (done_frames = 0; done_frames < budget; ) {
>> +		int ret;
>> +
>> +		ret = idpf_xdp_parse_cqe(&cq->comp[ntc].common, gen);
>> +		if (ret >= 0) {
>> +			__clear_bit(ret, mask);
> 
> [Severity: High]
> Could this result in an out-of-bounds write?
> 
> The 16-bit completion tag parsed from hardware by idpf_xdp_parse_cqe() is
> used directly as an index for __clear_bit(). Since the mask is sized based
> on desc_count, a completion tag larger than desc_count returned from the
> hardware or firmware might cause an out-of-bounds write in the kernel
> heap.

HW never returns tags larger than desc_count because we actually set
these tags when filling Tx descriptors -- a completion descriptor only
carries the tag which was written by us to the corresponding Tx descriptor.

> 
> Should the tag value be validated against desc_count before using it as a
> bitmap index?
> 
>> +			done_frames++;
> 
> 
>> diff --git a/drivers/net/ethernet/intel/idpf/xdp.h b/drivers/net/ethernet/intel/idpf/xdp.h
>> index 63e56f7d43e00..95e47ea2ccba8 100644
>> --- a/drivers/net/ethernet/intel/idpf/xdp.h
>> +++ b/drivers/net/ethernet/intel/idpf/xdp.h
>> @@ -93,12 +93,34 @@ static inline void idpf_xdp_tx_xmit(struct libeth_xdp_tx_desc desc, u32 i,
>>  #endif
>>  }
>>  
>> -static inline void idpf_xdpsq_set_rs(const struct idpf_tx_queue *xdpsq)
>> +static inline void idpf_xdpsq_set_rs_fb(struct idpf_tx_queue *xdpsq)
>> +{
>> +	u32 ntu = xdpsq->next_to_use;
>> +	u32 old = xdpsq->last_ntu;
>> +
>> +	if (old > ntu) {
>> +		bitmap_set(xdpsq->pending_mask, old, xdpsq->desc_count - old);
> 
> [Severity: High]
> Could there be a data race on pending_mask and last_ntu here?
> 
> When idpf_xdp_tx_finalize() calls idpf_xdpsq_set_rs_fb(), the xdp_lock may
> have already been released (for example, by the bulk flush callback).

idpf_xdp_tx_finalize() takes the lock.

> 
> If multiple CPUs submit XDP frames concurrently when xdpsq_share is true,
> it appears we could have unprotected, concurrent, non-atomic bitmap_set()
> operations and updates to last_ntu. This might also race with the
> __clear_bit() operations from the consumer context in
> idpf_xdpsq_poll_fb().

idpf_xdpsq_poll_fb() is called by prep callbacks, all of them take the lock.

> 
>> +		if (ntu)
>> +			bitmap_set(xdpsq->pending_mask, 0, ntu);
>> +	} else {
>> +		bitmap_set(xdpsq->pending_mask, old, ntu - old);
>> +	}
>> +
>> +	xdpsq->last_ntu = ntu;
>> +}

Thanks,
Olek