[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <683a002e73efd_14767f294d6@willemb.c.googlers.com.notmuch>
Date: Fri, 30 May 2025 14:59:58 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Simon Horman <horms@...nel.org>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Shiming Cheng <shiming.cheng@...iatek.com>,
willemb@...gle.com,
edumazet@...gle.com,
davem@...emloft.net,
kuba@...nel.org,
pabeni@...hat.com,
matthias.bgg@...il.com,
linux-kernel@...r.kernel.org,
netdev@...r.kernel.org,
lena.wang@...iatek.com
Subject: Re: [PATCH net v6] net: fix udp gso skb_segment after pull from
frag_list
Simon Horman wrote:
> On Fri, May 30, 2025 at 08:39:35AM -0400, Willem de Bruijn wrote:
> > Shiming Cheng wrote:
> > > Commit a1e40ac5b5e9 ("net: gso: fix udp gso fraglist segmentation after
> > > pull from frag_list") detected invalid geometry in frag_list skbs and
> > > redirects them from skb_segment_list to more robust skb_segment. But some
> > > packets with modified geometry can also hit bugs in that code. We don't
> > > know how many such cases exist. Addressing each one by one also requires
> > > touching the complex skb_segment code, which risks introducing bugs for
> > > other types of skbs. Instead, linearize all these packets that fail the
> > > basic invariants on gso fraglist skbs. That is more robust.
> > >
> > > If only part of the fraglist payload is pulled into head_skb, it will
> > > always cause exception when splitting skbs by skb_segment. For detailed
> > > call stack information, see below.
> > >
> > > Valid SKB_GSO_FRAGLIST skbs
> > > - consist of two or more segments
> > > - the head_skb holds the protocol headers plus first gso_size
> > > - one or more frag_list skbs hold exactly one segment
> > > - all but the last must be gso_size
> > >
> > > Optional datapath hooks such as NAT and BPF (bpf_skb_pull_data) can
> > > modify fraglist skbs, breaking these invariants.
> > >
> > > In extreme cases they pull one part of data into skb linear. For UDP,
> > > this causes three payloads with lengths of (11,11,10) bytes were
> > > pulled tail to become (12,10,10) bytes.
> > >
> > > The skbs no longer meets the above SKB_GSO_FRAGLIST conditions because
> > > payload was pulled into head_skb, it needs to be linearized before pass
> > > to regular skb_segment.
> > >
> > > skb_segment+0xcd0/0xd14
> > > __udp_gso_segment+0x334/0x5f4
> > > udp4_ufo_fragment+0x118/0x15c
> > > inet_gso_segment+0x164/0x338
> > > skb_mac_gso_segment+0xc4/0x13c
> > > __skb_gso_segment+0xc4/0x124
> > > validate_xmit_skb+0x9c/0x2c0
> > > validate_xmit_skb_list+0x4c/0x80
> > > sch_direct_xmit+0x70/0x404
> > > __dev_queue_xmit+0x64c/0xe5c
> > > neigh_resolve_output+0x178/0x1c4
> > > ip_finish_output2+0x37c/0x47c
> > > __ip_finish_output+0x194/0x240
> > > ip_finish_output+0x20/0xf4
> > > ip_output+0x100/0x1a0
> > > NF_HOOK+0xc4/0x16c
> > > ip_forward+0x314/0x32c
> > > ip_rcv+0x90/0x118
> > > __netif_receive_skb+0x74/0x124
> > > process_backlog+0xe8/0x1a4
> > > __napi_poll+0x5c/0x1f8
> > > net_rx_action+0x154/0x314
> > > handle_softirqs+0x154/0x4b8
> > >
> > > [118.376811] [C201134] rxq0_pus: [name:bug&]kernel BUG at net/core/skbuff.c:4278!
> > > [118.376829] [C201134] rxq0_pus: [name:traps&]Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
> > > [118.470774] [C201134] rxq0_pus: [name:mrdump&]Kernel Offset: 0x178cc00000 from 0xffffffc008000000
> > > [118.470810] [C201134] rxq0_pus: [name:mrdump&]PHYS_OFFSET: 0x40000000
> > > [118.470827] [C201134] rxq0_pus: [name:mrdump&]pstate: 60400005 (nZCv daif +PAN -UAO)
> > > [118.470848] [C201134] rxq0_pus: [name:mrdump&]pc : [0xffffffd79598aefc] skb_segment+0xcd0/0xd14
> > > [118.470900] [C201134] rxq0_pus: [name:mrdump&]lr : [0xffffffd79598a5e8] skb_segment+0x3bc/0xd14
> > > [118.470928] [C201134] rxq0_pus: [name:mrdump&]sp : ffffffc008013770
> > >
> > > Fixes: a1e40ac5b5e9 ("gso: fix udp gso fraglist segmentation after pull from frag_list")
> > > Signed-off-by: Shiming Cheng <shiming.cheng@...iatek.com>
Reviewed-by: Willem de Bruijn <willemb@...gle.com>
> >
> > Is this effectively a repost of v5?
> >
> > I think Simon suggested changing the subject line from starting with
> > "net:" to starting with "gso:", but this revision does not make such
> > a change.
>
> FTR, my suggestion was to correct the subject embedded in the Fixes tag.
> And that appears to be addressed in this revision (v6).
Oh, your comment was on the Fixes tag. I misunderstood. Good catch, thanks.
Powered by blists - more mailing lists