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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 29 Jan 2021 21:00:02 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     Tony Nguyen <anthony.l.nguyen@...el.com>,
        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 Fri, Jan 29, 2021 at 7:09 PM Jacob Keller <jacob.e.keller@...el.com> wrote:
>
>
>
> 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.

That's reasonable. The original patch is over three years old.

If it is considered safe to revert now, I would just articulate that
point in the commit.