netdev - Re: [PATCH net-next v3 2/5] net: Disable GRO_HW when generic XDP is installed on a device.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKgT0UcOKMnUTmfyxaRm+wk0q=i4AukN1CqKov4ptmJfJe8kcQ@mail.gmail.com>
Date:   Sun, 10 Dec 2017 19:03:22 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Michael Chan <michael.chan@...adcom.com>
Cc:     David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Andrew Gospodarek <andrew.gospodarek@...adcom.com>,
        Ariel Elior <Ariel.Elior@...ium.com>,
        everest-linux-l2@...ium.com
Subject: Re: [PATCH net-next v3 2/5] net: Disable GRO_HW when generic XDP is
 installed on a device.

On Sat, Dec 9, 2017 at 10:49 PM, Michael Chan <michael.chan@...adcom.com> wrote:
> On Sat, Dec 9, 2017 at 2:37 PM, Alexander Duyck
> <alexander.duyck@...il.com> wrote:
>> On Sat, Dec 9, 2017 at 1:40 PM, Michael Chan <michael.chan@...adcom.com> wrote:
>>> On Sat, Dec 9, 2017 at 10:56 AM, Alexander Duyck
>>> <alexander.duyck@...il.com> wrote:
>>>> I think these two lines are redundant in dev_disable_lro, since
>>>> netdev_update_features should propagate the disable to all of the
>>>> lower devices.
>>>
>>> Right.  But for GRO_HW, there is no automatic propagation.
>>
>> Right, but that is also an issue since the automatic propagation is
>> what prevents LRO from being re-enabled on the lower devices.
>>
>>>> Also this doesn't prevent the lower devices from
>>>> re-enabling gro_hw.
>>>
>>> Right.  You can re-enable LRO on the upper device as well.
>>
>> On the upper device yes, but not on the lower devices. That was what I
>> was getting at. With LRO there is netdev_sync_upper_features() and
>> that prevents you from enabling it if the upper device has it
>> disabled. The problem is right now there is nothing that sets it for
>> the upper devices when they are added to something like a bond so that
>> is one of the pieces that still has to be worked on before we can just
>> use the existing sync logic.
>
> I understand.  But if the user really wants to re-enable LRO, he can
> just re-enable LRO on the upper device first and then re-enable LRO on
> the lower devices.
>
> To permanently disable a feature, I think additional infrastructure
> may be required so that the feature can be cleared in hw_features and
> then re-enabled later when it is allowed.

I'm not saying we have to permanently ban it. But we want to prevent
any mix-ups. If someone re-enables LRO on the upper device and then
goes down the chain manually re-enabling it then they definitely
wanted to do that. However if they disable LRO on the bond they cannot
re-enable it on one of the slaves in the bond. That is the kind of
propagation I am looking at. Basically if I disable some feature in
the Rx path I should not see any frames that make use of that feature
come by after it has been disabled. That is why I am thinking GRO,
LRO, Rx Checksum, and GRO_HW all need to handle this sort of
propagation, but doing it right is going to take some time since we
have to make certain to enable things like Rx checksum on an upper
device then if the feature is present on a lower device and that logic
doesn't exist now and will be needed across multiple drivers such as
bond and team among others.

So for now I would say don't worry about lower devices. Just focus on
the immediate device and we can work on getting the synchronization
between the upper and lower devices sorted out over the next couple of
weeks.