netdev - Re: [PATCH V1 mlx5-next 04/13] PCI/IOV: Allow SRIOV VF drivers to reach the drvdata of a PF

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]

Message-ID: <0b3d2152-d925-4bca-bd81-447c186b4b41@nvidia.com>
Date:   Sun, 17 Oct 2021 16:43:17 +0300
From:   Yishai Hadas <yishaih@...dia.com>
To:     Alex Williamson <alex.williamson@...hat.com>, <bhelgaas@...gle.com>
CC:     <jgg@...dia.com>, <saeedm@...dia.com>, <linux-pci@...r.kernel.org>,
        <kvm@...r.kernel.org>, <netdev@...r.kernel.org>, <kuba@...nel.org>,
        <leonro@...dia.com>, <kwankhede@...dia.com>,
        <mgurtovoy@...dia.com>, <maorg@...dia.com>
Subject: Re: [PATCH V1 mlx5-next 04/13] PCI/IOV: Allow SRIOV VF drivers to
 reach the drvdata of a PF

On 10/15/2021 1:11 AM, Alex Williamson wrote:
> On Wed, 13 Oct 2021 12:46:58 +0300
> Yishai Hadas <yishaih@...dia.com> wrote:
>
>> From: Jason Gunthorpe <jgg@...dia.com>
>>
>> There are some cases where a SRIOV VF driver will need to reach into and
>> interact with the PF driver. This requires accessing the drvdata of the PF.
>>
>> Provide a function pci_iov_get_pf_drvdata() to return this PF drvdata in a
>> safe way. Normally accessing a drvdata of a foreign struct device would be
>> done using the device_lock() to protect against device driver
>> probe()/remove() races.
>>
>> However, due to the design of pci_enable_sriov() this will result in a
>> ABBA deadlock on the device_lock as the PF's device_lock is held during PF
>> sriov_configure() while calling pci_enable_sriov() which in turn holds the
>> VF's device_lock while calling VF probe(), and similarly for remove.
>>
>> This means the VF driver can never obtain the PF's device_lock.
>>
>> Instead use the implicit locking created by pci_enable/disable_sriov(). A
>> VF driver can access its PF drvdata only while its own driver is attached,
>> and the PF driver can control access to its own drvdata based on when it
>> calls pci_enable/disable_sriov().
>>
>> To use this API the PF driver will setup the PF drvdata in the probe()
>> function. pci_enable_sriov() is only called from sriov_configure() which
>> cannot happen until probe() completes, ensuring no VF races with drvdata
>> setup.
>>
>> For removal, the PF driver must call pci_disable_sriov() in its remove
>> function before destroying any of the drvdata. This ensures that all VF
>> drivers are unbound before returning, fencing concurrent access to the
>> drvdata.
>>
>> The introduction of a new function to do this access makes clear the
>> special locking scheme and the documents the requirements on the PF/VF
>> drivers using this.
>>
>> Signed-off-by: Jason Gunthorpe <jgg@...dia.com>
>> Signed-off-by: Leon Romanovsky <leonro@...dia.com>
>> Signed-off-by: Yishai Hadas <yishaih@...dia.com>
>> ---
>>   drivers/pci/iov.c   | 29 +++++++++++++++++++++++++++++
>>   include/linux/pci.h |  7 +++++++
>>   2 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index e7751fa3fe0b..ca696730f761 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -47,6 +47,35 @@ int pci_iov_vf_id(struct pci_dev *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(pci_iov_vf_id);
>>   
>> +/**
>> + * pci_iov_get_pf_drvdata - Return the drvdata of a PF
>> + * @dev - VF pci_dev
>> + * @pf_driver - Device driver required to own the PF
>> + *
>> + * This must be called from a context that ensures that a VF driver is attached.
>> + * The value returned is invalid once the VF driver completes its remove()
>> + * callback.
>> + *
>> + * Locking is achieved by the driver core. A VF driver cannot be probed until
>> + * pci_enable_sriov() is called and pci_disable_sriov() does not return until
>> + * all VF drivers have completed their remove().
>> + *
>> + * The PF driver must call pci_disable_sriov() before it begins to destroy the
>> + * drvdata.
>> + */
>> +void *pci_iov_get_pf_drvdata(struct pci_dev *dev, struct pci_driver *pf_driver)
>> +{
>> +	struct pci_dev *pf_dev;
>> +
>> +	if (dev->is_physfn)
>> +		return ERR_PTR(-EINVAL);
> I think we're trying to make this only accessible to VFs, so shouldn't
> we test (!dev->is_virtfn)?  is_physfn will be zero for either a PF with
> failed SR-IOV configuration or for a non-SR-IOV device afaict.  Thanks,
>
> Alex
>

Yes, this should be accessible only for VFs.

We can go with your suggestion to explicitly check (!dev->is_virtfn) as 
this seems cleaner and safer as you mentioned.

We already got ACK on this patch from Bjorn but as your suggestion seems 
straight forward I may put the Acked-by as part of V2 in any case.

Yishai