Re: [PATCH net v2] xsk: set network header on skb at generic transmit

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

On Fri, Sep 04, 2026 at 12:10:37PM +0800, Jason Xing wrote:
> 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")

Hi!

I'd keep this separate and focus on Eric's hint to fix the drivers so they
use skb_eth_hdr() consistently

> 2. reset network header like this patch. (commit 3914d88f7608 "xsk:
> Respect device's headroom and tailroom")

For packets without CHECKSUM_PARTIAL, resetting network_header should be
enough to fix the negative offset reported by syzbot.

For CHECKSUM_PARTIAL, however, I think we need to establish proper skb
header metadata. Existing drivers do rely on it. For example, i40e and ice
use both skb_network_header() and skb_transport_header() in their non-GSO
checksum offload paths, while ixgbe derives its offsets from
skb_network_offset() and skb_checksum_start_offset().

For Ethernet I would avoid depending on mac_header here. The frame can
also be non-linear with IFF_TX_SKB_NO_LINEAR, so using
skb_header_pointer() to inspect the Ethernet header and the fragment-safe
VLAN helpers looks more appropriate than dev_parse_header_protocol().

> 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