Netdev List
 help / color / mirror / Atom feed
* [PATCH net v3] xsk: set network header on skb at generic transmit
@ 2026-09-02 12:10 Jason Xing
  2026-09-04  1:23 ` Jason Xing
  2026-09-07  4:19 ` netdev-bot+sashiko
  0 siblings, 2 replies; 4+ messages in thread
From: Jason Xing @ 2026-09-02 12:10 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). Unlike
AF_PACKET, AF_XDP does not need to locate the real L2/L3 boundary: an
AF_XDP frame is always a complete, opaque 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>
---
V3
Link: https://lore.kernel.org/all/20260901083325.3445-1-kerneljasonxing@gmail.com/
1. remove the misleading description about af_packet

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] 4+ messages in thread

* Re: [PATCH net v3] xsk: set network header on skb at generic transmit
  2026-09-02 12:10 [PATCH net v3] xsk: set network header on skb at generic transmit Jason Xing
@ 2026-09-04  1:23 ` Jason Xing
  2026-09-07  4:19 ` netdev-bot+sashiko
  1 sibling, 0 replies; 4+ messages in thread
From: Jason Xing @ 2026-09-04  1:23 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, syzbot+f16c1b285c9f63994eec

On Wed, Sep 2, 2026 at 8:10 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> 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). Unlike
> AF_PACKET, AF_XDP does not need to locate the real L2/L3 boundary: an
> AF_XDP frame is always a complete, opaque 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>

Ah, I noticed sashiko in V2 explicitly pointed out the good reason why
probing is needed. Let me rethink more on this.

Thanks,
Jason

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net v3] xsk: set network header on skb at generic transmit
  2026-09-02 12:10 [PATCH net v3] xsk: set network header on skb at generic transmit Jason Xing
  2026-09-04  1:23 ` Jason Xing
@ 2026-09-07  4:19 ` netdev-bot+sashiko
  2026-09-07 12:01   ` Jason Xing
  1 sibling, 1 reply; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-09-07  4:19 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 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 2 · Low: 0

- [Medium] net/xdp/xsk.c: xsk_skb_init_misc() (line 950) now does
  skb_reset_network_header(skb) unconditionally, i.e. it declares the…
- [Medium] By making skb_network_offset() valid (0), the patch makes
  tunnel ndo_start_xmit paths that were previously *always* rejected…

--

xsk: set network header on skb at generic transmit

This adds skb_reset_network_header() to xsk_skb_init_misc() so that
skb_network_offset() no longer returns -hr for skbs built by the XSK
generic transmit path.  The negative offset was promoted to unsigned in
pskb_may_pull_reason() and tripped the DEBUG_NET_WARN_ON_ONCE(len >
INT_MAX) check when such an skb reached a tunnel xmit routine.

Closes: https://syzkaller.appspot.com/bug?extid=f16c1b285c9f63994eec
Link: https://lore.kernel.org/all/20260901083325.3445-1-kerneljasonxing@gmail.com/
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 changelog says:

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

Is the checksum offload part of that accurate?  xsk_skb_metadata(), in this
same generic xmit path, 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);
	...
}

So when the descriptor carries XDP_TX_METADATA with
XDP_TXMD_FLAGS_CHECKSUM and pool->tx_sw_csum is clear, the skb is handed to
the driver with CHECKSUM_PARTIAL, and hardware checksum offload does apply
to AF_XDP generic xmit.

With network_header now anchored at skb->data, on an ARPHRD_ETHER device
that offset points at the userspace-supplied struct ethhdr rather than the
L3 boundary, so skb_network_offset() is 0 where the drivers expect
ETH_HLEN.  Drivers derive descriptor geometry from it:

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 |= skb_network_offset(skb) << IXGBE_ADVTXD_MACLEN_SHIFT;
	...
}

That programs MACLEN as 0 and an L3 length that is ETH_HLEN too large
(igb_tx_csum() is the same shape, and i40e_tx_enable_csum() sets ip.hdr =
skb_network_header(skb) and then reads ip.v4->version, which now inspects
the destination MAC bytes).

For comparison, the AF_PACKET complete-frame path anchors both headers:

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);
	...
		skb_set_network_header(skb, depth);
	...
}

Could the changelog either drop the checksum offload claim and document the
remaining descriptor inaccuracy, or could the network header be set to the
real L3 boundary (mac header plus dev->hard_header_len / VLAN depth, or
consistently with csum_start when XDP_TXMD_FLAGS_CHECKSUM is requested)?
Note the pre-patch offset of -hr was worse, so this is not a regression
relative to current behaviour, but the value chosen is wrong for every
ARPHRD_ETHER device.

[Severity: Medium]
Does making skb_network_offset() valid also newly admit AF_XDP frames into
tunnel xmit code that parses the inner L2 header without any length check?

Before this change, erspan_xmit() dropped every AF_XDP skb at its first
statement, because pskb_inet_may_pull() -> pskb_network_may_pull_reason()
computed (unsigned)(-hr) + nhlen and failed the len > skb->len test:

net/ipv4/ip_gre.c:erspan_xmit() {
	...
	if (!pskb_inet_may_pull(skb))
		goto free_skb;
	...
}

skb->protocol is never assigned in net/xdp/xsk.c, so nhlen is 0 and after
this patch pskb_may_pull_reason(skb, 0) succeeds and the frame reaches
erspan_build_header() / erspan_build_header_v2():

include/net/erspan.h:erspan_build_header() {
	...
	tos = is_ipv4 ? ip_hdr(skb)->tos : ...
	if (eth->h_proto == htons(ETH_P_8021Q)) {
		qp = (struct qtag_prefix *)(skb->data + 2 * ETH_ALEN);
		vlan_tci = ntohs(qp->tci);
	...
}

Those read skb->data[1], skb->data[12..13] and skb->data[14..15] with no
validation, and the results end up in the transmitted header via
ershdr->cos = tos_to_cos(tos) and set_vlan(ershdr, vlan_tci).

Is there anything in the AF_XDP path that guarantees the frame is at least
16 bytes long?  Descriptor validation only rejects zero length:

net/xdp/xsk_queue.h:xp_aligned_validate_desc() {
	...
	if (!len)
		return false;
	...
}

and neither __xsk_generic_xmit() nor __dev_direct_xmit() enforces
dev->hard_header_len, unlike AF_PACKET:

net/packet/af_packet.c:packet_snd() {
	...
	if ((sock->type == SOCK_RAW &&
	     !dev_validate_header(dev, skb->data, len)) || !skb->len) {
		err = -EINVAL;
	...
}

For a descriptor with len between 1 and 15 those offsets are past skb->len
inside the non-zeroed head from sock_alloc_send_skb(hr + len + tr), so can
this copy a couple of bytes of uninitialized slab memory into the packet on
the wire (and produce a KMSAN use-of-uninitialized-value report)?  The
"complete, opaque blob from userspace" premise in the changelog is what
opens this, since nothing checks that the frame contains the device's link
layer header.

An approximately equivalent primitive already exists via AF_PACKET SOCK_RAW
with a 14-byte frame on the same device, so this mainly exposes the shorter
frame variant.  Would a minimum length or header check for AF_XDP generic
xmit be appropriate here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902121028.74092-1-kerneljasonxing%40gmail.com

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net v3] xsk: set network header on skb at generic transmit
  2026-09-07  4:19 ` netdev-bot+sashiko
@ 2026-09-07 12:01   ` Jason Xing
  0 siblings, 0 replies; 4+ messages in thread
From: Jason Xing @ 2026-09-07 12:01 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 Mon, Sep 7, 2026 at 12:19 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 2 · Low: 0
>
> - [Medium] net/xdp/xsk.c: xsk_skb_init_misc() (line 950) now does
>   skb_reset_network_header(skb) unconditionally, i.e. it declares the…
> - [Medium] By making skb_network_offset() valid (0), the patch makes
>   tunnel ndo_start_xmit paths that were previously *always* rejected…
>
> --
>
> xsk: set network header on skb at generic transmit
>
> This adds skb_reset_network_header() to xsk_skb_init_misc() so that
> skb_network_offset() no longer returns -hr for skbs built by the XSK
> generic transmit path.  The negative offset was promoted to unsigned in
> pskb_may_pull_reason() and tripped the DEBUG_NET_WARN_ON_ONCE(len >
> INT_MAX) check when such an skb reached a tunnel xmit routine.
>
> Closes: https://syzkaller.appspot.com/bug?extid=f16c1b285c9f63994eec
> Link: https://lore.kernel.org/all/20260901083325.3445-1-kerneljasonxing@gmail.com/
> 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 changelog says:
>
>     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().
>
> Is the checksum offload part of that accurate?  xsk_skb_metadata(), in this
> same generic xmit path, 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);
>         ...
> }
>
> So when the descriptor carries XDP_TX_METADATA with
> XDP_TXMD_FLAGS_CHECKSUM and pool->tx_sw_csum is clear, the skb is handed to
> the driver with CHECKSUM_PARTIAL, and hardware checksum offload does apply
> to AF_XDP generic xmit.
>
> With network_header now anchored at skb->data, on an ARPHRD_ETHER device
> that offset points at the userspace-supplied struct ethhdr rather than the
> L3 boundary, so skb_network_offset() is 0 where the drivers expect
> ETH_HLEN.  Drivers derive descriptor geometry from it:
>
> 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 |= skb_network_offset(skb) << IXGBE_ADVTXD_MACLEN_SHIFT;
>         ...
> }
>
> That programs MACLEN as 0 and an L3 length that is ETH_HLEN too large
> (igb_tx_csum() is the same shape, and i40e_tx_enable_csum() sets ip.hdr =
> skb_network_header(skb) and then reads ip.v4->version, which now inspects
> the destination MAC bytes).
>
> For comparison, the AF_PACKET complete-frame path anchors both headers:
>
> 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);
>         ...
>                 skb_set_network_header(skb, depth);
>         ...
> }
>
> Could the changelog either drop the checksum offload claim and document the
> remaining descriptor inaccuracy, or could the network header be set to the
> real L3 boundary (mac header plus dev->hard_header_len / VLAN depth, or
> consistently with csum_start when XDP_TXMD_FLAGS_CHECKSUM is requested)?
> Note the pre-patch offset of -hr was worse, so this is not a regression
> relative to current behaviour, but the value chosen is wrong for every
> ARPHRD_ETHER device.

I think it's a pre-existing bug associated with csum logic, which is
what I've already replied in the V2 thread:"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")"

However, I strongly feel a need to discuss this before moving on to V4.

https://lore.kernel.org/all/CAL+tcoDpyG-Hy9mtDnLSGu0nvU_VDqTjs-k51r2+OPwVyHtABA@mail.gmail.com/

Thanks,
Jason

>
> [Severity: Medium]
> Does making skb_network_offset() valid also newly admit AF_XDP frames into
> tunnel xmit code that parses the inner L2 header without any length check?
>
> Before this change, erspan_xmit() dropped every AF_XDP skb at its first
> statement, because pskb_inet_may_pull() -> pskb_network_may_pull_reason()
> computed (unsigned)(-hr) + nhlen and failed the len > skb->len test:
>
> net/ipv4/ip_gre.c:erspan_xmit() {
>         ...
>         if (!pskb_inet_may_pull(skb))
>                 goto free_skb;
>         ...
> }
>
> skb->protocol is never assigned in net/xdp/xsk.c, so nhlen is 0 and after
> this patch pskb_may_pull_reason(skb, 0) succeeds and the frame reaches
> erspan_build_header() / erspan_build_header_v2():
>
> include/net/erspan.h:erspan_build_header() {
>         ...
>         tos = is_ipv4 ? ip_hdr(skb)->tos : ...
>         if (eth->h_proto == htons(ETH_P_8021Q)) {
>                 qp = (struct qtag_prefix *)(skb->data + 2 * ETH_ALEN);
>                 vlan_tci = ntohs(qp->tci);
>         ...
> }
>
> Those read skb->data[1], skb->data[12..13] and skb->data[14..15] with no
> validation, and the results end up in the transmitted header via
> ershdr->cos = tos_to_cos(tos) and set_vlan(ershdr, vlan_tci).
>
> Is there anything in the AF_XDP path that guarantees the frame is at least
> 16 bytes long?  Descriptor validation only rejects zero length:
>
> net/xdp/xsk_queue.h:xp_aligned_validate_desc() {
>         ...
>         if (!len)
>                 return false;
>         ...
> }
>
> and neither __xsk_generic_xmit() nor __dev_direct_xmit() enforces
> dev->hard_header_len, unlike AF_PACKET:
>
> net/packet/af_packet.c:packet_snd() {
>         ...
>         if ((sock->type == SOCK_RAW &&
>              !dev_validate_header(dev, skb->data, len)) || !skb->len) {
>                 err = -EINVAL;
>         ...
> }
>
> For a descriptor with len between 1 and 15 those offsets are past skb->len
> inside the non-zeroed head from sock_alloc_send_skb(hr + len + tr), so can
> this copy a couple of bytes of uninitialized slab memory into the packet on
> the wire (and produce a KMSAN use-of-uninitialized-value report)?  The
> "complete, opaque blob from userspace" premise in the changelog is what
> opens this, since nothing checks that the frame contains the device's link
> layer header.
>
> An approximately equivalent primitive already exists via AF_PACKET SOCK_RAW
> with a 14-byte frame on the same device, so this mainly exposes the shorter
> frame variant.  Would a minimum length or header check for AF_XDP generic
> xmit be appropriate here?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902121028.74092-1-kerneljasonxing%40gmail.com

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-09-07 12:02 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-02 12:10 [PATCH net v3] xsk: set network header on skb at generic transmit Jason Xing
2026-09-04  1:23 ` Jason Xing
2026-09-07  4:19 ` netdev-bot+sashiko
2026-09-07 12:01   ` Jason Xing

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox