Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
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)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
Hi,
* Jason Gunthorpe [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-t...@atomide.com/T/#u
Regards,
Tony
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
* Jason Gunthorpe [220330 14:21]:
> On Wed, Mar 30, 2022 at 05:00:39PM +0300, Tony Lindgren wrote:
> > Hi,
> >
> > * Lu Baolu [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 0008:
> >
> > __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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
On Wed, Mar 30, 2022 at 05:00:39PM +0300, Tony Lindgren wrote:
> Hi,
>
> * Lu Baolu [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 0008:
>
> __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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
Hi,
* Lu Baolu [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 0008:
__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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
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.
Suggested-by: Robin Murphy
Signed-off-by: Lu Baolu
Reviewed-by: Christoph Hellwig
Reviewed-by: Jason Gunthorpe
---
include/linux/iommu.h | 2 +-
drivers/iommu/amd/amd_iommu.h | 3 +--
drivers/iommu/amd/iommu.c | 3 +--
drivers/iommu/amd/iommu_v2.c | 2 +-
drivers/iommu/intel/iommu.c | 3 +--
drivers/iommu/iommu.c | 15 ++-
6 files changed, 11 insertions(+), 17 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 90f1b5e3809d..1ded6076a75c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -269,7 +269,7 @@ struct iommu_ops {
void (*put_resv_regions)(struct device *dev, struct list_head *list);
int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
- bool (*is_attach_deferred)(struct iommu_domain *domain, struct device
*dev);
+ bool (*is_attach_deferred)(struct device *dev);
/* Per device IOMMU features */
bool (*dev_has_feat)(struct device *dev, enum iommu_dev_features f);
diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index bb95edf74415..982798c7a3c8 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -117,8 +117,7 @@ void amd_iommu_domain_clr_pt_root(struct protection_domain
*domain)
extern bool translation_pre_enabled(struct amd_iommu *iommu);
-extern bool amd_iommu_is_attach_deferred(struct iommu_domain *domain,
-struct device *dev);
+extern bool amd_iommu_is_attach_deferred(struct device *dev);
extern int __init add_special_device(u8 type, u8 id, u16 *devid,
bool cmd_line);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index a18b549951bb..7e4e82158a80 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2221,8 +2221,7 @@ static void amd_iommu_get_resv_regions(struct device *dev,
list_add_tail(>list, head);
}
-bool amd_iommu_is_attach_deferred(struct iommu_domain *domain,
- struct device *dev)
+bool amd_iommu_is_attach_deferred(struct device *dev)
{
struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index 58da08cc3d01..7c94ec05d289 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -537,7 +537,7 @@ static int ppr_notifier(struct notifier_block *nb, unsigned
long e, void *data)
ret = NOTIFY_DONE;
/* In kdump kernel pci dev is not initialized yet -> send INVALID */
- if (amd_iommu_is_attach_deferred(NULL, >dev)) {
+ if (amd_iommu_is_attach_deferred(>dev)) {
amd_iommu_complete_ppr(pdev, iommu_fault->pasid,
PPR_INVALID, tag);
goto out;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 2b5f4e57a8bb..80f1294be634 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -5052,8 +5052,7 @@ intel_iommu_dev_disable_feat(struct device *dev, enum
iommu_dev_features feat)
}
}
-static bool intel_iommu_is_attach_deferred(struct iommu_domain *domain,
- struct device *dev)
+static bool intel_iommu_is_attach_deferred(struct device *dev)
{
return attach_deferred(dev);
}
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7af0ee670deb..27276421d253 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -831,13 +831,12 @@ static int iommu_create_device_direct_mappings(struct
iommu_group *group,
return ret;
}
-static bool iommu_is_attach_deferred(struct iommu_domain *domain,
-struct device *dev)
+static bool iommu_is_attach_deferred(struct device *dev)
{
const struct iommu_ops *ops = dev_iommu_ops(dev);
if (ops->is_attach_deferred)
- return ops->is_attach_deferred(domain, dev);
+ return ops->is_attach_deferred(dev);
return false;
}
@@ -894,7 +893,7 @@ int iommu_group_add_device(struct iommu_group *group,
struct device *dev)
mutex_lock(>mutex);
list_add_tail(>list, >devices);
- if (group->domain && !iommu_is_attach_deferred(group->domain, dev))
+ if (group->domain && !iommu_is_attach_deferred(dev))
ret = __iommu_attach_device(group->domain, dev);
mutex_unlock(>mutex);
if (ret)
@@ -1745,7 +1744,7 @@ static int iommu_group_do_dma_attach(struct device *dev,
void *data)
struct iommu_domain *domain = data;
int ret = 0;
- if (!iommu_is_attach_deferred(domain, dev))
+ if (!iommu_is_attach_deferred(dev))
ret = __iommu_attach_device(domain, dev);
