xsk: fix bugs around xsk skb allocation
Subsystems (4)
v4 cover xsk: fix bugs around xsk skb allocation
From: Jason Xing <[email protected]>
There are rare issues around xsk_build_skb(). Some of them were founded by Sashiko[1][2].
[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://lore.kernel.org/all/[email protected]/
---
v4
1. fix 32-bit arch issue in patch 8 (Alexander && Maciej)
2. add acked-by tags (Stan)
v3
1. use !xs->skb as the indicator of first frag in patch 3 (Stan)
2. move init skb after it's allocated successfully, so patch 4,5,6 have been adjusted accordingly (Stan)
3. do not support 32-bit arch (Stan)
4. add acked-by tags (Stan)
v2
1. add four patches spotted by sashiko to fix buggy pre-existing behavior
2. adjust the order of 8 patches.
Jason Xing (8):
xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
xsk: handle NULL dereference of the skb without frags issue
xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path
xsk: prevent CQ desync when freeing half-built skbs in xsk_build_skb()
xsk: avoid skb leak in XDP_TX_METADATA case
xsk: free the skb when hitting the upper bound MAX_SKB_FRAGS
xsk: fix xsk_addrs slab leak on multi-buffer error path
xsk: fix u64 descriptor address truncation on 32-bit architectures
net/xdp/xsk.c | 65 +++++++++++++++++++++++++++++++---------- net/xdp/xsk_buff_pool.c | 3 ++ 2 files changed, 52 insertions(+), 16 deletions(-)
Signature · 1 line
2.41.3
PATCH v4 1/8 xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
From: Jason Xing <[email protected]>
skb_checksum_help() is a common helper that writes the folded 16-bit checksum back via skb->data + csum_start + csum_offset, i.e. it relies on the skb's linear head and fails (with WARN_ONCE and -EINVAL) when skb_headlen() is 0.
AF_XDP generic xmit takes two very different paths depending on the netdev. Drivers that advertise IFF_TX_SKB_NO_LINEAR (e.g. virtio_net) skip the "copy payload into a linear head" step on purpose as a performance optimisation: xsk_build_skb_zerocopy() only attaches UMEM pages as frags and never calls skb_put(), so skb_headlen() stays 0 for the whole skb. For these skbs there is simply no linear area for skb_checksum_help() to write the csum into - the sw-csum fallback is structurally inapplicable.
The patch tries to catch this and reject the combination with error at setup time. Rejecting at bind() converts this silent per-packet failure into a synchronous, actionable -EOPNOTSUPP at setup time. HW csum and launch_time metadata on IFF_TX_SKB_NO_LINEAR drivers are unaffected because they do not call skb_checksum_help().
Without the patch, every descriptor carrying 'XDP_TX_METADATA | XDP_TXMD_FLAGS_CHECKSUM' produces:
1) a WARN_ONCE "offset (N) >= skb_headlen() (0)" from skb_checksum_help(),
2) sendmsg() returning -EINVAL without consuming the descriptor (invalid_descs is not incremented),
3) a wedged TX ring: __xsk_generic_xmit() does not advance the consumer on non-EOVERFLOW errors, so the next sendmsg() re-reads the same descriptor and re-hits the same WARN until the socket is closed.
--- net/xdp/xsk_buff_pool.c | 3 +++ 1 file changed, 3 insertions(+)
net/xdp/xsk_buff_pool.c +3 -0
@@ -169,6 +169,9 @@ int xp_assign_dev(struct xsk_buff_pool *pool,if (force_zc && force_copy)return -EINVAL ; +if (pool->tx_sw_csum && (netdev->priv_flags &IFF_TX_SKB_NO_LINEAR )) +return -EOPNOTSUPP ; +if (xsk_get_pool_from_qid (netdev, queue_id))return -EBUSY ;
Signature · 1 line
2.41.3
Re: [PATCH net v4 1/8] xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
Sashiko AI review found 1 potential issue(s): - [High] The patch incompletely fixes the WARN_ON_ONCE in skb_checksum_help() triggered by pure-fragment SKBs. It successfully prevents the explicit…
Sashiko AI review found 1 potential issue(s):
- [High] The patch incompletely fixes the WARN_ON_ONCE in skb_checksum_help() triggered by pure-fragment SKBs. It successfully prevents the explicit fallback in xsk_skb_metadata() (which wedged the TX ring), but leaves the implicit generic network stack fallback vulnerable. If a user sets XDP_TXMD_FLAGS_CHECKSUM on an IFF_TX_SKB_NO_LINEAR device and the hardware checksum offload is disabled at runtime (e.g., via ethtool -K eth0 tx off), the core stack's validate_xmit_skb() takes over. Because NETIF_F_SG is allowed to remain active even without checksum features, skb_needs_linearize() evaluates to false, leaving the SKB unlinearized. The SKB is then passed to skb_csum_hwoffload_help() -> skb_checksum_help(), which hits the exact same WARN_ON_ONCE(offset >= skb_headlen(skb)) because skb_headlen is 0. --
commit 86ab0a4900aa60bee0c3df7393a2846b5ff74ccb
Author: Jason Xing <[email protected]>
xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
This commit attempts to fix a warning triggered in skb_checksum_help() by AF_XDP pure-fragment SKBs. It rejects sw-csum UMEM binding for drivers advertising IFF_TX_SKB_NO_LINEAR at setup time.
net/xdp/xsk_buff_pool.c int xp_assign_dev(struct xsk_buff_pool *pool, · line 180
Quoted 11 lines
net/xdp/xsk_buff_pool.c +3
-0@@ -175,6 +175,9 @@ int xp_assign_dev(struct xsk_buff_pool *pool,if (force_zc && force_copy)return -EINVAL ; +if (pool->tx_sw_csum && (netdev->priv_flags &IFF_TX_SKB_NO_LINEAR )) +return -EOPNOTSUPP ; +
Does this still leave a path for the warning in skb_checksum_help() to trigger if hardware checksum offloading is disabled at runtime?
If a user sets XDP_TXMD_FLAGS_CHECKSUM on an IFF_TX_SKB_NO_LINEAR device, and hardware checksum offload is later disabled (e.g., via ethtool -K eth0 tx off), the core stack's validate_xmit_skb() will take over.
Because NETIF_F_SG can remain active even without checksum features, skb_needs_linearize() evaluates to false, leaving the SKB unlinearized.
The SKB is then passed to skb_csum_hwoffload_help() which falls back to skb_checksum_help(). Since the pure-fragment SKB still has a headlen of 0, won't this hit the exact same WARN_ON_ONCE(offset >= skb_headlen(skb)) warning?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [PATCH net v4 1/8] xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
Quoted 38 lines · On Sat, Apr 25, 2026 at 1:40 PM <[email protected]> wrote:
Sashiko AI review found 1 potential issue(s):
- [High] The patch incompletely fixes theWARN_ON_ONCEinskb_checksum_help()triggered by pure-fragment SKBs. It successfully prevents the explicit fallback inxsk_skb_metadata()(which wedged the TX ring), but leaves the implicit generic network stack fallback vulnerable. If a user setsXDP_TXMD_FLAGS_CHECKSUMon anIFF_TX_SKB_NO_LINEARdevice and the hardware checksum offload is disabled at runtime (e.g., viaethtool -K eth0 tx off), the core stack'svalidate_xmit_skb()takes over. BecauseNETIF_F_SGis allowed to remain active even without checksum features,skb_needs_linearize()evaluates to false, leaving the SKB unlinearized. The SKB is then passed toskb_csum_hwoffload_help()->skb_checksum_help(), which hits the exact sameWARN_ON_ONCE(offset >= skb_headlen(skb))becauseskb_headlenis 0. --commit
86ab0a4900aa60bee0c3df7393a2846b5ff74ccb
Author: Jason Xing <[email protected]>xsk: reject sw-csum UMEM binding to
IFF_TX_SKB_NO_LINEARdevicesThis commit attempts to fix a warning triggered in
skb_checksum_help()byAF_XDPpure-fragment SKBs. It rejects sw-csum UMEM binding for drivers advertisingIFF_TX_SKB_NO_LINEARat setup time.Fixes 30c3055f9c0d ("xsk: wrap generic metadata handling onto separate function")Signed-off-by Jason Xing <[email protected]>net/xdp/xsk_buff_pool.c int xp_assign_dev(struct xsk_buff_pool *pool, · line 180
Quoted 11 lines
net/xdp/xsk_buff_pool.c +3
-0@@ -175,6 +175,9 @@ int xp_assign_dev(struct xsk_buff_pool *pool,if (force_zc && force_copy)return -EINVAL ; +if (pool->tx_sw_csum && (netdev->priv_flags &IFF_TX_SKB_NO_LINEAR )) +return -EOPNOTSUPP ; +Does this still leave a path for the warning in
skb_checksum_help()to trigger if hardware checksum offloading is disabled at runtime?If a user sets
XDP_TXMD_FLAGS_CHECKSUMon anIFF_TX_SKB_NO_LINEARdevice, and hardware checksum offload is later disabled (e.g., via ethtool -K eth0 tx off), the core stack'svalidate_xmit_skb()will take over.
Quoted 7 lines
Because
NETIF_F_SGcan remain active even without checksum features,skb_needs_linearize()evaluates to false, leaving the SKB unlinearized.The SKB is then passed to
skb_csum_hwoffload_help()which falls back toskb_checksum_help(). Since the pure-fragment SKB still has a headlen of 0, won't this hit the exact sameWARN_ON_ONCE(offset >= skb_headlen(skb))warning?
This is a new case that happens around sw csum feature. It's not like how I did in this patch (this kind of combination proved to be a no-go from the start. people should be notified of this).
But your concern about the scenario you're describing is right. The problem only happens when the setting of the driver is changed. If hardware doesn't support csum, and resorting to sw csum fails, that will cause the skb drop instance. It's not that particularly strange. People will soon get warned and adjust their code or policy. Maybe in the future we'll be able to add more observability code into xsk to include this case, like adding one drop reason to let admins be aware of it. This is already on my todo-list.
Apparently IMHO it's a bug at all.
Thanks,
Jason
Quoted 3 lines
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Paolo Abeni
maintainerRe: [PATCH net v4 1/8] xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
In reply to [email protected]Quoted 37 lines · On 4/25/26 7:40 AM, [email protected] wrote:
Sashiko AI review found 1 potential issue(s):
- [High] The patch incompletely fixes theWARN_ON_ONCEinskb_checksum_help()triggered by pure-fragment SKBs. It successfully prevents the explicit fallback inxsk_skb_metadata()(which wedged the TX ring), but leaves the implicit generic network stack fallback vulnerable. If a user setsXDP_TXMD_FLAGS_CHECKSUMon anIFF_TX_SKB_NO_LINEARdevice and the hardware checksum offload is disabled at runtime (e.g., viaethtool -K eth0 tx off), the core stack'svalidate_xmit_skb()takes over. BecauseNETIF_F_SGis allowed to remain active even without checksum features,skb_needs_linearize()evaluates to false, leaving the SKB unlinearized. The SKB is then passed toskb_csum_hwoffload_help()->skb_checksum_help(), which hits the exact sameWARN_ON_ONCE(offset >= skb_headlen(skb))becauseskb_headlenis 0. --commit
86ab0a4900aa60bee0c3df7393a2846b5ff74ccb
Author: Jason Xing <[email protected]>xsk: reject sw-csum UMEM binding to
IFF_TX_SKB_NO_LINEARdevicesThis commit attempts to fix a warning triggered in
skb_checksum_help()byAF_XDPpure-fragment SKBs. It rejects sw-csum UMEM binding for drivers advertisingIFF_TX_SKB_NO_LINEARat setup time.Fixes 30c3055f9c0d ("xsk: wrap generic metadata handling onto separate function")Signed-off-by Jason Xing <[email protected]>net/xdp/xsk_buff_pool.c int xp_assign_dev(struct xsk_buff_pool *pool, · line 180
Quoted 11 lines
net/xdp/xsk_buff_pool.c +3
-0@@ -175,6 +175,9 @@ int xp_assign_dev(struct xsk_buff_pool *pool,if (force_zc && force_copy)return -EINVAL ; +if (pool->tx_sw_csum && (netdev->priv_flags &IFF_TX_SKB_NO_LINEAR )) +return -EOPNOTSUPP ; +Does this still leave a path for the warning in
skb_checksum_help()to trigger if hardware checksum offloading is disabled at runtime?If a user sets
XDP_TXMD_FLAGS_CHECKSUMon anIFF_TX_SKB_NO_LINEARdevice, and hardware checksum offload is later disabled (e.g., via ethtool -K eth0 tx off), the core stack'svalidate_xmit_skb()will take over.
Quoted 7 lines
Because
NETIF_F_SGcan remain active even without checksum features,skb_needs_linearize()evaluates to false, leaving the SKB unlinearized.The SKB is then passed to
skb_csum_hwoffload_help()which falls back toskb_checksum_help(). Since the pure-fragment SKB still has a headlen of 0, won't this hit the exact sameWARN_ON_ONCE(offset >= skb_headlen(skb))warning?
My take here is the issue describe above should be handled separately. (And on top of my head I don't see an easy way to address it short of an additional check in the tx path, which I think we want to avoid).
/P
Re: [PATCH net v4 1/8] xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
Quoted 42 lines · Paolo Abeni
Quoted 40 lines · On 4/25/26 7:40 AM, [email protected] wrote:
Sashiko AI review found 1 potential issue(s):
- [High] The patch incompletely fixes theWARN_ON_ONCEinskb_checksum_help()triggered by pure-fragment SKBs. It successfully prevents the explicit fallback inxsk_skb_metadata()(which wedged the TX ring), but leaves the implicit generic network stack fallback vulnerable. If a user setsXDP_TXMD_FLAGS_CHECKSUMon anIFF_TX_SKB_NO_LINEARdevice and the hardware checksum offload is disabled at runtime (e.g., viaethtool -K eth0 tx off), the core stack'svalidate_xmit_skb()takes over. BecauseNETIF_F_SGis allowed to remain active even without checksum features,skb_needs_linearize()evaluates to false, leaving the SKB unlinearized. The SKB is then passed toskb_csum_hwoffload_help()->skb_checksum_help(), which hits the exact sameWARN_ON_ONCE(offset >= skb_headlen(skb))becauseskb_headlenis 0. --commit
86ab0a4900aa60bee0c3df7393a2846b5ff74ccb
Author: Jason Xing <[email protected]>xsk: reject sw-csum UMEM binding to
IFF_TX_SKB_NO_LINEARdevicesThis commit attempts to fix a warning triggered in
skb_checksum_help()byAF_XDPpure-fragment SKBs. It rejects sw-csum UMEM binding for drivers advertisingIFF_TX_SKB_NO_LINEARat setup time.Fixes 30c3055f9c0d ("xsk: wrap generic metadata handling onto separate function")Signed-off-by Jason Xing <[email protected]>net/xdp/xsk_buff_pool.c int xp_assign_dev(struct xsk_buff_pool *pool, · line 180
Quoted 11 lines
net/xdp/xsk_buff_pool.c +3
-0@@ -175,6 +175,9 @@ int xp_assign_dev(struct xsk_buff_pool *pool,if (force_zc && force_copy)return -EINVAL ; +if (pool->tx_sw_csum && (netdev->priv_flags &IFF_TX_SKB_NO_LINEAR )) +return -EOPNOTSUPP ; +Does this still leave a path for the warning in
skb_checksum_help()to trigger if hardware checksum offloading is disabled at runtime?If a user sets
XDP_TXMD_FLAGS_CHECKSUMon anIFF_TX_SKB_NO_LINEARdevice, and hardware checksum offload is later disabled (e.g., via ethtool -K eth0 tx off), the core stack'svalidate_xmit_skb()will take over.Because
NETIF_F_SGcan remain active even without checksum features,skb_needs_linearize()evaluates to false, leaving the SKB unlinearized.
Quoted 8 lines
Quoted 4 lines
The SKB is then passed to
skb_csum_hwoffload_help()which falls back toskb_checksum_help(). Since the pure-fragment SKB still has a headlen of 0, won't this hit the exact sameWARN_ON_ONCE(offset >= skb_headlen(skb))warning?My take here is the issue describe above should be handled separately. (And on top of my head I don't see an easy way to address it short of an additional check in the tx path, which I think we want to avoid).
Agreed. I've already reworded the commit message locally :)
Thanks,
Jason
PATCH v4 2/8 xsk: handle NULL dereference of the skb without frags issue
In reply to Jason XingFrom: Jason Xing <[email protected]>
When a first descriptor (xs->skb == NULL) triggers -EOVERFLOW in xsk_build_skb_zerocopy (e.g., MAX_SKB_FRAGS exceeded), the free_err EOVERFLOW handler unconditionally dereferences xs->skb via xsk_inc_num_desc(xs->skb) and xsk_drop_skb(xs->skb), causing a NULL pointer dereference.
In this series, the skb is already freed by kfree_skb() inside xsk_build_skb_zerocopy for the first-descriptor case, so we only need to do the bookkeeping: cancel the one reserved CQ slot and account for the single invalid descriptor.
Guard the existing xsk_inc_num_desc/xsk_drop_skb calls with an xs->skb check (for the continuation case), and add an else branch for the first-descriptor case that manually cancels the CQ slot and increments invalid_descs by one.
--- net/xdp/xsk.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
net/xdp/xsk.c +8 -3
@@ -893,9 +893,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,kfree_skb (skb);if (err == -EOVERFLOW ) { -/* Drop the packet */ -xsk_inc_num_desc (xs->skb); -xsk_drop_skb (xs->skb); +if (xs->skb) { +/* Drop the packet */ +xsk_inc_num_desc (xs->skb); +xsk_drop_skb (xs->skb); + }else { +xsk_cq_cancel_locked (xs->pool,1 ); + xs->tx->invalid_descs++; + }xskq_cons_release (xs->tx); }else {/* Let application retry */
Signature · 1 line
2.41.3
Simon Horman
reviewerRe: [PATCH net v4 2/8] xsk: handle NULL dereference of the skb without frags issue
From: 'Simon Horman' <[email protected]>
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://netdev-ai.bots.linux.dev/sashiko/
Simon says:
Hi Jason,
I'm providing this for informational purposes. My unedrstanding is that the problem flagged below is not present with the full 8 patches of the series applied.
---
xsk: handle NULL dereference of the skb without frags issue
This change guards the existing xsk_inc_num_desc()/xsk_drop_skb() calls in xsk_build_skb()'s free_err path with an xs->skb check, and adds a new else branch that cancels the reserved CQ slot and bumps invalid_descs when xs->skb is NULL.
Quoted 9 lines
net/xdp/xsk.c +0
-1@@ -973,9 +973,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,kfree_skb (skb);if (err == -EOVERFLOW ) { -/* Drop the packet */
net/xdp/xsk.c static struct sk_buff *xsk_build_skb(struct xdp_sock *xs, · line 983
Quoted 10 lines
net/xdp/xsk.c +8
-2-xsk_inc_num_desc (xs->skb); -xsk_drop_skb (xs->skb); +if (xs->skb) { +/* Drop the packet */ +xsk_inc_num_desc (xs->skb); +xsk_drop_skb (xs->skb); + }else { +xsk_cq_cancel_locked (xs->pool,1 ); + xs->tx->invalid_descs++; + }
The changelog states:
In this series, the skb is already freed by kfree_skb() inside xsk_build_skb_zerocopy for the first-descriptor case, so we only need to do the bookkeeping: cancel the one reserved CQ slot and account for the single invalid descriptor.
Is that accurate for this commit?
Looking at xsk_build_skb_zerocopy() as it stands here, neither of its error returns actually frees the skb:
if (!skb) {
...
skb = sock_alloc_send_skb(&xs->sk, hr, 1, &err);
if (unlikely(!skb))
return ERR_PTR(err);
skb_reserve(skb, hr);
xsk_skb_init_misc(skb, xs, desc->addr);
if (desc->options & XDP_TX_METADATA) {
err = xsk_skb_metadata(skb, buffer, desc, pool, hr);
if (unlikely(err))
return ERR_PTR(err);
}
}
...
for (copied = 0, i = skb_shinfo(skb)->nr_frags; copied < len; i++) {
if (unlikely(i >= MAX_SKB_FRAGS))
return ERR_PTR(-EOVERFLOW);
page = pool->umem->pgs[addr >> PAGE_SHIFT];
get_page(page);
...
skb_fill_page_desc(skb, i, page, offset, copy);
...
}So on the first-descriptor -EOVERFLOW path (xs->skb == NULL, IFF_TX_SKB_NO_LINEAR device), doesn't xsk_build_skb_zerocopy() return with the skb still allocated, up to MAX_SKB_FRAGS pages pinned via get_page() and attached through skb_fill_page_desc(), and an sk_wmem_alloc charge held by xsk_destruct_skb (installed by xsk_skb_init_misc)?
Control then returns to xsk_build_skb():
skb = xsk_build_skb_zerocopy(xs, desc);
if (IS_ERR(skb)) {
err = PTR_ERR(skb);
skb = NULL;
goto free_err;
}
...
free_err:
if (skb && !skb_shinfo(skb)->nr_frags)
kfree_skb(skb);Since the local skb has been set to NULL, the kfree_skb() guard is skipped, and the new else-branch only performs the CQ cancel and invalid_descs bump.
Doesn't that leak the sk_buff, the attached frag pages, and the sk_wmem_alloc charge every time a first descriptor hits the i >= MAX_SKB_FRAGS condition (or the xsk_skb_metadata failure)?
A later patch in the series, "xsk: free the skb when hitting the upper bound MAX_SKB_FRAGS", adds an "if (!xs->skb) kfree_skb(skb);" inside xsk_build_skb_zerocopy() before the -EOVERFLOW return, which seems to confirm the skb is not freed at this commit. Should the changelog's premise be reworked, or the free added here so each patch in the series stands on its own?
Re: [PATCH net v4 2/8] xsk: handle NULL dereference of the skb without frags issue
Quoted 7 lines · Simon Horman
From: 'Simon Horman' <[email protected]>
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://netdev-ai.bots.linux.dev/sashiko/
Quoted 8 lines
Simon says:
Hi Jason,
I'm providing this for informational purposes. My unedrstanding is that the problem flagged below is not present with the full 8 patches of the series applied.
Hi simon,
Thanks for letting me know:)
Yes, it will disappear after applying the whole series.
Quoted 31 lines
---
xsk: handle NULL dereference of the skb without frags issueThis change guards the existing xsk_inc_num_desc()/xsk_drop_skb() calls in xsk_build_skb()'s
free_errpath with anxs->skbcheck, and adds a new else branch that cancels the reserved CQ slot and bumpsinvalid_descswhenxs->skbis NULL.Quoted 9 lines
net/xdp/xsk.c +0
-1@@ -973,9 +973,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,kfree_skb (skb);if (err == -EOVERFLOW ) { -/* Drop the packet */ net/xdp/xsk.c static struct sk_buff *xsk_build_skb(struct xdp_sock *xs, · line 983
Quoted 10 lines
net/xdp/xsk.c +8
-2-xsk_inc_num_desc (xs->skb); -xsk_drop_skb (xs->skb); +if (xs->skb) { +/* Drop the packet */ +xsk_inc_num_desc (xs->skb); +xsk_drop_skb (xs->skb); + }else { +xsk_cq_cancel_locked (xs->pool,1 ); + xs->tx->invalid_descs++; + }The changelog states:
Quoted 6 lines
In this series, the skb is already freed by
kfree_skb()insidexsk_build_skb_zerocopyfor the first-descriptor case, so we only need to do the bookkeeping: cancel the one reserved CQ slot and account for the single invalid descriptor.Is that accurate for this commit?
The commit says "in this series,..." which might confuse the AI reviewer.
Quoted 51 lines
Looking at
xsk_build_skb_zerocopy()as it stands here, neither of its error returns actually frees the skb:if (!skb) { ... skb = sock_alloc_send_skb(&xs->sk, hr, 1, &err); if (unlikely(!skb)) return ERR_PTR(err); skb_reserve(skb, hr); xsk_skb_init_misc(skb, xs, desc->addr); if (desc->options & XDP_TX_METADATA) { err = xsk_skb_metadata(skb, buffer, desc, pool, hr); if (unlikely(err)) return ERR_PTR(err); } } ... for (copied = 0, i = skb_shinfo(skb)->nr_frags; copied < len; i++) { if (unlikely(i >= MAX_SKB_FRAGS)) return ERR_PTR(-EOVERFLOW); page = pool->umem->pgs[addr >> PAGE_SHIFT]; get_page(page); ... skb_fill_page_desc(skb, i, page, offset, copy); ... }So on the first-descriptor
-EOVERFLOWpath (xs->skb== NULL,IFF_TX_SKB_NO_LINEARdevice), doesn'txsk_build_skb_zerocopy()return with the skb still allocated, up toMAX_SKB_FRAGSpages pinned viaget_page()and attached throughskb_fill_page_desc(), and ansk_wmem_alloccharge held byxsk_destruct_skb(installed byxsk_skb_init_misc)?Control then returns to
xsk_build_skb():skb = xsk_build_skb_zerocopy(xs, desc); if (IS_ERR(skb)) { err = PTR_ERR(skb); skb = NULL; goto free_err; } ... free_err: if (skb && !skb_shinfo(skb)->nr_frags) kfree_skb(skb);
Quoted 7 lines
Since the local skb has been set to NULL, the
kfree_skb()guard is skipped, and the new else-branch only performs the CQ cancel andinvalid_descsbump.Doesn't that leak the
sk_buff, the attached frag pages, and thesk_wmem_alloccharge every time a first descriptor hits the i >=MAX_SKB_FRAGScondition (or thexsk_skb_metadatafailure)?
This is the issue that I fixed in the subsequent patches.
Quoted 7 lines
A later patch in the series, "xsk: free the skb when hitting the upper bound
MAX_SKB_FRAGS", adds an "if (!xs->skb)kfree_skb(skb);" insidexsk_build_skb_zerocopy()before the-EOVERFLOWreturn, which seems to confirm the skb is not freed at this commit. Should the changelog's premise be reworked, or the free added here so each patch in the series stands on its own?
It's only about the description of the patch and the whole series IIUC. If a new version is needed, I will send a v5 then :)
Thanks,
Jason
PATCH v4 3/8 xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path
In reply to Jason XingFrom: Jason Xing <[email protected]>
When xsk_build_skb() processes multi-buffer packets in copy mode, the first descriptor stores data into the skb linear area without adding any frags, so nr_frags stays at 0. The caller then sets xs->skb = skb to accumulate subsequent descriptors.
If a continuation descriptor fails (e.g. alloc_page returns NULL with -EAGAIN), we jump to free_err where the condition:
if (skb && !skb_shinfo(skb)->nr_frags)
kfree_skb(skb);evaluates to true because nr_frags is still 0 (the first descriptor used the linear area, not frags). This frees the skb while xs->skb still points to it, creating a dangling pointer. On the next transmit attempt or socket close, xs->skb is dereferenced, causing a use-after-free or double-free.
Fix by using a !xs->skb check to handle first frag situation, ensuring we only free skbs that were freshly allocated in this call (xs->skb is NULL) and never free an in-progress multi-buffer skb that the caller still references.
--- net/xdp/xsk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/xdp/xsk.c +1 -1
@@ -889,7 +889,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,return skb; free_err: -if (skb && !skb_shinfo (skb)->nr_frags) +if (skb && !xs->skb)kfree_skb (skb);if (err == -EOVERFLOW ) {
Signature · 1 line
2.41.3
PATCH v4 4/8 xsk: prevent CQ desync when freeing half-built skbs in xsk_build_skb()
In reply to Jason XingFrom: Jason Xing <[email protected]>
Once xsk_skb_init_misc() has been called on an skb, its destructor is set to xsk_destruct_skb(), which submits the descriptor address(es) to the completion queue and advances the CQ producer. If such an skb is subsequently freed via kfree_skb() along an error path - before the skb has ever been handed to the driver - the destructor still runs and submits a bogus, half-initialized address to the CQ.
Postpone the init phase when we believe the allocation of first frag is successfully completed. Before this init, skb can be safely freed by kfree_skb().
--- net/xdp/xsk.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
net/xdp/xsk.c +2 -3
@@ -739,8 +739,6 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,return ERR_PTR (err);skb_reserve (skb, hr); - -xsk_skb_init_misc (skb, xs, desc->addr);if (desc->options &XDP_TX_METADATA ) { err =xsk_skb_metadata (skb, buffer, desc, pool, hr);if (unlikely (err))
@@ -834,7 +832,6 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,if (unlikely (err)) goto free_err; -xsk_skb_init_misc (skb, xs, desc->addr);if (desc->options &XDP_TX_METADATA ) { err =xsk_skb_metadata (skb, buffer, desc, xs->pool, hr);
@@ -884,6 +881,8 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs, } } +if (!xs->skb) +xsk_skb_init_misc (skb, xs, desc->addr);xsk_inc_num_desc (skb);return skb;
Signature · 1 line
2.41.3
PATCH v4 5/8 xsk: avoid skb leak in XDP_TX_METADATA case
In reply to Jason XingFrom: Jason Xing <[email protected]>
Fix it by explicitly adding kfree_skb() before returning back to its caller.
How to reproduce it in virtio_net:
1. the current skb is the first one (which means no frag and xs->skb is NULL) and users enable metadata feature.
2. xsk_skb_metadata() returns a error code.
3. the caller xsk_build_skb() clears skb by using 'skb = NULL;'.
4. there is no chance to free this skb anymore.
--- net/xdp/xsk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
net/xdp/xsk.c +3 -1
@@ -741,8 +741,10 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,skb_reserve (skb, hr);if (desc->options &XDP_TX_METADATA ) { err =xsk_skb_metadata (skb, buffer, desc, pool, hr); -if (unlikely (err)) +if (unlikely (err)) { +kfree_skb (skb);return ERR_PTR (err); + } } }else {struct xsk_addrs *xsk_addr;
Signature · 1 line
2.41.3
PATCH v4 6/8 xsk: free the skb when hitting the upper bound MAX_SKB_FRAGS
In reply to Jason XingFrom: Jason Xing <[email protected]>
Fix it by explicitly adding kfree_skb() before returning back to its caller.
How to reproduce it in virtio_net:
1. the current skb is the first one (which means xs->skb is NULL) and hit the limit MAX_SKB_FRAGS.
2. xsk_build_skb_zerocopy() returns -EOVERFLOW.
3. the caller xsk_build_skb() clears skb by using 'skb = NULL;'. This is why bug can be triggered.
4. there is no chance to free this skb anymore.
Note that if in this case the xs->skb is not NULL, xsk_build_skb() will call xsk_drop_skb(xs->skb) to do the right thing.
--- net/xdp/xsk.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
net/xdp/xsk.c +4 -1
@@ -776,8 +776,11 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs, addr = buffer - pool->addrs;for (copied =0 , i =skb_shinfo (skb)->nr_frags; copied < len; i++) { -if (unlikely (i >=MAX_SKB_FRAGS )) +if (unlikely (i >=MAX_SKB_FRAGS )) { +if (!xs->skb) +kfree_skb (skb);return ERR_PTR (-EOVERFLOW ); + } page = pool->umem->pgs[addr >>PAGE_SHIFT ];get_page (page);
Signature · 1 line
2.41.3
PATCH v4 7/8 xsk: fix xsk_addrs slab leak on multi-buffer error path
In reply to Jason XingFrom: Jason Xing <[email protected]>
When xsk_build_skb() / xsk_build_skb_zerocopy() sees the first continuation descriptor, it promotes destructor_arg from an inlined address to a freshly allocated xsk_addrs (num_descs = 1). The counter is bumped to >= 2 only at the very end of a successful build (by calling xsk_inc_num_desc()).
If the build fails in between (e.g. alloc_page() returns NULL with -EAGAIN, or the MAX_SKB_FRAGS overflow hits), we jump to free_err, skip calling xsk_inc_num_desc() to increment num_descs and leave the half-built skb attached to xs->skb for the app to retry. The skb now has
1) destructor_arg = a real xsk_addrs pointer,
2) num_descs = 1
If the app never retries and just close()s the socket, xsk_release() calls xsk_drop_skb() -> xsk_consume_skb(), which decides whether to free xsk_addrs by testing num_descs > 1:
if (unlikely(num_descs > 1))
kmem_cache_free(xsk_tx_generic_cache, destructor_arg);
Because num_descs is exactly 1 the branch is skipped and the xsk_addrs object is leaked to the xsk_tx_generic_cache slab.
Fix it by directly testing if destructor_arg is still addr. Or else it is modified and used to store the newly allocated memory from xsk_tx_generic_cache regardless of increment of num_desc, which we need to handle.
--- net/xdp/xsk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
net/xdp/xsk.c +2 -2
@@ -605,7 +605,7 @@ static void xsk_cq_submit_addr_locked(struct xsk_buff_pool *pool,spin_lock_irqsave (&pool->cq_prod_lock, flags); idx =xskq_get_prod (pool->cq); -if (unlikely (num_descs >1 )) { +if (unlikely (!xsk_skb_destructor_is_addr (skb))) { xsk_addr = (struct xsk_addrs *)skb_shinfo (skb)->destructor_arg;for (i =0 ; i < num_descs; i++) {
@@ -660,7 +660,7 @@ static void xsk_consume_skb(struct sk_buff *skb)u32 num_descs =xsk_get_num_desc (skb);struct xsk_addrs *xsk_addr; -if (unlikely (num_descs >1 )) { +if (unlikely (!xsk_skb_destructor_is_addr (skb))) { xsk_addr = (struct xsk_addrs *)skb_shinfo (skb)->destructor_arg;kmem_cache_free (xsk_tx_generic_cache, xsk_addr); }
Signature · 1 line
2.41.3
PATCH v4 8/8 xsk: fix u64 descriptor address truncation on 32-bit architectures
In reply to Jason XingFrom: Jason Xing <[email protected]>
In copy mode TX, xsk_skb_destructor_set_addr() stores the 64-bit descriptor address into skb_shinfo(skb)->destructor_arg (void *) via a uintptr_t cast:
skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);
On 32-bit architectures uintptr_t is 32 bits, so the upper 32 bits of the descriptor address are silently dropped. In unaligned mode the chunk offset is encoded in bits 48-63 of the descriptor address (XSK_UNALIGNED_BUF_OFFSET_SHIFT = 48), meaning the offset is lost entirely. The completion queue then returns a truncated address to userspace, making buffer recycling impossible.
Fix this by handling the 32-bit case in the destructor_arg helpers:
- xsk_skb_destructor_set_addr(): on !CONFIG_64BIT, allocate an xsk_addrs struct via kmem_cache_zalloc() to store the full u64 address. Leave num_descs as 0 (zalloc) so that the subsequent xsk_inc_num_desc() brings it to the correct count of 1.
- xsk_skb_destructor_is_addr(): on !CONFIG_64BIT, return true only when destructor_arg is NULL (not yet set), false when it points to an xsk_addrs struct.
- xsk_skb_init_misc(): call xsk_skb_destructor_set_addr() first before touching any other skb fields; on failure return early so the skb destructor is never changed from sock_wfree.
The existing xsk_consume_skb() already handles 32-bit correctly after these changes: xsk_skb_destructor_is_addr() returns false for any allocated xsk_addrs, so the kmem_cache_free path is always taken.
The overhead is one extra kmem_cache_zalloc per first descriptor on 32-bit only; 64-bit builds are completely unchanged.
--- net/xdp/xsk.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)
net/xdp/xsk.c +31 -7
@@ -558,7 +558,10 @@ static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool)static bool xsk_skb_destructor_is_addr (struct sk_buff *skb) { -return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +if (IS_ENABLED (CONFIG_64BIT )) +return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +else +return !skb_shinfo (skb)->destructor_arg; }static u64 xsk_skb_destructor_get_addr (struct sk_buff *skb)
@@ -566,9 +569,21 @@ static u64 xsk_skb_destructor_get_addr(struct sk_buff *skb)return (u64)((uintptr_t )skb_shinfo (skb)->destructor_arg & ~0x1UL ); } -static void xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) +static int xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) { +if (!IS_ENABLED (CONFIG_64BIT )) { +struct xsk_addrs *xsk_addr; + + xsk_addr =kmem_cache_zalloc (xsk_tx_generic_cache,GFP_KERNEL ); +if (!xsk_addr) +return -ENOMEM ; + xsk_addr->addrs[0 ] = addr; +skb_shinfo (skb)->destructor_arg = (void *)xsk_addr; +return 0 ; + } +skb_shinfo (skb)->destructor_arg = (void *)((uintptr_t )addr |0x1UL ); +return 0 ; }static void xsk_inc_num_desc (struct sk_buff *skb)
@@ -644,14 +659,20 @@ void xsk_destruct_skb(struct sk_buff *skb)sock_wfree (skb); } -static void xsk_skb_init_misc (struct sk_buff *skb,struct xdp_sock *xs, -u64 addr) +static int xsk_skb_init_misc (struct sk_buff *skb,struct xdp_sock *xs, +u64 addr) { +int err; + + err =xsk_skb_destructor_set_addr (skb, addr); +if (err) +return err; + skb->dev = xs->dev; skb->priority =READ_ONCE (xs->sk.sk_priority); skb->mark =READ_ONCE (xs->sk.sk_mark); skb->destructor = xsk_destruct_skb; -xsk_skb_destructor_set_addr (skb, addr); +return 0 ; }static void xsk_consume_skb (struct sk_buff *skb)
@@ -886,8 +907,11 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs, } } -if (!xs->skb) -xsk_skb_init_misc (skb, xs, desc->addr); +if (!xs->skb) { + err =xsk_skb_init_misc (skb, xs, desc->addr); +if (unlikely (err)) + goto free_err; + }xsk_inc_num_desc (skb);return skb;
Signature · 1 line
2.41.3
Paolo Abeni
maintainerRe: [PATCH net v4 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures
Quoted 34 lines · Jason Xing
From: Jason Xing <[email protected]>
In copy mode TX,
xsk_skb_destructor_set_addr()stores the 64-bit descriptor address into skb_shinfo(skb)->destructor_arg (void *) via auintptr_tcast:skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);On 32-bit architectures
uintptr_tis 32 bits, so the upper 32 bits of the descriptor address are silently dropped. In unaligned mode the chunk offset is encoded in bits 48-63 of the descriptor address (XSK_UNALIGNED_BUF_OFFSET_SHIFT= 48), meaning the offset is lost entirely. The completion queue then returns a truncated address to userspace, making buffer recycling impossible.Fix this by handling the 32-bit case in the
destructor_arghelpers:-
xsk_skb_destructor_set_addr(): on !CONFIG_64BIT, allocate anxsk_addrsstruct viakmem_cache_zalloc()to store the full u64 address. Leavenum_descsas 0 (zalloc) so that the subsequentxsk_inc_num_desc()brings it to the correct count of 1.-
xsk_skb_destructor_is_addr(): on !CONFIG_64BIT, return true only whendestructor_argis NULL (not yet set), false when it points to anxsk_addrsstruct.-
xsk_skb_init_misc(): callxsk_skb_destructor_set_addr()first before touching any other skb fields; on failure return early so the skb destructor is never changed fromsock_wfree.The existing
xsk_consume_skb()already handles 32-bit correctly after these changes:xsk_skb_destructor_is_addr()returns false for any allocatedxsk_addrs, so thekmem_cache_freepath is always taken.
Quoted 6 lines
The overhead is one extra
kmem_cache_zallocper first descriptor on 32-bit only; 64-bit builds are completely unchanged.Fixes 0ebc27a4c67d ("xsk: avoid data corruption on cq descriptor number")Signed-off-by Jason Xing <[email protected]>
LGTM, but waiting a bit more for Magnus, Maciej or Stan's ack.
/P
Re: [PATCH net v4 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures
In reply to Jason XingQuoted 49 lines · Jason Xing
From: Jason Xing <[email protected]>
In copy mode TX,
xsk_skb_destructor_set_addr()stores the 64-bit descriptor address into skb_shinfo(skb)->destructor_arg (void *) via auintptr_tcast:skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);On 32-bit architectures
uintptr_tis 32 bits, so the upper 32 bits of the descriptor address are silently dropped. In unaligned mode the chunk offset is encoded in bits 48-63 of the descriptor address (XSK_UNALIGNED_BUF_OFFSET_SHIFT= 48), meaning the offset is lost entirely. The completion queue then returns a truncated address to userspace, making buffer recycling impossible.Fix this by handling the 32-bit case in the
destructor_arghelpers:-
xsk_skb_destructor_set_addr(): on !CONFIG_64BIT, allocate anxsk_addrsstruct viakmem_cache_zalloc()to store the full u64 address. Leavenum_descsas 0 (zalloc) so that the subsequentxsk_inc_num_desc()brings it to the correct count of 1.-
xsk_skb_destructor_is_addr(): on !CONFIG_64BIT, return true only whendestructor_argis NULL (not yet set), false when it points to anxsk_addrsstruct.-
xsk_skb_init_misc(): callxsk_skb_destructor_set_addr()first before touching any other skb fields; on failure return early so the skb destructor is never changed fromsock_wfree.The existing
xsk_consume_skb()already handles 32-bit correctly after these changes:xsk_skb_destructor_is_addr()returns false for any allocatedxsk_addrs, so thekmem_cache_freepath is always taken.The overhead is one extra
kmem_cache_zallocper first descriptor on 32-bit only; 64-bit builds are completely unchanged.Fixes 0ebc27a4c67d ("xsk: avoid data corruption on cq descriptor number")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)net/xdp/xsk.c +0
-0
net/xdp/xsk.c static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool) · line 564
Quoted 8 lines
net/xdp/xsk.c +4
-1@@ -558,7 +558,10 @@ static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool)static bool xsk_skb_destructor_is_addr (struct sk_buff *skb) { -return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +if (IS_ENABLED (CONFIG_64BIT )) +return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +else +return !skb_shinfo (skb)->destructor_arg;
Don't understand why we need to special case CONFIG_64BIT here? Shouldn't the same existing condition work on 32bit?
Quoted 10 lines
net/xdp/xsk.c +1
-1}static u64 xsk_skb_destructor_get_addr (struct sk_buff *skb)@@ -566,9 +569,21 @@ static u64 xsk_skb_destructor_get_addr(struct sk_buff *skb)return (u64)((uintptr_t )skb_shinfo (skb)->destructor_arg & ~0x1UL ); } -static void xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) +static int xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) {
[..]
net/xdp/xsk.c static u64 xsk_skb_destructor_get_addr(struct sk_buff *skb) · line 586
Quoted 13 lines
net/xdp/xsk.c +12
-0+if (!IS_ENABLED (CONFIG_64BIT )) { +struct xsk_addrs *xsk_addr; + + xsk_addr =kmem_cache_zalloc (xsk_tx_generic_cache,GFP_KERNEL ); +if (!xsk_addr) +return -ENOMEM ; + xsk_addr->addrs[0 ] = addr; +skb_shinfo (skb)->destructor_arg = (void *)xsk_addr; +return 0 ; + } +skb_shinfo (skb)->destructor_arg = (void *)((uintptr_t )addr |0x1UL ); +return 0 ;
I think this is gonna be a 3rd copy paste of the same logic? Let's move to a new helper and replace existing kmem_cache_zalloc places?
xsk_skb_destructor_alloc_list(prev_addr) ?
Re: [PATCH net v4 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures
Quoted 52 lines · Stanislav Fomichev
Quoted 50 lines · Jason Xing
From: Jason Xing <[email protected]>
In copy mode TX,
xsk_skb_destructor_set_addr()stores the 64-bit descriptor address into skb_shinfo(skb)->destructor_arg (void *) via auintptr_tcast:skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);On 32-bit architectures
uintptr_tis 32 bits, so the upper 32 bits of the descriptor address are silently dropped. In unaligned mode the chunk offset is encoded in bits 48-63 of the descriptor address (XSK_UNALIGNED_BUF_OFFSET_SHIFT= 48), meaning the offset is lost entirely. The completion queue then returns a truncated address to userspace, making buffer recycling impossible.Fix this by handling the 32-bit case in the
destructor_arghelpers:-
xsk_skb_destructor_set_addr(): on !CONFIG_64BIT, allocate anxsk_addrsstruct viakmem_cache_zalloc()to store the full u64 address. Leavenum_descsas 0 (zalloc) so that the subsequentxsk_inc_num_desc()brings it to the correct count of 1.-
xsk_skb_destructor_is_addr(): on !CONFIG_64BIT, return true only whendestructor_argis NULL (not yet set), false when it points to anxsk_addrsstruct.-
xsk_skb_init_misc(): callxsk_skb_destructor_set_addr()first before touching any other skb fields; on failure return early so the skb destructor is never changed fromsock_wfree.The existing
xsk_consume_skb()already handles 32-bit correctly after these changes:xsk_skb_destructor_is_addr()returns false for any allocatedxsk_addrs, so thekmem_cache_freepath is always taken.The overhead is one extra
kmem_cache_zallocper first descriptor on 32-bit only; 64-bit builds are completely unchanged.Fixes 0ebc27a4c67d ("xsk: avoid data corruption on cq descriptor number")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)net/xdp/xsk.c +0
-0@@ -558,7 +558,10 @@ static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool)
Quoted 10 lines
net/xdp/xsk.c static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool) · line 564
Quoted 7 lines
net/xdp/xsk.c +4
-1static bool xsk_skb_destructor_is_addr (struct sk_buff *skb) { -return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +if (IS_ENABLED (CONFIG_64BIT )) +return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +else +return !skb_shinfo (skb)->destructor_arg;Don't understand why we need to special case
CONFIG_64BIThere? Shouldn't the same existing condition work on 32bit?
Because 0x1UL is the particular semantic applied on a 64-bit arch. xsk_skb_destructor_set_addr() sets it while xsk_skb_destructor_is_addr() recognizes it. They are a pair.
As you noticed, one liner works but is not that appropriate: on a 32-bit arch, this member should be either a NULL point or a valid pointer pointing to a memory region. Testing if it's NULL can be helpful as to the long term maintenance because of its readability and robustness/safety.
The error path in allocation of skb is really complex, which is why I'm so cautious to take care of it :)
I've noticed the status has been changed to 'changes requested'. Does that mean one way or another I have to post a new version?
Thanks,
Jason
Quoted 32 lines
Quoted 11 lines
net/xdp/xsk.c +1
-1}static u64 xsk_skb_destructor_get_addr (struct sk_buff *skb)@@ -566,9 +569,21 @@ static u64 xsk_skb_destructor_get_addr(struct sk_buff *skb)return (u64)((uintptr_t )skb_shinfo (skb)->destructor_arg & ~0x1UL ); } -static void xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) +static int xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) {[..]
net/xdp/xsk.c static u64 xsk_skb_destructor_get_addr(struct sk_buff *skb) · line 586
Quoted 13 lines
net/xdp/xsk.c +12
-0+if (!IS_ENABLED (CONFIG_64BIT )) { +struct xsk_addrs *xsk_addr; + + xsk_addr =kmem_cache_zalloc (xsk_tx_generic_cache,GFP_KERNEL ); +if (!xsk_addr) +return -ENOMEM ; + xsk_addr->addrs[0 ] = addr; +skb_shinfo (skb)->destructor_arg = (void *)xsk_addr; +return 0 ; + } +skb_shinfo (skb)->destructor_arg = (void *)((uintptr_t )addr |0x1UL ); +return 0 ;I think this is gonna be a 3rd copy paste of the same logic? Let's move to a new helper and replace existing
kmem_cache_zallocplaces?
xsk_skb_destructor_alloc_list(prev_addr)?
Re: [PATCH net v4 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures
Quoted 68 lines · Jason Xing
Quoted 52 lines · Stanislav Fomichev
Quoted 50 lines · Jason Xing
From: Jason Xing <[email protected]>
In copy mode TX,
xsk_skb_destructor_set_addr()stores the 64-bit descriptor address into skb_shinfo(skb)->destructor_arg (void *) via auintptr_tcast:skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);On 32-bit architectures
uintptr_tis 32 bits, so the upper 32 bits of the descriptor address are silently dropped. In unaligned mode the chunk offset is encoded in bits 48-63 of the descriptor address (XSK_UNALIGNED_BUF_OFFSET_SHIFT= 48), meaning the offset is lost entirely. The completion queue then returns a truncated address to userspace, making buffer recycling impossible.Fix this by handling the 32-bit case in the
destructor_arghelpers:-
xsk_skb_destructor_set_addr(): on !CONFIG_64BIT, allocate anxsk_addrsstruct viakmem_cache_zalloc()to store the full u64 address. Leavenum_descsas 0 (zalloc) so that the subsequentxsk_inc_num_desc()brings it to the correct count of 1.-
xsk_skb_destructor_is_addr(): on !CONFIG_64BIT, return true only whendestructor_argis NULL (not yet set), false when it points to anxsk_addrsstruct.-
xsk_skb_init_misc(): callxsk_skb_destructor_set_addr()first before touching any other skb fields; on failure return early so the skb destructor is never changed fromsock_wfree.The existing
xsk_consume_skb()already handles 32-bit correctly after these changes:xsk_skb_destructor_is_addr()returns false for any allocatedxsk_addrs, so thekmem_cache_freepath is always taken.The overhead is one extra
kmem_cache_zallocper first descriptor on 32-bit only; 64-bit builds are completely unchanged.Fixes 0ebc27a4c67d ("xsk: avoid data corruption on cq descriptor number")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)net/xdp/xsk.c +0
-0@@ -558,7 +558,10 @@ static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool)Quoted 10 lines
net/xdp/xsk.c static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool) · line 564
Quoted 7 lines
net/xdp/xsk.c +4
-1static bool xsk_skb_destructor_is_addr (struct sk_buff *skb) { -return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +if (IS_ENABLED (CONFIG_64BIT )) +return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +else +return !skb_shinfo (skb)->destructor_arg;Don't understand why we need to special case
CONFIG_64BIThere? Shouldn't the same existing condition work on 32bit?Because
0x1ULis the particular semantic applied on a 64-bit arch.xsk_skb_destructor_set_addr()sets it whilexsk_skb_destructor_is_addr()recognizes it. They are a pair.
Quoted 8 lines
As you noticed, one liner works but is not that appropriate: on a 32-bit arch, this member should be either a NULL point or a valid pointer pointing to a memory region. Testing if it's NULL can be helpful as to the long term maintenance because of its readability and robustness/safety.
The error path in allocation of skb is really complex, which is why I'm so cautious to take care of it :)
Let's cleanup the error path instead of adding more complexity? Similar to what you do with your "xsk: fix xsk_addrs slab leak on multi-buffer error path", but maybe add a few NULL checks?
Instead of 32 vs 64, I'd like to reason about whether destructor_arg is an address or an allocated array (not whether we have 1 or >1 descriptors). And we special case 32 bit by always allocating it.
Haven't checked, but maybe this is all you need (besides your _set_addr changes)?
net/xdp/xsk.c +2 -0
@@ -589,6 +589,8 @@ static u32 xsk_get_num_desc(struct sk_buff *skb)return 1 ; xsk_addr = (struct xsk_addrs *)skb_shinfo (skb)->destructor_arg; +if (!xsk_addr) +return 0 ;return xsk_addr->num_descs; }
Quoted 2 lines
I've noticed the status has been changed to 'changes requested'. Does that mean one way or another I have to post a new version?
That wasn't me :-) From my POW, patches 1-7 are good to go..
Quoted 31 lines
Quoted 31 lines
Quoted 11 lines
net/xdp/xsk.c +1
-1}static u64 xsk_skb_destructor_get_addr (struct sk_buff *skb)@@ -566,9 +569,21 @@ static u64 xsk_skb_destructor_get_addr(struct sk_buff *skb)return (u64)((uintptr_t )skb_shinfo (skb)->destructor_arg & ~0x1UL ); } -static void xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) +static int xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) {[..]
net/xdp/xsk.c
Quoted 13 lines
net/xdp/xsk.c +12
-0+if (!IS_ENABLED (CONFIG_64BIT )) { +struct xsk_addrs *xsk_addr; + + xsk_addr =kmem_cache_zalloc (xsk_tx_generic_cache,GFP_KERNEL ); +if (!xsk_addr) +return -ENOMEM ; + xsk_addr->addrs[0 ] = addr; +skb_shinfo (skb)->destructor_arg = (void *)xsk_addr; +return 0 ; + } +skb_shinfo (skb)->destructor_arg = (void *)((uintptr_t )addr |0x1UL ); +return 0 ;I think this is gonna be a 3rd copy paste of the same logic? Let's move to a new helper and replace existing
kmem_cache_zallocplaces?
Quoted 1 line
Quoted 1 line
xsk_skb_destructor_alloc_list(prev_addr)?
Any comments on this?
Re: [PATCH net v4 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures
Quoted 76 lines · Stanislav Fomichev
Quoted 74 lines · Jason Xing
Quoted 52 lines · Stanislav Fomichev
Quoted 50 lines · Jason Xing
From: Jason Xing <[email protected]>
In copy mode TX,
xsk_skb_destructor_set_addr()stores the 64-bit descriptor address into skb_shinfo(skb)->destructor_arg (void *) via auintptr_tcast:skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);On 32-bit architectures
uintptr_tis 32 bits, so the upper 32 bits of the descriptor address are silently dropped. In unaligned mode the chunk offset is encoded in bits 48-63 of the descriptor address (XSK_UNALIGNED_BUF_OFFSET_SHIFT= 48), meaning the offset is lost entirely. The completion queue then returns a truncated address to userspace, making buffer recycling impossible.Fix this by handling the 32-bit case in the
destructor_arghelpers:-
xsk_skb_destructor_set_addr(): on !CONFIG_64BIT, allocate anxsk_addrsstruct viakmem_cache_zalloc()to store the full u64 address. Leavenum_descsas 0 (zalloc) so that the subsequentxsk_inc_num_desc()brings it to the correct count of 1.-
xsk_skb_destructor_is_addr(): on !CONFIG_64BIT, return true only whendestructor_argis NULL (not yet set), false when it points to anxsk_addrsstruct.-
xsk_skb_init_misc(): callxsk_skb_destructor_set_addr()first before touching any other skb fields; on failure return early so the skb destructor is never changed fromsock_wfree.The existing
xsk_consume_skb()already handles 32-bit correctly after these changes:xsk_skb_destructor_is_addr()returns false for any allocatedxsk_addrs, so thekmem_cache_freepath is always taken.The overhead is one extra
kmem_cache_zallocper first descriptor on 32-bit only; 64-bit builds are completely unchanged.Fixes 0ebc27a4c67d ("xsk: avoid data corruption on cq descriptor number")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)net/xdp/xsk.c +0
-0@@ -558,7 +558,10 @@ static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool)Quoted 10 lines
net/xdp/xsk.c static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool) · line 564
Quoted 7 lines
net/xdp/xsk.c +4
-1static bool xsk_skb_destructor_is_addr (struct sk_buff *skb) { -return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +if (IS_ENABLED (CONFIG_64BIT )) +return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +else +return !skb_shinfo (skb)->destructor_arg;Don't understand why we need to special case
CONFIG_64BIThere? Shouldn't the same existing condition work on 32bit?Because
0x1ULis the particular semantic applied on a 64-bit arch.xsk_skb_destructor_set_addr()sets it whilexsk_skb_destructor_is_addr()recognizes it. They are a pair.As you noticed, one liner works but is not that appropriate: on a 32-bit arch, this member should be either a NULL point or a valid pointer pointing to a memory region. Testing if it's NULL can be helpful as to the long term maintenance because of its readability and robustness/safety.
Quoted 6 lines
Quoted 2 lines
The error path in allocation of skb is really complex, which is why I'm so cautious to take care of it :)
Let's cleanup the error path instead of adding more complexity? Similar to what you do with your "xsk: fix
xsk_addrsslab leak on multi-buffer error path", but maybe add a few NULL checks?
Good suggestion. I think I can cook a follow up patch to do such a thing targetting net-next tree. This patch 8 belongs to net material which means it will be backported to the older stable kernel as soon as it gets merged. IIUC, the better way is to make it as simple as possible?
Quoted 13 lines
Instead of 32 vs 64, I'd like to reason about whether
destructor_argis an address or an allocated array (not whether we have 1 or >1 descriptors). And we special case 32 bit by always allocating it.Haven't checked, but maybe this is all you need (besides your
_set_addrchanges)?net/xdp/xsk.c +0
-0
net/xdp/xsk.c static u32 xsk_get_num_desc(struct sk_buff *skb) · line 596
Quoted 8 lines
net/xdp/xsk.c +2
-0@@ -589,6 +589,8 @@ static u32 xsk_get_num_desc(struct sk_buff *skb)return 1 ; xsk_addr = (struct xsk_addrs *)skb_shinfo (skb)->destructor_arg; +if (!xsk_addr) +return 0 ;return xsk_addr->num_descs; }
Right, as I mentioned, how about posting a new cleanup patch with your suggested-by tag?
Quoted 5 lines
Quoted 3 lines
I've noticed the status has been changed to 'changes requested'. Does that mean one way or another I have to post a new version?
That wasn't me :-) From my POW, patches 1-7 are good to go..
Great! Thanks for the review. My hope is to get this series merged soon in the net tree.
Quoted 32 lines
Quoted 32 lines
Quoted 32 lines
Quoted 12 lines
net/xdp/xsk.c +1
-1}static u64 xsk_skb_destructor_get_addr (struct sk_buff *skb)@@ -566,9 +569,21 @@ static u64 xsk_skb_destructor_get_addr(struct sk_buff *skb)return (u64)((uintptr_t )skb_shinfo (skb)->destructor_arg & ~0x1UL ); } -static void xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) +static int xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) {[..]
net/xdp/xsk.c
Quoted 13 lines
net/xdp/xsk.c +12
-0+if (!IS_ENABLED (CONFIG_64BIT )) { +struct xsk_addrs *xsk_addr; + + xsk_addr =kmem_cache_zalloc (xsk_tx_generic_cache,GFP_KERNEL ); +if (!xsk_addr) +return -ENOMEM ; + xsk_addr->addrs[0 ] = addr; +skb_shinfo (skb)->destructor_arg = (void *)xsk_addr; +return 0 ; + } +skb_shinfo (skb)->destructor_arg = (void *)((uintptr_t )addr |0x1UL ); +return 0 ;I think this is gonna be a 3rd copy paste of the same logic? Let's move to a new helper and replace existing
kmem_cache_zallocplaces?
Quoted 3 lines
Quoted 1 line
Quoted 1 line
xsk_skb_destructor_alloc_list(prev_addr)?Any comments on this?
I didn't comment on this because I thought I was not that sure if we needed to wrap it up in the stable kernels :)
Of course, it would be easier for me to work on the net-next tree to make the code look
more neat and elegant.
Thanks,
Jason
Re: [PATCH net v4 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures
Quoted 158 lines · Jason Xing
Quoted 76 lines · Stanislav Fomichev
Quoted 74 lines · Jason Xing
Quoted 52 lines · Stanislav Fomichev
Quoted 50 lines · Jason Xing
From: Jason Xing <[email protected]>
In copy mode TX,
xsk_skb_destructor_set_addr()stores the 64-bit descriptor address into skb_shinfo(skb)->destructor_arg (void *) via auintptr_tcast:skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);On 32-bit architectures
uintptr_tis 32 bits, so the upper 32 bits of the descriptor address are silently dropped. In unaligned mode the chunk offset is encoded in bits 48-63 of the descriptor address (XSK_UNALIGNED_BUF_OFFSET_SHIFT= 48), meaning the offset is lost entirely. The completion queue then returns a truncated address to userspace, making buffer recycling impossible.Fix this by handling the 32-bit case in the
destructor_arghelpers:-
xsk_skb_destructor_set_addr(): on !CONFIG_64BIT, allocate anxsk_addrsstruct viakmem_cache_zalloc()to store the full u64 address. Leavenum_descsas 0 (zalloc) so that the subsequentxsk_inc_num_desc()brings it to the correct count of 1.-
xsk_skb_destructor_is_addr(): on !CONFIG_64BIT, return true only whendestructor_argis NULL (not yet set), false when it points to anxsk_addrsstruct.-
xsk_skb_init_misc(): callxsk_skb_destructor_set_addr()first before touching any other skb fields; on failure return early so the skb destructor is never changed fromsock_wfree.The existing
xsk_consume_skb()already handles 32-bit correctly after these changes:xsk_skb_destructor_is_addr()returns false for any allocatedxsk_addrs, so thekmem_cache_freepath is always taken.The overhead is one extra
kmem_cache_zallocper first descriptor on 32-bit only; 64-bit builds are completely unchanged.Fixes 0ebc27a4c67d ("xsk: avoid data corruption on cq descriptor number")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)net/xdp/xsk.c +0
-0@@ -558,7 +558,10 @@ static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool)Quoted 10 lines
net/xdp/xsk.c static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool) · line 564
Quoted 7 lines
net/xdp/xsk.c +4
-1static bool xsk_skb_destructor_is_addr (struct sk_buff *skb) { -return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +if (IS_ENABLED (CONFIG_64BIT )) +return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +else +return !skb_shinfo (skb)->destructor_arg;Don't understand why we need to special case
CONFIG_64BIThere? Shouldn't the same existing condition work on 32bit?Because
0x1ULis the particular semantic applied on a 64-bit arch.xsk_skb_destructor_set_addr()sets it whilexsk_skb_destructor_is_addr()recognizes it. They are a pair.As you noticed, one liner works but is not that appropriate: on a 32-bit arch, this member should be either a NULL point or a valid pointer pointing to a memory region. Testing if it's NULL can be helpful as to the long term maintenance because of its readability and robustness/safety.
Quoted 6 lines
Quoted 2 lines
The error path in allocation of skb is really complex, which is why I'm so cautious to take care of it :)
Let's cleanup the error path instead of adding more complexity? Similar to what you do with your "xsk: fix
xsk_addrsslab leak on multi-buffer error path", but maybe add a few NULL checks?Good suggestion. I think I can cook a follow up patch to do such a thing targetting net-next tree. This patch 8 belongs to net material which means it will be backported to the older stable kernel as soon as it gets merged. IIUC, the better way is to make it as simple as possible?
Quoted 13 lines
Instead of 32 vs 64, I'd like to reason about whether
destructor_argis an address or an allocated array (not whether we have 1 or >1 descriptors). And we special case 32 bit by always allocating it.Haven't checked, but maybe this is all you need (besides your
_set_addrchanges)?net/xdp/xsk.c +0
-0net/xdp/xsk.c static u32 xsk_get_num_desc(struct sk_buff *skb) · line 596
Quoted 8 lines
net/xdp/xsk.c +2
-0@@ -589,6 +589,8 @@ static u32 xsk_get_num_desc(struct sk_buff *skb)return 1 ; xsk_addr = (struct xsk_addrs *)skb_shinfo (skb)->destructor_arg; +if (!xsk_addr) +return 0 ;return xsk_addr->num_descs; }Right, as I mentioned, how about posting a new cleanup patch with your suggested-by tag?
Quoted 5 lines
Quoted 3 lines
I've noticed the status has been changed to 'changes requested'. Does that mean one way or another I have to post a new version?
That wasn't me :-) From my POW, patches 1-7 are good to go..
Great! Thanks for the review. My hope is to get this series merged soon in the net tree.
Quoted 34 lines
Quoted 33 lines
Quoted 33 lines
Quoted 12 lines
net/xdp/xsk.c +1
-1}static u64 xsk_skb_destructor_get_addr (struct sk_buff *skb)@@ -566,9 +569,21 @@ static u64 xsk_skb_destructor_get_addr(struct sk_buff *skb)return (u64)((uintptr_t )skb_shinfo (skb)->destructor_arg & ~0x1UL ); } -static void xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) +static int xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) {[..]
net/xdp/xsk.c
Quoted 13 lines
net/xdp/xsk.c +12
-0+if (!IS_ENABLED (CONFIG_64BIT )) { +struct xsk_addrs *xsk_addr; + + xsk_addr =kmem_cache_zalloc (xsk_tx_generic_cache,GFP_KERNEL ); +if (!xsk_addr) +return -ENOMEM ; + xsk_addr->addrs[0 ] = addr; +skb_shinfo (skb)->destructor_arg = (void *)xsk_addr; +return 0 ; + } +skb_shinfo (skb)->destructor_arg = (void *)((uintptr_t )addr |0x1UL ); +return 0 ;I think this is gonna be a 3rd copy paste of the same logic? Let's move to a new helper and replace existing
kmem_cache_zallocplaces?
xsk_skb_destructor_alloc_list(prev_addr)?
Quoted 8 lines
Quoted 1 line
Any comments on this?
I didn't comment on this because I thought I was not that sure if we needed to wrap it up in the stable kernels :)
Of course, it would be easier for me to work on the net-next tree to make the code look
more neat and elegant.
Are you concerned that you're gonna break something in the net tree? Why not do it properly from the start? If you want, you can post 1-7 patches separately and we follow up with this one but still into net?
Re: [PATCH net v4 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures
Quoted 162 lines · Stanislav Fomichev
Quoted 160 lines · Jason Xing
Quoted 76 lines · Stanislav Fomichev
Quoted 74 lines · Jason Xing
Quoted 52 lines · Stanislav Fomichev
Quoted 50 lines · Jason Xing
From: Jason Xing <[email protected]>
In copy mode TX,
xsk_skb_destructor_set_addr()stores the 64-bit descriptor address into skb_shinfo(skb)->destructor_arg (void *) via auintptr_tcast:skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);On 32-bit architectures
uintptr_tis 32 bits, so the upper 32 bits of the descriptor address are silently dropped. In unaligned mode the chunk offset is encoded in bits 48-63 of the descriptor address (XSK_UNALIGNED_BUF_OFFSET_SHIFT= 48), meaning the offset is lost entirely. The completion queue then returns a truncated address to userspace, making buffer recycling impossible.Fix this by handling the 32-bit case in the
destructor_arghelpers:-
xsk_skb_destructor_set_addr(): on !CONFIG_64BIT, allocate anxsk_addrsstruct viakmem_cache_zalloc()to store the full u64 address. Leavenum_descsas 0 (zalloc) so that the subsequentxsk_inc_num_desc()brings it to the correct count of 1.-
xsk_skb_destructor_is_addr(): on !CONFIG_64BIT, return true only whendestructor_argis NULL (not yet set), false when it points to anxsk_addrsstruct.-
xsk_skb_init_misc(): callxsk_skb_destructor_set_addr()first before touching any other skb fields; on failure return early so the skb destructor is never changed fromsock_wfree.The existing
xsk_consume_skb()already handles 32-bit correctly after these changes:xsk_skb_destructor_is_addr()returns false for any allocatedxsk_addrs, so thekmem_cache_freepath is always taken.The overhead is one extra
kmem_cache_zallocper first descriptor on 32-bit only; 64-bit builds are completely unchanged.Fixes 0ebc27a4c67d ("xsk: avoid data corruption on cq descriptor number")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)net/xdp/xsk.c +0
-0@@ -558,7 +558,10 @@ static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool)Quoted 10 lines
net/xdp/xsk.c static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool) · line 564
Quoted 7 lines
net/xdp/xsk.c +4
-1static bool xsk_skb_destructor_is_addr (struct sk_buff *skb) { -return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +if (IS_ENABLED (CONFIG_64BIT )) +return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +else +return !skb_shinfo (skb)->destructor_arg;Don't understand why we need to special case
CONFIG_64BIThere? Shouldn't the same existing condition work on 32bit?Because
0x1ULis the particular semantic applied on a 64-bit arch.xsk_skb_destructor_set_addr()sets it whilexsk_skb_destructor_is_addr()recognizes it. They are a pair.As you noticed, one liner works but is not that appropriate: on a 32-bit arch, this member should be either a NULL point or a valid pointer pointing to a memory region. Testing if it's NULL can be helpful as to the long term maintenance because of its readability and robustness/safety.
Quoted 6 lines
Quoted 2 lines
The error path in allocation of skb is really complex, which is why I'm so cautious to take care of it :)
Let's cleanup the error path instead of adding more complexity? Similar to what you do with your "xsk: fix
xsk_addrsslab leak on multi-buffer error path", but maybe add a few NULL checks?Good suggestion. I think I can cook a follow up patch to do such a thing targetting net-next tree. This patch 8 belongs to net material which means it will be backported to the older stable kernel as soon as it gets merged. IIUC, the better way is to make it as simple as possible?
Quoted 13 lines
Instead of 32 vs 64, I'd like to reason about whether
destructor_argis an address or an allocated array (not whether we have 1 or >1 descriptors). And we special case 32 bit by always allocating it.Haven't checked, but maybe this is all you need (besides your
_set_addrchanges)?net/xdp/xsk.c +0
-0net/xdp/xsk.c static u32 xsk_get_num_desc(struct sk_buff *skb) · line 596
Quoted 8 lines
net/xdp/xsk.c +2
-0@@ -589,6 +589,8 @@ static u32 xsk_get_num_desc(struct sk_buff *skb)return 1 ; xsk_addr = (struct xsk_addrs *)skb_shinfo (skb)->destructor_arg; +if (!xsk_addr) +return 0 ;return xsk_addr->num_descs; }Right, as I mentioned, how about posting a new cleanup patch with your suggested-by tag?
Quoted 5 lines
Quoted 3 lines
I've noticed the status has been changed to 'changes requested'. Does that mean one way or another I have to post a new version?
That wasn't me :-) From my POW, patches 1-7 are good to go..
Great! Thanks for the review. My hope is to get this series merged soon in the net tree.
Quoted 35 lines
Quoted 32 lines
Quoted 32 lines
Quoted 12 lines
net/xdp/xsk.c +1
-1}static u64 xsk_skb_destructor_get_addr (struct sk_buff *skb)@@ -566,9 +569,21 @@ static u64 xsk_skb_destructor_get_addr(struct sk_buff *skb)return (u64)((uintptr_t )skb_shinfo (skb)->destructor_arg & ~0x1UL ); } -static void xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) +static int xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) {[..]
net/xdp/xsk.c
Quoted 13 lines
net/xdp/xsk.c +12
-0+if (!IS_ENABLED (CONFIG_64BIT )) { +struct xsk_addrs *xsk_addr; + + xsk_addr =kmem_cache_zalloc (xsk_tx_generic_cache,GFP_KERNEL ); +if (!xsk_addr) +return -ENOMEM ; + xsk_addr->addrs[0 ] = addr; +skb_shinfo (skb)->destructor_arg = (void *)xsk_addr; +return 0 ; + } +skb_shinfo (skb)->destructor_arg = (void *)((uintptr_t )addr |0x1UL ); +return 0 ;I think this is gonna be a 3rd copy paste of the same logic? Let's move to a new helper and replace existing
kmem_cache_zallocplaces?Quoted 1 line
Quoted 1 line
xsk_skb_destructor_alloc_list(prev_addr)?Any comments on this?
Quoted 8 lines
Quoted 6 lines
I didn't comment on this because I thought I was not that sure if we needed to wrap it up in the stable kernels :)
Of course, it would be easier for me to work on the net-next tree to make the code look
more neat and elegant.Are you concerned that you're gonna break something in the net tree? Why not
My primary concern is this patch would be a little bit more complicated (more insertions/deletions). IIUC, the fix should be as simple as possible to easily get backported while we can do some cleanup work on the next branch.
Please see this new version (v5) with your advice applied. Does it look good?
https://lore.kernel.org/all/[email protected]/
Thanks,
Jason