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

[sashiko-bot@kernel.org]

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

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?

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.

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

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().

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929231305.1515873-1-anthony.l.nguyen@intel.com?part=1