Re: [PATCH v4 net 4/6] xsk: reclaim invalid Tx descriptors in ZC batch path

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Thu, Jul 23, 2026 at 09:29:47AM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.

Ok, see inline. All the 'high' issues are IMHO not valid.
Mediums seem to not be show-stoppers. Two other patch responses land in
the same category I'd say.

> ---
> xsk: reclaim invalid Tx descriptors in ZC batch path
> 
> This change parses ZC Tx batches in packet units so that descriptors
> belonging to invalid or oversize packets are appended to the CQ address
> area for userspace reclaim instead of being silently consumed. New
> per-pool state (reclaim_descs, tx_zc_pending_descs) sequences reclaim
> publication against outstanding driver-visible Tx completions.
> 
> Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")
> 
> > diff --git a/Documentation/networking/af_xdp.rst b/Documentation/networking/af_xdp.rst
> > index 50d92084a49c..cc3f0d16b28f 100644
> > --- a/Documentation/networking/af_xdp.rst
> > +++ b/Documentation/networking/af_xdp.rst
> > @@ -504,21 +506,25 @@ will be treated as an invalid descriptor.
> >  These are the semantics for producing packets onto AF_XDP Tx ring
> >  consisting of multiple frames:
> >  
> > -* When an invalid descriptor is found, all the other
> > -  descriptors/frames of this packet are marked as invalid and not
> > -  completed. The next descriptor is treated as the start of a new
> > -  packet, even if this was not the intent (because we cannot guess
> > -  the intent). As before, if your program is producing invalid
> > -  descriptors you have a bug that must be fixed.
> > +* When an invalid descriptor is found, the complete packet is treated as
> > +  invalid. The kernel consumes descriptors through the descriptor marking
> > +  the end of the packet and returns all their frame addresses through the
> > +  COMPLETION ring. A standalone invalid descriptor is treated as a
> > +  one-descriptor invalid packet. The descriptor following the end of the
> > +  invalid packet is treated as the start of a new packet. As before, if
> > +  your program is producing invalid descriptors you have a bug that must
> > +  be fixed. Rejected descriptors are reported in the ``tx_invalid_descs``
> > +  statistic.
> 
> [Medium]
> Is the "Rejected descriptors are reported in the tx_invalid_descs
> statistic" claim accurate for the oversize case?
> 
> Looking at xskq_cons_read_desc_batch(), the xdp_zc_max_segs overflow path
> sets drain based on nr_frags alone:
> 
>     if (nr_frags == pool->xdp_zc_max_segs)
>         drain = true;
> 
> Every individual descriptor in the oversize packet passed
> xskq_cons_is_valid_desc(), so invalid_descs (bumped only from that
> per-descriptor helper) is never incremented in this path.
> 
> Should the documentation distinguish the oversize case, or should the
> oversize drain also increment tx_invalid_descs so the documented
> observability signal matches reality?
> 
> > @@ -504,21 +506,25 @@ will be treated as an invalid descriptor.
> >  * For copy mode, the maximum supported number of frames in a packet is
> > -  equal to CONFIG_MAX_SKB_FRAGS + 1. If it is exceeded, all
> > -  descriptors accumulated so far are dropped and treated as
> > -  invalid. To produce an application that will work on any system
> > -  regardless of this config setting, limit the number of frags to 18,
> > -  as the minimum value of the config is 17.
> > +  equal to CONFIG_MAX_SKB_FRAGS + 1. If it is exceeded, all descriptors
> > +  through the end of the oversized packet are consumed, treated as invalid,
> > +  and their frame addresses are returned through the COMPLETION ring. To
> > +  produce an application that will work on any system regardless of this
> > +  config setting, limit the number of frags to 18, as the minimum value of
> > +  the config is 17.
> 
> [ ... ]
> 
> > diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
> > index f5e737a83055..2bb1d122b1bc 100644
> > --- a/include/net/xsk_buff_pool.h
> > +++ b/include/net/xsk_buff_pool.h
> > @@ -78,6 +78,9 @@ struct xsk_buff_pool {
> >  	u32 chunk_size;
> >  	u32 chunk_shift;
> >  	u32 frame_len;
> > +	u32 tx_descs_nentries;
> > +	u32 reclaim_descs;
> > +	u32 tx_zc_pending_descs;
> >  	u32 xdp_zc_max_segs;
> 
> [High]
> How are pool->reclaim_descs and pool->tx_zc_pending_descs unwound when
> the Tx path stops without producing further completions?
> 
> The gate in xsk_tx_peek_release_desc_batch() is:
> 
>     if (unlikely(READ_ONCE(pool->reclaim_descs)))
>         goto out;
> 
> The only place these two fields are cleared is xsk_tx_completed() when
> enough driver-visible completions arrive. What happens if:
> 
>     - the netdev is brought down via ndo_stop mid-flight,
>     - the socket unbinds/closes while reclaim_descs is nonzero,
>     - the ZC driver resets and drops in-flight descriptors,
>     - link goes down and the pending Tx never completes?

These cases are covered via xsk_notifier and/or xp_disable_drv_zc where
eventually we hit ZC driver that will release pending descs holding umem
addrs via xsk_tx_completed(), see ice_xsk_clean_xdp_ring().

> 
> In each of these cases xsk_tx_completed() is never called with the
> outstanding count, pool->reclaim_descs stays nonzero forever, and every
> subsequent xsk_tx_peek_release_desc_batch() early-returns. The reclaim
> CQ slots that xskq_prod_write_addr_batch() has already written are never
> made visible to userspace because the CQ producer is never advanced.
> 
> In a shared-UMEM pool, one socket tearing down mid-drain would block ZC
> batch Tx for every peer socket on that pool. There does not seem to be
> any teardown path (xsk_release, xp_disable_drv_zc/xp_enable_drv_zc,
> driver rebind) that zeroes these new fields.

No. If mid-drain means we did not hit EOP desc in current batch, there are
two cases:
- no descs given to HW, but reclaim_descs != 0 -> this is published to CQ
  immediately via xsk_tx_completed(pool, 0); in xsk_tx_commit_batch()
- tx_zc_pending_descs != 0, reclaim_descs != 0 -> reclaim descs will be
  published after HW is done with tx_zc_pending_descs and once Tx irq is
  cleaned, which will call xsk_tx_completed() for us.

> 
> Similarly, xs->drain_cont is per-socket state that can persist across
> close. If the socket goes away mid-drain, is anything expected to reset
> the pool-level counters that were set on its behalf?
> 
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 091792d1d82d..f906d51b6699 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -499,6 +499,23 @@ void __xsk_map_flush(struct list_head *flush_list)
> >  
> >  void xsk_tx_completed(struct xsk_buff_pool *pool, u32 nb_entries)
> >  {
> > +	u32 reclaim_descs = READ_ONCE(pool->reclaim_descs);
> > +
> > +	if (unlikely(reclaim_descs)) {
> > +		u32 pending_descs = READ_ONCE(pool->tx_zc_pending_descs);
> > +
> > +		if (nb_entries < pending_descs) {
> > +			WRITE_ONCE(pool->tx_zc_pending_descs,
> > +				   pending_descs - nb_entries);
> > +			xskq_prod_submit_n(pool->cq, nb_entries);
> > +			return;
> > +		}
> > +
> > +		WRITE_ONCE(pool->tx_zc_pending_descs, 0);
> > +		nb_entries += reclaim_descs;
> > +		WRITE_ONCE(pool->reclaim_descs, 0);
> > +	}
> > +
> >  	xskq_prod_submit_n(pool->cq, nb_entries);
> >  }
> >  EXPORT_SYMBOL(xsk_tx_completed);
> 
> [High]
> Is memory ordering between reclaim_descs and tx_zc_pending_descs
> sufficient here?
> 
> The writer in xsk_tx_commit_batch() publishes tx_zc_pending_descs first,
> then reclaim_descs:
> 
>     WRITE_ONCE(pool->tx_zc_pending_descs,
>                batch->tx_descs + cq_pending_descs);
>     WRITE_ONCE(pool->reclaim_descs, batch->reclaim_descs);
> 
> The reader here loads reclaim_descs first, then tx_zc_pending_descs.
> WRITE_ONCE / READ_ONCE prevent tearing and compiler reordering but do
> not emit CPU barriers.
> 
> On weakly-ordered architectures, if the reader observes
> reclaim_descs != 0 but a stale tx_zc_pending_descs == 0, the
> 
>     if (nb_entries < pending_descs)
> 
> branch is skipped, reclaim_descs is added to nb_entries, and the CQ is
> advanced past the driver-visible Tx slots that were supposed to precede
> the reclaim entries. This is the ordering the commit message says the
> new fields exist to enforce ("CQ is positional. Descriptors already
> written but not submitted must complete before any reclaim-only
> descriptors appended below").
> 
> Should the flag store use smp_store_release() paired with an
> smp_load_acquire() on the reader, or an explicit smp_wmb/smp_rmb between
> the two fields on each side?
> 
> [High]
> Is this sequence safe against concurrent xsk_tx_completed() callers on
> the same pool?

All of that regarding barriers and concurrency does not make sense. Single
NAPI instance serializes pool operations. Shared pool is only when sockets
are attached to same <netdev, qid> tuple. Therefore we can not have a
situation where CPU0 is in write side (xsk_tx_commit_batch()) and CPU1 is
at clean side (xsk_tx_completed()) at the same time.

Way more nasty issues would occur if stuff raised above would indeed be
valid...

> 
>     reclaim_descs = READ_ONCE(pool->reclaim_descs);
>     if (unlikely(reclaim_descs)) {
>         ...
>         WRITE_ONCE(pool->tx_zc_pending_descs, 0);
>         nb_entries += reclaim_descs;
>         WRITE_ONCE(pool->reclaim_descs, 0);
>     }
> 
> xsk_tx_completed() is EXPORT_SYMBOL and called by several drivers
> (i40e, ice, ixgbe, mlx5, virtio_net, gve, ...). virtio_net in
> particular can invoke it from more than one context.
> 
> If two callers both READ_ONCE(reclaim_descs) = R, both take the reclaim
> branch, and both add R to their local nb_entries before clearing
> reclaim_descs, do they not together publish 2*R reclaim entries on the
> CQ, releasing the same UMEM frame addresses to userspace twice?
> 
> The existing CQ producer paths (xsk_cq_reserve_locked /
> xsk_cq_cancel_locked) already take pool->cq->cq_cached_prod_lock
> precisely because the CQ can be produced from multiple contexts. Should
> the new RMW on reclaim_descs / tx_zc_pending_descs extend that lock
> coverage, or use an atomic RMW / cmpxchg?
> 
> > @@ -574,24 +591,157 @@ static u32 xsk_tx_peek_release_fallback(struct xsk_buff_pool *pool, u32 max_entr
> >  	return nb_pkts;
> >  }
> >  
> > +static void xsk_tx_commit_batch(struct xsk_buff_pool *pool,
> > +				struct xsk_tx_batch *batch)
> > +{
> > +	u32 nb_descs = xsk_tx_batch_cq_descs(batch);
> > +	u32 cq_cached_prod;
> > +
> > +	if (!nb_descs)
> > +		return;
> > +
> > +	cq_cached_prod = pool->cq->cached_prod;
> > +	xskq_prod_write_addr_batch(pool->cq, pool->tx_descs, nb_descs);
> > +
> > +	if (unlikely(batch->reclaim_descs)) {
> > +		u32 cq_pending_descs;
> > +
> > +		/* CQ is positional. Descriptors already written but not
> > +		 * submitted must complete before any reclaim-only descriptors
> > +		 * appended below.
> > +		 */
> > +		cq_pending_descs = cq_cached_prod - xskq_get_prod(pool->cq);
> > +
> > +		WRITE_ONCE(pool->tx_zc_pending_descs,
> > +			   batch->tx_descs + cq_pending_descs);
> > +		WRITE_ONCE(pool->reclaim_descs, batch->reclaim_descs);
> > +		if (unlikely(!pool->tx_zc_pending_descs))
> > +			xsk_tx_completed(pool, 0);
> > +	}
> > +}
> > +
> > +static struct xsk_tx_batch
> > +__xsk_tx_peek_release_desc_batch(struct xsk_buff_pool *pool, struct xdp_sock *xs,
> > +				 struct xdp_desc *descs, u32 max_descs)
> > +{
> > +	struct xsk_tx_batch batch = {};
> > +	u32 entries;
> > +
> > +	entries = xskq_cons_nb_entries(xs->tx, max_descs);
> > +	if (!entries)
> > +		return batch;
> > +
> > +	batch = xskq_cons_read_desc_batch(xs, pool, descs, max_descs);
> > +	if (!xsk_tx_batch_cq_descs(&batch)) {
> > +		xs->tx->queue_empty_descs++;
> > +	} else {
> > +		__xskq_cons_release(xs->tx);
> > +		xs->sk.sk_write_space(&xs->sk);
> > +	}
> > +	return batch;
> > +}
> 
> [Medium]
> Is queue_empty_descs the right counter to bump when the batch returned
> zero because of the budget_limited path?
> 
> xskq_cons_read_desc_batch() can return
> 
>     batch = { tx_descs = 0, reclaim_descs = 0, budget_limited = true }
> 
> when it reads descriptors mid-packet, hits nb_entries == max before the
> end-of-packet descriptor, and rewinds cached_cons. In that case the
> queue is not empty; the caller was just out of budget. Should this case
> be distinguished from a truly-empty consumer queue for the userspace
> statistic?
> 
> > @@ -603,19 +753,16 @@ u32 xsk_tx_peek_release_desc_batch(struct xsk_buff_pool *pool, u32 nb_pkts)
> > [ ... ]
> > +	batch = umem_shared ?
> > +		xsk_tx_peek_release_shared_desc_batch(pool, nb_pkts) :
> > +		__xsk_tx_peek_release_desc_batch(pool, xs,
> > +						 pool->tx_descs,
> > +						 nb_pkts);
> > +	xsk_tx_commit_batch(pool, &batch);
> 
> [High]
> xsk_tx_peek_release_desc_batch() checks pool->reclaim_descs under only
> rcu_read_lock():
> 
>     if (unlikely(READ_ONCE(pool->reclaim_descs)))
>         goto out;
> 
> and xsk_tx_commit_batch() later overwrites both fields unconditionally
> with WRITE_ONCE. If two producers pass the gating check concurrently
> (the batch API is not documented as single-threaded, and the pre-existing
> shared-UMEM CQ producer paths already require cq_cached_prod_lock), can
> one batch's pending-drain accounting silently overwrite the other's,
> causing reclaim entries to be published before the driver-visible
> descriptors from the other batch have completed?

Mixing copy and zero-copy mode here. Also see above.

> 
> > diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
> > index 3e3fbb73d23e..1bc42c8902f4 100644
> > --- a/net/xdp/xsk_queue.h
> > +++ b/net/xdp/xsk_queue.h
> > @@ -263,17 +274,18 @@ static inline void parse_desc(struct xsk_queue *q, struct xsk_buff_pool *pool,
> >  	parsed->mb = xp_mb_desc(desc);
> >  }
> >  
> > -static inline
> > -u32 xskq_cons_read_desc_batch(struct xsk_queue *q, struct xsk_buff_pool *pool,
> > -			      u32 max)
> > +static inline struct xsk_tx_batch
> > +xskq_cons_read_desc_batch(struct xdp_sock *xs, struct xsk_buff_pool *pool,
> > +			  struct xdp_desc *descs, u32 max)
> >  {
> [ ... ]
> >  		descs[nb_entries] = ring->desc[idx];
> >  		cached_cons++;
> >  		parse_desc(q, pool, &descs[nb_entries], &parsed);
> >  		if (unlikely(!parsed.valid))
> > -			break;
> > +			drain = true;
> > +
> > +		nr_frags++;
> > +		nb_entries++;
> 
> [Medium]
> descs[nb_entries] is populated from ring->desc[idx] before parse_desc()
> even for invalid descriptors, and xsk_tx_commit_batch() later writes
> descs[i].addr straight into the CQ via xskq_prod_write_addr_batch() for
> the reclaim range. No xp_aligned_validate_desc / xp_unaligned_validate_desc
> range check is applied to invalid-descriptor addr values before they
> appear on the CQ.

Invalid descs are now intentionally produced to CQ. That's the approach we
picked to avoid losing umem addresses that would cause potential buffer
starvation.

> 
> In XDP_SHARED_UMEM configurations where multiple sockets share a CQ, does
> this allow one socket to publish an arbitrary attacker-chosen 64-bit
> value into the shared CQ by submitting a single invalid Tx descriptor?
> 
> Downstream code that treats CQ entries as UMEM offsets (e.g. paths that
> feed them back through xp_raw_get_data on subsequent Tx) would then
> operate on that raw value. Prior semantics dropped invalid-descriptor
> addresses silently.
> 
> Should the reclaim CQ entry be sanitized (masked to a frame-aligned base
> or omitted) rather than published verbatim?