linux-kernel - RE: [PATCH v7 10/28] iommufd/access: Bypass access->ops->unmap for internal use

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BN9PR11MB5276BED9EC2F89E805F6BA678C43A@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Thu, 3 Jul 2025 04:57:34 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Nicolin Chen <nicolinc@...dia.com>
CC: "jgg@...dia.com" <jgg@...dia.com>, "corbet@....net" <corbet@....net>,
	"will@...nel.org" <will@...nel.org>, "bagasdotme@...il.com"
	<bagasdotme@...il.com>, "robin.murphy@....com" <robin.murphy@....com>,
	"joro@...tes.org" <joro@...tes.org>, "thierry.reding@...il.com"
	<thierry.reding@...il.com>, "vdumpa@...dia.com" <vdumpa@...dia.com>,
	"jonathanh@...dia.com" <jonathanh@...dia.com>, "shuah@...nel.org"
	<shuah@...nel.org>, "jsnitsel@...hat.com" <jsnitsel@...hat.com>,
	"nathan@...nel.org" <nathan@...nel.org>, "peterz@...radead.org"
	<peterz@...radead.org>, "Liu, Yi L" <yi.l.liu@...el.com>,
	"mshavit@...gle.com" <mshavit@...gle.com>, "praan@...gle.com"
	<praan@...gle.com>, "zhangzekun11@...wei.com" <zhangzekun11@...wei.com>,
	"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-tegra@...r.kernel.org"
	<linux-tegra@...r.kernel.org>, "linux-kselftest@...r.kernel.org"
	<linux-kselftest@...r.kernel.org>, "patches@...ts.linux.dev"
	<patches@...ts.linux.dev>, "mochs@...dia.com" <mochs@...dia.com>,
	"alok.a.tiwari@...cle.com" <alok.a.tiwari@...cle.com>, "vasant.hegde@....com"
	<vasant.hegde@....com>, "dwmw2@...radead.org" <dwmw2@...radead.org>,
	"baolu.lu@...ux.intel.com" <baolu.lu@...ux.intel.com>
Subject: RE: [PATCH v7 10/28] iommufd/access: Bypass access->ops->unmap for
 internal use

> From: Nicolin Chen <nicolinc@...dia.com>
> Sent: Thursday, July 3, 2025 4:12 AM
> 
> On Wed, Jul 02, 2025 at 09:45:26AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@...dia.com>
> > > Sent: Friday, June 27, 2025 3:35 AM
> > >
> > > +int iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned
> long
> > > iova,
> > > +				unsigned long length)
> > >  {
> > >  	struct iommufd_ioas *ioas =
> > >  		container_of(iopt, struct iommufd_ioas, iopt);
> > >  	struct iommufd_access *access;
> > >  	unsigned long index;
> > > +	int ret = 0;
> > >
> > >  	xa_lock(&ioas->iopt.access_list);
> > > +	/* Bypass any unmap if there is an internal access */
> > > +	xa_for_each(&ioas->iopt.access_list, index, access) {
> > > +		if (iommufd_access_is_internal(access)) {
> > > +			ret = -EBUSY;
> > > +			goto unlock;
> > > +		}
> > > +	}
> > > +
> >
> > hmm all those checks are per iopt. Could do one-off check in
> > iopt_unmap_iova_range() and store the result in a local flag.
> >
> > Then use that flag to decide whether to return -EBUSY if
> > area->num_accesses is true in the loop.
> 
> I don't quite follow this...
> 
> Do you suggest to move this xa_for_each to iopt_unmap_iova_range?

yes

> 
> What's that local flag used for?
> 

I meant something like below:

iopt_unmap_iova_range()
{
	bool internal_access = false;

	down_read(&iopt->domains_rwsem);
	down_write(&iopt->iova_rwsem);
	/* Bypass any unmap if there is an internal access */
	xa_for_each(&iopt->access_list, index, access) {
		if (iommufd_access_is_internal(access)) {
			internal_access = true;
			break;
		}
	}

	while ((area = iopt_area_iter_first(iopt, start, last))) {
		if (area->num_access) {
			if (internal_access) {
				rc = -EBUSY;
				goto out_unlock_iova;
			}
			up_write(&iopt->iova_rwsem);
			up_read(&iopt->domains_rwsem);
			iommufd_access_notify_unmap(iopt, area_first, length);	
		}
	}
}

it checks the access_list in the common path, but the cost should be
negligible when there is no access attached to this iopt. The upside
is that now unmap is denied explicitly in the area loop instead of 
still trying to unmap and then handling errors.

but the current way is also fine. After another thought I'm neutral
to it. 😊