netdev - Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

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]

Date:   Mon, 16 Dec 2019 04:47:38 +0000
From:   Parav Pandit <parav@...lanox.com>
To:     Shannon Nelson <snelson@...sando.io>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

On 12/13/2019 4:10 AM, Shannon Nelson wrote:
> On 12/12/19 2:24 PM, Parav Pandit wrote:
>> On 12/12/2019 3:35 PM, Jakub Kicinski wrote:
>>> On Thu, 12 Dec 2019 11:59:50 -0800, Shannon Nelson wrote:
>>>> On 12/12/19 11:52 AM, Jakub Kicinski wrote:
>>>>> On Thu, 12 Dec 2019 06:53:42 +0000, Parav Pandit wrote:
>>>>>>>    static void ionic_remove(struct pci_dev *pdev)
>>>>>>>    {
>>>>>>>        struct ionic *ionic = pci_get_drvdata(pdev);
>>>>>>> @@ -257,6 +338,9 @@ static void ionic_remove(struct pci_dev *pdev)
>>>>>>>        if (!ionic)
>>>>>>>            return;
>>>>>>>    +    if (pci_num_vf(pdev))
>>>>>>> +        ionic_sriov_configure(pdev, 0);
>>>>>>> +
>>>>>> Usually sriov is left enabled while removing PF.
>>>>>> It is not the role of the pci PF removal to disable it sriov.
>>>>> I don't think that's true. I consider igb and ixgbe to set the
>>>>> standard
>>>>> for legacy SR-IOV handling since they were one of the first (the
>>>>> first?)
>>>>> and Alex Duyck wrote them.
>>>>>
>>>>> mlx4, bnxt and nfp all disable SR-IOV on remove.
>>>> This was my understanding as well, but now I can see that ixgbe and
>>>> i40e
>>>> are both checking for existing VFs in probe and setting up to use them,
>>>> as well as the newer ice driver.  I found this today by looking for
>>>> where they use pci_num_vf().
>>> Right, if the VFs very already enabled on probe they are set up.
>>>
>>> It's a bit of a asymmetric design, in case some other driver left
>>> SR-IOV on, I guess.
>>>
>> I remember on one email thread on netdev list from someone that in one
>> use case, they upgrade the PF driver while VFs are still bound and
>> SR-IOV kept enabled.
>> I am not sure how much it is used in practice/or practical.
>> Such use case may be the reason to keep SR-IOV enabled.
> 
> This brings up a potential corner case where it would be better for the
> driver to use its own num_vfs value rather than relying on the
> pci_num_vf() when answering the ndo_get_vf_*() callbacks, and at least
> the igb may be susceptible.  
Please do not cache num_vfs in driver. Use the pci core's pci_num_vf()
in the new code that you are adding.
More below.
> If the driver hasn't set up its vf[] data
> arrays because there was an error in setting them up in the probe(), and
> later someone tries to get VF statistics, the ndo_get_vf_stats callback
> could end up dereferencing bad pointers because vf is less than
> pci_num_vf() but more than the number of vf[] structs set up by the driver.
> 
> I suppose the argument could be made that PF's probe should if the VF
> config fails, but it might be nice to have the PF driver running to help
> fix up whatever when sideways in the VF configuration.
> 
> sln
> 
I not have strong opinion on letting sriov enabled/disabled on PF device
removal.
But it should be symmetric on probe() and remove() for PF.
If you want to keep it enabled on PF removal, you need to check on probe
and allocate VF metadata you have by using helper function in
sriov_configure() and in probe().
This is followed by mlx5 driver.