From: Daniel Axtens <dja@axtens•net>
To: Willem de Bruijn <willemdebruijn.kernel@gmail•com>,
Ben Hutchings <ben.hutchings@codethink•co.uk>
Cc: "David S. Miller" <davem@davemloft•net>,
netdev <netdev@vger•kernel.org>,
Eric Dumazet <edumazet@google•com>,
Mahesh Bandewar <maheshb@google•com>,
michael.chan@broadcom•com
Subject: Re: GSO where gso_size is too big for hardware
Date: Wed, 23 Jan 2019 10:41:16 +1100 [thread overview]
Message-ID: <87zhrsp8f7.fsf@linkitivity.dja.id.au> (raw)
In-Reply-To: <CA+FuTSd92upBAyENTat8yKhB+qw8zvYjkT3hWgvyP6F4xDo1Cw@mail.gmail.com>
Willem de Bruijn <willemdebruijn.kernel@gmail•com> writes:
> On Tue, Jan 22, 2019 at 11:24 AM Ben Hutchings
> <ben.hutchings@codethink•co.uk> wrote:
>>
>> Last year you applied these fixes for a potential denial-of-service in
>> the bnx2x driver:
>>
>> commit 2b16f048729bf35e6c28a40cbfad07239f9dcd90
>> Author: Daniel Axtens <dja@axtens•net>
>> Date: Wed Jan 31 14:15:33 2018 +1100
>>
>> net: create skb_gso_validate_mac_len()
>>
>> commit 8914a595110a6eca69a5e275b323f5d09e18f4f9
>> Author: Daniel Axtens <dja@axtens•net>
>> Date: Wed Jan 31 14:15:34 2018 +1100
>>
>> bnx2x: disable GSO where gso_size is too big for hardware
>>
>> However I don't understand why the check is done only in the bnx2x
>> driver. Shouldn't the networking core ensure that gso_size + L3/L4
>> headers is <= the device MTU? If not, is every driver that does TSO
>> expected to check this?
>>
>> Also, should these fixes go to stable? I'm not sure whether you're
>> still handling stable patches for any of the unfixed versions (< 4.16)
>> now.
>>
>> Ben.
>
> Irrespective of the GSO issue, this sounds relevant to this other thread
>
> Stack sends oversize UDP packet to the driver
> https://www.mail-archive.com/netdev@vger.kernel.org/msg279006.html
>
> which discusses a specific cause of larger than MTU packets and its
> effect on the bnxt.
>
> Perhaps these patches were initially applied to the bnx2x driver only,
> because at the time that was the only nic known to lock up on such
> packets? Either way, a device independent validation is indeed
> probably preferable (independent of fixing that lo flapping root
> cause).
I did try to propose a more generic approach:
1) "[PATCH 0/3] Check gso_size of packets when forwarding"
(https://www.spinics.net/lists/netdev/msg478634.html)
In this series I looked just at forwarding, where there is a check
against regular MTU but not against GSO size. I added checks to
is_skb_forwardable and the Open vSwitch forwarding path, but in feedback
it was pointed out that I missed other ways a packet could be forwarded
such as tc-mired and bpf. A more generic approach was desired, so I
proposed:
2) "[PATCH v2 0/4] Check size of packets before sending"
(https://www.spinics.net/lists/netdev/msg480847.html)
This added a check to is_skb_forwardable and another check to
validate_xmit_skb. It was not considered desirable to include this as a
primary fix because it would be very hard to backport, so we included
the fix for the bnx2x card specifically instead. I think the idea was to
come back to this fix later.
I do remember then spending quite a bit of time trying to get the
generic fix sorted out after that. I remember working on the intricacies
of verifing various GSO stuff - I sent some fixes to GSO_BY_FRAGS
handling, and I know I got sidetracked by GSO_DODGY somehow as well. I
think the problem was that without dealing with dodgy, you end up with
edge cases where untrusted providers of dodgy packets can bypass a naive
length check. Anyway, then I got busy - my job at that point was mostly
support-driven and customers keep having new urgent issues! - so I
didn't get to finish it. This was about a year ago, so my recollection
may be wrong and I may have misunderstood things.
Regards,
Daniel
prev parent reply other threads:[~2019-01-22 23:41 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-22 16:24 GSO where gso_size is too big for hardware Ben Hutchings
2019-01-22 17:02 ` Willem de Bruijn
2019-01-22 23:41 ` Daniel Axtens [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87zhrsp8f7.fsf@linkitivity.dja.id.au \
--to=dja@axtens$(echo .)net \
--cc=ben.hutchings@codethink$(echo .)co.uk \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=maheshb@google$(echo .)com \
--cc=michael.chan@broadcom$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=willemdebruijn.kernel@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox