Re: [PATCH v2 06/17] iommu: Add iommu_fwspec_alloc/dealloc()

From: Jason Gunthorpe <jgg@nvidia.com>
To: Hector Martin <marcan@marcan.st>
Cc: acpica-devel@lists.linux.dev,
	Alyssa Rosenzweig <alyssa@rosenzweig.io>,
	Albert Ou <aou@eecs.berkeley.edu>,
	asahi@lists.linux.dev, Catalin Marinas <catalin.marinas@arm.com>,
	Dexuan Cui <decui@microsoft.com>,
	devicetree@vger.kernel.org, David Woodhouse <dwmw2@infradead.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Hanjun Guo <guohanjun@huawei.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	iommu@lists.linux.dev,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Joerg Roedel <joro@8bytes.org>,
	"K. Y. Srinivasan" <kys@microsoft.com>,
	Len Brown <lenb@kernel.org>,
	linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-hyperv@vger.kernel.org, linux-mips@vger.kernel.org,
	linux-riscv@lists.infradead.org,
	linux-snps-arc@lists.infradead.org, linux-tegra@vger.kernel.org,
	Russell King <linux@armlinux.org.uk>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	patches@lists.linux.dev, Paul Walmsley <paul.walmsley@sifive.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Robert Moore <robert.moore@intel.com>,
	Rob Herring <robh+dt@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Sven Peter <sven@svenpeter.dev>,
	Thierry Reding <thierry.reding@gmail.com>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Krishna Reddy <vdumpa@nvidia.com>,
	Vineet Gupta <vgupta@kernel.org>,
	virtualization@lists.linux.dev, Wei Liu <wei.liu@kernel.org>,
	Will Deacon <will@kernel.org>
Subject: Re: [PATCH v2 06/17] iommu: Add iommu_fwspec_alloc/dealloc()
Date: Sun, 19 Nov 2023 10:13:29 -0400	[thread overview]
Message-ID: <20231119141329.GA6083@nvidia.com> (raw)
In-Reply-To: <1eb12c35-e64e-4c32-af99-8743dc2ec266@marcan.st>

On Sun, Nov 19, 2023 at 06:19:43PM +0900, Hector Martin wrote:
> >> +static int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec,
> >> +				     struct device *dev,
> >> +				     struct fwnode_handle *iommu_fwnode)
> >> +{
> >> +	const struct iommu_ops *ops;
> >> +
> >> +	if (fwspec->iommu_fwnode) {
> >> +		/*
> >> +		 * fwspec->iommu_fwnode is the first iommu's fwnode. In the rare
> >> +		 * case of multiple iommus for one device they must point to the
> >> +		 * same driver, checked via same ops.
> >> +		 */
> >> +		ops = iommu_ops_from_fwnode(iommu_fwnode);
> > 
> > This carries over a related bug from the original code: If a device has
> > two IOMMUs and the first one probes but the second one defers, ops will
> > be NULL here and the check will fail with EINVAL.
> > 
> > Adding a check for that case here fixes it:
> > 
> > 		if (!ops)
> > 			return driver_deferred_probe_check_state(dev);

Yes!

> > With that, for the whole series:
> > 
> > Tested-by: Hector Martin <marcan@marcan.st>
> > 
> > I can't specifically test for the probe races the series intends to fix
> > though, since that bug we only hit extremely rarely. I'm just testing
> > that nothing breaks.
> 
> Actually no, this fix is not sufficient. If the first IOMMU is ready
> then the xlate path allocates dev->iommu, which then
> __iommu_probe_device takes as a sign that all IOMMUs are ready and does
> the device init.

It doesn't.. The code there is:

	if (!fwspec && dev->iommu)
		fwspec = dev->iommu->fwspec;
	if (fwspec)
		ops = fwspec->ops;
	else
		ops = dev->bus->iommu_ops;
	if (!ops) {
		ret = -ENODEV;
		goto out_unlock;
	}

Which is sensitive only to !NULL fwspec, and if EPROBE_DEFER is
returned fwspec will be freed and dev->iommu->fwspec will be NULL
here.

In the NULL case it does a 'bus probe' with a NULL fwspec and all the
fwspec drivers return immediately from their probe functions.

Did I miss something?

> Then when the xlate comes along again after suceeding
> with the second IOMMU, __iommu_probe_device sees the device is already
> in a group and never initializes the second IOMMU, leaving the device
> with only one IOMMU.

This should be fixed by the first hunk to check every iommu and fail?

BTW, do you have a systems with same device attached to multiple
iommus?

I've noticed another bug here, many drivers don't actually support
differing iommu instances and nothing seems to check it..

Thanks,
Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel