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]

Date:   Thu, 3 Feb 2022 11:45:33 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Eric Dumazet <edumazet@...gle.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 11:17 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> 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 ?

It is the fact that you are adding IPv6 specific code to the
net/core/skbuff.c block here. Logically speaking if you are adding the
header in ipv6_gro_receive then it really seems li:ke the logic to
remove the header really belongs in ipv6_gso_segment. I suppose this
is an attempt to optimize it though, since normally updates to the
header are done after segmentation instead of before.