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

[Alexander H Duyck <alexander.duyck@gmail.com>]

On Wed, 2022-02-02 at 17:51 -0800, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> ipv6 tcp and gro stacks will soon be able to build big TCP packets,
> with an added temporary Hop By Hop header.
> 
> If GSO is involved for these large packets, we need to remove
> the temporary HBH header before segmentation happens.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  include/net/ipv6.h | 31 +++++++++++++++++++++++++++++++
>  net/core/skbuff.c  | 21 ++++++++++++++++++++-
>  2 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index ea2a4351b654f8bc96503aae2b9adcd478e1f8b2..96e916fb933c3e7d4288e86790fcb2bb1353a261 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -464,6 +464,37 @@ bool ipv6_opt_accepted(const struct sock *sk, const struct sk_buff *skb,
>  struct ipv6_txoptions *ipv6_update_options(struct sock *sk,
>  					   struct ipv6_txoptions *opt);
>  
> +/* This helper is specialized for BIG TCP needs.
> + * It assumes the hop_jumbo_hdr will immediately follow the IPV6 header.
> + * It assumes headers are already in skb->head, thus the sk argument is only read.
> + */
> +static inline bool ipv6_has_hopopt_jumbo(const struct sk_buff *skb)
> +{
> +	struct hop_jumbo_hdr *jhdr;
> +	struct ipv6hdr *nhdr;
> +
> +	if (likely(skb->len <= GRO_MAX_SIZE))
> +		return false;
> +
> +	if (skb->protocol != htons(ETH_P_IPV6))
> +		return false;
> +
> +	if (skb_network_offset(skb) +
> +	    sizeof(struct ipv6hdr) +
> +	    sizeof(struct hop_jumbo_hdr) > skb_headlen(skb))
> +		return false;
> +
> +	nhdr = ipv6_hdr(skb);
> +
> +	if (nhdr->nexthdr != NEXTHDR_HOP)
> +		return false;
> +
> +	jhdr = (struct hop_jumbo_hdr *) (nhdr + 1);
> +	if (jhdr->tlv_type != IPV6_TLV_JUMBO || jhdr->hdrlen != 0)
> +		return false;
> +	return true;

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.

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.

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