* 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).