linux-kernel - Re: [PATCH v1 2/8] iommu: Introduce a new iommu_group_replace

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: <Y90iOAmnBtqQtmiA@ziepe.ca>
Date:   Fri, 3 Feb 2023 11:03:20 -0400
From:   Jason Gunthorpe <jgg@...dia.com>
To:     "Tian, Kevin" <kevin.tian@...el.com>
Cc:     Nicolin Chen <nicolinc@...dia.com>,
        "joro@...tes.org" <joro@...tes.org>,
        "will@...nel.org" <will@...nel.org>,
        "robin.murphy@....com" <robin.murphy@....com>,
        "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
        "shuah@...nel.org" <shuah@...nel.org>,
        "Liu, Yi L" <yi.l.liu@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
        "baolu.lu@...ux.intel.com" <baolu.lu@...ux.intel.com>
Subject: Re: [PATCH v1 2/8] iommu: Introduce a new
 iommu_group_replace_domain() API

On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@...dia.com>
> > Sent: Thursday, February 2, 2023 3:05 PM
> > 
> > All drivers are already required to support changing between active
> > UNMANAGED domains when using their attach_dev ops.
> 
> All drivers which don't have *broken* UNMANAGED domain?

No, all drivers.. It has always been used by VFIO.

> > + */
> > +int iommu_group_replace_domain(struct iommu_group *group,
> > +			       struct iommu_domain *new_domain)
> 
> what actual value does 'replace' give us? It's just a wrapper of
> __iommu_group_set_domain() then calling it set_domain is
> probably clearer. You can clarify the 'replace' behavior in the
> comment.

As the APIs are setup:

attach demands that no domain is currently set (eg the domain must be blocking)

replace permits the domain to be currently set

'set' vs 'attach' is really unclear what the intended difference is.

We could also address this by simply removing the protection from
attach, but it is not so clear if that is safe for the few users.

> > +{
> > +	int ret;
> > +
> > +	if (!new_domain)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&group->mutex);
> > +	ret = __iommu_group_set_domain(group, new_domain);
> > +	if (ret) {
> > +		if (__iommu_group_set_domain(group, group->domain))
> > +			__iommu_group_set_core_domain(group);
> > +	}
> 
> Can you elaborate the error handling here? Ideally if
> __iommu_group_set_domain() fails then group->domain shouldn't
> be changed. 

That isn't what it implements though. The internal helper leaves
things in a mess, it is for the caller to fix it, and it depends on
the caller what that means.

In this case the API cannot retain a hidden reference to the new
domain, so it must be purged, one way or another.

Jason