lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 9 Mar 2023 21:08:55 -0400 From: Jason Gunthorpe <jgg@...dia.com> To: Lu Baolu <baolu.lu@...ux.intel.com> Cc: iommu@...ts.linux.dev, Joerg Roedel <joro@...tes.org>, Christoph Hellwig <hch@...radead.org>, Kevin Tian <kevin.tian@...el.com>, Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>, linux-kernel@...r.kernel.org Subject: Re: [PATCH v3 3/6] iommu: Same critical region for device release and removal On Mon, Mar 06, 2023 at 10:58:01AM +0800, Lu Baolu wrote: > In a non-driver context, it is crucial to ensure the consistency of a > device's iommu ops. Otherwise, it may result in a situation where a > device is released but it's iommu ops are still used. > > Put the ops->release_device and __iommu_group_remove_device() in a some > group->mutext critical region, so that, as long as group->mutex is held > and the device is in its group's device list, its iommu ops are always > consistent. Add check of group ownership if the released device is the > last one. > > Signed-off-by: Jason Gunthorpe <jgg@...dia.com> > Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com> > --- > drivers/iommu/iommu.c | 30 ++++++++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index bd9b293e07a8..0bcd9625090d 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -507,18 +507,44 @@ static void __iommu_group_release_device(struct iommu_group *group, > > void iommu_release_device(struct device *dev) > { > + struct iommu_group *group = dev->iommu_group; > + struct group_device *device; > const struct iommu_ops *ops; > > - if (!dev->iommu) > + if (!dev->iommu || !group) > return; > > iommu_device_unlink(dev->iommu->iommu_dev, dev); > > + mutex_lock(&group->mutex); > + device = __iommu_group_remove_device(group, dev); > + > + /* > + * If the group has become empty then ownership must have been released, > + * and the current domain must be set back to NULL or the default > + * domain. > + */ > + if (list_empty(&group->devices)) > + WARN_ON(group->owner_cnt || > + group->domain != group->default_domain); > + > + /* > + * release_device() must stop using any attached domain on the device. > + * If there are still other devices in the group they are not effected > + * by this callback. > + * > + * The IOMMU driver must set the device to either an identity or > + * blocking translation and stop using any domain pointer, as it is > + * going to be freed. > + */ > ops = dev_iommu_ops(dev); > if (ops->release_device) > ops->release_device(dev); > + mutex_unlock(&group->mutex); IMHO it is best to be locked like this But for this series, if you run into problems with ARM and release_device I think we could still safely unlock group->mutex before calling this? It would still be OK because the iommu_group_first_dev() will either return NULL so iommu_group_store_type() wills top, or it will block the ultimate call to release here which invalidate's ops. Reviewed-by: Jason Gunthorpe <jgg@...dia.com> Jason
Powered by blists - more mailing lists