Received-SPF: pass (google.com: domain of linux-kernel+bounces-14661-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <127b8199-1cd4-42d7-9b2b-875abaad93fe@gmail.com> <90117449-1f4a-47d7-baf4-2ed6540bc436@gmail.com>
In-Reply-To: <90117449-1f4a-47d7-baf4-2ed6540bc436@gmail.com>
From: Eric Dumazet <edumazet@google.com>
Date: Tue, 2 Jan 2024 18:33:58 +0100
Message-ID: <CANn89i+GJOgcDWK=C0+vmomt2ShotrOKyLiXzFkfT1W8vpJv1Q@mail.gmail.com>
Subject: Re: [PATCH net-next v2 2/3] net: gro: parse ipv6 ext headers without
 frag0 invalidation
To: Richard Gobert <richardbgobert@gmail.com>
Cc: davem@davemloft.net, dsahern@kernel.org, kuba@kernel.org, 
	pabeni@redhat.com, shuah@kernel.org, netdev@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Jan 2, 2024 at 2:25=E2=80=AFPM Richard Gobert <richardbgobert@gmail=
.com> wrote:
>
> The existing code always pulls the IPv6 header and sets the transport
> offset initially. Then optionally again pulls any extension headers in
> ipv6_gso_pull_exthdrs and sets the transport offset again on return from
> that call. skb->data is set at the start of the first extension header
> before calling ipv6_gso_pull_exthdrs, and must disable the frag0
> optimization because that function uses pskb_may_pull/pskb_pull instead o=
f
> skb_gro_ helpers. It sets the GRO offset to the TCP header with
> skb_gro_pull and sets the transport header. Then returns skb->data to its
> position before this block.
>
> This commit introduces a new helper function - ipv6_gro_pull_exthdrs -
> which is used in ipv6_gro_receive to pull ipv6 ext headers instead of
> ipv6_gso_pull_exthdrs. Thus, there is no modification of skb->data, all
> operations use skb_gro_* helpers, and the frag0 fast path can be taken fo=
r
> IPv6 packets with ext headers.
>
> Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> ---
>  include/net/ipv6.h     |  1 +
>  net/ipv6/ip6_offload.c | 51 +++++++++++++++++++++++++++++++++---------
>  2 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index 78d38dd88aba..217240efa182 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -26,6 +26,7 @@ struct ip_tunnel_info;
>  #define SIN6_LEN_RFC2133       24
>
>  #define IPV6_MAXPLEN           65535
> +#define IPV6_MIN_EXTHDR_LEN    8

// Hmm see my following comment.

>
>  /*
>   *     NextHeader field of IPv6 header
> diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
> index 0e0b5fed0995..c07111d8f56a 100644
> --- a/net/ipv6/ip6_offload.c
> +++ b/net/ipv6/ip6_offload.c
> @@ -37,6 +37,40 @@
>                 INDIRECT_CALL_L4(cb, f2, f1, head, skb);        \
>  })
>
> +static int ipv6_gro_pull_exthdrs(struct sk_buff *skb, int off, int proto=
)
> +{
> +       const struct net_offload *ops =3D NULL;
> +       struct ipv6_opt_hdr *opth;
> +
> +       for (;;) {
> +               int len;
> +
> +               ops =3D rcu_dereference(inet6_offloads[proto]);
> +
> +               if (unlikely(!ops))
> +                       break;
> +
> +               if (!(ops->flags & INET6_PROTO_GSO_EXTHDR))
> +                       break;
> +
> +               opth =3D skb_gro_header(skb, off + IPV6_MIN_EXTHDR_LEN, o=
ff);

I do not see a compelling reason for adding yet another constant here.

I would stick to

   opth =3D skb_gro_header(skb, off + sizeof(*opth), off);

Consistency with similar helpers is desirable.

> +               if (unlikely(!opth))
> +                       break;
> +
> +               len =3D ipv6_optlen(opth);
> +
> +               opth =3D skb_gro_header(skb, off + len, off);

Note this call will take care of precise pull.

> +               if (unlikely(!opth))
> +                       break;
> +               proto =3D opth->nexthdr;
> +
> +               off +=3D len;
> +       }
> +
> +       skb_gro_pull(skb, off - skb_network_offset(skb));
> +       return proto;
> +}
> +
>  static int ipv6_gso_pull_exthdrs(struct sk_buff *skb, int proto)
>  {
>         const struct net_offload *ops =3D NULL;
> @@ -203,28 +237,25 @@ INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_re=
ceive(struct list_head *head,
>                 goto out;
>
>         skb_set_network_header(skb, off);
> -       skb_gro_pull(skb, sizeof(*iph));
> -       skb_set_transport_header(skb, skb_gro_offset(skb));
>
> -       flush +=3D ntohs(iph->payload_len) !=3D skb_gro_len(skb);
> +       flush +=3D ntohs(iph->payload_len) !=3D skb->len - hlen;
>
>         proto =3D iph->nexthdr;
>         ops =3D rcu_dereference(inet6_offloads[proto]);
>         if (!ops || !ops->callbacks.gro_receive) {
> -               pskb_pull(skb, skb_gro_offset(skb));
> -               skb_gro_frag0_invalidate(skb);
> -               proto =3D ipv6_gso_pull_exthdrs(skb, proto);
> -               skb_gro_pull(skb, -skb_transport_offset(skb));
> -               skb_reset_transport_header(skb);
> -               __skb_push(skb, skb_gro_offset(skb));
> +               proto =3D ipv6_gro_pull_exthdrs(skb, hlen, proto);
>
>                 ops =3D rcu_dereference(inet6_offloads[proto]);
>                 if (!ops || !ops->callbacks.gro_receive)
>                         goto out;
>
> -               iph =3D ipv6_hdr(skb);
> +               iph =3D skb_gro_network_header(skb);
> +       } else {
> +               skb_gro_pull(skb, sizeof(*iph));
>         }
>
> +       skb_set_transport_header(skb, skb_gro_offset(skb));
> +
>         NAPI_GRO_CB(skb)->proto =3D proto;
>
>         flush--;
> --
> 2.36.1
>