netdev - Re: [PATCH net-next 05/15] ipv6/gso: remove temporary HBH/jumbo header

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iK7snFJ2GQ6cuDc2t4LC-Ufzki5TaQrLwDOWE8qDyYATQ@mail.gmail.com>
Date:   Thu, 3 Feb 2022 11:17:24 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Alexander H Duyck <alexander.duyck@...il.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>, Coco Li <lixiaoyan@...gle.com>
Subject: Re: [PATCH net-next 05/15] ipv6/gso: remove temporary HBH/jumbo header

On Thu, Feb 3, 2022 at 10:53 AM Alexander H Duyck
<alexander.duyck@...il.com> wrote:
>
>
> Rather than having to perform all of these checkes would it maybe make
> sense to add SKB_GSO_JUMBOGRAM as a gso_type flag? Then it would make
> it easier for drivers to indicate if they support the new offload or
> not.

Yes, this could be an option.

>
> An added bonus is that it would probably make it easier to do something
> like a GSO_PARTIAL for this since then it would just be a matter of
> flagging it, stripping the extra hop-by-hop header, and chopping it
> into gso_max_size chunks.
>
> > +}
> > +
> >  static inline bool ipv6_accept_ra(struct inet6_dev *idev)
> >  {
> >       /* If forwarding is enabled, RA are not accepted unless the special
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 0118f0afaa4fce8da167ddf39de4c9f3880ca05b..53f17c7392311e7123628fcab4617efc169905a1 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -3959,8 +3959,9 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
> >       skb_frag_t *frag = skb_shinfo(head_skb)->frags;
> >       unsigned int mss = skb_shinfo(head_skb)->gso_size;
> >       unsigned int doffset = head_skb->data - skb_mac_header(head_skb);
> > +     int hophdr_len = sizeof(struct hop_jumbo_hdr);
> >       struct sk_buff *frag_skb = head_skb;
> > -     unsigned int offset = doffset;
> > +     unsigned int offset;
> >       unsigned int tnl_hlen = skb_tnl_header_len(head_skb);
> >       unsigned int partial_segs = 0;
> >       unsigned int headroom;
> > @@ -3968,6 +3969,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
> >       __be16 proto;
> >       bool csum, sg;
> >       int nfrags = skb_shinfo(head_skb)->nr_frags;
> > +     struct ipv6hdr *h6;
> >       int err = -ENOMEM;
> >       int i = 0;
> >       int pos;
> > @@ -3992,6 +3994,23 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
> >       }
> >
> >       __skb_push(head_skb, doffset);
> > +
> > +     if (ipv6_has_hopopt_jumbo(head_skb)) {
> > +             /* remove the HBH header.
> > +              * Layout: [Ethernet header][IPv6 header][HBH][TCP header]
> > +              */
> > +             memmove(head_skb->data + hophdr_len,
> > +                     head_skb->data,
> > +                     ETH_HLEN + sizeof(struct ipv6hdr));
> > +             head_skb->data += hophdr_len;
> > +             head_skb->len -= hophdr_len;
> > +             head_skb->network_header += hophdr_len;
> > +             head_skb->mac_header += hophdr_len;
> > +             doffset -= hophdr_len;
> > +             h6 = (struct ipv6hdr *)(head_skb->data + ETH_HLEN);
> > +             h6->nexthdr = IPPROTO_TCP;
> > +     }
>
> Does it really make the most sense to be doing this here, or should
> this be a part of the IPv6 processing? It seems like of asymmetric when
> compared with the change in the next patch to add the header in GRO.
>

Not sure what you mean. We do have to strip the header here, I do not
see where else to make this ?

> > +     offset = doffset;
> >       proto = skb_network_protocol(head_skb, NULL);
> >       if (unlikely(!proto))
> >               return ERR_PTR(-EINVAL);
>
>