[v2,1/2] PCI/IOV: Revert "PCI/IOV: Serialize sysfs sriov_numvfs reads vs writes"

  If an SR-IOV enabled device is held by vfio, and the device is removed,
vfio will hold device lock and notify userspace of the removal. If
userspace reads the sriov_numvfs sysfs entry, that thread will be blocked
since sriov_numvfs_show() also tries to acquire the device lock. If that
same thread is responsible for releasing the device to vfio, it results in
a deadlock.

The proper way to detect a change to the num_VFs value is to listen for a
sysfs event, not to add a device_lock() on the attribute _show() in the
kernel.

This reverts commit 35ff867b76576e32f34c698ccd11343f7d616204.
Revert had a small conflict, the sprintf() is now changed to sysfs_emit().

Link: https://lore.kernel.org/linux-pci/ZXJI5+f8bUelVXqu@ubuntu/
Suggested-by: Leon Romanovsky <leonro@nvidia.com>
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
Signed-off-by: Jim Harris <jim.harris@samsung.com>
---
 drivers/pci/iov.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)
  

On 2/9/24 3:52 PM, Jim Harris wrote:
> If an SR-IOV enabled device is held by vfio, and the device is removed,
> vfio will hold device lock and notify userspace of the removal. If
> userspace reads the sriov_numvfs sysfs entry, that thread will be blocked
> since sriov_numvfs_show() also tries to acquire the device lock. If that
> same thread is responsible for releasing the device to vfio, it results in
> a deadlock.
>
> The proper way to detect a change to the num_VFs value is to listen for a
> sysfs event, not to add a device_lock() on the attribute _show() in the
> kernel.

Since you are reverting a commit that synchronizes SysFS read
/write, please add some comments about why it is not an
issue anymore.

