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: <20230706130800.85963-1-squirrel.prog@gmail.com>
 <CANn89iKxGVDQba1UCbpYKhNo=rZXuN2j=610DbJx0z=kWa7N3w@mail.gmail.com> <CAJfyFi3OEz2Dz9gopigkVJRa4qCToJ+ob952O_qkOFiNn08LwA@mail.gmail.com>
In-Reply-To: <CAJfyFi3OEz2Dz9gopigkVJRa4qCToJ+ob952O_qkOFiNn08LwA@mail.gmail.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Thu, 6 Jul 2023 17:05:28 +0200
Message-ID: <CANn89iLBO7U40gNDysTD0wCPMAsgUQV3u_T1yzv1oaa=sLKyfQ@mail.gmail.com>
Subject: Re: [PATCH] gro: check returned skb of napi_frags_skb() against NULL
To:     Kaiyu Zhang <squirrel.prog@gmail.com>
Cc:     davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Jul 6, 2023 at 4:35=E2=80=AFPM Kaiyu Zhang <squirrel.prog@gmail.com=
> wrote:
>
> Yes the network adapter needs to be fixed for passing invalid packets to =
GRO. And a message like "dropping impossible skb" should be indicative enou=
gh for developers to do so.
>
> And yet I propose this patch because an immediate system crash caused by =
NULL pointer access after the above warning message is a bit more difficult=
 to analyse on some test bed where developers have to restore kernel messag=
es after a reboot .
>
> Furthermore, the adapter I'm working with is capable of receiving very sm=
all packets (1 or 2 bytes). They insist this is a feature rather than a bug=
. I can, and I will check packet length before passing them to GRO, for thi=
s particular adapter. But maybe a non-crash warning in general might be hel=
pful to other developers who will face the same problem and spare them some=
 effort on debugging.

1 or 2 bytes would still be stored in an skb, right ?

GRO is already very slow, we do not want to make it even slower so
that some developers can save time.
A kernel crash would point to the NULL deref just fine.

If we continue this path, we will soon add "if (!skb) be_kind" to all
functions accepting an skb as a parameter,
then everything else...