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

On Thu, Feb 3, 2022 at 11:17 AM Eric Dumazet [off-list ref] wrote:

On Thu, Feb 3, 2022 at 10:53 AM Alexander H Duyck
[off-list ref] wrote:

quoted

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.

quoted

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.

quoted

+}
+
 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.