Re: [PATCH] net: fix udp gso skb_segment after pull from frag_list

["Shiming Cheng (成诗明)" <Shiming.Cheng@mediatek.com>]

Dear Willem

frag_list assignment trigger:
frag_list is assigned to the next skb without to enable device feature
NETIF_F_GRO_FRAGLIST if head_frag is zero.

After packet in fraglist is processed by BPF pull tial, performing GSO
segmentation directly will cause problems.


udp_gro_receive_segment
  skb_gro_receive
   if (skb->head_frag==0)
     goto merge;


merge:
        /* sk ownership - if any - completely transferred to the
aggregated packet */
        skb->destructor = NULL;
        skb->sk = NULL;
        delta_truesize = skb->truesize;
        if (offset > headlen) {
                unsigned int eat = offset - headlen;

                skb_frag_off_add(&skbinfo->frags[0], eat);
                skb_frag_size_sub(&skbinfo->frags[0], eat);
                skb->data_len -= eat;
                skb->len -= eat;
                offset = headlen;
        }

        __skb_pull(skb, offset);

        if (NAPI_GRO_CB(p)->last == p)
                skb_shinfo(p)->frag_list = skb;   
        <<< here frag_list is assigned to the next skb without to
enable device feature NETIF_F_GRO_FRAGLIST if head_frag is zero
        else
                NAPI_GRO_CB(p)->last->next = skb;
        NAPI_GRO_CB(p)->last = skb;
        __skb_header_release(skb);
        lp = p;




On Tue, 2026-01-06 at 14:15 -0500, Willem de Bruijn wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Shiming Cheng wrote:
> > Commit 3382a1ed7f77 ("net: fix udp gso skb_segment after  pull from
> > frag_list")
> > if gso_type is not SKB_GSO_FRAGLIST but skb->head_frag is zero,
> 
> What codepath triggers this scenario?
> 
> We should make sure that the fix covers all such instances. Likely
> instances of where some module in the datapath, like a BPF program,
> modifies a valid skb into one that is not safe to pass to
> skb_segment.
> 
> I don't fully understand yet that skb->head_frag == 0 is the only
> such condition in scope.
> 
> > then detected invalid geometry in frag_list skbs and call
> > skb_segment. But some packets with modified geometry can also hit
> > bugs in that code. Instead, linearize all these packets that fail
> > the basic invariants on gso fraglist skbs. That is more robust.
> > 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.
> 
> Most of this commit message duplicates the text in commit
> 3382a1ed7f77
> ("net: fix udp gso skb_segment after  pull from frag_list"). And
> somewhat garbles it, as in the first sentence.
> 
> But this is a different datapath, not related to SKB_GSO_FRAGLIST.
> So the fixes tag is also incorrect. The blamed commit fixes an issue
> with fraglist GRO. This new issue is with skbs that have a fraglist,
> but not one created with that feature. (the naming is confusing, but
> fraglist-gro is only one use of the skb frag_list).
> 
> >   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]
> >   [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: 3382a1ed7f77 ("net: fix udp gso skb_segment after pull from
> > frag_list")
> > Signed-off-by: Shiming Cheng <shiming.cheng@mediatek.com>
> > ---
> >  net/ipv4/udp_offload.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > index 19d0b5b09ffa..606d9ce8c98e 100644
> > --- a/net/ipv4/udp_offload.c
> > +++ b/net/ipv4/udp_offload.c
> > @@ -535,6 +535,12 @@ struct sk_buff *__udp_gso_segment(struct
> > sk_buff *gso_skb,
> >                       uh->check = ~udp_v4_check(gso_skb->len,
> >                                                 ip_hdr(gso_skb)-
> > >saddr,
> >                                                 ip_hdr(gso_skb)-
> > >daddr, 0);
> > +     } else if (skb_shinfo(gso_skb)->frag_list && gso_skb-
> > >head_frag == 0) {
> > +             if (skb_pagelen(gso_skb) - sizeof(*uh) !=
> > skb_shinfo(gso_skb)->gso_size) {
> > +                     ret = __skb_linearize(gso_skb);
> > +                     if (ret)
> > +                             return ERR_PTR(ret);
> > +             }
> >       }
> > 
> >       skb_pull(gso_skb, sizeof(*uh));
> > --
> > 2.45.2
> > 
> 
>