Received-SPF: pass (google.com: domain of linux-kernel+bounces-95989-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <2ce1600b-e733-448b-91ac-9d0ae2b866a4@gmail.com> <2076d2ff-cd17-4ab0-b1db-4875d96bf9a6@gmail.com>
In-Reply-To: <2076d2ff-cd17-4ab0-b1db-4875d96bf9a6@gmail.com>
From: Eric Dumazet <edumazet@google.com>
Date: Thu, 7 Mar 2024 18:59:10 +0100
Message-ID: <CANn89iKq755tvJ1BZFXG5aX2YNd9AycbKu57taxi8gaSWn5Syw@mail.gmail.com>
Subject: Re: [PATCH net-next v2 3/4] net: gro: set inner_network_header in
 receive phase
To: Richard Gobert <richardbgobert@gmail.com>
Cc: davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com, 
	willemdebruijn.kernel@gmail.com, dsahern@kernel.org, shuah@kernel.org, 
	idosch@nvidia.com, 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 Thu, Mar 7, 2024 at 2:28=E2=80=AFPM Richard Gobert <richardbgobert@gmail=
com> wrote:
>
> This patch sets network_header and inner_network_header to their respecti=
ve
> values during the receive phase of GRO. This allows us to use
> inner_network_header later on in GRO. network_header is already set in
> dev_gro_receive and under encapsulation inner_network_header is set.
>

> +static inline int skb_gro_network_offset(const struct sk_buff *skb)
> +{
> +       const u32 mask =3D NAPI_GRO_CB(skb)->encap_mark - 1;
> +
> +       return (skb_network_offset(skb) & mask) | (skb_inner_network_offs=
et(skb) & ~mask);

Presumably this is not needed.

> +}
> +
>  static inline void *skb_gro_network_header(const struct sk_buff *skb)
>  {
> +       const int offset =3D skb_gro_network_offset(skb);
> +
>         if (skb_gro_may_pull(skb, skb_gro_offset(skb)))
> -               return skb_gro_header_fast(skb, skb_network_offset(skb));
> +               return skb_gro_header_fast(skb, offset);
>
> -       return skb_network_header(skb);
> +       return skb->data + offset;
>  }

I would instead add a new offset parameter to this function.

Again, ideally GRO should work without touching any skb->{offset}.

GRO stack should maintain the offsets it needs in its own storage
(stack parameter, or other storage if needed)

Upper stack can not trust any of these skb fields, otherwise we would
have some troubles with napi_reuse_skb()