Received-SPF: pass (google.com: domain of linux-kernel+bounces-62507-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Jim Harris <jim.harris@samsung.com>
To: Bjorn Helgaas <helgaas@kernel.org>
CC: Leon Romanovsky <leonro@nvidia.com>, Kuppuswamy Sathyanarayanan
	<sathyanarayanan.kuppuswamy@linux.intel.com>, Bjorn Helgaas
	<bhelgaas@google.com>, "linux-pci@vger.kernel.org"
	<linux-pci@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, Jason Gunthorpe <jgg@nvidia.com>, "Alex
 Williamson" <alex.williamson@redhat.com>, =?iso-8859-1?Q?Pierre_Cr=E9gut?=
	<pierre.cregut@orange.com>
Subject: Re: [PATCH v2 1/2] PCI/IOV: Revert
 "PCI/IOV: Serialize sysfs sriov_numvfs reads vs writes"
Thread-Topic: [PATCH v2 1/2] PCI/IOV: Revert "PCI/IOV: Serialize sysfs
 sriov_numvfs reads vs writes"
Thread-Index: AQHaW7MAtwi2xwjfZkaUZDAOVEJQA7EDbuYAgAHuDQCAAlV9AIAAKmoA
Date: Mon, 12 Feb 2024 22:59:03 +0000
Message-ID: <ZcqitnWTh+zQ+H4p@ubuntu>
In-Reply-To: <20240212202714.GA1142983@bhelgaas>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="iso-8859-1"
Content-ID: <3A79157BD081A34FBC00A59BC81DF820@ssi.samsung.com>
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
CMS-TYPE: 301P
References: <20240211084844.GA805332@unreal>
	<20240212202714.GA1142983@bhelgaas>
	<CGME20240212225904uscas1p19a3d7743607a0abe3627fb8c4829f27b@uscas1p1.samsung.com>

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 wr=
ote:
> > > On 2/9/24 3:52 PM, Jim Harris wrote:
> > > > If an SR-IOV enabled device is held by vfio, and the device is remo=
ved,
> > > > vfio will hold device lock and notify userspace of the removal. If
> > > > userspace reads the sriov_numvfs sysfs entry, that thread will be b=
locked
> > > > since sriov_numvfs_show() also tries to acquire the device lock. If=
 that
> > > > same thread is responsible for releasing the device to vfio, it res=
ults 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.
>=20
> The lock was not about detecting a change; Pierre did this:
>=20
>   ip monitor dev ${DEVICE} | grep --line-buffered "^${id}:" | while read =
line; do \
>     cat ${path}/device/sriov_numvfs; \
>=20
> 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=3D202991).

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 =3D 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.stel=
lus.in/

>=20
> So I would drop this sentence because I don't think it accurately
> reflects the reason for 35ff867b7657.
>=20
> > > Since you are reverting a commit that synchronizes SysFS read
> > > /write, please add some comments about why it is not an
> > > issue anymore.
> >=20
> > It was never an issue, the idea that sysfs read and write should be
> > serialized by kernel is not correct by definition.=20
>=20
> 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.
>=20
> 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.
>=20
> It would be awesome if you had time to verify that these patches also
> resolve the problem you saw, Pierre.
>=20
> I think we should also add:
>=20
>   Fixes: 35ff867b7657 ("PCI/IOV: Serialize sysfs sriov_numvfs reads vs wr=
ites")
>=20
> as a trigger for backporting this to kernels that include
> 35ff867b7657.
>=20
> Bjorn
>=20
> > > > 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 =3D to_pci_dev(dev);
> > > > -	u16 num_vfs;
> > > > -
> > > > -	/* Serialize vs sriov_numvfs_store() so readers see valid num_VFs=
 */
> > > > -	device_lock(&pdev->dev);
> > > > -	num_vfs =3D pdev->sriov->num_VFs;
> > > > -	device_unlock(&pdev->dev);
> > > > =20
> > > > -	return sysfs_emit(buf, "%u\n", num_vfs);
> > > > +	return sysfs_emit(buf, "%u\n", pdev->sriov->num_VFs);
> > > >  }
> > > > =20
> > > >  /*
> > > >
> > > --=20
> > > Sathyanarayanan Kuppuswamy
> > > Linux Kernel Developer
> > > =