* [PATCH net v2] xsk: set network header on skb at generic transmit
@ 2026-09-01 8:33 Jason Xing
2026-09-02 8:33 ` sashiko-bot
2026-09-03 23:34 ` netdev-bot+sashiko
0 siblings, 2 replies; 6+ messages in thread
From: Jason Xing @ 2026-09-01 8:33 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, bjorn, magnus.karlsson,
maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk,
john.fastabend, aleksander.lobakin
Cc: bpf, netdev, Jason Xing, syzbot+f16c1b285c9f63994eec
When constructing an skb in the XSK generic transmit path, neither
xsk_build_skb() nor xsk_build_skb_zerocopy() calls
skb_reset_network_header(). After __finalize_skb_around(), the
network_header offset defaults to 0 (pointing to skb->head). The
subsequent skb_reserve(skb, hr) moves skb->data forward by hr bytes,
but leaves network_header still at 0. This makes skb_network_offset()
return a negative value (-hr).
When such an skb reaches a tunnel driver like ipgre_xmit(), the call
chain pskb_inet_may_pull() -> pskb_network_may_pull_reason() computes:
pskb_may_pull_reason(skb, skb_network_offset(skb) + nhlen)
The negative int from skb_network_offset() is implicitly promoted to
unsigned int, wrapping around to a huge value. This triggers the
DEBUG_NET_WARN_ON_ONCE(len > INT_MAX) in pskb_may_pull_reason().
Call trace from syzkaller:
ipgre_xmit+0x958/0xcd0 net/ipv4/ip_gre.c:658
__netdev_start_xmit include/linux/netdevice.h:5400
netdev_start_xmit include/linux/netdevice.h:5409
__dev_direct_xmit+0x4b6/0x730 net/core/dev.c:4942
__xsk_generic_xmit net/xdp/xsk.c:1079
xsk_generic_xmit+0x277a/0x40a0 net/xdp/xsk.c:1120
Reset the network header so skb_network_offset() is valid (0). Note this
anchors it at the start of the frame (L2 for an ethernet device), which
is the same convention AF_PACKET uses on TX. Unlike AF_PACKET, AF_XDP
does not need to locate the real L2/L3 boundary or probe the transport
header: an AF_XDP frame is always a complete, opaque L2 blob from
userspace. AF_PACKET only cares about the boundary because it can cook an
L2 header itself (SOCK_DGRAM) and supports GSO/checksum offload; none of
that applies here, where the frame is sent as-is via __dev_direct_xmit().
So resetting the network header is enough to restore the invariant and
avoid the crash.
Fixes: 3914d88f7608 ("xsk: Respect device's headroom and tailroom on generic xmit path")
Reported-by: syzbot+f16c1b285c9f63994eec@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=f16c1b285c9f63994eec
Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
V2
Link: https://lore.kernel.org/all/20260826012052.51028-1-kerneljasonxing@gmail.com/
1. add more clarification about why we don't probe the header like packet at
the end of the commit message (Jakub)
---
net/xdp/xsk.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 33475b180ea6..2077bb6299ca 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -947,6 +947,8 @@ static int xsk_skb_init_misc(struct sk_buff *skb, struct xdp_sock *xs,
{
int err;
+ skb_reset_network_header(skb);
+
err = xsk_skb_destructor_set_addr(skb, addr);
if (unlikely(err))
return err;
--
2.43.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net v2] xsk: set network header on skb at generic transmit 2026-09-01 8:33 [PATCH net v2] xsk: set network header on skb at generic transmit Jason Xing @ 2026-09-02 8:33 ` sashiko-bot 2026-09-02 11:55 ` Jason Xing 2026-09-03 23:34 ` netdev-bot+sashiko 1 sibling, 1 reply; 6+ messages in thread From: sashiko-bot @ 2026-09-02 8:33 UTC (permalink / raw) To: Jason Xing; +Cc: bpf Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: New issues: - [Medium] The patch incorrectly anchors the network header at the L2 boundary (MAC header), which violates standard SKB semantics and contradicts the AF_PACKET convention it claims to follow. Pre-existing issues: - [High] Missing initialization of `skb->mac_header` in the AF_XDP generic transmit path leaves it at `~0U` (0xFFFF), causing out-of-bounds memory accesses in network drivers that unconditionally read the MAC header. -- commit 8222314e7d9b572394dd3fc2b9acb0b3706abd3e Author: Jason Xing <kerneljasonxing@gmail.com> xsk: set network header on skb at generic transmit This commit fixes an issue in the XSK generic transmit path where the network header was left uninitialized. It adds a call to skb_reset_network_header(skb) in xsk_skb_init_misc() to ensure the network header is correctly anchored to the start of the frame. This prevents a crash when the skb is handled by tunnel drivers. Closes: https://syzkaller.appspot.com/bug?extid=f16c1b285c9f63994eec Link: https://lore.kernel.org/all/20260826012052.51028-1-kerneljasonxing@gmail.com/ > Reset the network header so skb_network_offset() is valid (0). Note this > anchors it at the start of the frame (L2 for an ethernet device), which > is the same convention AF_PACKET uses on TX. [Severity: Medium] Does anchoring the network header at the L2 boundary (MAC header) here violate SKB semantics and contradict the AF_PACKET convention? In AF_PACKET's packet_snd(), skb_reset_network_header(skb) is explicitly called before skb_reserve(skb, -reserve) prepends the L2 header, correctly anchoring the network header at the L3 boundary (after hard_header_len bytes). By anchoring the network header directly at skb->data (the L2 header) in xsk_skb_init_misc(), won't any subsystem, offload logic, or tunneling driver that relies on skb_network_header(skb) erroneously parse the Ethernet header as an IP header? > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > index 33475b180ea6f..2077bb6299ca9 100644 > --- a/net/xdp/xsk.c > +++ b/net/xdp/xsk.c > @@ -947,6 +947,8 @@ static int xsk_skb_init_misc(struct sk_buff *skb, struct xdp_sock *xs, > { > int err; > > + skb_reset_network_header(skb); > + [Severity: High] This isn't a bug introduced by this patch, but is it also necessary to call skb_reset_mac_header(skb) here? The AF_XDP generic TX path allocates packets via __alloc_skb(), which leaves mac_header uninitialized at ~0U (0xFFFF). Because AF_XDP generic TX bypasses standard egress paths (like __dev_queue_xmit()) that would normally set the MAC header, the SKB is handed to __dev_direct_xmit() with an invalid mac_header. When physical NIC drivers unconditionally read eth->h_proto via skb_mac_header(skb) during transmit, this evaluates to skb->head + 0xFFFF. For example, in drivers/net/ethernet/intel/ice/ice_txrx.c:ice_xmit_frame_ring(): eth = (struct ethhdr *)skb_mac_header(skb); if ((ice_is_switchdev_running(vsi->back) || ice_lag_is_switchdev_running(vsi->back)) && vsi->type != ICE_VSI_SF) ice_eswitch_set_target_vsi(skb, &offload); else if (unlikely((skb->priority == TC_PRIO_CONTROL || eth->h_proto == htons(ETH_P_LLDP)) && vsi->type == ICE_VSI_PF && vsi->port_info->qos_cfg.is_sw_lldp)) Dereferencing this out-of-bounds pointer crosses page boundaries and causes a paging violation (kernel panic). Should skb_reset_mac_header(skb) be added alongside the network header reset to prevent this out-of-bounds read? > err = xsk_skb_destructor_set_addr(skb, addr); > if (unlikely(err)) > return err; -- Sashiko AI review · https://sashiko.dev/#/patchset/20260901083325.3445-1-kerneljasonxing@gmail.com?part=1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] xsk: set network header on skb at generic transmit 2026-09-02 8:33 ` sashiko-bot @ 2026-09-02 11:55 ` Jason Xing 0 siblings, 0 replies; 6+ messages in thread From: Jason Xing @ 2026-09-02 11:55 UTC (permalink / raw) To: sashiko-reviews; +Cc: bpf On Wed, Sep 2, 2026 at 4:33 PM <sashiko-bot@kernel.org> wrote: > > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: > > New issues: > - [Medium] The patch incorrectly anchors the network header at the L2 boundary (MAC header), which violates standard SKB semantics and contradicts the AF_PACKET convention it claims to follow. > > Pre-existing issues: > - [High] Missing initialization of `skb->mac_header` in the AF_XDP generic transmit path leaves it at `~0U` (0xFFFF), causing out-of-bounds memory accesses in network drivers that unconditionally read the MAC header. > -- > > commit 8222314e7d9b572394dd3fc2b9acb0b3706abd3e > Author: Jason Xing <kerneljasonxing@gmail.com> > > xsk: set network header on skb at generic transmit > > This commit fixes an issue in the XSK generic transmit path where the network > header was left uninitialized. It adds a call to skb_reset_network_header(skb) > in xsk_skb_init_misc() to ensure the network header is correctly anchored to > the start of the frame. This prevents a crash when the skb is handled by > tunnel drivers. > > Closes: https://syzkaller.appspot.com/bug?extid=f16c1b285c9f63994eec > Link: https://lore.kernel.org/all/20260826012052.51028-1-kerneljasonxing@gmail.com/ > > > Reset the network header so skb_network_offset() is valid (0). Note this > > anchors it at the start of the frame (L2 for an ethernet device), which > > is the same convention AF_PACKET uses on TX. > > [Severity: Medium] > Does anchoring the network header at the L2 boundary (MAC header) here > violate SKB semantics and contradict the AF_PACKET convention? > > In AF_PACKET's packet_snd(), skb_reset_network_header(skb) is explicitly > called before skb_reserve(skb, -reserve) prepends the L2 header, correctly > anchoring the network header at the L3 boundary (after hard_header_len bytes). I will remove this in V3. I misunderstood the boundary in AF_PACKET logic. > > By anchoring the network header directly at skb->data (the L2 header) in > xsk_skb_init_misc(), won't any subsystem, offload logic, or tunneling driver > that relies on skb_network_header(skb) erroneously parse the Ethernet header > as an IP header? No, at least until now. That's why resetting is sufficient. > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > > index 33475b180ea6f..2077bb6299ca9 100644 > > --- a/net/xdp/xsk.c > > +++ b/net/xdp/xsk.c > > @@ -947,6 +947,8 @@ static int xsk_skb_init_misc(struct sk_buff *skb, struct xdp_sock *xs, > > { > > int err; > > > > + skb_reset_network_header(skb); > > + > > [Severity: High] > This isn't a bug introduced by this patch, but is it also necessary to call > skb_reset_mac_header(skb) here? I think it should be merged as a fix: https://lore.kernel.org/all/20260817153957.3177627-1-poros@redhat.com/ Thanks, Jason > > The AF_XDP generic TX path allocates packets via __alloc_skb(), which leaves > mac_header uninitialized at ~0U (0xFFFF). Because AF_XDP generic TX bypasses > standard egress paths (like __dev_queue_xmit()) that would normally set the > MAC header, the SKB is handed to __dev_direct_xmit() with an invalid > mac_header. > > When physical NIC drivers unconditionally read eth->h_proto via > skb_mac_header(skb) during transmit, this evaluates to skb->head + 0xFFFF. > > For example, in drivers/net/ethernet/intel/ice/ice_txrx.c:ice_xmit_frame_ring(): > > eth = (struct ethhdr *)skb_mac_header(skb); > > if ((ice_is_switchdev_running(vsi->back) || > ice_lag_is_switchdev_running(vsi->back)) && > vsi->type != ICE_VSI_SF) > ice_eswitch_set_target_vsi(skb, &offload); > else if (unlikely((skb->priority == TC_PRIO_CONTROL || > eth->h_proto == htons(ETH_P_LLDP)) && > vsi->type == ICE_VSI_PF && > vsi->port_info->qos_cfg.is_sw_lldp)) > > Dereferencing this out-of-bounds pointer crosses page boundaries and causes > a paging violation (kernel panic). Should skb_reset_mac_header(skb) be added > alongside the network header reset to prevent this out-of-bounds read? > > > err = xsk_skb_destructor_set_addr(skb, addr); > > if (unlikely(err)) > > return err; > > -- > Sashiko AI review · https://sashiko.dev/#/patchset/20260901083325.3445-1-kerneljasonxing@gmail.com?part=1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] xsk: set network header on skb at generic transmit 2026-09-01 8:33 [PATCH net v2] xsk: set network header on skb at generic transmit Jason Xing 2026-09-02 8:33 ` sashiko-bot @ 2026-09-03 23:34 ` netdev-bot+sashiko 2026-09-04 3:53 ` Jason Xing 1 sibling, 1 reply; 6+ messages in thread From: netdev-bot+sashiko @ 2026-09-03 23:34 UTC (permalink / raw) To: kerneljasonxing Cc: davem, edumazet, kuba, pabeni, bjorn, magnus.karlsson, maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk, john.fastabend, aleksander.lobakin, bpf, netdev, syzbot+f16c1b285c9f63994eec Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 1 · Low: 0 - [Medium] net/xdp/xsk.c: the new skb_reset_network_header(skb) in xsk_skb_init_misc() (net/xdp/xsk.c:950) anchors the *network* header… -- xsk: set network header on skb at generic transmit The XSK generic transmit path never reset the network header, so after skb_reserve(hr) the stored offset was left behind skb->data and skb_network_offset() returned -hr, which tripped the DEBUG_NET_WARN_ON_ONCE(len > INT_MAX) in pskb_may_pull_reason() when the skb reached a tunnel device. This adds skb_reset_network_header() in xsk_skb_init_misc() so the offset is 0. Closes: https://syzkaller.appspot.com/bug?extid=f16c1b285c9f63994eec Link: https://lore.kernel.org/all/20260826012052.51028-1-kerneljasonxing@gmail.com/ > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > index 33475b180ea6f..2077bb6299ca9 100644 > --- a/net/xdp/xsk.c > +++ b/net/xdp/xsk.c > @@ -947,6 +947,8 @@ static int xsk_skb_init_misc(struct sk_buff *skb, struct xdp_sock *xs, > { > int err; > > + skb_reset_network_header(skb); > + [Severity: Medium] The negative offset does look closed by this: xsk_skb_init_misc() runs after skb_reserve(hr) in xsk_build_skb() and xsk_build_skb_zerocopy(), so skb_network_offset() becomes 0. Two parts of the reasoning look off though. First, the commit message says: "Note this anchors it at the start of the frame (L2 for an ethernet device), which is the same convention AF_PACKET uses on TX." Is that what AF_PACKET does? packet_parse_headers() anchors the MAC header, not the network header: net/packet/af_packet.c:packet_parse_headers() { /* On TX skb->data is the L2 header; anchor it for all socket types. */ skb_reset_mac_header(skb); if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) && sock->type == SOCK_RAW) skb->protocol = dev_parse_header_protocol(skb); ... } and packet_snd() for SOCK_RAW resets network_header before moving skb->data back over the hard header, so network_header stays at the L3 boundary: skb_reset_network_header(skb); ... } else if (reserve) { skb_reserve(skb, -reserve); So after this patch an AF_XDP skb differs from an AF_PACKET one by hard_header_len in network_header, and mac_header is still at the unset sentinel left by __finalize_skb_around(), i.e. skb_mac_header_was_set() is false. Second, the commit message says AF_PACKET "supports GSO/checksum offload; none of that applies here". Does checksum offload really not apply? xsk_skb_metadata() in the same file does: net/xdp/xsk.c:xsk_skb_metadata() { skb->csum_start = hr + csum_start; skb->csum_offset = csum_offset; skb->ip_summed = CHECKSUM_PARTIAL; if (unlikely(pool->tx_sw_csum)) { ... err = skb_checksum_help(skb); ... } The software fallback only happens when the user opted in via XDP_UMEM_TX_SW_CSUM, so a CHECKSUM_PARTIAL AF_XDP skb does reach the driver through __dev_direct_xmit(), and drivers take the L2/L3 split from network_header: drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_tx_csum() { vlan_macip_lens = skb_checksum_start_offset(skb) - skb_network_offset(skb); no_csum: /* vlan_macip_lens: MACLEN, VLAN tag */ vlan_macip_lens |= skb_network_offset(skb) << IXGBE_ADVTXD_MACLEN_SHIFT; ... } With skb_network_offset() now 0 on an ethernet frame, does this program MACLEN == 0 and an IPLEN that swallows the ethernet header, so the NIC inserts a checksum derived from the wrong header layout? Would it make sense to also call skb_reset_mac_header() and place network_header at the real L3 boundary here? Alternatively, if the minimal reset is the intent, could the commit message describe network_header as a placeholder and drop the claims about the AF_PACKET convention and the absence of checksum offload? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901083325.3445-1-kerneljasonxing%40gmail.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] xsk: set network header on skb at generic transmit 2026-09-03 23:34 ` netdev-bot+sashiko @ 2026-09-04 3:53 ` Jason Xing 2026-09-04 4:10 ` Jason Xing 0 siblings, 1 reply; 6+ messages in thread From: Jason Xing @ 2026-09-04 3:53 UTC (permalink / raw) To: netdev-bot+sashiko Cc: davem, edumazet, kuba, pabeni, bjorn, magnus.karlsson, maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk, john.fastabend, aleksander.lobakin, bpf, netdev, syzbot+f16c1b285c9f63994eec On Fri, Sep 4, 2026 at 7:34 AM <netdev-bot+sashiko@kernel.org> wrote: > > Thank you for your contribution! Sashiko AI review found 1 potential > issue(s) to consider: > > Critical: 0 · High: 0 · Medium: 1 · Low: 0 > > - [Medium] net/xdp/xsk.c: the new skb_reset_network_header(skb) in > xsk_skb_init_misc() (net/xdp/xsk.c:950) anchors the *network* header… > > -- > > xsk: set network header on skb at generic transmit > > The XSK generic transmit path never reset the network header, so after > skb_reserve(hr) the stored offset was left behind skb->data and > skb_network_offset() returned -hr, which tripped the > DEBUG_NET_WARN_ON_ONCE(len > INT_MAX) in pskb_may_pull_reason() when the > skb reached a tunnel device. This adds skb_reset_network_header() in > xsk_skb_init_misc() so the offset is 0. > > Closes: https://syzkaller.appspot.com/bug?extid=f16c1b285c9f63994eec > Link: https://lore.kernel.org/all/20260826012052.51028-1-kerneljasonxing@gmail.com/ > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > > index 33475b180ea6f..2077bb6299ca9 100644 > > --- a/net/xdp/xsk.c > > +++ b/net/xdp/xsk.c > > @@ -947,6 +947,8 @@ static int xsk_skb_init_misc(struct sk_buff *skb, struct xdp_sock *xs, > > { > > int err; > > > > + skb_reset_network_header(skb); > > + > > [Severity: Medium] > The negative offset does look closed by this: xsk_skb_init_misc() runs > after skb_reserve(hr) in xsk_build_skb() and xsk_build_skb_zerocopy(), so > skb_network_offset() becomes 0. Two parts of the reasoning look off > though. > > First, the commit message says: > > "Note this anchors it at the start of the frame (L2 for an ethernet > device), which is the same convention AF_PACKET uses on TX." > > Is that what AF_PACKET does? packet_parse_headers() anchors the MAC > header, not the network header: > > net/packet/af_packet.c:packet_parse_headers() { > /* On TX skb->data is the L2 header; anchor it for all socket types. */ > skb_reset_mac_header(skb); > > if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) && > sock->type == SOCK_RAW) > skb->protocol = dev_parse_header_protocol(skb); > ... > } I'm going to remove this part. > > and packet_snd() for SOCK_RAW resets network_header before moving > skb->data back over the hard header, so network_header stays at the L3 > boundary: > > skb_reset_network_header(skb); > ... > } else if (reserve) { > skb_reserve(skb, -reserve); > > So after this patch an AF_XDP skb differs from an AF_PACKET one by > hard_header_len in network_header, and mac_header is still at the unset > sentinel left by __finalize_skb_around(), i.e. skb_mac_header_was_set() > is false. > > Second, the commit message says AF_PACKET "supports GSO/checksum > offload; none of that applies here". Does checksum offload really not > apply? xsk_skb_metadata() in the same file does: > > net/xdp/xsk.c:xsk_skb_metadata() { > skb->csum_start = hr + csum_start; > skb->csum_offset = csum_offset; > skb->ip_summed = CHECKSUM_PARTIAL; > > if (unlikely(pool->tx_sw_csum)) { > ... > err = skb_checksum_help(skb); > ... > } > > The software fallback only happens when the user opted in via > XDP_UMEM_TX_SW_CSUM, so a CHECKSUM_PARTIAL AF_XDP skb does reach the > driver through __dev_direct_xmit(), and drivers take the L2/L3 split > from network_header: > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_tx_csum() { > vlan_macip_lens = skb_checksum_start_offset(skb) - > skb_network_offset(skb); > no_csum: > /* vlan_macip_lens: MACLEN, VLAN tag */ > vlan_macip_lens |= skb_network_offset(skb) << IXGBE_ADVTXD_MACLEN_SHIFT; > ... > } After digging into the probing logic, I strongly feel that Jakub is right to consider whether to add the probing logic. > > With skb_network_offset() now 0 on an ethernet frame, does this program > MACLEN == 0 and an IPLEN that swallows the ethernet header, so the NIC > inserts a checksum derived from the wrong header layout? > > Would it make sense to also call skb_reset_mac_header() and place > network_header at the real L3 boundary here? Alternatively, if the > minimal reset is the intent, could the commit message describe The minimal reset doesn't help with the checksum problem mentioned above. Adding simple logic like resetting l2/l3 headers doesn't guarantee that csum problem won't be triggered. Adding complete probing hurts perfomance for sure. Maybe we can cook two patches: 1. Fix the current bug syzbot reported as is. 2. Fix offload logic by adding complete probing @Jakub, Maciej, Stan Do we need a complex logic like af_packet to handle this? Thanks, Jason > network_header as a placeholder and drop the claims about the AF_PACKET > convention and the absence of checksum offload? > > -- > Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901083325.3445-1-kerneljasonxing%40gmail.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] xsk: set network header on skb at generic transmit 2026-09-04 3:53 ` Jason Xing @ 2026-09-04 4:10 ` Jason Xing 0 siblings, 0 replies; 6+ messages in thread From: Jason Xing @ 2026-09-04 4:10 UTC (permalink / raw) To: netdev-bot+sashiko Cc: davem, edumazet, kuba, pabeni, bjorn, magnus.karlsson, maciej.fijalkowski, jonathan.lemon, sdf, ast, daniel, hawk, john.fastabend, aleksander.lobakin, bpf, netdev, syzbot+f16c1b285c9f63994eec On Fri, Sep 4, 2026 at 11:53 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > On Fri, Sep 4, 2026 at 7:34 AM <netdev-bot+sashiko@kernel.org> wrote: > > > > Thank you for your contribution! Sashiko AI review found 1 potential > > issue(s) to consider: > > > > Critical: 0 · High: 0 · Medium: 1 · Low: 0 > > > > - [Medium] net/xdp/xsk.c: the new skb_reset_network_header(skb) in > > xsk_skb_init_misc() (net/xdp/xsk.c:950) anchors the *network* header… > > > > -- > > > > xsk: set network header on skb at generic transmit > > > > The XSK generic transmit path never reset the network header, so after > > skb_reserve(hr) the stored offset was left behind skb->data and > > skb_network_offset() returned -hr, which tripped the > > DEBUG_NET_WARN_ON_ONCE(len > INT_MAX) in pskb_may_pull_reason() when the > > skb reached a tunnel device. This adds skb_reset_network_header() in > > xsk_skb_init_misc() so the offset is 0. > > > > Closes: https://syzkaller.appspot.com/bug?extid=f16c1b285c9f63994eec > > Link: https://lore.kernel.org/all/20260826012052.51028-1-kerneljasonxing@gmail.com/ > > > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > > > index 33475b180ea6f..2077bb6299ca9 100644 > > > --- a/net/xdp/xsk.c > > > +++ b/net/xdp/xsk.c > > > @@ -947,6 +947,8 @@ static int xsk_skb_init_misc(struct sk_buff *skb, struct xdp_sock *xs, > > > { > > > int err; > > > > > > + skb_reset_network_header(skb); > > > + > > > > [Severity: Medium] > > The negative offset does look closed by this: xsk_skb_init_misc() runs > > after skb_reserve(hr) in xsk_build_skb() and xsk_build_skb_zerocopy(), so > > skb_network_offset() becomes 0. Two parts of the reasoning look off > > though. > > > > First, the commit message says: > > > > "Note this anchors it at the start of the frame (L2 for an ethernet > > device), which is the same convention AF_PACKET uses on TX." > > > > Is that what AF_PACKET does? packet_parse_headers() anchors the MAC > > header, not the network header: > > > > net/packet/af_packet.c:packet_parse_headers() { > > /* On TX skb->data is the L2 header; anchor it for all socket types. */ > > skb_reset_mac_header(skb); > > > > if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) && > > sock->type == SOCK_RAW) > > skb->protocol = dev_parse_header_protocol(skb); > > ... > > } > > I'm going to remove this part. > > > > > and packet_snd() for SOCK_RAW resets network_header before moving > > skb->data back over the hard header, so network_header stays at the L3 > > boundary: > > > > skb_reset_network_header(skb); > > ... > > } else if (reserve) { > > skb_reserve(skb, -reserve); > > > > So after this patch an AF_XDP skb differs from an AF_PACKET one by > > hard_header_len in network_header, and mac_header is still at the unset > > sentinel left by __finalize_skb_around(), i.e. skb_mac_header_was_set() > > is false. > > > > Second, the commit message says AF_PACKET "supports GSO/checksum > > offload; none of that applies here". Does checksum offload really not > > apply? xsk_skb_metadata() in the same file does: > > > > net/xdp/xsk.c:xsk_skb_metadata() { > > skb->csum_start = hr + csum_start; > > skb->csum_offset = csum_offset; > > skb->ip_summed = CHECKSUM_PARTIAL; > > > > if (unlikely(pool->tx_sw_csum)) { > > ... > > err = skb_checksum_help(skb); > > ... > > } > > > > The software fallback only happens when the user opted in via > > XDP_UMEM_TX_SW_CSUM, so a CHECKSUM_PARTIAL AF_XDP skb does reach the > > driver through __dev_direct_xmit(), and drivers take the L2/L3 split > > from network_header: > > > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_tx_csum() { > > vlan_macip_lens = skb_checksum_start_offset(skb) - > > skb_network_offset(skb); > > no_csum: > > /* vlan_macip_lens: MACLEN, VLAN tag */ > > vlan_macip_lens |= skb_network_offset(skb) << IXGBE_ADVTXD_MACLEN_SHIFT; > > ... > > } > > After digging into the probing logic, I strongly feel that Jakub is > right to consider whether to add the probing logic. > > > > > With skb_network_offset() now 0 on an ethernet frame, does this program > > MACLEN == 0 and an IPLEN that swallows the ethernet header, so the NIC > > inserts a checksum derived from the wrong header layout? > > > > Would it make sense to also call skb_reset_mac_header() and place > > network_header at the real L3 boundary here? Alternatively, if the > > minimal reset is the intent, could the commit message describe > > The minimal reset doesn't help with the checksum problem mentioned above. > > Adding simple logic like resetting l2/l3 headers doesn't guarantee > that csum problem won't be triggered. Adding complete probing hurts > perfomance for sure. > > Maybe we can cook two patches: > 1. Fix the current bug syzbot reported as is. > 2. Fix offload logic by adding complete probing > > @Jakub, Maciej, Stan > Do we need a complex logic like af_packet to handle this? Until now, it seems we've got three fixes in the queue: 1. reset mac header: https://lore.kernel.org/all/20260817153957.3177627-1-poros@redhat.com/ (commit 35fcde7f8deb "xsk: support for Tx") 2. reset network header like this patch. (commit 3914d88f7608 "xsk: Respect device's headroom and tailroom") 3. handle csum offload issue by adding probing code. (commit 48eb03dd2630 "xsk: Add TX ... checksum offload support") Thanks, Jason > > Thanks, > Jason > > > network_header as a placeholder and drop the claims about the AF_PACKET > > convention and the absence of checksum offload? > > > > -- > > Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901083325.3445-1-kerneljasonxing%40gmail.com ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-04 4:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-09-01 8:33 [PATCH net v2] xsk: set network header on skb at generic transmit Jason Xing 2026-09-02 8:33 ` sashiko-bot 2026-09-02 11:55 ` Jason Xing 2026-09-03 23:34 ` netdev-bot+sashiko 2026-09-04 3:53 ` Jason Xing 2026-09-04 4:10 ` Jason Xing
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox