| Message ID | 3-v1-3c8177327a47+256-iommu_group_locking_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refine the locking for dev->iommu_group | expand |
On 2023/7/19 3:05, Jason Gunthorpe wrote: > This implements the common pattern seen in drivers of a single > iommu_group for the entire iommu driver. Implement this in core code > so the drivers that want this can select it from their ops. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/iommu.c | 25 +++++++++++++++++++++++++ > include/linux/iommu.h | 3 +++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 9e41ad4e3219b6..1e0c5d9a0370fb 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -289,6 +289,9 @@ void iommu_device_unregister(struct iommu_device *iommu) > spin_lock(&iommu_device_lock); > list_del(&iommu->list); > spin_unlock(&iommu_device_lock); > + > + /* Pairs with the alloc in generic_single_device_group() */ > + iommu_group_put(iommu->singleton_group); > } > EXPORT_SYMBOL_GPL(iommu_device_unregister); > > @@ -1595,6 +1598,28 @@ struct iommu_group *generic_device_group(struct device *dev) > } > EXPORT_SYMBOL_GPL(generic_device_group); > > +/* > + * Generic device_group call-back function. It just allocates one > + * iommu-group per iommu driver. > + */ > +struct iommu_group *generic_single_device_group(struct device *dev) > +{ > + struct iommu_device *iommu = dev->iommu->iommu_dev; > + > + lockdep_assert_held(&dev_iommu_group_lock); > + > + if (!iommu->singleton_group) { > + struct iommu_group *group; > + > + group = iommu_group_alloc(); > + if (IS_ERR(group)) > + return group; > + iommu->singleton_group = group; > + } > + return iommu_group_ref_get(iommu->singleton_group); > +} > +EXPORT_SYMBOL_GPL(generic_single_device_group); When allocating the singleton group for the first time, the group's refcount is taken twice. This can cause memory leaks even after calling iommu_device_unregister(). Perhaps it can be adjusted as follows? struct iommu_group *generic_single_device_group(struct device *dev) { struct iommu_device *iommu = dev->iommu->iommu_dev; struct iommu_group *group; lockdep_assert_held(&dev_iommu_group_lock); if (iommu->singleton_group) return iommu_group_ref_get(iommu->singleton_group); group = iommu_group_alloc(); if (!IS_ERR(group)) iommu->singleton_group = group; return group; } > + > /* > * Use standard PCI bus topology, isolation features, and DMA alias quirks > * to find or create an IOMMU group for a device. > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index b1dcb1b9b17040..f1e18e81fca78b 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -361,6 +361,7 @@ struct iommu_domain_ops { > * @list: Used by the iommu-core to keep a list of registered iommus > * @ops: iommu-ops for talking to this iommu > * @dev: struct device for sysfs handling > + * @singleton_group: Used internally for drivers that have only one group > * @max_pasids: number of supported PASIDs > */ > struct iommu_device { > @@ -368,6 +369,7 @@ struct iommu_device { > const struct iommu_ops *ops; > struct fwnode_handle *fwnode; > struct device *dev; > + struct iommu_group *singleton_group; > u32 max_pasids; > }; > > @@ -640,6 +642,7 @@ extern struct iommu_group *pci_device_group(struct device *dev); > extern struct iommu_group *generic_device_group(struct device *dev); > /* FSL-MC device grouping function */ > struct iommu_group *fsl_mc_device_group(struct device *dev); > +extern struct iommu_group *generic_single_device_group(struct device *dev); "extern" is not necessary. > > /** > * struct iommu_fwspec - per-device IOMMU instance data Best regards, baolu
On Thu, Jul 20, 2023 at 03:39:27PM +0800, Baolu Lu wrote: > On 2023/7/19 3:05, Jason Gunthorpe wrote: > > This implements the common pattern seen in drivers of a single > > iommu_group for the entire iommu driver. Implement this in core code > > so the drivers that want this can select it from their ops. > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > drivers/iommu/iommu.c | 25 +++++++++++++++++++++++++ > > include/linux/iommu.h | 3 +++ > > 2 files changed, 28 insertions(+) > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > > index 9e41ad4e3219b6..1e0c5d9a0370fb 100644 > > --- a/drivers/iommu/iommu.c > > +++ b/drivers/iommu/iommu.c > > @@ -289,6 +289,9 @@ void iommu_device_unregister(struct iommu_device *iommu) > > spin_lock(&iommu_device_lock); > > list_del(&iommu->list); > > spin_unlock(&iommu_device_lock); > > + > > + /* Pairs with the alloc in generic_single_device_group() */ > > + iommu_group_put(iommu->singleton_group); > > } > > EXPORT_SYMBOL_GPL(iommu_device_unregister); > > @@ -1595,6 +1598,28 @@ struct iommu_group *generic_device_group(struct device *dev) > > } > > EXPORT_SYMBOL_GPL(generic_device_group); > > +/* > > + * Generic device_group call-back function. It just allocates one > > + * iommu-group per iommu driver. > > + */ > > +struct iommu_group *generic_single_device_group(struct device *dev) > > +{ > > + struct iommu_device *iommu = dev->iommu->iommu_dev; > > + > > + lockdep_assert_held(&dev_iommu_group_lock); > > + > > + if (!iommu->singleton_group) { > > + struct iommu_group *group; > > + > > + group = iommu_group_alloc(); > > + if (IS_ERR(group)) > > + return group; > > + iommu->singleton_group = group; > > + } > > + return iommu_group_ref_get(iommu->singleton_group); > > +} > > +EXPORT_SYMBOL_GPL(generic_single_device_group); > > When allocating the singleton group for the first time, the group's > refcount is taken twice. Yes, that is correct. The refcount from alloc belongs to iommu->singleton_group and the pair'd put is here: @@ -289,6 +289,9 @@ void iommu_device_unregister(struct iommu_device *iommu) spin_lock(&iommu_device_lock); list_del(&iommu->list); spin_unlock(&iommu_device_lock); + + /* Pairs with the alloc in generic_single_device_group() */ + iommu_group_put(iommu->singleton_group); } The refcount from iommu_group_ref_get() belongs to the caller and the caller must have a paired put. > struct iommu_group *generic_single_device_group(struct device *dev) > { > struct iommu_device *iommu = dev->iommu->iommu_dev; > struct iommu_group *group; > > lockdep_assert_held(&dev_iommu_group_lock); > > if (iommu->singleton_group) > return iommu_group_ref_get(iommu->singleton_group); > > group = iommu_group_alloc(); > if (!IS_ERR(group)) > iommu->singleton_group = group; > > return group; This will UAF the iommu->singleton_group, consider a caller that does: iommu_group_put(generic_single_device_group(dev)) Jason
On 2023/7/20 20:04, Jason Gunthorpe wrote: > On Thu, Jul 20, 2023 at 03:39:27PM +0800, Baolu Lu wrote: >> On 2023/7/19 3:05, Jason Gunthorpe wrote: >>> This implements the common pattern seen in drivers of a single >>> iommu_group for the entire iommu driver. Implement this in core code >>> so the drivers that want this can select it from their ops. >>> >>> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com> >>> --- >>> drivers/iommu/iommu.c | 25 +++++++++++++++++++++++++ >>> include/linux/iommu.h | 3 +++ >>> 2 files changed, 28 insertions(+) >>> >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>> index 9e41ad4e3219b6..1e0c5d9a0370fb 100644 >>> --- a/drivers/iommu/iommu.c >>> +++ b/drivers/iommu/iommu.c >>> @@ -289,6 +289,9 @@ void iommu_device_unregister(struct iommu_device *iommu) >>> spin_lock(&iommu_device_lock); >>> list_del(&iommu->list); >>> spin_unlock(&iommu_device_lock); >>> + >>> + /* Pairs with the alloc in generic_single_device_group() */ >>> + iommu_group_put(iommu->singleton_group); >>> } >>> EXPORT_SYMBOL_GPL(iommu_device_unregister); >>> @@ -1595,6 +1598,28 @@ struct iommu_group *generic_device_group(struct device *dev) >>> } >>> EXPORT_SYMBOL_GPL(generic_device_group); >>> +/* >>> + * Generic device_group call-back function. It just allocates one >>> + * iommu-group per iommu driver. >>> + */ >>> +struct iommu_group *generic_single_device_group(struct device *dev) >>> +{ >>> + struct iommu_device *iommu = dev->iommu->iommu_dev; >>> + >>> + lockdep_assert_held(&dev_iommu_group_lock); >>> + >>> + if (!iommu->singleton_group) { >>> + struct iommu_group *group; >>> + >>> + group = iommu_group_alloc(); >>> + if (IS_ERR(group)) >>> + return group; >>> + iommu->singleton_group = group; >>> + } >>> + return iommu_group_ref_get(iommu->singleton_group); >>> +} >>> +EXPORT_SYMBOL_GPL(generic_single_device_group); >> When allocating the singleton group for the first time, the group's >> refcount is taken twice. > Yes, that is correct. > > The refcount from alloc belongs to iommu->singleton_group and the > pair'd put is here: > > @@ -289,6 +289,9 @@ void iommu_device_unregister(struct iommu_device *iommu) > spin_lock(&iommu_device_lock); > list_del(&iommu->list); > spin_unlock(&iommu_device_lock); > + > + /* Pairs with the alloc in generic_single_device_group() */ > + iommu_group_put(iommu->singleton_group); > } > > The refcount from iommu_group_ref_get() belongs to the caller and the > caller must have a paired put. Oh, yes! The extra reference counter is paired with above put. Thanks for the explanation. Then, another small comment: iommu->singleton_group will be freed with above put, right? Do you need to set iommu->singleton_group to NULL? Given that iommu_device is not freed here. Best regards, baolu
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Wednesday, July 19, 2023 3:06 AM > > This implements the common pattern seen in drivers of a single > iommu_group for the entire iommu driver. Implement this in core code > so the drivers that want this can select it from their ops. strictly speaking it's per-iommu-instance group.
On Thu, Jul 20, 2023 at 10:01:54PM +0800, Baolu Lu wrote: > Then, another small comment: > > iommu->singleton_group will be freed with above put, right? Do you need > to set iommu->singleton_group to NULL? Given that iommu_device is not > freed here. Well, I think the general API is we expect the caller to free the iommu_device after calling unregister so this would be like all the other places we free or put something then go on to release the memory. At the very least if the caller thinks it should re-use the iommu_device then it needs to zero it. Notice this also doesn't hold the lock while putting it, we require no concurrent calls to probe with unregister. Jason
On 2023/7/22 1:19, Jason Gunthorpe wrote: > On Thu, Jul 20, 2023 at 10:01:54PM +0800, Baolu Lu wrote: > >> Then, another small comment: >> >> iommu->singleton_group will be freed with above put, right? Do you need >> to set iommu->singleton_group to NULL? Given that iommu_device is not >> freed here. > Well, I think the general API is we expect the caller to free the > iommu_device after calling unregister so this would be like all the > other places we free or put something then go on to release the > memory. > > At the very least if the caller thinks it should re-use the > iommu_device then it needs to zero it. > > Notice this also doesn't hold the lock while putting it, we require no > concurrent calls to probe with unregister. Fair enough. Thanks for the explanation. Best regards, baolu
On 2023/7/19 3:05, Jason Gunthorpe wrote: > This implements the common pattern seen in drivers of a single > iommu_group for the entire iommu driver. Implement this in core code > so the drivers that want this can select it from their ops. > > Signed-off-by: Jason Gunthorpe<jgg@nvidia.com> > --- > drivers/iommu/iommu.c | 25 +++++++++++++++++++++++++ > include/linux/iommu.h | 3 +++ > 2 files changed, 28 insertions(+) Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> Best regards, baolu
On Fri, Jul 21, 2023 at 07:17:39AM +0000, Tian, Kevin wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Wednesday, July 19, 2023 3:06 AM > > > > This implements the common pattern seen in drivers of a single > > iommu_group for the entire iommu driver. Implement this in core code > > so the drivers that want this can select it from their ops. > > strictly speaking it's per-iommu-instance group.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 9e41ad4e3219b6..1e0c5d9a0370fb 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -289,6 +289,9 @@ void iommu_device_unregister(struct iommu_device *iommu) spin_lock(&iommu_device_lock); list_del(&iommu->list); spin_unlock(&iommu_device_lock); + + /* Pairs with the alloc in generic_single_device_group() */ + iommu_group_put(iommu->singleton_group); } EXPORT_SYMBOL_GPL(iommu_device_unregister); @@ -1595,6 +1598,28 @@ struct iommu_group *generic_device_group(struct device *dev) } EXPORT_SYMBOL_GPL(generic_device_group); +/* + * Generic device_group call-back function. It just allocates one + * iommu-group per iommu driver. + */ +struct iommu_group *generic_single_device_group(struct device *dev) +{ + struct iommu_device *iommu = dev->iommu->iommu_dev; + + lockdep_assert_held(&dev_iommu_group_lock); + + if (!iommu->singleton_group) { + struct iommu_group *group; + + group = iommu_group_alloc(); + if (IS_ERR(group)) + return group; + iommu->singleton_group = group; + } + return iommu_group_ref_get(iommu->singleton_group); +} +EXPORT_SYMBOL_GPL(generic_single_device_group); + /* * Use standard PCI bus topology, isolation features, and DMA alias quirks * to find or create an IOMMU group for a device. diff --git a/include/linux/iommu.h b/include/linux/iommu.h index b1dcb1b9b17040..f1e18e81fca78b 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -361,6 +361,7 @@ struct iommu_domain_ops { * @list: Used by the iommu-core to keep a list of registered iommus * @ops: iommu-ops for talking to this iommu * @dev: struct device for sysfs handling + * @singleton_group: Used internally for drivers that have only one group * @max_pasids: number of supported PASIDs */ struct iommu_device { @@ -368,6 +369,7 @@ struct iommu_device { const struct iommu_ops *ops; struct fwnode_handle *fwnode; struct device *dev; + struct iommu_group *singleton_group; u32 max_pasids; }; @@ -640,6 +642,7 @@ extern struct iommu_group *pci_device_group(struct device *dev); extern struct iommu_group *generic_device_group(struct device *dev); /* FSL-MC device grouping function */ struct iommu_group *fsl_mc_device_group(struct device *dev); +extern struct iommu_group *generic_single_device_group(struct device *dev); /** * struct iommu_fwspec - per-device IOMMU instance data
This implements the common pattern seen in drivers of a single iommu_group for the entire iommu driver. Implement this in core code so the drivers that want this can select it from their ops. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/iommu.c | 25 +++++++++++++++++++++++++ include/linux/iommu.h | 3 +++ 2 files changed, 28 insertions(+)