>
> This reverts commit 35ff867b76576e32f34c698ccd11343f7d616204.
> Revert had a small conflict, the sprintf() is now changed to sysfs_emit().
>
> Link: https://lore.kernel.org/linux-pci/ZXJI5+f8bUelVXqu@ubuntu/
> Suggested-by: Leon Romanovsky <leonro@nvidia.com>
> Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
> Signed-off-by: Jim Harris <jim.harris@samsung.com>
> ---
>  drivers/pci/iov.c |    8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index aaa33e8dc4c9..0ca20cd518d5 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -395,14 +395,8 @@ static ssize_t sriov_numvfs_show(struct device *dev,
>  				 char *buf)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	u16 num_vfs;
> -
> -	/* Serialize vs sriov_numvfs_store() so readers see valid num_VFs */
> -	device_lock(&pdev->dev);
> -	num_vfs = pdev->sriov->num_VFs;
> -	device_unlock(&pdev->dev);
>  
> -	return sysfs_emit(buf, "%u\n", num_vfs);
> +	return sysfs_emit(buf, "%u\n", pdev->sriov->num_VFs);
>  }
>  
>  /*
>
  

On Fri, Feb 09, 2024 at 07:20:28PM -0800, Kuppuswamy Sathyanarayanan wrote:
> 
> On 2/9/24 3:52 PM, Jim Harris wrote:
> > If an SR-IOV enabled device is held by vfio, and the device is removed,
> > vfio will hold device lock and notify userspace of the removal. If
> > userspace reads the sriov_numvfs sysfs entry, that thread will be blocked
> > since sriov_numvfs_show() also tries to acquire the device lock. If that
> > same thread is responsible for releasing the device to vfio, it results in
> > a deadlock.
> >
> > The proper way to detect a change to the num_VFs value is to listen for a
> > sysfs event, not to add a device_lock() on the attribute _show() in the
> > kernel.
> 
> Since you are reverting a commit that synchronizes SysFS read
> /write, please add some comments about why it is not an
> issue anymore.

It was never an issue, the idea that sysfs read and write should be serialized by kernel
is not correct by definition. 

Thanks

> 
> >
> > This reverts commit 35ff867b76576e32f34c698ccd11343f7d616204.
> > Revert had a small conflict, the sprintf() is now changed to sysfs_emit().
> >
> > Link: https://lore.kernel.org/linux-pci/ZXJI5+f8bUelVXqu@ubuntu/
> > Suggested-by: Leon Romanovsky <leonro@nvidia.com>
> > Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
> > Signed-off-by: Jim Harris <jim.harris@samsung.com>
> > ---
> >  drivers/pci/iov.c |    8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> >
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index aaa33e8dc4c9..0ca20cd518d5 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -395,14 +395,8 @@ static ssize_t sriov_numvfs_show(struct device *dev,
> >  				 char *buf)
> >  {
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> > -	u16 num_vfs;
> > -
> > -	/* Serialize vs sriov_numvfs_store() so readers see valid num_VFs */
> > -	device_lock(&pdev->dev);
> > -	num_vfs = pdev->sriov->num_VFs;
> > -	device_unlock(&pdev->dev);
> >  
> > -	return sysfs_emit(buf, "%u\n", num_vfs);
> > +	return sysfs_emit(buf, "%u\n", pdev->sriov->num_VFs);
> >  }
> >  
> >  /*
> >
> -- 
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>
  

On 2/11/24 12:48 AM, Leon Romanovsky wrote:
> On Fri, Feb 09, 2024 at 07:20:28PM -0800, Kuppuswamy Sathyanarayanan wrote:
>> On 2/9/24 3:52 PM, Jim Harris wrote:
>>> If an SR-IOV enabled device is held by vfio, and the device is removed,
>>> vfio will hold device lock and notify userspace of the removal. If
>>> userspace reads the sriov_numvfs sysfs entry, that thread will be blocked
>>> since sriov_numvfs_show() also tries to acquire the device lock. If that
>>> same thread is responsible for releasing the device to vfio, it results in
>>> a deadlock.
>>>
>>> The proper way to detect a change to the num_VFs value is to listen for a
>>> sysfs event, not to add a device_lock() on the attribute _show() in the
>>> kernel.
>> Since you are reverting a commit that synchronizes SysFS read
>> /write, please add some comments about why it is not an
>> issue anymore.
> It was never an issue, the idea that sysfs read and write should be serialized by kernel
> is not correct by definition. 

What:           /sys/bus/pci/devices/.../sriov_numvfs
Date:           November 2012
Contact:        Donald Dutile <ddutile@redhat.com>
Description:
                This file appears when a physical PCIe device supports SR-IOV.
                Userspace applications can read and write to this file to
                determine and control the enablement or disablement of Virtual
                Functions (VFs) on the physical function (PF). A read of this
                file will return the number of VFs that are enabled on this PF.

I am not very clear about the user of this SysFs. But, as per above description,
this sysfs seems to controls the number of VFs. A typical usage is to allow user
to write a value and then read to check the enabled/disabled number of VMs,
right?

If you are not synchronizing, then the value returned may not reflect the actual
number of enabled / disabled VFs. So wont this change affect the existing user
of this SysFS.

>
> Thanks
>
>>> This reverts commit 35ff867b76576e32f34c698ccd11343f7d616204.
>>> Revert had a small conflict, the sprintf() is now changed to sysfs_emit().
>>>
>>> Link: https://lore.kernel.org/linux-pci/ZXJI5+f8bUelVXqu@ubuntu/
>>> Suggested-by: Leon Romanovsky <leonro@nvidia.com>
>>> Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
>>> Signed-off-by: Jim Harris <jim.harris@samsung.com>
>>> ---
>>>  drivers/pci/iov.c |    8 +-------
>>>  1 file changed, 1 insertion(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>> index aaa33e8dc4c9..0ca20cd518d5 100644
>>> --- a/drivers/pci/iov.c
>>> +++ b/drivers/pci/iov.c
>>> @@ -395,14 +395,8 @@ static ssize_t sriov_numvfs_show(struct device *dev,
>>>  				 char *buf)
>>>  {
>>>  	struct pci_dev *pdev = to_pci_dev(dev);
>>> -	u16 num_vfs;
>>> -
>>> -	/* Serialize vs sriov_numvfs_store() so readers see valid num_VFs */
>>> -	device_lock(&pdev->dev);
>>> -	num_vfs = pdev->sriov->num_VFs;
>>> -	device_unlock(&pdev->dev);
>>>  
>>> -	return sysfs_emit(buf, "%u\n", num_vfs);
>>> +	return sysfs_emit(buf, "%u\n", pdev->sriov->num_VFs);
>>>  }
>>>  
>>>  /*
>>>
>> -- 
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
>>
  

On Sun, Feb 11, 2024 at 10:48:44AM +0200, Leon Romanovsky wrote:
> On Fri, Feb 09, 2024 at 07:20:28PM -0800, Kuppuswamy Sathyanarayanan wrote:
> > On 2/9/24 3:52 PM, Jim Harris wrote:
> > > If an SR-IOV enabled device is held by vfio, and the device is removed,
> > > vfio will hold device lock and notify userspace of the removal. If
> > > userspace reads the sriov_numvfs sysfs entry, that thread will be blocked
> > > since sriov_numvfs_show() also tries to acquire the device lock. If that
> > > same thread is responsible for releasing the device to vfio, it results in
> > > a deadlock.
> > >
> > > The proper way to detect a change to the num_VFs value is to listen for a
> > > sysfs event, not to add a device_lock() on the attribute _show() in the
> > > kernel.

The lock was not about detecting a change; Pierre did this:

  ip monitor dev ${DEVICE} | grep --line-buffered "^${id}:" | while read line; do \
    cat ${path}/device/sriov_numvfs; \

which I assume works by listening for sysfs events.  The problem was
that after the event occurred, the sriov_numvfs read got a stale value
(see https://bugzilla.kernel.org/show_bug.cgi?id=202991).

So I would drop this sentence because I don't think it accurately
reflects the reason for 35ff867b7657.

> > Since you are reverting a commit that synchronizes SysFS read
> > /write, please add some comments about why it is not an
> > issue anymore.
> 
> It was never an issue, the idea that sysfs read and write should be
> serialized by kernel is not correct by definition. 

I think it *was* an issue.  The behavior Pierre observed at was
clearly wrong, and we added 35ff867b7657 ("PCI/IOV: Serialize sysfs
sriov_numvfs reads vs writes") to resolve it.

We should try to avoid reintroducing the problem, so I think we should
probably squash these two patches and describe it as a deadlock fix
instead of dismissing 35ff867b7657 as being based on false premises.

It would be awesome if you had time to verify that these patches also
resolve the problem you saw, Pierre.

I think we should also add:

  Fixes: 35ff867b7657 ("PCI/IOV: Serialize sysfs sriov_numvfs reads vs writes")

as a trigger for backporting this to kernels that include
35ff867b7657.

Bjorn

> > > This reverts commit 35ff867b76576e32f34c698ccd11343f7d616204.
> > > Revert had a small conflict, the sprintf() is now changed to sysfs_emit().
> > >
> > > Link: https://lore.kernel.org/linux-pci/ZXJI5+f8bUelVXqu@ubuntu/
> > > Suggested-by: Leon Romanovsky <leonro@nvidia.com>
> > > Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
> > > Signed-off-by: Jim Harris <jim.harris@samsung.com>
> > > ---
> > >  drivers/pci/iov.c |    8 +-------
> > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > > index aaa33e8dc4c9..0ca20cd518d5 100644
> > > --- a/drivers/pci/iov.c
> > > +++ b/drivers/pci/iov.c
> > > @@ -395,14 +395,8 @@ static ssize_t sriov_numvfs_show(struct device *dev,
> > >  				 char *buf)
> > >  {
> > >  	struct pci_dev *pdev = to_pci_dev(dev);
> > > -	u16 num_vfs;
> > > -
> > > -	/* Serialize vs sriov_numvfs_store() so readers see valid num_VFs */
> > > -	device_lock(&pdev->dev);
> > > -	num_vfs = pdev->sriov->num_VFs;
> > > -	device_unlock(&pdev->dev);
> > >  
> > > -	return sysfs_emit(buf, "%u\n", num_vfs);
> > > +	return sysfs_emit(buf, "%u\n", pdev->sriov->num_VFs);
> > >  }
> > >  
> > >  /*
> > >
> > -- 
> > Sathyanarayanan Kuppuswamy
> > Linux Kernel Developer
> >
  

On Mon, Feb 12, 2024 at 02:27:14PM -0600, Bjorn Helgaas wrote:
> On Sun, Feb 11, 2024 at 10:48:44AM +0200, Leon Romanovsky wrote:
> > On Fri, Feb 09, 2024 at 07:20:28PM -0800, Kuppuswamy Sathyanarayanan wrote:
> > > On 2/9/24 3:52 PM, Jim Harris wrote:
> > > > If an SR-IOV enabled device is held by vfio, and the device is removed,
> > > > vfio will hold device lock and notify userspace of the removal. If
> > > > userspace reads the sriov_numvfs sysfs entry, that thread will be blocked
> > > > since sriov_numvfs_show() also tries to acquire the device lock. If that
> > > > same thread is responsible for releasing the device to vfio, it results in
> > > > a deadlock.
> > > >
> > > > The proper way to detect a change to the num_VFs value is to listen for a
> > > > sysfs event, not to add a device_lock() on the attribute _show() in the
> > > > kernel.
> 
> The lock was not about detecting a change; Pierre did this:
> 
>   ip monitor dev ${DEVICE} | grep --line-buffered "^${id}:" | while read line; do \
>     cat ${path}/device/sriov_numvfs; \
> 
> which I assume works by listening for sysfs events.  The problem was
> that after the event occurred, the sriov_numvfs read got a stale value
> (see https://bugzilla.kernel.org/show_bug.cgi?id=202991).

I don't think 'ip monitor dev' listens for any sysfs events. Or at least if
I have this running and write values to sriov_numvfs, I don't see any
output.

It looks like the original bug report was against v5.0 (matching by dates
and the patch file attached). In that code, we have:

    kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
    iov->num_VFs = nr_virtfn;

which is identical to how the code looks today. Is it possible that
userspace could react to this uevent and read the stale num_VFs before
iov->num_VFs gets written here? I mean, theoretically it's possible, but
from the bug report it seems like the scenario Pierre was facing was
100% reproducible.

It would be great if we could get input from Pierre on this. It isn't clear
to me from the bug report what exactly is updating the sriov_numvfs sysfs
entry, and what is triggering that update.

We could also revisit my original suggestion, which was to use a
discrete lock just for this sysfs entry, rather than overloading the
device lock. That probably has lower risk of introducing an unintended
regression.

https://lore.kernel.org/linux-pci/ZXNNQkXzluoyeguu@bgt-140510-bm01.eng.stellus.in/

> 
> So I would drop this sentence because I don't think it accurately
> reflects the reason for 35ff867b7657.
> 
> > > Since you are reverting a commit that synchronizes SysFS read
> > > /write, please add some comments about why it is not an
> > > issue anymore.
> > 
> > It was never an issue, the idea that sysfs read and write should be
> > serialized by kernel is not correct by definition. 
> 
> I think it *was* an issue.  The behavior Pierre observed at was
> clearly wrong, and we added 35ff867b7657 ("PCI/IOV: Serialize sysfs
> sriov_numvfs reads vs writes") to resolve it.
> 
> We should try to avoid reintroducing the problem, so I think we should
> probably squash these two patches and describe it as a deadlock fix
> instead of dismissing 35ff867b7657 as being based on false premises.
> 
> It would be awesome if you had time to verify that these patches also
> resolve the problem you saw, Pierre.
> 
> I think we should also add:
> 
>   Fixes: 35ff867b7657 ("PCI/IOV: Serialize sysfs sriov_numvfs reads vs writes")
> 
> as a trigger for backporting this to kernels that include
> 35ff867b7657.
> 
> Bjorn
> 
> > > > This reverts commit 35ff867b76576e32f34c698ccd11343f7d616204.
> > > > Revert had a small conflict, the sprintf() is now changed to sysfs_emit().
> > > >
> > > > Link: https://lore.kernel.org/linux-pci/ZXJI5+f8bUelVXqu@ubuntu/
> > > > Suggested-by: Leon Romanovsky <leonro@nvidia.com>
> > > > Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
> > > > Signed-off-by: Jim Harris <jim.harris@samsung.com>
> > > > ---
> > > >  drivers/pci/iov.c |    8 +-------
> > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > > > index aaa33e8dc4c9..0ca20cd518d5 100644
> > > > --- a/drivers/pci/iov.c
> > > > +++ b/drivers/pci/iov.c
> > > > @@ -395,14 +395,8 @@ static ssize_t sriov_numvfs_show(struct device *dev,
> > > >  				 char *buf)
> > > >  {
> > > >  	struct pci_dev *pdev = to_pci_dev(dev);
> > > > -	u16 num_vfs;
> > > > -
> > > > -	/* Serialize vs sriov_numvfs_store() so readers see valid num_VFs */
> > > > -	device_lock(&pdev->dev);
> > > > -	num_vfs = pdev->sriov->num_VFs;
> > > > -	device_unlock(&pdev->dev);
> > > >  
> > > > -	return sysfs_emit(buf, "%u\n", num_vfs);
> > > > +	return sysfs_emit(buf, "%u\n", pdev->sriov->num_VFs);
> > > >  }
> > > >  
> > > >  /*
> > > >
> > > -- 
> > > Sathyanarayanan Kuppuswamy
> > > Linux Kernel Developer
> > >
  

On Mon, Feb 12, 2024 at 02:27:14PM -0600, Bjorn Helgaas wrote:
> On Sun, Feb 11, 2024 at 10:48:44AM +0200, Leon Romanovsky wrote:
> > On Fri, Feb 09, 2024 at 07:20:28PM -0800, Kuppuswamy Sathyanarayanan wrote:
> > > On 2/9/24 3:52 PM, Jim Harris wrote:
> > > > If an SR-IOV enabled device is held by vfio, and the device is removed,
> > > > vfio will hold device lock and notify userspace of the removal. If
> > > > userspace reads the sriov_numvfs sysfs entry, that thread will be blocked
> > > > since sriov_numvfs_show() also tries to acquire the device lock. If that
> > > > same thread is responsible for releasing the device to vfio, it results in
> > > > a deadlock.
> > > >
> > > > The proper way to detect a change to the num_VFs value is to listen for a
> > > > sysfs event, not to add a device_lock() on the attribute _show() in the
> > > > kernel.
> 
> The lock was not about detecting a change; Pierre did this:
> 
>   ip monitor dev ${DEVICE} | grep --line-buffered "^${id}:" | while read line; do \
>     cat ${path}/device/sriov_numvfs; \
> 
> which I assume works by listening for sysfs events.  

It is not, "ip monitor ..." listens to netlink events emitted by netdev
core and not sysfs events. Sysfs events are not involved in this case.

> The problem was that after the event occurred, the sriov_numvfs
> read got a stale value (see https://bugzilla.kernel.org/show_bug.cgi?id=202991).

Yes, and it is outcome of such cross-subsytem involvement, which
is racy by definition. Someone can come with even simpler example of why
locking sysfs read and write is not a good idea. 

For example, let's consider the following scenario with two CPUs and
locks on sysfs read and write:

 CPU1					CPU2
 echo 1 > ${path}/device/sriov_numvfs
		 context_switch ->
 					cat ${path}/device/sriov_numvfs
					lock
					return 0
					unlock
		context_switch <-
 lock
 set 1
 unlock

 CPU1					CPU2
 echo 1 > ${path}/device/sriov_numvfs
 lock
 set 1
 unlock
		 context_switch ->
 					cat ${path}/device/sriov_numvfs
					lock
					return 1
					unlock

So same scenario will return different values if user doesn't protect
such case with external to the kernel lock.

But if we return back to Pierre report and if you want to provide
completely bullet proof solution to solve cross-subsystem interaction,
you will need to prohibit device probe till sriov_numvfs update is completed.
However, it is overkill for something that is not a real issue.

> 
> So I would drop this sentence because I don't think it accurately
> reflects the reason for 35ff867b7657.
> 
> > > Since you are reverting a commit that synchronizes SysFS read
> > > /write, please add some comments about why it is not an
> > > issue anymore.
> > 
> > It was never an issue, the idea that sysfs read and write should be
> > serialized by kernel is not correct by definition. 
> 
> I think it *was* an issue.  The behavior Pierre observed at was
> clearly wrong, 

I disagree with this sentence. 

> and we added 35ff867b7657 ("PCI/IOV: Serialize sysfs
> sriov_numvfs reads vs writes") to resolve it.
> 
> We should try to avoid reintroducing the problem, so I think we should
> probably squash these two patches and describe it as a deadlock fix
> instead of dismissing 35ff867b7657 as being based on false premises.
> 
> It would be awesome if you had time to verify that these patches also
> resolve the problem you saw, Pierre.

They won't resolve his problem, because he is not listening to sysfs events,
but rely on something from netdev side.

> 
> I think we should also add:
> 
>   Fixes: 35ff867b7657 ("PCI/IOV: Serialize sysfs sriov_numvfs reads vs writes")
> 
> as a trigger for backporting this to kernels that include
> 35ff867b7657.
> 
> Bjorn
> 
> > > > This reverts commit 35ff867b76576e32f34c698ccd11343f7d616204.
> > > > Revert had a small conflict, the sprintf() is now changed to sysfs_emit().
> > > >
> > > > Link: https://lore.kernel.org/linux-pci/ZXJI5+f8bUelVXqu@ubuntu/
> > > > Suggested-by: Leon Romanovsky <leonro@nvidia.com>
> > > > Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
> > > > Signed-off-by: Jim Harris <jim.harris@samsung.com>
> > > > ---
> > > >  drivers/pci/iov.c |    8 +-------
> > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > > > index aaa33e8dc4c9..0ca20cd518d5 100644
> > > > --- a/drivers/pci/iov.c
> > > > +++ b/drivers/pci/iov.c
> > > > @@ -395,14 +395,8 @@ static ssize_t sriov_numvfs_show(struct device *dev,
> > > >  				 char *buf)
> > > >  {
> > > >  	struct pci_dev *pdev = to_pci_dev(dev);
> > > > -	u16 num_vfs;
> > > > -
> > > > -	/* Serialize vs sriov_numvfs_store() so readers see valid num_VFs */
> > > > -	device_lock(&pdev->dev);
> > > > -	num_vfs = pdev->sriov->num_VFs;
> > > > -	device_unlock(&pdev->dev);
> > > >  
> > > > -	return sysfs_emit(buf, "%u\n", num_vfs);
> > > > +	return sysfs_emit(buf, "%u\n", pdev->sriov->num_VFs);
> > > >  }
> > > >  
> > > >  /*
> > > >
> > > -- 
> > > Sathyanarayanan Kuppuswamy
> > > Linux Kernel Developer
> > > 
>
  

On Mon, Feb 12, 2024 at 10:59:03PM +0000, Jim Harris wrote:
> On Mon, Feb 12, 2024 at 02:27:14PM -0600, Bjorn Helgaas wrote:
> > On Sun, Feb 11, 2024 at 10:48:44AM +0200, Leon Romanovsky wrote:
> > > On Fri, Feb 09, 2024 at 07:20:28PM -0800, Kuppuswamy Sathyanarayanan wrote:
> > > > On 2/9/24 3:52 PM, Jim Harris wrote:
> > > > > If an SR-IOV enabled device is held by vfio, and the device is removed,
> > > > > vfio will hold device lock and notify userspace of the removal. If
> > > > > userspace reads the sriov_numvfs sysfs entry, that thread will be blocked
> > > > > since sriov_numvfs_show() also tries to acquire the device lock. If that
> > > > > same thread is responsible for releasing the device to vfio, it results in
> > > > > a deadlock.
> > > > >
> > > > > The proper way to detect a change to the num_VFs value is to listen for a
> > > > > sysfs event, not to add a device_lock() on the attribute _show() in the
> > > > > kernel.
> > 
> > The lock was not about detecting a change; Pierre did this:
> > 
> >   ip monitor dev ${DEVICE} | grep --line-buffered "^${id}:" | while read line; do \
> >     cat ${path}/device/sriov_numvfs; \
> > 
> > which I assume works by listening for sysfs events.  The problem was
> > that after the event occurred, the sriov_numvfs read got a stale value
> > (see https://bugzilla.kernel.org/show_bug.cgi?id=202991).
> 
> I don't think 'ip monitor dev' listens for any sysfs events. Or at least if
> I have this running and write values to sriov_numvfs, I don't see any
> output.
> 
> It looks like the original bug report was against v5.0 (matching by dates
> and the patch file attached). In that code, we have:
> 
>     kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
>     iov->num_VFs = nr_virtfn;
> 
> which is identical to how the code looks today. Is it possible that
> userspace could react to this uevent and read the stale num_VFs before
> iov->num_VFs gets written here? I mean, theoretically it's possible, but
> from the bug report it seems like the scenario Pierre was facing was
> 100% reproducible.
> 
> It would be great if we could get input from Pierre on this. It isn't clear
> to me from the bug report what exactly is updating the sriov_numvfs sysfs
> entry, and what is triggering that update.
> 
> We could also revisit my original suggestion, which was to use a
> discrete lock just for this sysfs entry, rather than overloading the
> device lock. That probably has lower risk of introducing an unintended
> regression.

The idea that lock issues are need to be solved by adding more locks
doesn't sound good to me.

Thanks

> 
> https://lore.kernel.org/linux-pci/ZXNNQkXzluoyeguu@bgt-140510-bm01.eng.stellus.in/
> 
> > 
> > So I would drop this sentence because I don't think it accurately
> > reflects the reason for 35ff867b7657.
> > 
> > > > Since you are reverting a commit that synchronizes SysFS read
> > > > /write, please add some comments about why it is not an
> > > > issue anymore.
> > > 
> > > It was never an issue, the idea that sysfs read and write should be
> > > serialized by kernel is not correct by definition. 
> > 
> > I think it *was* an issue.  The behavior Pierre observed at was
> > clearly wrong, and we added 35ff867b7657 ("PCI/IOV: Serialize sysfs
> > sriov_numvfs reads vs writes") to resolve it.
> > 
> > We should try to avoid reintroducing the problem, so I think we should
> > probably squash these two patches and describe it as a deadlock fix
> > instead of dismissing 35ff867b7657 as being based on false premises.
> > 
> > It would be awesome if you had time to verify that these patches also
> > resolve the problem you saw, Pierre.
> > 
> > I think we should also add:
> > 
> >   Fixes: 35ff867b7657 ("PCI/IOV: Serialize sysfs sriov_numvfs reads vs writes")
> > 
> > as a trigger for backporting this to kernels that include
> > 35ff867b7657.
> > 
> > Bjorn
> > 
> > > > > This reverts commit 35ff867b76576e32f34c698ccd11343f7d616204.
> > > > > Revert had a small conflict, the sprintf() is now changed to sysfs_emit().
> > > > >
> > > > > Link: https://lore.kernel.org/linux-pci/ZXJI5+f8bUelVXqu@ubuntu/
> > > > > Suggested-by: Leon Romanovsky <leonro@nvidia.com>
> > > > > Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
> > > > > Signed-off-by: Jim Harris <jim.harris@samsung.com>
> > > > > ---
> > > > >  drivers/pci/iov.c |    8 +-------
> > > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > > > > index aaa33e8dc4c9..0ca20cd518d5 100644
> > > > > --- a/drivers/pci/iov.c
> > > > > +++ b/drivers/pci/iov.c
> > > > > @@ -395,14 +395,8 @@ static ssize_t sriov_numvfs_show(struct device *dev,
> > > > >  				 char *buf)
> > > > >  {
> > > > >  	struct pci_dev *pdev = to_pci_dev(dev);
> > > > > -	u16 num_vfs;
> > > > > -
> > > > > -	/* Serialize vs sriov_numvfs_store() so readers see valid num_VFs */
> > > > > -	device_lock(&pdev->dev);
> > > > > -	num_vfs = pdev->sriov->num_VFs;
> > > > > -	device_unlock(&pdev->dev);
> > > > >  
> > > > > -	return sysfs_emit(buf, "%u\n", num_vfs);
> > > > > +	return sysfs_emit(buf, "%u\n", pdev->sriov->num_VFs);
> > > > >  }
> > > > >  
> > > > >  /*
> > > > >
> > > > -- 
> > > > Sathyanarayanan Kuppuswamy
> > > > Linux Kernel Developer
> > > >
  

First sorry for not answering earlier but it is a long time ago. I do
not work on the topic any more (a monitoring tool Skydive, an open
source project no more actively developed as far as I know) and only
have vague memories of it.

> 
>      kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
>      iov->num_VFs = nr_virtfn;
> 
> which is identical to how the code looks today. Is it possible that
> userspace could react to this uevent and read the stale num_VFs before
> iov->num_VFs gets written here? I mean, theoretically it's possible, but
> from the bug report it seems like the scenario Pierre was facing was
> 100% reproducible.
 From my memories yes that was exactly the problem. Any stable method
that could detect the change of configuration in user land and ensure
that we get a reliable value of num_vfs after we received it would be
fine.

Best regards,

Pierre Crégut

____________________________________________________________________________________________________________
Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.
  

On Mon, Feb 12, 2024 at 10:59:03PM +0000, Jim Harris wrote:
> On Mon, Feb 12, 2024 at 02:27:14PM -0600, Bjorn Helgaas wrote:
> > On Sun, Feb 11, 2024 at 10:48:44AM +0200, Leon Romanovsky wrote:
> > > On Fri, Feb 09, 2024 at 07:20:28PM -0800, Kuppuswamy Sathyanarayanan wrote:
> > > > On 2/9/24 3:52 PM, Jim Harris wrote:
> > > > > If an SR-IOV enabled device is held by vfio, and the device is removed,
> > > > > vfio will hold device lock and notify userspace of the removal. If
> > > > > userspace reads the sriov_numvfs sysfs entry, that thread will be blocked
> > > > > since sriov_numvfs_show() also tries to acquire the device lock. If that
> > > > > same thread is responsible for releasing the device to vfio, it results in
> > > > > a deadlock.
> > > > >
> > > > > The proper way to detect a change to the num_VFs value is to listen for a
> > > > > sysfs event, not to add a device_lock() on the attribute _show() in the
> > > > > kernel.
> > 
> > The lock was not about detecting a change; Pierre did this:
> > 
> >   ip monitor dev ${DEVICE} | grep --line-buffered "^${id}:" | while read line; do \
> >     cat ${path}/device/sriov_numvfs; \
> > 
> > which I assume works by listening for sysfs events.  The problem was
> > that after the event occurred, the sriov_numvfs read got a stale value
> > (see https://bugzilla.kernel.org/show_bug.cgi?id=202991).
> 
> I don't think 'ip monitor dev' listens for any sysfs events. Or at least if
> I have this running and write values to sriov_numvfs, I don't see any
> output.

The issue is that the sysfs change inadvertently throws out a netlink
event (or udev event, or whatever) and something can observe that
event and then turn around and read the sysfs and observe a sysfs
result that hasn't caught up to the event launch.

The lock fixed this because it held it across the event launch and the
update of the internal state.

> It looks like the original bug report was against v5.0 (matching by dates
> and the patch file attached). In that code, we have:
> 
>     kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
>     iov->num_VFs = nr_virtfn;

This is a udev event, I suspect the ip monitor event was thrown by
driver binding during the VF creation.

Jason
  

On Tue, Feb 13, 2024 at 09:34:50AM +0200, Leon Romanovsky wrote:
> On Mon, Feb 12, 2024 at 02:27:14PM -0600, Bjorn Helgaas wrote:
> > On Sun, Feb 11, 2024 at 10:48:44AM +0200, Leon Romanovsky wrote:
> > > On Fri, Feb 09, 2024 at 07:20:28PM -0800, Kuppuswamy Sathyanarayanan wrote:
> > > > On 2/9/24 3:52 PM, Jim Harris wrote:
> > > > > If an SR-IOV enabled device is held by vfio, and the device
> > > > > is removed, vfio will hold device lock and notify userspace
> > > > > of the removal. If userspace reads the sriov_numvfs sysfs
> > > > > entry, that thread will be blocked since sriov_numvfs_show()
> > > > > also tries to acquire the device lock. If that same thread
> > > > > is responsible for releasing the device to vfio, it results
> > > > > in a deadlock.
> > > > >
> > > > > The proper way to detect a change to the num_VFs value is to
> > > > > listen for a sysfs event, not to add a device_lock() on the
> > > > > attribute _show() in the kernel.
> > 
> > The lock was not about detecting a change; Pierre did this:
> > 
> >   ip monitor dev ${DEVICE} | grep --line-buffered "^${id}:" | while read line; do \
> >     cat ${path}/device/sriov_numvfs; \
> > 
> > which I assume works by listening for sysfs events.  
> 
> It is not, "ip monitor ..." listens to netlink events emitted by
> netdev core and not sysfs events. Sysfs events are not involved in
> this case.

Thanks for correcting my hasty assumption!

> > The problem was that after the event occurred, the sriov_numvfs
> > read got a stale value (see https://bugzilla.kernel.org/show_bug.cgi?id=202991).
> 
> Yes, and it is outcome of such cross-subsytem involvement, which
> is racy by definition. Someone can come with even simpler example of why
> locking sysfs read and write is not a good idea. 
> 
> For example, let's consider the following scenario with two CPUs and
> locks on sysfs read and write:
> 
>  CPU1					CPU2
>  echo 1 > ${path}/device/sriov_numvfs
> 		 context_switch ->
>  					cat ${path}/device/sriov_numvfs
> 					lock
> 					return 0
> 					unlock
> 		context_switch <-
>  lock
>  set 1
>  unlock
> 
>  CPU1					CPU2
>  echo 1 > ${path}/device/sriov_numvfs
>  lock
>  set 1
>  unlock
> 		 context_switch ->
>  					cat ${path}/device/sriov_numvfs
> 					lock
> 					return 1
> 					unlock
> 
> So same scenario will return different values if user doesn't protect
> such case with external to the kernel lock.
> 
> But if we return back to Pierre report and if you want to provide
> completely bullet proof solution to solve cross-subsystem interaction,
> you will need to prohibit device probe till sriov_numvfs update is completed.
> However, it is overkill for something that is not a real issue.

Pierre wanted to detect the configuration change and learn the new
num_vfs, which seems like a reasonable thing to do.  Is there a way to
do both via netlink or some other mechanism?

> > So I would drop this sentence because I don't think it accurately
> > reflects the reason for 35ff867b7657.
> > 
> > > > Since you are reverting a commit that synchronizes SysFS read
> > > > /write, please add some comments about why it is not an
> > > > issue anymore.
> > > 
> > > It was never an issue, the idea that sysfs read and write should be
> > > serialized by kernel is not correct by definition. 
> > 
> > I think it *was* an issue.  The behavior Pierre observed at was
> > clearly wrong, 
> 
> I disagree with this sentence. 
> 
> > and we added 35ff867b7657 ("PCI/IOV: Serialize sysfs
> > sriov_numvfs reads vs writes") to resolve it.
> > 
> > We should try to avoid reintroducing the problem, so I think we should
> > probably squash these two patches and describe it as a deadlock fix
> > instead of dismissing 35ff867b7657 as being based on false premises.
> > 
> > It would be awesome if you had time to verify that these patches also
> > resolve the problem you saw, Pierre.
> 
> They won't resolve his problem, because he is not listening to sysfs
> events, but rely on something from netdev side.

I guess that means that if we apply this revert, the problem Pierre
reported will return.  Obviously the deadlock is more important than
the inconsistency Pierre observed, but from the user's point of view
this will look like a regression.

Maybe listening to netlink and then looking at sysfs isn't the
"correct" way to do this, but I don't want to just casually break
existing user code.  If we do contemplate doing the revert, at the
very least we should include specific details about what the user code
*should* do instead, at the level of the actual commands to use
instead of "ip monitor dev; cat ${path}/device/sriov_numvfs".

Bjorn