netdev - Re: [PATCH net 4/4] i40e: Revert "i40e: don't report link up for a VF who hasn't enabled queues"

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: <e27cb35b-a413-ccdd-fa42-d65e7162747f@intel.com>
Date:   Fri, 29 Jan 2021 16:09:20 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Tony Nguyen <anthony.l.nguyen@...el.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Aleksandr Loktionov <aleksandr.loktionov@...el.com>,
        Network Development <netdev@...r.kernel.org>,
        sassmann@...hat.com,
        Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
        Konrad Jankowski <konrad0.jankowski@...el.com>
Subject: Re: [PATCH net 4/4] i40e: Revert "i40e: don't report link up for a VF
 who hasn't enabled queues"

On 1/29/2021 12:23 PM, Willem de Bruijn wrote:
> On Thu, Jan 28, 2021 at 4:45 PM Tony Nguyen <anthony.l.nguyen@...el.com> wrote:
>>
>> From: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
>>
>> This reverts commit 2ad1274fa35ace5c6360762ba48d33b63da2396c
>>
>> VF queues were not brought up when PF was brought up after being
>> downed if the VF driver disabled VFs queues during PF down.
>> This could happen in some older or external VF driver implementations.
>> The problem was that PF driver used vf->queues_enabled as a condition
>> to decide what link-state it would send out which caused the issue.
>>
>> Remove the check for vf->queues_enabled in the VF link notify.
>> Now VF will always be notified of the current link status.
>> Also remove the queues_enabled member from i40e_vf structure as it is
>> not used anymore. Otherwise VNF implementation was broken and caused
>> a link flap.
>>
>> Fixes: 2ad1274fa35a ("i40e: don't report link up for a VF who hasn't enabled")
>> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>> Tested-by: Konrad Jankowski <konrad0.jankowski@...el.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> 
> Doesn't this reintroduce the bug that the original patch aimed to solve?
> 
> Commit 2ad1274fa35a itself was also a fix.
> 

Yea this might re-introduce the issue described in that commit. However
I believe the bug in question was due to very old versions of VF
drivers, (including an ancient version of FreeBSD if I recall).

Perhaps there is some better mechanism for handling this, but I think
reverting this is ok given that it causes problems in certain situations
where the link status wasn't reported properly.

Maybe there is a solution for both cases? but I would worry less about
an issue with the incredibly old VFs because we know that the issue is
fixed in newer VF code and the real problem is that the VF driver is
incorrectly assuming link up means it is ready to send.

Thus, I am comfortable with this revert: It simplifies the state for
both the PF and VF.

I would be open to alternatives as long as the issue described here is
also fixed.

Caveat: I was not involved in the decision to revert this and wasn't
aware of it until now, so I almost certainly have out of date information.

Thanks,
Jake