Message-ID: <CAOrHB_COqPYRcG3j6zSB+yY0UMQ4KTPHZhEWYEuLOkEo3ZVW0Q@mail.gmail.com>
Date:   Fri, 19 Jan 2018 13:54:15 -0800
From:   Pravin Shelar <pshelar@....org>
To:     Daniel Axtens <dja@...ens.net>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Manish.Chopra@...ium.com, ovs dev <dev@...nvswitch.org>
Subject: Re: [PATCH 0/3] Check gso_size of packets when forwarding

On Fri, Jan 19, 2018 at 3:58 AM, Daniel Axtens <dja@...ens.net> wrote:
> Pravin Shelar <pshelar@....org> writes:
>
>> On Thu, Jan 18, 2018 at 5:28 PM, Daniel Axtens <dja@...ens.net> wrote:
>>> Pravin Shelar <pshelar@....org> writes:
>>>
>>>> On Thu, Jan 18, 2018 at 5:08 AM, Daniel Axtens <dja@...ens.net> wrote:
>>>>> Pravin Shelar <pshelar@....org> writes:
>>>>>
>>>>>> On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja@...ens.net> wrote:
>>>>>>> When regular packets are forwarded, we validate their size against the
>>>>>>> MTU of the destination device. However, when GSO packets are
>>>>>>> forwarded, we do not validate their size against the MTU. We
>>>>>>> implicitly assume that when they are segmented, the resultant packets
>>>>>>> will be correctly sized.
>>>>>>>
>>>>>>> This is not always the case.
>>>>>>>
>>>>>>> We observed a case where a packet received on an ibmveth device had a
>>>>>>> GSO size of around 10kB. This was forwarded by Open vSwitch to a bnx2x
>>>>>>> device, where it caused a firmware assert. This is described in detail
>>>>>>> at [0] and was the genesis of this series. Rather than fixing it in
>>>>>>> the driver, this series fixes the forwarding path.
>>>>>>>
>>>>>> Are there any other possible forwarding path in networking stack? TC
>>>>>> is one subsystem that could forward such a packet to the bnx2x device,
>>>>>> how is that handled ?
>>>>>
>>>>> So far I have only looked at bridges, openvswitch and macvlan. In
>>>>> general, if the code uses dev_forward_skb() it should automatically be
>>>>> fine as that invokes is_skb_forwardable(), which we patch.
>>>>>
>>>> But there are other ways to forward packets, e.g tc-mirred or bpf
>>>> redirect. We need to handle all these cases rather than fixing one at
>>>> a time. As Jason suggested netif_needs_gso() looks like good function
>>>> to validate if a device is capable of handling given GSO packet.
>>>
>>> I am not entirely sure this is a better solution.
>>>
>>> The biggest reason I am uncomfortable with this is that if
>>> netif_needs_gso() returns true, the skb will be segmented. The segment
>>> sizes will be based on gso_size. Since gso_size is greater than the MTU,
>>> the resulting segments will themselves be over-MTU. Those over-MTU
>>> segments will then be passed to the network card. I think we should not
>>> be creating over-MTU segments; we should instead be dropping the packet
>>> and logging.
>>>
>>
>> Would this oversized segment cause the firmware assert?
>> If this solves the assert issue then I do not see much value in adding
>> checks in fast-path just for logging.
>
> No - I tested this (or rather, as I don't have direct access to a bnx2x
> card, this was tested on my behalf): as long as the packet is not a GSO
> packet, it doesn't cause the crash. So we *could* segment them, I just
> think that knowingly creating invalid segments is not particularly
> pleasant.
>
I agree it is not perfect. But the other proposed patch does not fix
the connectivity issue. It only adds log msg in such cases at cost of
extra checks/code. Therefore I prefer the easier fix for the issue
which also fixes for all cases of packet forwarding rather than just
OVS and Bridge.

Powered by blists - more mailing lists