linux-kernel - Re: [PATCH v2 02/22] iommufd: Use iommu_user_domain

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: <20240605120223.GD791043@ziepe.ca>
Date: Wed, 5 Jun 2024 09:02:23 -0300
From: Jason Gunthorpe <jgg@...pe.ca>
To: Baolu Lu <baolu.lu@...ux.intel.com>
Cc: Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
	Robin Murphy <robin.murphy@....com>,
	Kevin Tian <kevin.tian@...el.com>, Yi Liu <yi.l.liu@...el.com>,
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
	Kalle Valo <kvalo@...nel.org>,
	Bjorn Andersson <andersson@...nel.org>,
	Mathieu Poirier <mathieu.poirier@...aro.org>,
	Alex Williamson <alex.williamson@...hat.com>, mst@...hat.com,
	Jason Wang <jasowang@...hat.com>,
	Thierry Reding <thierry.reding@...il.com>,
	Jonathan Hunter <jonathanh@...dia.com>,
	Mikko Perttunen <mperttunen@...dia.com>, iommu@...ts.linux.dev,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 02/22] iommufd: Use iommu_user_domain_alloc()

On Wed, Jun 05, 2024 at 10:17:07AM +0800, Baolu Lu wrote:
> On 6/5/24 12:51 AM, Jason Gunthorpe wrote:
> > On Tue, Jun 04, 2024 at 09:51:14AM +0800, Lu Baolu wrote:
> > > Replace iommu_domain_alloc() with iommu_user_domain_alloc().
> > > 
> > > Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
> > > ---
> > >   drivers/iommu/iommufd/hw_pagetable.c | 20 +++++---------------
> > >   1 file changed, 5 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> > > index 33d142f8057d..ada05fccb36a 100644
> > > --- a/drivers/iommu/iommufd/hw_pagetable.c
> > > +++ b/drivers/iommu/iommufd/hw_pagetable.c
> > > @@ -127,21 +127,11 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
> > >   	hwpt_paging->ioas = ioas;
> > >   	hwpt_paging->nest_parent = flags & IOMMU_HWPT_ALLOC_NEST_PARENT;
> > > -	if (ops->domain_alloc_user) {
> > > -		hwpt->domain = ops->domain_alloc_user(idev->dev, flags, NULL,
> > > -						      user_data);
> >                                                       ^^^^^^^^^^^^
> > 
> > > -		if (IS_ERR(hwpt->domain)) {
> > > -			rc = PTR_ERR(hwpt->domain);
> > > -			hwpt->domain = NULL;
> > > -			goto out_abort;
> > > -		}
> > > -		hwpt->domain->owner = ops;
> > > -	} else {
> > > -		hwpt->domain = iommu_domain_alloc(idev->dev->bus);
> > > -		if (!hwpt->domain) {
> > > -			rc = -ENOMEM;
> > > -			goto out_abort;
> > > -		}
> > > +	hwpt->domain = iommu_user_domain_alloc(idev->dev, flags);
> > > +	if (IS_ERR(hwpt->domain)) {
> > 
> > Where did the user_data go???
> 
> The user_data is not used in allocating a paging user domain, so I
> skipped it.

That's not true.. We have no driver using it right now, but it is
definately part of the uAPI and a driver could start using it. That is
why it was hooked up in the first place.

> In my first try, I simply replaced iommu_domain_alloc() with a new
> paging domain allocation interface. On second thought, it occurred to me
> why not use separate interfaces for different purposes? Even though from
> an iommu perspective, they both use paging domains.

If we want to do that then it needs to forward all the args

> The @flags and @user_data are defined in uapi/linux/iommufd.h, which is
> specific to iommufd. Wrapping them in a common interface for broader use
> seems awkward.

Right, you'd have to forward declare the struct and just let it
be. Really nothing but iommufd could call this API.
 
> So, perhaps we could return to my original idea? Let's just expose one
> interface, iommu_paging_domain_alloc(), and replace iommu_domain_alloc()
> with it everywhere?

That's OK too, this above doesn't really need to be changed, some of
the concept here was that iommufd-only ops would just be directly
called by iommufd itself, to discourage future abuse.

Jason