Received-SPF: pass (google.com: domain of linux-kernel+bounces-63052-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.161 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.161; helo=mail.nvidia.com; pr=C
Date: Tue, 13 Feb 2024 09:37:10 +0200
From: Leon Romanovsky <leonro@nvidia.com>
To: Jim Harris <jim.harris@samsung.com>
CC: Bjorn Helgaas <helgaas@kernel.org>, 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>, Pierre =?iso-8859-1?Q?Cr=E9gut?=
	<pierre.cregut@orange.com>
Subject: Re: [PATCH v2 1/2] PCI/IOV: Revert "PCI/IOV: Serialize sysfs
 sriov_numvfs reads vs writes"
Message-ID: <20240213073710.GB52640@unreal>
References: <20240211084844.GA805332@unreal>
 <20240212202714.GA1142983@bhelgaas>
 <CGME20240212225904uscas1p19a3d7743607a0abe3627fb8c4829f27b@uscas1p1.samsung.com>
 <ZcqitnWTh+zQ+H4p@ubuntu>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZcqitnWTh+zQ+H4p@ubuntu>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Feb 2024 07:37:26.3984
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 1790257f-4982-4586-e115-08dc2c66a02f
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.161];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource:
	SN1PEPF000252A2.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV2PR12MB6013

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
> > > >