Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220610103653.15261-1-zajec5@gmail.com>
In-Reply-To: <20220610103653.15261-1-zajec5@gmail.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Fri, 10 Jun 2022 07:08:28 -0700
Message-ID: <CANn89iLJwdKXcfCHuEkRT7tknsXpD=UgFh-f61M1UAL9b8JMJw@mail.gmail.com>
Subject: Re: [PATCH] net: gro: respect nf_conntrack_checksum for skipping csum verification
To:     =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>
Cc:     openwrt-devel@lists.openwrt.org,
        "David S . Miller" <davem@davemloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        David Ahern <dsahern@kernel.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        netdev <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jun 10, 2022 at 3:37 AM Rafa=C5=82 Mi=C5=82ecki <zajec5@gmail.com> =
wrote:
>
> From: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>
> Netfilter allows disabling checksum verification of incoming packets by
> setting nf_conntrack_checksum variable. That feature is very useful for
> home routers which:
> 1. Most of the time just /forward/ network traffic
> 2. Have slow CPU(s) and csum calculation is a challenge
>
> Some projects like OpenWrt set nf_conntrack_checksum to 0 by default.
>
> It would be nice to allow similar optimization in the GRO code paths.
> This patch simply reuses nf_conntrack_checksum variable to skip
> skb_gro_checksum_validate() calls if applicable.
>

Problem is that GRO will be followed by TSO on the egress side.

TSO will generate segments with recomputed checksums for each one of them.

GRO only keeps one copy of the headers, so does not track original
checksums for all
segments at ingress side.

So if you want to use TSO, GRO has to validate checksums.

I am afraid this nf_conntrack_checksum idea can not be transposed to GRO.

> Signed-off-by: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
> ---
> Hi guys,
>
> I'm not very familiar with net subsystem, please let me know if there is
> a better way of implementing such a feature.
> ---
>  net/ipv4/tcp_offload.c   | 3 +++
>  net/ipv6/tcpv6_offload.c | 3 +++
>  2 files changed, 6 insertions(+)
>
> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index 30abde86db45..734a3c0f3d4a 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
> @@ -311,6 +311,9 @@ struct sk_buff *tcp4_gro_receive(struct list_head *he=
ad, struct sk_buff *skb)
>  {
>         /* Don't bother verifying checksum if we're going to flush anyway=
. */
>         if (!NAPI_GRO_CB(skb)->flush &&
> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
> +           dev_net(skb->dev)->ct.sysctl_checksum &&
> +#endif
>             skb_gro_checksum_validate(skb, IPPROTO_TCP,
>                                       inet_gro_compute_pseudo)) {
>                 NAPI_GRO_CB(skb)->flush =3D 1;
> diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
> index 39db5a226855..2144afa56fa3 100644
> --- a/net/ipv6/tcpv6_offload.c
> +++ b/net/ipv6/tcpv6_offload.c
> @@ -18,6 +18,9 @@ struct sk_buff *tcp6_gro_receive(struct list_head *head=
, struct sk_buff *skb)
>  {
>         /* Don't bother verifying checksum if we're going to flush anyway=
. */
>         if (!NAPI_GRO_CB(skb)->flush &&
> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
> +           dev_net(skb->dev)->ct.sysctl_checksum &&
> +#endif
>             skb_gro_checksum_validate(skb, IPPROTO_TCP,
>                                       ip6_gro_compute_pseudo)) {
>                 NAPI_GRO_CB(skb)->flush =3D 1;
> --
> 2.34.1
>