* Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
[not found] ` <20220216025249.3459465-9-baolu.lu@linux.intel.com>
@ 2022-03-30 14:00 ` Tony Lindgren
2022-03-30 14:23 ` Jason Gunthorpe
0 siblings, 1 reply; 6+ messages in thread
From: Tony Lindgren @ 2022-03-30 14:00 UTC (permalink / raw)
To: Lu Baolu
Cc: Joerg Roedel, Jason Gunthorpe, Christoph Hellwig, Ben Skeggs,
Kevin Tian, Ashok Raj, Will Deacon, Robin Murphy,
Alex Williamson, Eric Auger, Liu Yi L, Jacob jun Pan,
David Airlie, Daniel Vetter, Thierry Reding, Jonathan Hunter,
iommu, linux-kernel, Christoph Hellwig, linux-omap,
linux-arm-kernel, regressions
Hi,
* Lu Baolu <baolu.lu@linux.intel.com> [700101 02:00]:
> The is_attach_deferred iommu_ops callback is a device op. The domain
> argument is unnecessary and never used. Remove it to make code clean.
Looks like this causes a regression for at least drivers/iommu/omap-iommu.c.
To me it seems the issue is there is no is_attach_deferred implemented, so
we get a NULL pointer dereference at virtual address 00000008:
__iommu_probe_device from probe_iommu_group+0x2c/0x38
probe_iommu_group from bus_for_each_dev+0x74/0xbc
bus_for_each_dev from bus_iommu_probe+0x34/0x2e8
bus_iommu_probe from bus_set_iommu+0x80/0xc8
bus_set_iommu from omap_iommu_init+0x88/0xcc
omap_iommu_init from do_one_initcall+0x44/0x24c
Any ideas for a fix?
It would be good to fix this quickly so we don't end up with a broken
v5.18-rc1..
For reference, this is mainline commit 41bb23e70b50 ("iommu: Remove unused
argument in is_attach_deferred").
Regards,
Tony
#regzbot ^introduced 41bb23e70b50
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
2022-03-30 14:00 ` [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred Tony Lindgren
@ 2022-03-30 14:23 ` Jason Gunthorpe
2022-03-30 17:19 ` Tony Lindgren
0 siblings, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2022-03-30 14:23 UTC (permalink / raw)
To: Tony Lindgren
Cc: Lu Baolu, Joerg Roedel, Christoph Hellwig, Ben Skeggs,
Kevin Tian, Ashok Raj, Will Deacon, Robin Murphy,
Alex Williamson, Eric Auger, Liu Yi L, Jacob jun Pan,
David Airlie, Daniel Vetter, Thierry Reding, Jonathan Hunter,
iommu, linux-kernel, Christoph Hellwig, linux-omap,
linux-arm-kernel, regressions
On Wed, Mar 30, 2022 at 05:00:39PM +0300, Tony Lindgren wrote:
> Hi,
>
> * Lu Baolu <baolu.lu@linux.intel.com> [700101 02:00]:
> > The is_attach_deferred iommu_ops callback is a device op. The domain
> > argument is unnecessary and never used. Remove it to make code clean.
>
> Looks like this causes a regression for at least drivers/iommu/omap-iommu.c.
>
> To me it seems the issue is there is no is_attach_deferred implemented, so
> we get a NULL pointer dereference at virtual address 00000008:
>
> __iommu_probe_device from probe_iommu_group+0x2c/0x38
> probe_iommu_group from bus_for_each_dev+0x74/0xbc
> bus_for_each_dev from bus_iommu_probe+0x34/0x2e8
> bus_iommu_probe from bus_set_iommu+0x80/0xc8
> bus_set_iommu from omap_iommu_init+0x88/0xcc
> omap_iommu_init from do_one_initcall+0x44/0x24c
>
> Any ideas for a fix?
>
> It would be good to fix this quickly so we don't end up with a broken
> v5.18-rc1..
>
> For reference, this is mainline commit 41bb23e70b50 ("iommu: Remove unused
> argument in is_attach_deferred").
Are you confident in the bisection? I don't see how that commit could
NULL deref..
Can you find the code that is the NULL deref?
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
2022-03-30 14:23 ` Jason Gunthorpe
@ 2022-03-30 17:19 ` Tony Lindgren
2022-03-30 17:33 ` Jason Gunthorpe
0 siblings, 1 reply; 6+ messages in thread
From: Tony Lindgren @ 2022-03-30 17:19 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Lu Baolu, Joerg Roedel, Christoph Hellwig, Ben Skeggs,
Kevin Tian, Ashok Raj, Will Deacon, Robin Murphy,
Alex Williamson, Eric Auger, Liu Yi L, Jacob jun Pan,
David Airlie, Daniel Vetter, Thierry Reding, Jonathan Hunter,
iommu, linux-kernel, Christoph Hellwig, linux-omap,
linux-arm-kernel, regressions
* Jason Gunthorpe <jgg@nvidia.com> [220330 14:21]:
> On Wed, Mar 30, 2022 at 05:00:39PM +0300, Tony Lindgren wrote:
> > Hi,
> >
> > * Lu Baolu <baolu.lu@linux.intel.com> [700101 02:00]:
> > > The is_attach_deferred iommu_ops callback is a device op. The domain
> > > argument is unnecessary and never used. Remove it to make code clean.
> >
> > Looks like this causes a regression for at least drivers/iommu/omap-iommu.c.
> >
> > To me it seems the issue is there is no is_attach_deferred implemented, so
> > we get a NULL pointer dereference at virtual address 00000008:
> >
> > __iommu_probe_device from probe_iommu_group+0x2c/0x38
> > probe_iommu_group from bus_for_each_dev+0x74/0xbc
> > bus_for_each_dev from bus_iommu_probe+0x34/0x2e8
> > bus_iommu_probe from bus_set_iommu+0x80/0xc8
> > bus_set_iommu from omap_iommu_init+0x88/0xcc
> > omap_iommu_init from do_one_initcall+0x44/0x24c
> >
> > Any ideas for a fix?
> >
> > It would be good to fix this quickly so we don't end up with a broken
> > v5.18-rc1..
> >
> > For reference, this is mainline commit 41bb23e70b50 ("iommu: Remove unused
> > argument in is_attach_deferred").
>
> Are you confident in the bisection? I don't see how that commit could
> NULL deref..
Oops sorry you're right, the breaking commit is a different patch, it's
3f6634d997db ("iommu: Use right way to retrieve iommu_ops") instead. I must
have picked the wrong commit while testing.
> Can you find the code that is the NULL deref?
I can debug a bit more tomorrow.
Regards,
Tony
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
2022-03-30 17:19 ` Tony Lindgren
@ 2022-03-30 17:33 ` Jason Gunthorpe
2022-03-31 6:25 ` Tony Lindgren
2022-03-31 6:40 ` Drew Fustini
0 siblings, 2 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2022-03-30 17:33 UTC (permalink / raw)
To: Tony Lindgren
Cc: Lu Baolu, Joerg Roedel, Christoph Hellwig, Ben Skeggs,
Kevin Tian, Ashok Raj, Will Deacon, Robin Murphy,
Alex Williamson, Eric Auger, Liu Yi L, Jacob jun Pan,
David Airlie, Daniel Vetter, Thierry Reding, Jonathan Hunter,
iommu, linux-kernel, Christoph Hellwig, linux-omap,
linux-arm-kernel, regressions
On Wed, Mar 30, 2022 at 08:19:37PM +0300, Tony Lindgren wrote:
> > > __iommu_probe_device from probe_iommu_group+0x2c/0x38
> > > probe_iommu_group from bus_for_each_dev+0x74/0xbc
> > > bus_for_each_dev from bus_iommu_probe+0x34/0x2e8
> > > bus_iommu_probe from bus_set_iommu+0x80/0xc8
> > > bus_set_iommu from omap_iommu_init+0x88/0xcc
> > > omap_iommu_init from do_one_initcall+0x44/0x24c
> > >
> > > Any ideas for a fix?
> > >
> > > It would be good to fix this quickly so we don't end up with a broken
> > > v5.18-rc1..
> > >
> > > For reference, this is mainline commit 41bb23e70b50 ("iommu: Remove unused
> > > argument in is_attach_deferred").
> >
> > Are you confident in the bisection? I don't see how that commit could
> > NULL deref..
>
> Oops sorry you're right, the breaking commit is a different patch, it's
> 3f6634d997db ("iommu: Use right way to retrieve iommu_ops") instead. I must
> have picked the wrong commit while testing.
That makes alot more sense
> > Can you find the code that is the NULL deref?
>
> I can debug a bit more tomorrow.
Looks like omap's omap_iommu_probe_device() is buggy, it returns 0 on
error paths:
num_iommus = of_property_count_elems_of_size(dev->of_node, "iommus",
sizeof(phandle));
if (num_iommus < 0)
return 0;
This function needs to return an errno -ENODEV
Otherwise it causes a NULL dev->iommu->iommu_dev and dev_iommu_ops() will
crash.
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
2022-03-30 17:33 ` Jason Gunthorpe
@ 2022-03-31 6:25 ` Tony Lindgren
2022-03-31 6:40 ` Drew Fustini
1 sibling, 0 replies; 6+ messages in thread
From: Tony Lindgren @ 2022-03-31 6:25 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Lu Baolu, Joerg Roedel, Christoph Hellwig, Ben Skeggs,
Kevin Tian, Ashok Raj, Will Deacon, Robin Murphy,
Alex Williamson, Eric Auger, Liu Yi L, Jacob jun Pan,
David Airlie, Daniel Vetter, Thierry Reding, Jonathan Hunter,
iommu, linux-kernel, Christoph Hellwig, linux-omap,
linux-arm-kernel, regressions
Hi,
* Jason Gunthorpe <jgg@nvidia.com> [220330 17:31]:
> On Wed, Mar 30, 2022 at 08:19:37PM +0300, Tony Lindgren wrote:
>
> > > > __iommu_probe_device from probe_iommu_group+0x2c/0x38
> > > > probe_iommu_group from bus_for_each_dev+0x74/0xbc
> > > > bus_for_each_dev from bus_iommu_probe+0x34/0x2e8
> > > > bus_iommu_probe from bus_set_iommu+0x80/0xc8
> > > > bus_set_iommu from omap_iommu_init+0x88/0xcc
> > > > omap_iommu_init from do_one_initcall+0x44/0x24c
> > > >
> > > > Any ideas for a fix?
> > > >
> > > > It would be good to fix this quickly so we don't end up with a broken
> > > > v5.18-rc1..
> > > >
> > > > For reference, this is mainline commit 41bb23e70b50 ("iommu: Remove unused
> > > > argument in is_attach_deferred").
> > >
> > > Are you confident in the bisection? I don't see how that commit could
> > > NULL deref..
> >
> > Oops sorry you're right, the breaking commit is a different patch, it's
> > 3f6634d997db ("iommu: Use right way to retrieve iommu_ops") instead. I must
> > have picked the wrong commit while testing.
>
> That makes alot more sense
>
> > > Can you find the code that is the NULL deref?
> >
> > I can debug a bit more tomorrow.
>
> Looks like omap's omap_iommu_probe_device() is buggy, it returns 0 on
> error paths:
>
> num_iommus = of_property_count_elems_of_size(dev->of_node, "iommus",
> sizeof(phandle));
> if (num_iommus < 0)
> return 0;
>
> This function needs to return an errno -ENODEV
>
> Otherwise it causes a NULL dev->iommu->iommu_dev and dev_iommu_ops() will
> crash.
You got it. This fixes the issue for me. Looks like the regression already
happened earlier and recent changes just expose it.
For reference, fix posted at:
https://lore.kernel.org/linux-iommu/20220331062301.24269-1-tony@atomide.com/T/#u
Regards,
Tony
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
2022-03-30 17:33 ` Jason Gunthorpe
2022-03-31 6:25 ` Tony Lindgren
@ 2022-03-31 6:40 ` Drew Fustini
1 sibling, 0 replies; 6+ messages in thread
From: Drew Fustini @ 2022-03-31 6:40 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Tony Lindgren, Lu Baolu, Joerg Roedel, Christoph Hellwig,
Ben Skeggs, Kevin Tian, Ashok Raj, Will Deacon, Robin Murphy,
Alex Williamson, Eric Auger, Liu Yi L, Jacob jun Pan,
David Airlie, Daniel Vetter, Thierry Reding, Jonathan Hunter,
iommu, linux-kernel, Christoph Hellwig, linux-omap,
linux-arm-kernel, regressions
On Wed, Mar 30, 2022 at 02:33:23PM -0300, Jason Gunthorpe wrote:
> On Wed, Mar 30, 2022 at 08:19:37PM +0300, Tony Lindgren wrote:
>
> > > > __iommu_probe_device from probe_iommu_group+0x2c/0x38
> > > > probe_iommu_group from bus_for_each_dev+0x74/0xbc
> > > > bus_for_each_dev from bus_iommu_probe+0x34/0x2e8
> > > > bus_iommu_probe from bus_set_iommu+0x80/0xc8
> > > > bus_set_iommu from omap_iommu_init+0x88/0xcc
> > > > omap_iommu_init from do_one_initcall+0x44/0x24c
> > > >
> > > > Any ideas for a fix?
> > > >
> > > > It would be good to fix this quickly so we don't end up with a broken
> > > > v5.18-rc1..
> > > >
> > > > For reference, this is mainline commit 41bb23e70b50 ("iommu: Remove unused
> > > > argument in is_attach_deferred").
> > >
> > > Are you confident in the bisection? I don't see how that commit could
> > > NULL deref..
> >
> > Oops sorry you're right, the breaking commit is a different patch, it's
> > 3f6634d997db ("iommu: Use right way to retrieve iommu_ops") instead. I must
> > have picked the wrong commit while testing.
>
> That makes alot more sense
>
> > > Can you find the code that is the NULL deref?
> >
> > I can debug a bit more tomorrow.
>
> Looks like omap's omap_iommu_probe_device() is buggy, it returns 0 on
> error paths:
>
> num_iommus = of_property_count_elems_of_size(dev->of_node, "iommus",
> sizeof(phandle));
> if (num_iommus < 0)
> return 0;
>
> This function needs to return an errno -ENODEV
>
> Otherwise it causes a NULL dev->iommu->iommu_dev and dev_iommu_ops() will
> crash.
>
> Jason
I tried to boot current mainline (787af64d05cd) on am57xx-evm and it
failed to boot. I made the change you suggested and it boots okay now:
https://gist.github.com/pdp7/918eefe03b5c5db3b5d28d819f6a27f6
thanks,
drew
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 4aab631ef517..d9cf2820c02e 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1661,7 +1661,7 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
num_iommus = of_property_count_elems_of_size(dev->of_node, "iommus",
sizeof(phandle));
if (num_iommus < 0)
- return 0;
+ return ERR_PTR(-ENODEV);
arch_data = kcalloc(num_iommus + 1, sizeof(*arch_data), GFP_KERNEL);
if (!arch_data)
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-31 6:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20220216025249.3459465-1-baolu.lu@linux.intel.com>
[not found] ` <20220216025249.3459465-9-baolu.lu@linux.intel.com>
2022-03-30 14:00 ` [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred Tony Lindgren
2022-03-30 14:23 ` Jason Gunthorpe
2022-03-30 17:19 ` Tony Lindgren
2022-03-30 17:33 ` Jason Gunthorpe
2022-03-31 6:25 ` Tony Lindgren
2022-03-31 6:40 ` Drew Fustini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).