Message-ID: <87zhrsp8f7.fsf@linkitivity.dja.id.au>
Date:   Wed, 23 Jan 2019 10:41:16 +1100
From:   Daniel Axtens <dja@...ens.net>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Ben Hutchings <ben.hutchings@...ethink.co.uk>
Cc:     "David S. Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Mahesh Bandewar <maheshb@...gle.com>, michael.chan@...adcom.com
Subject: Re: GSO where gso_size is too big for hardware

Willem de Bruijn <willemdebruijn.kernel@...il.com> writes:

> On Tue, Jan 22, 2019 at 11:24 AM Ben Hutchings
> <ben.hutchings@...ethink.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@...ens.net>
>> Date:   Wed Jan 31 14:15:33 2018 +1100
>>
>>     net: create skb_gso_validate_mac_len()
>>
>> commit 8914a595110a6eca69a5e275b323f5d09e18f4f9
>> Author: Daniel Axtens <dja@...ens.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

Powered by blists - more mailing lists