Re: [PATCH v5 09/13] iommufd/selftest: Add refcount to mock_iommu_device
On Tue, Oct 29, 2024 at 09:02:58AM -0700, Nicolin Chen wrote: > On Tue, Oct 29, 2024 at 12:34:38PM -0300, Jason Gunthorpe wrote: > > On Fri, Oct 25, 2024 at 04:49:49PM -0700, Nicolin Chen wrote: > > > For an iommu_dev that can unplug (so far only this selftest does so), the > > > viommu->iommu_dev pointer has no guarantee of its life cycle after it is > > > copied from the idev->dev->iommu->iommu_dev. > > > > > > Track the user count of the iommu_dev. Postpone the exit routine using a > > > completion, if refcount is unbalanced. The refcount inc/dec will be added > > > in the following patch. > > > > > > Signed-off-by: Nicolin Chen > > > --- > > > drivers/iommu/iommufd/selftest.c | 32 > > > 1 file changed, 24 insertions(+), 8 deletions(-) > > > > Reviewed-by: Jason Gunthorpe > > > > Since this is built into the iommufd module it can't be unloaded > > without also unloading iommufd, which is impossible as long as any > > iommufd FDs are open. So I expect that the WARN_ON can never happen. > > Hmm, I assume we still need this patch then? I was thinking, I think it still is a reasonable example of what it might look like You might include the above remark as a comment above the WARN_ON though. > Could a faulty "--force" possibly trigger it? I'm not sure, I suspect not? Jason
Re: [PATCH v5 09/13] iommufd/selftest: Add refcount to mock_iommu_device
On Tue, Oct 29, 2024 at 12:34:38PM -0300, Jason Gunthorpe wrote: > On Fri, Oct 25, 2024 at 04:49:49PM -0700, Nicolin Chen wrote: > > For an iommu_dev that can unplug (so far only this selftest does so), the > > viommu->iommu_dev pointer has no guarantee of its life cycle after it is > > copied from the idev->dev->iommu->iommu_dev. > > > > Track the user count of the iommu_dev. Postpone the exit routine using a > > completion, if refcount is unbalanced. The refcount inc/dec will be added > > in the following patch. > > > > Signed-off-by: Nicolin Chen > > --- > > drivers/iommu/iommufd/selftest.c | 32 > > 1 file changed, 24 insertions(+), 8 deletions(-) > > Reviewed-by: Jason Gunthorpe > > Since this is built into the iommufd module it can't be unloaded > without also unloading iommufd, which is impossible as long as any > iommufd FDs are open. So I expect that the WARN_ON can never happen. Hmm, I assume we still need this patch then? Could a faulty "--force" possibly trigger it? Nicolin
Re: [PATCH v5 09/13] iommufd/selftest: Add refcount to mock_iommu_device
On Fri, Oct 25, 2024 at 04:49:49PM -0700, Nicolin Chen wrote: > For an iommu_dev that can unplug (so far only this selftest does so), the > viommu->iommu_dev pointer has no guarantee of its life cycle after it is > copied from the idev->dev->iommu->iommu_dev. > > Track the user count of the iommu_dev. Postpone the exit routine using a > completion, if refcount is unbalanced. The refcount inc/dec will be added > in the following patch. > > Signed-off-by: Nicolin Chen > --- > drivers/iommu/iommufd/selftest.c | 32 > 1 file changed, 24 insertions(+), 8 deletions(-) Reviewed-by: Jason Gunthorpe Since this is built into the iommufd module it can't be unloaded without also unloading iommufd, which is impossible as long as any iommufd FDs are open. So I expect that the WARN_ON can never happen. Jason
RE: [PATCH v5 09/13] iommufd/selftest: Add refcount to mock_iommu_device
> From: Nicolin Chen > Sent: Saturday, October 26, 2024 7:50 AM > > For an iommu_dev that can unplug (so far only this selftest does so), the > viommu->iommu_dev pointer has no guarantee of its life cycle after it is > copied from the idev->dev->iommu->iommu_dev. > > Track the user count of the iommu_dev. Postpone the exit routine using a > completion, if refcount is unbalanced. The refcount inc/dec will be added > in the following patch. > > Signed-off-by: Nicolin Chen Reviewed-by: Kevin Tian
