* [PATCH v4 net 1/6] xsk: fix buffer leak in xsk_drop_skb() for AF_XDP multi-buffer Tx
2026-07-19 13:56 [PATCH v4 net 0/6] xsk: fix AF_XDP multi-buffer Tx descriptor reclaim Maciej Fijalkowski
@ 2026-07-19 13:56 ` Maciej Fijalkowski
2026-07-19 13:56 ` [PATCH v4 net 2/6] xsk: drain continuation descs after overflow in xsk_build_skb() Maciej Fijalkowski
` (6 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Maciej Fijalkowski @ 2026-07-19 13:56 UTC (permalink / raw)
To: netdev
Cc: bpf, magnus.karlsson, stfomichev, kuba, pabeni, horms, bjorn,
kerneljasonxing, Jason Xing, Maciej Fijalkowski
From: Jason Xing <kernelxing@tencent.com>
This patch is inspired by the check[1] from sashiko. It says when
overflow happens, the address of cq to be published is invalid.
Actually the severer thing is the whole process of publishing the
address of cq in this particular case is not right: it should truely
publish the address and advance the cached_prod in cq as long as it
reads descriptors from txq.
The following is the full analysis.
xsk_drop_skb() is called in three places, which all discard a partially
built multi-buffer skb:
1) xsk_build_skb() -EOVERFLOW error path: packet exceeds MAX_SKB_FRAGS
2) __xsk_generic_xmit() post-loop cleanup: an invalid descriptor in
the TX ring prevents the partial packet from completing
3) xsk_release(): socket close while xs->skb holds an incomplete packet
In all three cases, the TX descriptors for the already-processed frags
have been consumed from the TX ring (xskq_cons_release), and CQ slots
have been reserved. However, xsk_drop_skb() calls xsk_consume_skb()
which cancels the CQ reservations via xsk_cq_cancel_locked(). Since
the buffer addresses never appear in the completion queue, userspace
permanently loses track of these buffers.
Fix this by letting consume_skb() trigger the existing xsk_destruct_skb
destructor, which already submits buffer addresses to the CQ via
xsk_cq_submit_addr_locked().
Note that cancelling the descriptors back to the TX ring (via
xskq_cons_cancel_n) is not a appropriate option because an oversized
packet that always exceeds MAX_SKB_FRAGS would be retried indefinitely,
which is an obviously deadlock bug in the TX path.
Also move the desc->addr assignment in xsk_build_skb() above the
overflow check so that the current descriptor's address is recorded
before a potential -EOVERFLOW jump to free_err, consistent with the
zerocopy path in xsk_build_skb_zerocopy().
[1]: https://lore.kernel.org/all/20260425041726.85FB3C2BCB2@smtp.kernel.org/
Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")
Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
net/xdp/xsk.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index b970f30ea9b9..a7a83dc4546a 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -794,8 +794,11 @@ static void xsk_consume_skb(struct sk_buff *skb)
static void xsk_drop_skb(struct sk_buff *skb)
{
- xdp_sk(skb->sk)->tx->invalid_descs += xsk_get_num_desc(skb);
- xsk_consume_skb(skb);
+ struct xdp_sock *xs = xdp_sk(skb->sk);
+
+ xs->tx->invalid_descs += xsk_get_num_desc(skb);
+ consume_skb(skb);
+ xs->skb = NULL;
}
static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
@@ -877,7 +880,7 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
return ERR_PTR(-ENOMEM);
/* in case of -EOVERFLOW that could happen below,
- * xsk_consume_skb() will release this node as whole skb
+ * xsk_drop_skb() will release this node as whole skb
* would be dropped, which implies freeing all list elements
*/
xsk_addr->addrs[xsk_addr->num_descs] = desc->addr;
@@ -969,6 +972,8 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
goto free_err;
}
+ xsk_addr->addrs[xsk_addr->num_descs] = desc->addr;
+
if (unlikely(nr_frags == (MAX_SKB_FRAGS - 1) && xp_mb_desc(desc))) {
err = -EOVERFLOW;
goto free_err;
@@ -986,8 +991,6 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
skb_add_rx_frag(skb, nr_frags, page, 0, len, PAGE_SIZE);
refcount_add(PAGE_SIZE, &xs->sk.sk_wmem_alloc);
-
- xsk_addr->addrs[xsk_addr->num_descs] = desc->addr;
}
}
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v4 net 2/6] xsk: drain continuation descs after overflow in xsk_build_skb()
2026-07-19 13:56 [PATCH v4 net 0/6] xsk: fix AF_XDP multi-buffer Tx descriptor reclaim Maciej Fijalkowski
2026-07-19 13:56 ` [PATCH v4 net 1/6] xsk: fix buffer leak in xsk_drop_skb() for AF_XDP multi-buffer Tx Maciej Fijalkowski
@ 2026-07-19 13:56 ` Maciej Fijalkowski
2026-07-20 13:58 ` sashiko-bot
` (2 more replies)
2026-07-19 13:56 ` [PATCH v4 net 3/6] xsk: provide sufficient space in pool->tx_descs Maciej Fijalkowski
` (5 subsequent siblings)
7 siblings, 3 replies; 16+ messages in thread
From: Maciej Fijalkowski @ 2026-07-19 13:56 UTC (permalink / raw)
To: netdev
Cc: bpf, magnus.karlsson, stfomichev, kuba, pabeni, horms, bjorn,
kerneljasonxing, Jason Xing, Maciej Fijalkowski
From: Jason Xing <kernelxing@tencent.com>
Fix generic xmit path multi-buffer logic when packets are either too big
(count of descriptors exceed MAX_SKB_FRAGS) or an invalid descriptor is
included in fragmented packet. Introduce xdp_sock::drain_cont and act
upon this flag - when it is set, keep on consuming descriptors from
AF_XDP Tx ring and put them directly onto Cq. Previously these
descriptors were silently lost and could never be reached again.
Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")
Closes: https://lore.kernel.org/all/20260425041726.85FB3C2BCB2@smtp.kernel.org/
Reviewed-by: Jason Xing <kernelxing@tencent.com>
Co-developed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> # wrapped cq addr submission onto routine
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
include/net/xdp_sock.h | 1 +
net/xdp/xsk.c | 45 +++++++++++++++++++++++++++++++++++++++---
2 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index ebac60a3d8a1..8b51876efbed 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -80,6 +80,7 @@ struct xdp_sock {
* call of __xsk_generic_xmit().
*/
struct sk_buff *skb;
+ bool drain_cont;
struct list_head map_list;
/* Protects map_list */
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index a7a83dc4546a..12a845d012f6 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -737,6 +737,19 @@ static void xsk_cq_submit_addr_locked(struct xsk_buff_pool *pool,
spin_unlock_irqrestore(&pool->cq_prod_lock, flags);
}
+static void xsk_cq_submit_addr_single_locked(struct xsk_buff_pool *pool,
+ struct xdp_desc *desc)
+{
+ unsigned long flags;
+ u32 idx;
+
+ spin_lock_irqsave(&pool->cq_prod_lock, flags);
+ idx = xskq_get_prod(pool->cq);
+ xskq_prod_write_addr(pool->cq, idx, desc->addr);
+ xskq_prod_submit_n(pool->cq, 1);
+ spin_unlock_irqrestore(&pool->cq_prod_lock, flags);
+}
+
static void xsk_cq_cancel_locked(struct xsk_buff_pool *pool, u32 n)
{
spin_lock(&pool->cq->cq_cached_prod_lock);
@@ -1028,13 +1041,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
static int __xsk_generic_xmit(struct sock *sk)
{
struct xdp_sock *xs = xdp_sk(sk);
- bool sent_frame = false;
struct xdp_desc desc;
struct sk_buff *skb;
+ u32 cached_cons;
u32 max_batch;
int err = 0;
mutex_lock(&xs->mutex);
+ cached_cons = xs->tx->cached_cons;
/* Since we dropped the RCU read lock, the socket state might have changed. */
if (unlikely(!xsk_is_bound(xs))) {
@@ -1063,11 +1077,21 @@ static int __xsk_generic_xmit(struct sock *sk)
goto out;
}
+ if (unlikely(xs->drain_cont)) {
+ xsk_cq_submit_addr_single_locked(xs->pool, &desc);
+ xs->tx->invalid_descs++;
+ xskq_cons_release(xs->tx);
+ xs->drain_cont = xp_mb_desc(&desc);
+ continue;
+ }
+
skb = xsk_build_skb(xs, &desc);
if (IS_ERR(skb)) {
err = PTR_ERR(skb);
if (err != -EOVERFLOW)
goto out;
+ if (xp_mb_desc(&desc))
+ xs->drain_cont = true;
err = 0;
continue;
}
@@ -1096,18 +1120,33 @@ static int __xsk_generic_xmit(struct sock *sk)
goto out;
}
- sent_frame = true;
xs->skb = NULL;
}
if (xskq_has_descs(xs->tx)) {
+ bool drain = xs->skb || xs->drain_cont || xp_mb_desc(&desc);
+
+ err = xsk_cq_reserve_locked(xs->pool);
+ if (err) {
+ xs->tx->invalid_descs--;
+ if (xs->skb)
+ xsk_drop_skb(xs->skb);
+ xs->drain_cont = drain;
+ err = -EAGAIN;
+ goto out;
+ }
+
if (xs->skb)
xsk_drop_skb(xs->skb);
+
+ xsk_cq_submit_addr_single_locked(xs->pool, &desc);
+
xskq_cons_release(xs->tx);
+ xs->drain_cont = xp_mb_desc(&desc);
}
out:
- if (sent_frame)
+ if (xs->tx->cached_cons != cached_cons)
__xsk_tx_release(xs);
mutex_unlock(&xs->mutex);
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 net 2/6] xsk: drain continuation descs after overflow in xsk_build_skb()
2026-07-19 13:56 ` [PATCH v4 net 2/6] xsk: drain continuation descs after overflow in xsk_build_skb() Maciej Fijalkowski
@ 2026-07-20 13:58 ` sashiko-bot
2026-07-21 18:04 ` Maciej Fijalkowski
2026-07-23 16:29 ` Jakub Kicinski
2 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-07-20 13:58 UTC (permalink / raw)
To: Maciej Fijalkowski; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but rolling back a partial multi-buffer packet via xskq_cons_cancel_n() on NETDEV_TX_BUSY corrupts the Tx ring because the consumer index may have already been prematurely committed to user-space in a previous batch.
--
--- Patch [2]: [PATCH v4 net 2/6] xsk: drain continuation descs after overflow in xsk_build_skb() ---
commit ec981f427791ffa015b5a35518b1a57e2420d64d
Author: Jason Xing <kerneljasonxing@gmail.com>
xsk: drain continuation descs after overflow in xsk_build_skb()
Fixes generic xmit path multi-buffer logic when packets are oversized
or contain invalid descriptors. Introduces the xdp_sock::drain_cont flag
to continue consuming descriptors from the Tx ring and put them directly
onto Cq instead of silently losing them.
Closes: https://lore.kernel.org/all/20260425041726.85FB3C2BCB2@smtp.kernel.org/
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index a7a83dc4546a0..12a845d012f6f 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
[ ... ]
> @@ -1096,18 +1120,33 @@ static int __xsk_generic_xmit(struct sock *sk)
[ ... ]
> out:
> - if (sent_frame)
> + if (xs->tx->cached_cons != cached_cons)
> __xsk_tx_release(xs);
[Severity: High]
This isn't a bug introduced by this patch, but does this change to the
release condition expose a pre-existing issue where rolling back a partial
multi-buffer packet corrupts the Tx ring?
During multi-buffer packet assembly, if the max_batch limit is reached,
__xsk_generic_xmit() exits and commits the partially consumed descriptors
to ring->consumer. User-space observes this and may overwrite those slots.
On the next call to __xsk_generic_xmit(), packet assembly resumes. If
__dev_direct_xmit() subsequently returns NETDEV_TX_BUSY, the code calls
xskq_cons_cancel_n() to rewind the local cached_cons by the full packet
length:
if (err == NETDEV_TX_BUSY) {
/* Tell MAC device to consume more and retry later. */
xskq_cons_cancel_n(xs->tx, xsk_get_num_desc(skb));
...
Because cached_cons is rewound by the full length (including fragments
consumed in the previous call), it becomes smaller than the cached_cons
saved at the start of this current call.
Will this new condition (xs->tx->cached_cons != cached_cons) then evaluate
to true and commit the rewound index to the globally visible ring->consumer?
If ring->consumer jumps backwards, wouldn't the kernel fetch descriptors
that user-space may have already overwritten with new data upon retry?
>
> mutex_unlock(&xs->mutex);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260719135609.147823-1-maciej.fijalkowski@intel.com?part=2
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 net 2/6] xsk: drain continuation descs after overflow in xsk_build_skb()
2026-07-19 13:56 ` [PATCH v4 net 2/6] xsk: drain continuation descs after overflow in xsk_build_skb() Maciej Fijalkowski
2026-07-20 13:58 ` sashiko-bot
@ 2026-07-21 18:04 ` Maciej Fijalkowski
2026-07-22 3:09 ` Jason Xing
2026-07-23 16:29 ` Jakub Kicinski
2 siblings, 1 reply; 16+ messages in thread
From: Maciej Fijalkowski @ 2026-07-21 18:04 UTC (permalink / raw)
To: netdev
Cc: bpf, magnus.karlsson, stfomichev, kuba, pabeni, horms, bjorn,
kerneljasonxing, Jason Xing
On Sun, Jul 19, 2026 at 03:56:05PM +0200, Maciej Fijalkowski wrote:
> From: Jason Xing <kernelxing@tencent.com>
>
> Fix generic xmit path multi-buffer logic when packets are either too big
> (count of descriptors exceed MAX_SKB_FRAGS) or an invalid descriptor is
> included in fragmented packet. Introduce xdp_sock::drain_cont and act
> upon this flag - when it is set, keep on consuming descriptors from
> AF_XDP Tx ring and put them directly onto Cq. Previously these
> descriptors were silently lost and could never be reached again.
>
> Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")
> Closes: https://lore.kernel.org/all/20260425041726.85FB3C2BCB2@smtp.kernel.org/
> Reviewed-by: Jason Xing <kernelxing@tencent.com>
> Co-developed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> # wrapped cq addr submission onto routine
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
> include/net/xdp_sock.h | 1 +
> net/xdp/xsk.c | 45 +++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> index ebac60a3d8a1..8b51876efbed 100644
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
> @@ -80,6 +80,7 @@ struct xdp_sock {
> * call of __xsk_generic_xmit().
> */
> struct sk_buff *skb;
> + bool drain_cont;
>
> struct list_head map_list;
> /* Protects map_list */
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index a7a83dc4546a..12a845d012f6 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -737,6 +737,19 @@ static void xsk_cq_submit_addr_locked(struct xsk_buff_pool *pool,
> spin_unlock_irqrestore(&pool->cq_prod_lock, flags);
> }
>
> +static void xsk_cq_submit_addr_single_locked(struct xsk_buff_pool *pool,
> + struct xdp_desc *desc)
> +{
> + unsigned long flags;
> + u32 idx;
> +
> + spin_lock_irqsave(&pool->cq_prod_lock, flags);
> + idx = xskq_get_prod(pool->cq);
> + xskq_prod_write_addr(pool->cq, idx, desc->addr);
> + xskq_prod_submit_n(pool->cq, 1);
> + spin_unlock_irqrestore(&pool->cq_prod_lock, flags);
> +}
> +
> static void xsk_cq_cancel_locked(struct xsk_buff_pool *pool, u32 n)
> {
> spin_lock(&pool->cq->cq_cached_prod_lock);
> @@ -1028,13 +1041,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> static int __xsk_generic_xmit(struct sock *sk)
> {
> struct xdp_sock *xs = xdp_sk(sk);
> - bool sent_frame = false;
> struct xdp_desc desc;
> struct sk_buff *skb;
> + u32 cached_cons;
> u32 max_batch;
> int err = 0;
>
> mutex_lock(&xs->mutex);
> + cached_cons = xs->tx->cached_cons;
>
> /* Since we dropped the RCU read lock, the socket state might have changed. */
> if (unlikely(!xsk_is_bound(xs))) {
> @@ -1063,11 +1077,21 @@ static int __xsk_generic_xmit(struct sock *sk)
> goto out;
> }
>
> + if (unlikely(xs->drain_cont)) {
> + xsk_cq_submit_addr_single_locked(xs->pool, &desc);
> + xs->tx->invalid_descs++;
> + xskq_cons_release(xs->tx);
> + xs->drain_cont = xp_mb_desc(&desc);
> + continue;
> + }
> +
> skb = xsk_build_skb(xs, &desc);
> if (IS_ERR(skb)) {
> err = PTR_ERR(skb);
> if (err != -EOVERFLOW)
> goto out;
> + if (xp_mb_desc(&desc))
> + xs->drain_cont = true;
> err = 0;
> continue;
> }
> @@ -1096,18 +1120,33 @@ static int __xsk_generic_xmit(struct sock *sk)
> goto out;
> }
>
> - sent_frame = true;
> xs->skb = NULL;
> }
>
> if (xskq_has_descs(xs->tx)) {
> + bool drain = xs->skb || xs->drain_cont || xp_mb_desc(&desc);
> +
> + err = xsk_cq_reserve_locked(xs->pool);
> + if (err) {
> + xs->tx->invalid_descs--;
> + if (xs->skb)
> + xsk_drop_skb(xs->skb);
> + xs->drain_cont = drain;
> + err = -EAGAIN;
> + goto out;
> + }
> +
> if (xs->skb)
> xsk_drop_skb(xs->skb);
> +
> + xsk_cq_submit_addr_single_locked(xs->pool, &desc);
> +
> xskq_cons_release(xs->tx);
> + xs->drain_cont = xp_mb_desc(&desc);
> }
>
> out:
> - if (sent_frame)
> + if (xs->tx->cached_cons != cached_cons)
Sashiko says:
[Severity: High]
This isn't a bug introduced by this patch, but does this change to the
release condition expose a pre-existing issue where rolling back a partial
multi-buffer packet corrupts the Tx ring?
During multi-buffer packet assembly, if the max_batch limit is reached,
__xsk_generic_xmit() exits and commits the partially consumed descriptors
to ring->consumer. User-space observes this and may overwrite those slots.
On the next call to __xsk_generic_xmit(), packet assembly resumes. If
__dev_direct_xmit() subsequently returns NETDEV_TX_BUSY, the code calls
xskq_cons_cancel_n() to rewind the local cached_cons by the full packet
length:
if (err == NETDEV_TX_BUSY) {
/* Tell MAC device to consume more and retry later. */
xskq_cons_cancel_n(xs->tx, xsk_get_num_desc(skb));
...
Because cached_cons is rewound by the full length (including fragments
consumed in the previous call), it becomes smaller than the cached_cons
saved at the start of this current call.
Will this new condition (xs->tx->cached_cons != cached_cons) then evaluate
to true and commit the rewound index to the globally visible ring->consumer?
If ring->consumer jumps backwards, wouldn't the kernel fetch descriptors
that user-space may have already overwritten with new data upon retry?
Maciej says:
So generic xmit is still not bullet-proof, sigh. It's a problem that was
present even when `sent_frame` based consumer pointer update was used, so
sashiko correctly classified it as pre-existing issue.
I think this can be addressed after current set lands, as no new bugs are
introduced and seems it got acks from Stan and Jason.
> __xsk_tx_release(xs);
>
> mutex_unlock(&xs->mutex);
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 net 2/6] xsk: drain continuation descs after overflow in xsk_build_skb()
2026-07-21 18:04 ` Maciej Fijalkowski
@ 2026-07-22 3:09 ` Jason Xing
0 siblings, 0 replies; 16+ messages in thread
From: Jason Xing @ 2026-07-22 3:09 UTC (permalink / raw)
To: Maciej Fijalkowski
Cc: netdev, bpf, magnus.karlsson, stfomichev, kuba, pabeni, horms,
bjorn, Jason Xing
On Wed, Jul 22, 2026 at 2:04 AM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Sun, Jul 19, 2026 at 03:56:05PM +0200, Maciej Fijalkowski wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Fix generic xmit path multi-buffer logic when packets are either too big
> > (count of descriptors exceed MAX_SKB_FRAGS) or an invalid descriptor is
> > included in fragmented packet. Introduce xdp_sock::drain_cont and act
> > upon this flag - when it is set, keep on consuming descriptors from
> > AF_XDP Tx ring and put them directly onto Cq. Previously these
> > descriptors were silently lost and could never be reached again.
> >
> > Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")
> > Closes: https://lore.kernel.org/all/20260425041726.85FB3C2BCB2@smtp.kernel.org/
> > Reviewed-by: Jason Xing <kernelxing@tencent.com>
> > Co-developed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> # wrapped cq addr submission onto routine
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> > include/net/xdp_sock.h | 1 +
> > net/xdp/xsk.c | 45 +++++++++++++++++++++++++++++++++++++++---
> > 2 files changed, 43 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> > index ebac60a3d8a1..8b51876efbed 100644
> > --- a/include/net/xdp_sock.h
> > +++ b/include/net/xdp_sock.h
> > @@ -80,6 +80,7 @@ struct xdp_sock {
> > * call of __xsk_generic_xmit().
> > */
> > struct sk_buff *skb;
> > + bool drain_cont;
> >
> > struct list_head map_list;
> > /* Protects map_list */
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index a7a83dc4546a..12a845d012f6 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -737,6 +737,19 @@ static void xsk_cq_submit_addr_locked(struct xsk_buff_pool *pool,
> > spin_unlock_irqrestore(&pool->cq_prod_lock, flags);
> > }
> >
> > +static void xsk_cq_submit_addr_single_locked(struct xsk_buff_pool *pool,
> > + struct xdp_desc *desc)
> > +{
> > + unsigned long flags;
> > + u32 idx;
> > +
> > + spin_lock_irqsave(&pool->cq_prod_lock, flags);
> > + idx = xskq_get_prod(pool->cq);
> > + xskq_prod_write_addr(pool->cq, idx, desc->addr);
> > + xskq_prod_submit_n(pool->cq, 1);
> > + spin_unlock_irqrestore(&pool->cq_prod_lock, flags);
> > +}
> > +
> > static void xsk_cq_cancel_locked(struct xsk_buff_pool *pool, u32 n)
> > {
> > spin_lock(&pool->cq->cq_cached_prod_lock);
> > @@ -1028,13 +1041,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > static int __xsk_generic_xmit(struct sock *sk)
> > {
> > struct xdp_sock *xs = xdp_sk(sk);
> > - bool sent_frame = false;
> > struct xdp_desc desc;
> > struct sk_buff *skb;
> > + u32 cached_cons;
> > u32 max_batch;
> > int err = 0;
> >
> > mutex_lock(&xs->mutex);
> > + cached_cons = xs->tx->cached_cons;
> >
> > /* Since we dropped the RCU read lock, the socket state might have changed. */
> > if (unlikely(!xsk_is_bound(xs))) {
> > @@ -1063,11 +1077,21 @@ static int __xsk_generic_xmit(struct sock *sk)
> > goto out;
> > }
> >
> > + if (unlikely(xs->drain_cont)) {
> > + xsk_cq_submit_addr_single_locked(xs->pool, &desc);
> > + xs->tx->invalid_descs++;
> > + xskq_cons_release(xs->tx);
> > + xs->drain_cont = xp_mb_desc(&desc);
> > + continue;
> > + }
> > +
> > skb = xsk_build_skb(xs, &desc);
> > if (IS_ERR(skb)) {
> > err = PTR_ERR(skb);
> > if (err != -EOVERFLOW)
> > goto out;
> > + if (xp_mb_desc(&desc))
> > + xs->drain_cont = true;
> > err = 0;
> > continue;
> > }
> > @@ -1096,18 +1120,33 @@ static int __xsk_generic_xmit(struct sock *sk)
> > goto out;
> > }
> >
> > - sent_frame = true;
> > xs->skb = NULL;
> > }
> >
> > if (xskq_has_descs(xs->tx)) {
> > + bool drain = xs->skb || xs->drain_cont || xp_mb_desc(&desc);
> > +
> > + err = xsk_cq_reserve_locked(xs->pool);
> > + if (err) {
> > + xs->tx->invalid_descs--;
> > + if (xs->skb)
> > + xsk_drop_skb(xs->skb);
> > + xs->drain_cont = drain;
> > + err = -EAGAIN;
> > + goto out;
> > + }
> > +
> > if (xs->skb)
> > xsk_drop_skb(xs->skb);
> > +
> > + xsk_cq_submit_addr_single_locked(xs->pool, &desc);
> > +
> > xskq_cons_release(xs->tx);
> > + xs->drain_cont = xp_mb_desc(&desc);
> > }
> >
> > out:
> > - if (sent_frame)
> > + if (xs->tx->cached_cons != cached_cons)
>
> Sashiko says:
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does this change to the
> release condition expose a pre-existing issue where rolling back a partial
> multi-buffer packet corrupts the Tx ring?
>
> During multi-buffer packet assembly, if the max_batch limit is reached,
> __xsk_generic_xmit() exits and commits the partially consumed descriptors
> to ring->consumer. User-space observes this and may overwrite those slots.
>
> On the next call to __xsk_generic_xmit(), packet assembly resumes. If
> __dev_direct_xmit() subsequently returns NETDEV_TX_BUSY, the code calls
> xskq_cons_cancel_n() to rewind the local cached_cons by the full packet
> length:
>
> if (err == NETDEV_TX_BUSY) {
> /* Tell MAC device to consume more and retry later. */
> xskq_cons_cancel_n(xs->tx, xsk_get_num_desc(skb));
> ...
>
> Because cached_cons is rewound by the full length (including fragments
> consumed in the previous call), it becomes smaller than the cached_cons
> saved at the start of this current call.
>
> Will this new condition (xs->tx->cached_cons != cached_cons) then evaluate
> to true and commit the rewound index to the globally visible ring->consumer?
>
> If ring->consumer jumps backwards, wouldn't the kernel fetch descriptors
> that user-space may have already overwritten with new data upon retry?
>
> Maciej says:
>
> So generic xmit is still not bullet-proof, sigh. It's a problem that was
> present even when `sent_frame` based consumer pointer update was used, so
> sashiko correctly classified it as pre-existing issue.
>
> I think this can be addressed after current set lands, as no new bugs are
> introduced and seems it got acks from Stan and Jason.
I do agree. Don't bother iterating the series, IMHO.
Thanks,
Jason
>
> > __xsk_tx_release(xs);
> >
> > mutex_unlock(&xs->mutex);
> > --
> > 2.43.0
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 net 2/6] xsk: drain continuation descs after overflow in xsk_build_skb()
2026-07-19 13:56 ` [PATCH v4 net 2/6] xsk: drain continuation descs after overflow in xsk_build_skb() Maciej Fijalkowski
2026-07-20 13:58 ` sashiko-bot
2026-07-21 18:04 ` Maciej Fijalkowski
@ 2026-07-23 16:29 ` Jakub Kicinski
2 siblings, 0 replies; 16+ messages in thread
From: Jakub Kicinski @ 2026-07-23 16:29 UTC (permalink / raw)
To: maciej.fijalkowski
Cc: Jakub Kicinski, netdev, bpf, magnus.karlsson, stfomichev, pabeni,
horms, bjorn, kerneljasonxing, kernelxing
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
xsk: drain continuation descs after overflow in xsk_build_skb()
This patch fixes the AF_XDP generic xmit path so that when a
multi-buffer packet exceeds MAX_SKB_FRAGS or contains an invalid
descriptor, the remaining continuation descriptors are drained onto
the completion ring instead of being silently lost. A new
xdp_sock::drain_cont flag tracks whether draining is in progress
across sendmsg invocations.
Link: https://lore.kernel.org/all/20260425041726.85FB3C2BCB2@smtp.kernel.org/
> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> index ebac60a3d8a1..8b51876efbed 100644
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
> @@ -80,6 +80,7 @@ struct xdp_sock {
> * call of __xsk_generic_xmit().
> */
> struct sk_buff *skb;
> + bool drain_cont;
[Low]
Would it be worth documenting drain_cont here in the same style as the
preceding skb field? The semantics are non-trivial: the flag persists
across sendmsg invocations, only clears when a non-CONTD descriptor is
consumed, and can be transiently overwritten in the post-loop error
path with the composed expression:
bool drain = xs->skb || xs->drain_cont || xp_mb_desc(&desc);
Today those semantics can only be reconstructed by reading the four
separate assignment sites in net/xdp/xsk.c. The options field is
also user-influenceable (an invalid descriptor can still carry
XDP_PKT_CONTD), which is worth calling out for future readers.
>
> struct list_head map_list;
> /* Protects map_list */
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index a7a83dc4546a..12a845d012f6 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
[ ... ]
> @@ -1063,11 +1077,21 @@ static int __xsk_generic_xmit(struct sock *sk)
> goto out;
> }
>
> + if (unlikely(xs->drain_cont)) {
> + xsk_cq_submit_addr_single_locked(xs->pool, &desc);
> + xs->tx->invalid_descs++;
> + xskq_cons_release(xs->tx);
> + xs->drain_cont = xp_mb_desc(&desc);
> + continue;
> + }
> +
> skb = xsk_build_skb(xs, &desc);
> if (IS_ERR(skb)) {
> err = PTR_ERR(skb);
> if (err != -EOVERFLOW)
> goto out;
> + if (xp_mb_desc(&desc))
> + xs->drain_cont = true;
[Low]
Is the xp_mb_desc(&desc) check here ever false in practice? Looking at
xsk_build_skb() in net/xdp/xsk.c, the only place -EOVERFLOW is returned
is:
if (unlikely(nr_frags == (MAX_SKB_FRAGS - 1) && xp_mb_desc(desc)))
return ERR_PTR(-EOVERFLOW);
so reaching this point already implies xp_mb_desc(&desc) is true, and
the conditional appears to be a tautology.
Would it read more clearly as an unconditional assignment with a
comment noting the invariant, or alternatively a comment explaining
that a future non-CONTD -EOVERFLOW return would also want draining?
As written, adding a new -EOVERFLOW return site could silently
bypass the drain machinery.
> err = 0;
> continue;
> }
> @@ -1096,18 +1120,33 @@ static int __xsk_generic_xmit(struct sock *sk)
> goto out;
> }
>
> - sent_frame = true;
> xs->skb = NULL;
> }
>
> if (xskq_has_descs(xs->tx)) {
> + bool drain = xs->skb || xs->drain_cont || xp_mb_desc(&desc);
> +
> + err = xsk_cq_reserve_locked(xs->pool);
> + if (err) {
> + xs->tx->invalid_descs--;
[Low]
Can this decrement of xs->tx->invalid_descs be made less fragile?
The value being cancelled here appears to be the increment performed
inside xskq_cons_is_valid_desc() as called from
xskq_cons_peek_desc(), which incremented invalid_descs before
returning false and dropping out of the while loop. Nothing at the
decrement site documents this cross-function dependency.
invalid_descs is a u64 exposed to userspace through XDP_STATISTICS,
so if the increment location is ever refactored, or if a future path
lets xskq_has_descs() be true here without having gone through a
validity-check failure, this decrement will underflow the counter to
~2^64-1.
Would a short comment naming the paired increment site, or an explicit
save/restore around the peek, make the invariant more robust to future
changes?
> + if (xs->skb)
> + xsk_drop_skb(xs->skb);
> + xs->drain_cont = drain;
> + err = -EAGAIN;
> + goto out;
> + }
> +
> if (xs->skb)
> xsk_drop_skb(xs->skb);
> +
> + xsk_cq_submit_addr_single_locked(xs->pool, &desc);
> +
> xskq_cons_release(xs->tx);
> + xs->drain_cont = xp_mb_desc(&desc);
> }
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 net 3/6] xsk: provide sufficient space in pool->tx_descs
2026-07-19 13:56 [PATCH v4 net 0/6] xsk: fix AF_XDP multi-buffer Tx descriptor reclaim Maciej Fijalkowski
2026-07-19 13:56 ` [PATCH v4 net 1/6] xsk: fix buffer leak in xsk_drop_skb() for AF_XDP multi-buffer Tx Maciej Fijalkowski
2026-07-19 13:56 ` [PATCH v4 net 2/6] xsk: drain continuation descs after overflow in xsk_build_skb() Maciej Fijalkowski
@ 2026-07-19 13:56 ` Maciej Fijalkowski
2026-07-20 13:58 ` sashiko-bot
2026-07-19 13:56 ` [PATCH v4 net 4/6] xsk: reclaim invalid Tx descriptors in ZC batch path Maciej Fijalkowski
` (4 subsequent siblings)
7 siblings, 1 reply; 16+ messages in thread
From: Maciej Fijalkowski @ 2026-07-19 13:56 UTC (permalink / raw)
To: netdev
Cc: bpf, magnus.karlsson, stfomichev, kuba, pabeni, horms, bjorn,
kerneljasonxing, Maciej Fijalkowski, Jason Xing
The temporary Tx descriptor array in an XSK buffer pool is currently
sized from the Tx ring of the socket that creates the pool.
This is insufficient for shared-UMEM Tx. A later socket may have a
larger Tx ring and submit a valid multi-buffer packet containing more
descriptors than the first socket's ring, while still remaining within
the device's xdp_zc_max_segs limit.
A packet-framed batch parser bounded by the temporary array cannot reach
the end-of-packet descriptor in that case. It leaves the packet on the
Tx ring and encounters the same packet on every subsequent attempt,
stalling Tx processing for that socket.
Size the temporary descriptor array to the larger of the first Tx ring
and the device's xdp_zc_max_segs capability. This keeps the array large
enough to inspect one maximum-sized valid packet. Larger shared Tx rings
do not require further resizing, as they can be processed over multiple
batches.
Following commit will actually address the data path side.
Fixes: d5581966040f ("xsk: support ZC Tx multi-buffer in batch API")
Reviewed-by: Jason Xing <kernelxing@tencent.com>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
include/net/xsk_buff_pool.h | 6 ++++--
net/xdp/xsk.c | 10 +++++++---
net/xdp/xsk_buff_pool.c | 12 ++++++++----
3 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
index ccb3b350001f..f5e737a83055 100644
--- a/include/net/xsk_buff_pool.h
+++ b/include/net/xsk_buff_pool.h
@@ -102,12 +102,14 @@ struct xsk_buff_pool {
/* AF_XDP core. */
struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
- struct xdp_umem *umem);
+ struct xdp_umem *umem,
+ u32 max_segs);
int xp_assign_dev(struct xsk_buff_pool *pool, struct net_device *dev,
u16 queue_id, u16 flags);
int xp_assign_dev_shared(struct xsk_buff_pool *pool, struct xdp_sock *umem_xs,
struct net_device *dev, u16 queue_id);
-int xp_alloc_tx_descs(struct xsk_buff_pool *pool, struct xdp_sock *xs);
+int xp_alloc_tx_descs(struct xsk_buff_pool *pool, struct xdp_sock *xs,
+ u32 max_segs);
void xp_destroy(struct xsk_buff_pool *pool);
void xp_get_pool(struct xsk_buff_pool *pool);
bool xp_put_pool(struct xsk_buff_pool *pool);
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 12a845d012f6..091792d1d82d 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -1525,7 +1525,8 @@ static int xsk_bind(struct socket *sock, struct sockaddr_unsized *addr, int addr
* and/or device.
*/
xs->pool = xp_create_and_assign_umem(xs,
- umem_xs->umem);
+ umem_xs->umem,
+ dev->xdp_zc_max_segs);
if (!xs->pool) {
err = -ENOMEM;
sockfd_put(sock);
@@ -1557,7 +1558,8 @@ static int xsk_bind(struct socket *sock, struct sockaddr_unsized *addr, int addr
* utilizes
*/
if (xs->tx && !xs->pool->tx_descs) {
- err = xp_alloc_tx_descs(xs->pool, xs);
+ err = xp_alloc_tx_descs(xs->pool, xs,
+ dev->xdp_zc_max_segs);
if (err) {
xp_put_pool(xs->pool);
xs->pool = NULL;
@@ -1575,7 +1577,9 @@ static int xsk_bind(struct socket *sock, struct sockaddr_unsized *addr, int addr
goto out_unlock;
} else {
/* This xsk has its own umem. */
- xs->pool = xp_create_and_assign_umem(xs, xs->umem);
+ xs->pool = xp_create_and_assign_umem(xs, xs->umem,
+ dev->xdp_zc_max_segs);
+
if (!xs->pool) {
err = -ENOMEM;
goto out_unlock;
diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index 1f28a9641571..12c9fb29af05 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -42,9 +42,12 @@ void xp_destroy(struct xsk_buff_pool *pool)
kvfree(pool);
}
-int xp_alloc_tx_descs(struct xsk_buff_pool *pool, struct xdp_sock *xs)
+int xp_alloc_tx_descs(struct xsk_buff_pool *pool, struct xdp_sock *xs,
+ u32 max_segs)
{
- pool->tx_descs = kvzalloc_objs(*pool->tx_descs, xs->tx->nentries);
+ u32 nentries = max(xs->tx->nentries, max_segs);
+
+ pool->tx_descs = kvzalloc_objs(*pool->tx_descs, nentries);
if (!pool->tx_descs)
return -ENOMEM;
@@ -52,7 +55,8 @@ int xp_alloc_tx_descs(struct xsk_buff_pool *pool, struct xdp_sock *xs)
}
struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
- struct xdp_umem *umem)
+ struct xdp_umem *umem,
+ u32 max_segs)
{
bool unaligned = umem->flags & XDP_UMEM_UNALIGNED_CHUNK_FLAG;
struct xsk_buff_pool *pool;
@@ -69,7 +73,7 @@ struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
goto out;
if (xs->tx)
- if (xp_alloc_tx_descs(pool, xs))
+ if (xp_alloc_tx_descs(pool, xs, max_segs))
goto out;
pool->chunk_mask = ~((u64)umem->chunk_size - 1);
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 net 3/6] xsk: provide sufficient space in pool->tx_descs
2026-07-19 13:56 ` [PATCH v4 net 3/6] xsk: provide sufficient space in pool->tx_descs Maciej Fijalkowski
@ 2026-07-20 13:58 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-07-20 13:58 UTC (permalink / raw)
To: Maciej Fijalkowski; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Heap Buffer Overflow in batched Tx descriptor API when a shared XDP socket has a larger Tx ring.
--
--- Patch [3]: [PATCH v4 net 3/6] xsk: provide sufficient space in pool->tx_descs ---
commit 16fadec5330bdab35c81e060269bbbba543bb7cd
Author: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
xsk: provide sufficient space in pool->tx_descs
The temporary Tx descriptor array in an XSK buffer pool is sized from the Tx
ring of the socket that creates the pool. This commit sizes the temporary
descriptor array to the larger of the first Tx ring and the device's
xdp_zc_max_segs capability to prevent stalls in batch parsing for shared-UMEM.
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 12a845d012f6f..091792d1d82de 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
[ ... ]
> @@ -1557,7 +1558,8 @@ static int xsk_bind(struct socket *sock, struct sockaddr_unsized *addr, int addr
> * utilizes
> */
> if (xs->tx && !xs->pool->tx_descs) {
> - err = xp_alloc_tx_descs(xs->pool, xs);
> + err = xp_alloc_tx_descs(xs->pool, xs,
> + dev->xdp_zc_max_segs);
[Severity: Critical]
This is a pre-existing issue, but does this logic leave the system vulnerable
to a heap buffer overflow?
When a primary socket is configured with a small Tx ring, pool->tx_descs is
allocated with a small capacity. If a secondary shared-UMEM socket is then
bound with a much larger Tx ring, this check skips reallocating or resizing
pool->tx_descs.
Later, during batch reading, xsk_tx_peek_release_desc_batch() can overflow
this array:
net/xdp/xsk.c:xsk_tx_peek_release_desc_batch()
...
nb_pkts = xskq_cons_read_desc_batch(xs->tx, pool, nb_pkts);
...
Since nb_pkts is not clamped to pool->tx_descs_nentries, the batch size
can exceed the allocated capacity. This leads to an out-of-bounds write
when xskq_cons_read_desc_batch() copies descriptors:
net/xdp/xsk_queue.h:xskq_cons_read_desc_batch()
...
descs[nb_entries] = ring->desc[idx];
...
The same out-of-bounds write can occur in the fallback path:
net/xdp/xsk.c:xsk_tx_peek_release_fallback()
...
while (nb_pkts < max_entries && xsk_tx_peek_desc(pool, &descs[nb_pkts]))
nb_pkts++;
...
Since max_entries isn't clamped to the actual allocated capacity, couldn't
this corrupt adjacent kernel memory?
> if (err) {
> xp_put_pool(xs->pool);
> xs->pool = NULL;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260719135609.147823-1-maciej.fijalkowski@intel.com?part=3
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 net 4/6] xsk: reclaim invalid Tx descriptors in ZC batch path
2026-07-19 13:56 [PATCH v4 net 0/6] xsk: fix AF_XDP multi-buffer Tx descriptor reclaim Maciej Fijalkowski
` (2 preceding siblings ...)
2026-07-19 13:56 ` [PATCH v4 net 3/6] xsk: provide sufficient space in pool->tx_descs Maciej Fijalkowski
@ 2026-07-19 13:56 ` Maciej Fijalkowski
2026-07-23 16:29 ` Jakub Kicinski
2026-07-19 13:56 ` [PATCH v4 net 5/6] selftests/xsk: fix too-many-frags multi-buffer Tx test Maciej Fijalkowski
` (3 subsequent siblings)
7 siblings, 1 reply; 16+ messages in thread
From: Maciej Fijalkowski @ 2026-07-19 13:56 UTC (permalink / raw)
To: netdev
Cc: bpf, magnus.karlsson, stfomichev, kuba, pabeni, horms, bjorn,
kerneljasonxing, Maciej Fijalkowski, Jason Xing
The zero-copy Tx batch parser stops when it encounters an invalid
descriptor. If this happens after one or more continuation descriptors,
the Tx consumer can be advanced past fragments that are neither submitted
to the driver nor returned to userspace through the completion ring.
A similar problem occurs when a packet exceeds xdp_zc_max_segs. The
descriptors consumed up to the limit are released without completion, and
the remaining continuation descriptors can subsequently be interpreted
as the beginning of another packet.
Parse Tx batches in packet units and distinguish descriptors belonging to
complete valid packets from descriptors consumed while draining an
invalid or oversized packet. Return the former to the driver and append
the latter to the CQ address area so userspace can reclaim their UMEM
frames.
Treat a standalone invalid descriptor as a one-descriptor reclaim-only
packet. Advancing the Tx-ring consumer releases the ring slot, but does
not by itself return ownership of the referenced UMEM frame to userspace.
Once draining starts, continue until the packet's end-of-packet
descriptor is consumed. Preserve the drain state on the socket when EOP
has not yet been supplied, so draining can continue during a later call.
Leave incomplete but otherwise valid packets on the Tx ring.
Shared-UMEM pools using multi-buffer Tx also need packet-framed parsing.
Walk their Tx sockets one packet at a time, preserving the existing
per-socket fairness scheme, instead of using the legacy one-descriptor
fallback. Keep that fallback for shared pools that do not use
multi-buffer Tx. Since the drain state is maintained per socket and both
the singular and shared paths can resume an interrupted drain, changing
the socket list from singular to shared requires no special bind-time
transition.
CQ entries are positional, and drivers may complete only part of the Tx
work returned by xsk_tx_peek_release_desc_batch(). Therefore, reclaim-only
entries cannot be published immediately when earlier driver-visible
descriptors are still outstanding.
Track the number of driver-visible CQ entries preceding the reclaim
entries. Let xsk_tx_completed() publish partial hardware Tx completions,
and publish the reclaim entries only after every earlier Tx descriptor
has completed. Complete a reclaim-only batch immediately when there is no
driver-visible work in front of it, and prevent another Tx batch from
being appended while reclaim entries remain pending.
Also cap batch processing by the size of the pool's temporary descriptor
array, as Tx rings belonging to sockets sharing a UMEM may have different
sizes.
This ensures that every invalid Tx descriptor consumed by the ZC batch
path is either submitted to the driver as part of a valid packet or
returned to userspace without violating CQ completion ordering.
Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")
Reviewed-by: Jason Xing <kernelxing@tencent.com>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
Documentation/networking/af_xdp.rst | 54 ++++----
include/net/xsk_buff_pool.h | 3 +
net/xdp/xsk.c | 187 +++++++++++++++++++++++++---
net/xdp/xsk_buff_pool.c | 1 +
net/xdp/xsk_queue.h | 65 +++++++---
5 files changed, 248 insertions(+), 62 deletions(-)
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
@@ -43,12 +43,13 @@ UMEM also has two rings: the FILL ring and the COMPLETION ring. The
FILL ring is used by the application to send down addr for the kernel
to fill in with RX packet data. References to these frames will then
appear in the RX ring once each packet has been received. The
-COMPLETION ring, on the other hand, contains frame addr that the
-kernel has transmitted completely and can now be used again by user
-space, for either TX or RX. Thus, the frame addrs appearing in the
-COMPLETION ring are addrs that were previously transmitted using the
-TX ring. In summary, the RX and FILL rings are used for the RX path
-and the TX and COMPLETION rings are used for the TX path.
+COMPLETION ring, on the other hand, contains frame addresses from Tx
+descriptors that the kernel has finished processing and that can now be
+used again by user space, for either Tx or Rx. This includes frames whose
+transmission has completed as well as frames referenced by invalid Tx
+descriptors rejected by the kernel. A completion therefore returns
+ownership of a frame to user space, but does not by itself guarantee that
+the packet was successfully transmitted.
The socket is then finally bound with a bind() call to a device and a
specific queue id on that device, and it is not until bind is
@@ -169,14 +170,15 @@ chunks mode, then the incoming addr will be left untouched.
UMEM Completion Ring
~~~~~~~~~~~~~~~~~~~~
-The COMPLETION Ring is used transfer ownership of UMEM frames from
+The COMPLETION Ring is used to transfer ownership of UMEM frames from
kernel-space to user-space. Just like the FILL ring, UMEM indices are
-used.
-
-Frames passed from the kernel to user-space are frames that has been
-sent (TX ring) and can be used by user-space again.
-
-The user application consumes UMEM addrs from this ring.
+used. Frames passed from the kernel to user-space are frames referenced
+by Tx descriptors that the kernel has finished processing and can be
+used by user-space again. This includes both frames whose transmission
+has completed and frames referenced by invalid Tx descriptors that were
+rejected and reclaimed by the kernel. A completion entry does not
+guarantee successful packet transmission. The user application consumes
+UMEM addrs from this ring.
RX Ring
@@ -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.
* Zero length descriptors are treated as invalid descriptors.
* 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.
* For zero-copy mode, the limit is up to what the NIC HW
supports. Usually at least five on the NICs we have checked. We
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;
u8 tx_metadata_len; /* inherited from umem */
u8 cached_need_wakeup;
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);
@@ -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;
+}
+
+static struct xsk_tx_batch
+xsk_tx_peek_release_shared_desc_batch(struct xsk_buff_pool *pool, u32 max_descs)
+{
+ u32 cq_descs_before, cq_descs_after;
+ struct xsk_tx_batch sum_batch = {};
+ bool budget_exhausted;
+ u32 per_socket_budget;
+ struct xdp_sock *xs;
+
+ /* The fairness quota must allow one maximum-sized valid packet. */
+ per_socket_budget = max_t(u32, MAX_PER_SOCKET_BUDGET,
+ pool->xdp_zc_max_segs);
+
+again:
+ budget_exhausted = false;
+ cq_descs_before = xsk_tx_batch_cq_descs(&sum_batch);
+ list_for_each_entry_rcu(xs, &pool->xsk_tx_list, tx_list) {
+ u32 budget, budget_left, offset, remaining, used;
+ struct xsk_tx_batch curr_batch;
+
+ /* Once reclaim-only descriptors have been appended to the CQ
+ * address area, do not append driver-visible Tx descriptors
+ * from another socket after them. xsk_tx_completed() relies on
+ * all driver-visible descriptors preceding all reclaim-only
+ * descriptors in CQ order.
+ */
+ if (sum_batch.reclaim_descs)
+ break;
+
+ /* be gentle when playing with pool->tx_descs */
+ offset = xsk_tx_batch_cq_descs(&sum_batch);
+ if (offset >= max_descs)
+ break;
+
+ if (xs->tx_budget_spent >= per_socket_budget) {
+ if (xskq_cons_nb_entries(xs->tx, 1))
+ budget_exhausted = true;
+ continue;
+ }
+
+ budget_left = per_socket_budget - xs->tx_budget_spent;
+ remaining = max_descs - offset;
+ budget = min(remaining, budget_left);
+
+ curr_batch = __xsk_tx_peek_release_desc_batch(pool, xs,
+ pool->tx_descs + offset,
+ budget);
+ used = xsk_tx_batch_cq_descs(&curr_batch);
+ if (!used) {
+ if (curr_batch.budget_limited && budget_left < remaining)
+ budget_exhausted = true;
+ continue;
+ }
+
+ xs->tx_budget_spent += used;
+ sum_batch.tx_descs += curr_batch.tx_descs;
+ sum_batch.reclaim_descs = curr_batch.reclaim_descs;
+ }
+
+ cq_descs_after = xsk_tx_batch_cq_descs(&sum_batch);
+
+ if (sum_batch.reclaim_descs || cq_descs_after >= max_descs)
+ return sum_batch;
+
+ /* Continue filling the batch while this pass made progress */
+ if (cq_descs_before != cq_descs_after)
+ goto again;
+
+ if (!budget_exhausted)
+ return sum_batch;
+
+ list_for_each_entry_rcu(xs, &pool->xsk_tx_list, tx_list)
+ xs->tx_budget_spent = 0;
+ goto again;
+}
+
u32 xsk_tx_peek_release_desc_batch(struct xsk_buff_pool *pool, u32 nb_pkts)
{
+ struct xsk_tx_batch batch = {};
struct xdp_sock *xs;
+ bool umem_shared;
rcu_read_lock();
- if (!list_is_singular(&pool->xsk_tx_list)) {
- /* Fallback to the non-batched version */
- rcu_read_unlock();
- return xsk_tx_peek_release_fallback(pool, nb_pkts);
- }
+ if (unlikely(READ_ONCE(pool->reclaim_descs)))
+ goto out;
- xs = list_first_or_null_rcu(&pool->xsk_tx_list, struct xdp_sock, tx_list);
- if (!xs) {
- nb_pkts = 0;
+ xs = list_first_or_null_rcu(&pool->xsk_tx_list, struct xdp_sock,
+ tx_list);
+ if (!xs)
goto out;
- }
- nb_pkts = xskq_cons_nb_entries(xs->tx, nb_pkts);
+ nb_pkts = min(nb_pkts, pool->tx_descs_nentries);
+ if (!nb_pkts)
+ goto out;
+
+ umem_shared = !list_is_singular(&pool->xsk_tx_list);
+
+ if (umem_shared && !(pool->umem->flags & XDP_UMEM_SG_FLAG)) {
+ rcu_read_unlock();
+ return xsk_tx_peek_release_fallback(pool, nb_pkts);
+ }
/* This is the backpressure mechanism for the Tx path. Try to
* reserve space in the completion queue for all packets, but
@@ -603,19 +753,16 @@ u32 xsk_tx_peek_release_desc_batch(struct xsk_buff_pool *pool, u32 nb_pkts)
if (!nb_pkts)
goto out;
- nb_pkts = xskq_cons_read_desc_batch(xs->tx, pool, nb_pkts);
- if (!nb_pkts) {
- xs->tx->queue_empty_descs++;
- goto out;
- }
-
- __xskq_cons_release(xs->tx);
- xskq_prod_write_addr_batch(pool->cq, pool->tx_descs, nb_pkts);
- xs->sk.sk_write_space(&xs->sk);
+ 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);
out:
rcu_read_unlock();
- return nb_pkts;
+ return batch.tx_descs;
}
EXPORT_SYMBOL(xsk_tx_peek_release_desc_batch);
diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index 12c9fb29af05..a4089480b22b 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -51,6 +51,7 @@ int xp_alloc_tx_descs(struct xsk_buff_pool *pool, struct xdp_sock *xs,
if (!pool->tx_descs)
return -ENOMEM;
+ pool->tx_descs_nentries = nentries;
return 0;
}
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
@@ -58,6 +58,17 @@ struct parsed_desc {
u32 valid;
};
+struct xsk_tx_batch {
+ u32 tx_descs;
+ u32 reclaim_descs;
+ bool budget_limited;
+};
+
+static inline u32 xsk_tx_batch_cq_descs(const struct xsk_tx_batch *batch)
+{
+ return batch->tx_descs + batch->reclaim_descs;
+}
+
/* The structure of the shared state of the rings are a simple
* circular buffer, as outlined in
* Documentation/core-api/circular-buffers.rst. For the Rx and
@@ -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)
{
- u32 cached_cons = q->cached_cons, nb_entries = 0;
- struct xdp_desc *descs = pool->tx_descs;
- u32 total_descs = 0, nr_frags = 0;
+ bool drain = READ_ONCE(xs->drain_cont);
+ u32 cached_cons, nb_entries = 0;
+ struct xsk_tx_batch batch = {};
+ struct xsk_queue *q = xs->tx;
+ u32 nr_frags = 0;
+
+ cached_cons = q->cached_cons;
- /* track first entry, if stumble upon *any* invalid descriptor, rewind
- * current packet that consists of frags and stop the processing
- */
while (cached_cons != q->cached_prod && nb_entries < max) {
struct xdp_rxtx_ring *ring = (struct xdp_rxtx_ring *)q->ring;
u32 idx = cached_cons & q->ring_mask;
@@ -283,25 +295,42 @@ u32 xskq_cons_read_desc_batch(struct xsk_queue *q, struct xsk_buff_pool *pool,
cached_cons++;
parse_desc(q, pool, &descs[nb_entries], &parsed);
if (unlikely(!parsed.valid))
- break;
+ drain = true;
+
+ nr_frags++;
+ nb_entries++;
if (likely(!parsed.mb)) {
- total_descs += (nr_frags + 1);
- nr_frags = 0;
- } else {
- nr_frags++;
- if (nr_frags == pool->xdp_zc_max_segs) {
+ if (unlikely(drain)) {
+ batch.reclaim_descs = nr_frags;
+ WRITE_ONCE(xs->drain_cont, false);
nr_frags = 0;
break;
}
+
+ batch.tx_descs += nr_frags;
+ nr_frags = 0;
+ continue;
+ }
+
+ if (nr_frags == pool->xdp_zc_max_segs)
+ drain = true;
+ }
+
+ if (nr_frags) {
+ if (drain) {
+ batch.reclaim_descs = nr_frags;
+ WRITE_ONCE(xs->drain_cont, true);
+ } else {
+ if (nb_entries == max)
+ batch.budget_limited = true;
+ cached_cons -= nr_frags;
}
- nb_entries++;
}
- cached_cons -= nr_frags;
/* Release valid plus any invalid entries */
xskq_cons_release_n(q, cached_cons - q->cached_cons);
- return total_descs;
+ return batch;
}
/* Functions for consumers */
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 net 4/6] xsk: reclaim invalid Tx descriptors in ZC batch path
2026-07-19 13:56 ` [PATCH v4 net 4/6] xsk: reclaim invalid Tx descriptors in ZC batch path Maciej Fijalkowski
@ 2026-07-23 16:29 ` Jakub Kicinski
0 siblings, 0 replies; 16+ messages in thread
From: Jakub Kicinski @ 2026-07-23 16:29 UTC (permalink / raw)
To: maciej.fijalkowski
Cc: Jakub Kicinski, netdev, bpf, magnus.karlsson, stfomichev, pabeni,
horms, bjorn, kerneljasonxing, kernelxing
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
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?
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.
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?
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?
> 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.
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?
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 net 5/6] selftests/xsk: fix too-many-frags multi-buffer Tx test
2026-07-19 13:56 [PATCH v4 net 0/6] xsk: fix AF_XDP multi-buffer Tx descriptor reclaim Maciej Fijalkowski
` (3 preceding siblings ...)
2026-07-19 13:56 ` [PATCH v4 net 4/6] xsk: reclaim invalid Tx descriptors in ZC batch path Maciej Fijalkowski
@ 2026-07-19 13:56 ` Maciej Fijalkowski
2026-07-19 13:56 ` [PATCH v4 net 6/6] selftests/xsk: account reclaimed invalid Tx descriptors Maciej Fijalkowski
` (2 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Maciej Fijalkowski @ 2026-07-19 13:56 UTC (permalink / raw)
To: netdev
Cc: bpf, magnus.karlsson, stfomichev, kuba, pabeni, horms, bjorn,
kerneljasonxing, Maciej Fijalkowski, Jason Xing
The too-many-frags test describes a packet that is valid from the Tx
ring ownership point of view, but invalid for transmission because it
exceeds the supported number of fragments.
Keep the generated Tx descriptors valid so that __send_pkts() accounts
them as outstanding descriptors that must be reclaimed through the CQ.
Then mark the corresponding Rx packet invalid so the test still does
not expect the oversized packet to appear on the receive side.
Add a valid synchronization packet after the oversized packet so the
test can verify that the Tx path drains the bad packet and resumes at
the next packet boundary.
Reviewed-by: Jason Xing <kernelxing@tencent.com>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
.../selftests/bpf/prog_tests/test_xsk.c | 24 ++++++++++++-------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.c b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
index 477aedbb01ba..f0e0f3c4f7a3 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_xsk.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
@@ -2270,7 +2270,7 @@ int testapp_too_many_frags(struct test_spec *test)
max_frags += 1;
}
- pkts = calloc(2 * max_frags + 2, sizeof(struct pkt));
+ pkts = calloc(2 * max_frags + 3, sizeof(struct pkt));
if (!pkts)
return TEST_FAILURE;
@@ -2288,24 +2288,30 @@ int testapp_too_many_frags(struct test_spec *test)
}
pkts[max_frags].options = 0;
- /* An invalid packet with the max amount of frags but signals packet
- * continues on the last frag
- */
- for (i = max_frags + 1; i < 2 * max_frags + 1; i++) {
+ /* An invalid packet with the max + 1 amount of frags */
+ for (i = max_frags + 1; i < 2 * max_frags + 2; i++) {
pkts[i].len = MIN_PKT_SIZE;
pkts[i].options = XDP_PKT_CONTD;
- pkts[i].valid = false;
+ pkts[i].valid = true;
}
+ pkts[2 * max_frags + 1].options = 0;
/* Valid packet for synch */
- pkts[2 * max_frags + 1].len = MIN_PKT_SIZE;
- pkts[2 * max_frags + 1].valid = true;
+ pkts[2 * max_frags + 2].len = MIN_PKT_SIZE;
+ pkts[2 * max_frags + 2].valid = true;
- if (pkt_stream_generate_custom(test, pkts, 2 * max_frags + 2)) {
+ if (pkt_stream_generate_custom(test, pkts, 2 * max_frags + 3)) {
free(pkts);
return TEST_FAILURE;
}
+ /* The generated Tx stream must keep the too-big packet valid so that
+ * __send_pkts() accounts its descriptors in outstanding_tx. The Rx
+ * stream, however, must not expect this packet on the wire.
+ */
+ test->ifobj_rx->xsk->pkt_stream->pkts[2].valid = false;
+ test->ifobj_rx->xsk->pkt_stream->nb_valid_entries--;
+
ret = testapp_validate_traffic(test);
free(pkts);
return ret;
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v4 net 6/6] selftests/xsk: account reclaimed invalid Tx descriptors
2026-07-19 13:56 [PATCH v4 net 0/6] xsk: fix AF_XDP multi-buffer Tx descriptor reclaim Maciej Fijalkowski
` (4 preceding siblings ...)
2026-07-19 13:56 ` [PATCH v4 net 5/6] selftests/xsk: fix too-many-frags multi-buffer Tx test Maciej Fijalkowski
@ 2026-07-19 13:56 ` Maciej Fijalkowski
2026-07-23 16:29 ` Jakub Kicinski
2026-07-20 19:30 ` [PATCH v4 net 0/6] xsk: fix AF_XDP multi-buffer Tx descriptor reclaim Stanislav Fomichev
2026-07-23 16:31 ` Jakub Kicinski
7 siblings, 1 reply; 16+ messages in thread
From: Maciej Fijalkowski @ 2026-07-19 13:56 UTC (permalink / raw)
To: netdev
Cc: bpf, magnus.karlsson, stfomichev, kuba, pabeni, horms, bjorn,
kerneljasonxing, Maciej Fijalkowski, Jason Xing
Invalid Tx descriptors are now returned through the completion ring,
regardless of whether they form a standalone packet or belong to an
invalid multi-buffer packet.
The selftests previously counted only descriptors belonging to valid
packets, with a special exception for some invalid multi-buffer packets
in verbatim streams. This undercounts completion entries when a
standalone invalid descriptor or another invalid packet is reclaimed by
the kernel.
Keep valid_pkts as the number of packets expected on the Rx side, but
count every descriptor submitted to the Tx ring in valid_frags, as every
such descriptor is now expected to be returned through the completion
ring.
Make fragment counting in verbatim mode follow the packet boundary
instead of stopping at the first invalid fragment. Update custom stream
generation so an invalid middle fragment terminates the generated Rx
packet while Tx completion accounting still covers the complete invalid
packet.
Also add explicit end fragments after invalid middle descriptors. This
exercises the kernel drain logic and verifies that subsequent valid
packets are not interpreted as continuations of the invalid packet.
Reviewed-by: Jason Xing <kernelxing@tencent.com>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
.../selftests/bpf/prog_tests/test_xsk.c | 26 ++++++++++---------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.c b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
index f0e0f3c4f7a3..4549358cc8c2 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_xsk.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
@@ -427,14 +427,14 @@ static u32 pkt_nb_frags(u32 frame_size, struct pkt_stream *pkt_stream, struct pk
}
/* Search for the end of the packet in verbatim mode */
- if (!pkt_continues(pkt->options) || !pkt->valid)
+ if (!pkt_continues(pkt->options))
return nb_frags;
next_frag = pkt_stream->current_pkt_nb;
pkt++;
while (next_frag++ < pkt_stream->nb_pkts) {
nb_frags++;
- if (!pkt_continues(pkt->options) || !pkt->valid)
+ if (!pkt_continues(pkt->options))
break;
pkt++;
}
@@ -665,11 +665,11 @@ static struct pkt_stream *__pkt_stream_generate_custom(struct ifobject *ifobj, s
if (!frame->valid || !pkt_continues(frame->options))
payload++;
} else {
- if (frame->valid)
+ if (frame->valid) {
len += frame->len;
- if (frame->valid && pkt_continues(frame->options))
- continue;
-
+ if (pkt_continues(frame->options))
+ continue;
+ }
pkt->pkt_nb = pkt_nb;
pkt->len = len;
pkt->valid = frame->valid;
@@ -1250,10 +1250,9 @@ static int __send_pkts(struct ifobject *ifobject, struct xsk_socket_info *xsk,
}
}
- if (pkt && pkt->valid) {
+ if (pkt && pkt->valid)
valid_pkts++;
- valid_frags += nb_frags;
- }
+ valid_frags += nb_frags;
}
pthread_mutex_lock(&pacing_mutex);
@@ -2099,13 +2098,16 @@ int testapp_invalid_desc_mb(struct test_spec *test)
{0, 0, 0, false, 0},
/* Invalid address in the second frame */
{0, XSK_UMEM__LARGE_FRAME_SIZE, 0, false, XDP_PKT_CONTD},
- {umem_sz, XSK_UMEM__LARGE_FRAME_SIZE, 0, false, XDP_PKT_CONTD},
+ {umem_sz * 2, XSK_UMEM__LARGE_FRAME_SIZE, 0, false, XDP_PKT_CONTD},
+ {0, MIN_PKT_SIZE, 0, false, 0},
/* Invalid len in the middle */
{0, XSK_UMEM__LARGE_FRAME_SIZE, 0, false, XDP_PKT_CONTD},
{0, XSK_UMEM__INVALID_FRAME_SIZE, 0, false, XDP_PKT_CONTD},
+ {0, MIN_PKT_SIZE, 0, false, 0},
/* Invalid options in the middle */
{0, XSK_UMEM__LARGE_FRAME_SIZE, 0, false, XDP_PKT_CONTD},
{0, XSK_UMEM__LARGE_FRAME_SIZE, 0, false, XSK_DESC__INVALID_OPTION},
+ {0, MIN_PKT_SIZE, 0, false, 0},
/* Transmit 2 frags, receive 3 */
{0, XSK_UMEM__MAX_FRAME_SIZE, 0, true, XDP_PKT_CONTD},
{0, XSK_UMEM__MAX_FRAME_SIZE, 0, true, 0},
@@ -2117,8 +2119,8 @@ int testapp_invalid_desc_mb(struct test_spec *test)
if (umem->unaligned_mode) {
/* Crossing a chunk boundary allowed */
- pkts[12].valid = true;
- pkts[13].valid = true;
+ pkts[15].valid = true;
+ pkts[16].valid = true;
}
test->mtu = MAX_ETH_JUMBO_SIZE;
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 net 6/6] selftests/xsk: account reclaimed invalid Tx descriptors
2026-07-19 13:56 ` [PATCH v4 net 6/6] selftests/xsk: account reclaimed invalid Tx descriptors Maciej Fijalkowski
@ 2026-07-23 16:29 ` Jakub Kicinski
0 siblings, 0 replies; 16+ messages in thread
From: Jakub Kicinski @ 2026-07-23 16:29 UTC (permalink / raw)
To: maciej.fijalkowski
Cc: Jakub Kicinski, netdev, bpf, magnus.karlsson, stfomichev, pabeni,
horms, bjorn, kerneljasonxing, kernelxing
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
selftests/xsk: account reclaimed invalid Tx descriptors
This patch updates the AF_XDP selftests to count every Tx descriptor
submitted (including invalid ones) in valid_frags so the completion ring
accounting matches the kernel's new behavior of returning invalid Tx
descriptors through the completion ring. It also adjusts verbatim
fragment counting and adds explicit end fragments after invalid middle
descriptors.
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.c b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> index de17dd48f176..38ce6060b8fa 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
[ ... ]
> @@ -665,11 +665,11 @@ static struct pkt_stream *__pkt_stream_generate_custom(struct ifobject *ifobj, s
> if (!frame->valid || !pkt_continues(frame->options))
> payload++;
> } else {
> - if (frame->valid)
> + if (frame->valid) {
> len += frame->len;
> - if (frame->valid && pkt_continues(frame->options))
> - continue;
> -
> + if (pkt_continues(frame->options))
> + continue;
> + }
> pkt->pkt_nb = pkt_nb;
> pkt->len = len;
> pkt->valid = frame->valid;
[Low]
The commit message says "Update custom stream generation so an invalid
middle fragment terminates the generated Rx packet while Tx completion
accounting still covers the complete invalid packet."
Does this hunk actually change any behavior?
Walking through the four (valid, pkt_continues) combinations for the
non-verbatim branch shows the same control flow before and after:
valid=1, continues=1 -> len += frame->len; continue
valid=1, continues=0 -> len += frame->len; fall through to finalize
valid=0, continues=1 -> fall through to finalize
valid=0, continues=0 -> fall through to finalize
The Rx-packet termination described in the commit message appears to be
driven by the pkts[] additions and by removing the !pkt->valid early
exit in pkt_nb_frags(), not by this hunk. Would it be clearer to describe
this change as a pure refactor of __pkt_stream_generate_custom(), or
drop it from this commit?
[ ... ]
> @@ -2099,13 +2098,16 @@ int testapp_invalid_desc_mb(struct test_spec *test)
> {0, 0, 0, false, 0},
> /* Invalid address in the second frame */
> {0, XSK_UMEM__LARGE_FRAME_SIZE, 0, false, XDP_PKT_CONTD},
> - {umem_sz, XSK_UMEM__LARGE_FRAME_SIZE, 0, false, XDP_PKT_CONTD},
> + {umem_sz * 2, XSK_UMEM__LARGE_FRAME_SIZE, 0, false, XDP_PKT_CONTD},
> + {0, MIN_PKT_SIZE, 0, false, 0},
[Low]
Would it be worth calling out the umem_sz -> umem_sz * 2 change in the
commit message?
In shared_umem mode the UMEM size is doubled before registration:
umem_sz *= 2;
so a descriptor address of umem_sz can fall inside the valid UMEM range
for the second socket. Bumping this to umem_sz * 2 ensures the address
is out of range regardless of shared_umem, which is a nice correctness
tweak, but bundling it with the accounting change makes the intent less
obvious and complicates bisection.
> /* Invalid len in the middle */
> {0, XSK_UMEM__LARGE_FRAME_SIZE, 0, false, XDP_PKT_CONTD},
> {0, XSK_UMEM__INVALID_FRAME_SIZE, 0, false, XDP_PKT_CONTD},
> + {0, MIN_PKT_SIZE, 0, false, 0},
> /* Invalid options in the middle */
> {0, XSK_UMEM__LARGE_FRAME_SIZE, 0, false, XDP_PKT_CONTD},
> {0, XSK_UMEM__LARGE_FRAME_SIZE, 0, false, XSK_DESC__INVALID_OPTION},
> + {0, MIN_PKT_SIZE, 0, false, 0},
> /* Transmit 2 frags, receive 3 */
> {0, XSK_UMEM__MAX_FRAME_SIZE, 0, true, XDP_PKT_CONTD},
> {0, XSK_UMEM__MAX_FRAME_SIZE, 0, true, 0},
[ ... ]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 net 0/6] xsk: fix AF_XDP multi-buffer Tx descriptor reclaim
2026-07-19 13:56 [PATCH v4 net 0/6] xsk: fix AF_XDP multi-buffer Tx descriptor reclaim Maciej Fijalkowski
` (5 preceding siblings ...)
2026-07-19 13:56 ` [PATCH v4 net 6/6] selftests/xsk: account reclaimed invalid Tx descriptors Maciej Fijalkowski
@ 2026-07-20 19:30 ` Stanislav Fomichev
2026-07-23 16:31 ` Jakub Kicinski
7 siblings, 0 replies; 16+ messages in thread
From: Stanislav Fomichev @ 2026-07-20 19:30 UTC (permalink / raw)
To: Maciej Fijalkowski
Cc: netdev, bpf, magnus.karlsson, stfomichev, kuba, pabeni, horms,
bjorn, kerneljasonxing
On 07/19, Maciej Fijalkowski wrote:
> v3:
> https://lore.kernel.org/netdev/20260714140722.111645-1-maciej.fijalkowski@intel.com/T/
> v3->v4:
>
> * Return standalone invalid Tx descriptors through the completion ring in
> both the generic and zero-copy Tx paths. Advancing the Tx-ring consumer
> releases only the ring slot; returning the descriptor address through
> the CQ also transfers ownership of the corresponding UMEM frame back to
> userspace.
>
> * Remove xsk_tx_batch::consumed_descs. With standalone invalid descriptors
> now reclaimed through the CQ, every descriptor permanently removed from
> the Tx ring is represented by either tx_descs or reclaim_descs. Use
> their sum for Tx-consumer advancement, shared-UMEM fairness accounting,
> and progress detection.
>
> * Ensure that generic reclaim-only processing publishes the updated
> Tx-ring consumer even when no packet was submitted to the networking
> stack.
>
> * Update the XSK selftests to count every descriptor submitted to the Tx
> ring as an expected CQ entry, while continuing to count only valid
> packets as expected Rx traffic. This covers standalone invalid
> descriptors, invalid multi-buffer packets, oversized packets, and the
> non-verbatim STAT_TX_INVALID tests.
>
> * Update the AF_XDP documentation to describe the completion ring as an
> ownership-transfer mechanism and document that standalone, invalid
> multi-buffer, and oversized Tx packets are reclaimed through the CQ.
>
> * Add Jason's tags
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Kudos for updating the doc with the new expectations! (and I still hope
that we can separately redo the generic tx path)
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 net 0/6] xsk: fix AF_XDP multi-buffer Tx descriptor reclaim
2026-07-19 13:56 [PATCH v4 net 0/6] xsk: fix AF_XDP multi-buffer Tx descriptor reclaim Maciej Fijalkowski
` (6 preceding siblings ...)
2026-07-20 19:30 ` [PATCH v4 net 0/6] xsk: fix AF_XDP multi-buffer Tx descriptor reclaim Stanislav Fomichev
@ 2026-07-23 16:31 ` Jakub Kicinski
7 siblings, 0 replies; 16+ messages in thread
From: Jakub Kicinski @ 2026-07-23 16:31 UTC (permalink / raw)
To: Maciej Fijalkowski
Cc: netdev, bpf, magnus.karlsson, stfomichev, pabeni, horms, bjorn,
kerneljasonxing
On Sun, 19 Jul 2026 15:56:03 +0200 Maciej Fijalkowski wrote:
> Subject: [PATCH v4 net 0/6] xsk: fix AF_XDP multi-buffer Tx descriptor reclaim
Looks like Clashiko found things Sashiko didn't.
LMK if these are real, please, I'll look at one of the other 400
patches still in the queue ;)
^ permalink raw reply [flat|nested] 16+ messages in thread