linux-kernel - Re: [PATCH 1/2] iommufd/selftest: Use a fwnode to distinguish devices

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: <20231128163342.GI432016@ziepe.ca>
Date:   Tue, 28 Nov 2023 12:33:42 -0400
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Robin Murphy <robin.murphy@....com>
Cc:     joro@...tes.org, kevin.tian@...el.com, will@...nel.org,
        iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] iommufd/selftest: Use a fwnode to distinguish devices

On Tue, Nov 28, 2023 at 04:02:42PM +0000, Robin Murphy wrote:
> On 28/11/2023 2:43 pm, Jason Gunthorpe wrote:
> > On Tue, Nov 28, 2023 at 10:42:11AM +0000, Robin Murphy wrote:
> > > With bus ops gone, the trick of registering against a specific bus no
> > > longer really works, and we start getting given devices from other buses
> > > to probe,
> > 
> > Make sense
> > 
> > > which leads to spurious groups for devices with no IOMMU on
> > > arm64,
> > 
> > I'm not sure I'm fully understanding what this means?
> 
> It means on my arm64 ACPI system, random platform devices which are created
> after iommufd_test_init() has run get successfully probed by the mock
> driver, unexpectedly:

Okay that is what I guessed

> > I guess that the mock driver is matching random things once it starts
> > being called all the time because this is missing:
> > 
> >   static struct iommu_device *mock_probe_device(struct device *dev)
> >   {
> > +       if (dev->bus != &iommufd_mock_bus_type)
> > +               return -ENODEV;
> >          return &mock_iommu_device;
> >   }
> > 
> > Is that sufficient to solve the problem?
> 
> Unfortunately not...

I see, so we create the other problem that without bus ops we don't
get to have two 'global' drivers and with the above mock won't probe
on x86.

> I did intend coexistence to work on x86 too, where the "other" driver would
> be virtio-iommu using fwnodes, so aligning the mock driver that way seemed
> far neater than any more special-case hacks in core code.

Lets just do the above and what I suggested earlier. This is from a
WIP tree I have, it shows the idea but needs other stuff to work. If
you agree I'll pull its parts out and post a clean version of them.

commit 51c9a54cc111b4b31af6a0527015db82e782e1d3
Author: Jason Gunthorpe <jgg@...pe.ca>
Date:   Tue Nov 28 11:54:47 2023 -0400

    iommu: Call all drivers if there is no fwspec
    
    Real systems only have one ops, so this effectively invokes the single op
    in the system to probe each device. If there are multiple ops we invoke
    each one once, and drivers that don't understand the struct device should
    return -ENODEV.
    
    Signed-off-by: Jason Gunthorpe <jgg@...dia.com>

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7468a64778931b..54e3f14429b3b4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -241,6 +241,26 @@ static int remove_iommu_group(struct device *dev, void *data)
 	return 0;
 }
 
+static void iommu_device_add(struct iommu_device *iommu)
+{
+	struct iommu_device *cur;
+
+	/*
+	 * Keep the iommu_device_list grouped by ops so that
+	 * iommu_find_init_device() works efficiently.
+	 */
+	mutex_lock(&iommu_probe_device_lock);
+	list_for_each_entry(cur, &iommu_device_list, list) {
+		if (cur->ops == iommu->ops) {
+			list_add(&iommu->list, &cur->list);
+			goto out;
+		}
+	}
+	list_add(&iommu->list, &iommu_device_list);
+out:
+	mutex_unlock(&iommu_probe_device_lock);
+}
+
 /**
  * iommu_device_register() - Register an IOMMU hardware instance
  * @iommu: IOMMU handle for the instance
@@ -262,9 +282,7 @@ int iommu_device_register(struct iommu_device *iommu,
 	if (hwdev)
 		iommu->fwnode = dev_fwnode(hwdev);
 
-	mutex_lock(&iommu_probe_device_lock);
-	list_add_tail(&iommu->list, &iommu_device_list);
-	mutex_unlock(&iommu_probe_device_lock);
+	iommu_device_add(iommu);
 
 	for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++)
 		err = bus_iommu_probe(iommu_buses[i]);
@@ -502,6 +520,29 @@ static void iommu_deinit_device(struct device *dev)
 
 DEFINE_MUTEX(iommu_probe_device_lock);
 
+static int iommu_find_init_device(struct iommu_probe_info *pinf)
+{
+	const struct iommu_ops *ops = NULL;
+	struct iommu_device *iommu;
+	int ret;
+
+	lockdep_assert_held(&iommu_probe_device_lock);
+
+	/*
+	 * Each unique ops gets a chance to claim the device, -ENODEV means the
+	 * driver does not support the device.
+	 */
+	list_for_each_entry(iommu, &iommu_device_list, list) {
+		if (iommu->ops != ops) {
+			ops = iommu->ops;
+			ret = iommu_init_device(pinf, iommu->ops);
+			if (ret != -ENODEV)
+				return ret;
+		}
+	}
+	return -ENODEV;
+}
+
 static int __iommu_probe_device(struct iommu_probe_info *pinf)
 {
 	struct device *dev = pinf->dev;
@@ -524,13 +565,6 @@ static int __iommu_probe_device(struct iommu_probe_info *pinf)
 		ops = fwspec->ops;
 		if (!ops)
 			return -ENODEV;
-	} else {
-		struct iommu_device *iommu;
-
-		iommu = iommu_device_from_fwnode(NULL);
-		if (!iommu)
-			return -ENODEV;
-		ops = iommu->ops;
 	}
 
 	/*
@@ -546,7 +580,10 @@ static int __iommu_probe_device(struct iommu_probe_info *pinf)
 	if (dev->iommu_group)
 		return 0;
 
-	ret = iommu_init_device(pinf, ops);
+	if (ops)
+		ret = iommu_init_device(pinf, ops);
+	else
+		ret = iommu_find_init_device(pinf);
 	if (ret)
 		return ret;