Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
Hi Robin/Joerg,
-----Original Message----- From: Shameer Kolothum [mailto:shameerali.kolothum.thodi@huawei.com] Sent: 01 February 2021 12:41 To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org; will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>; linuxarm@openeuler.org Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
The device iommu probe/attach might have failed leaving dev->iommu to NULL and device drivers may still invoke these functions resulting in a crash in iommu vendor driver code. Hence make sure we check that.
Also added iommu_ops to the "struct dev_iommu" and set it if the dev is successfully associated with an iommu.
Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device") Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> --- v1 --> v2: -Added iommu_ops to struct dev_iommu based on the discussion with Robin. -Rebased against iommu-tree core branch.
A gentle ping on this... Thanks, Shameer
--- drivers/iommu/iommu.c | 19 +++++++------------ include/linux/iommu.h | 2 ++ 2 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index fd76e2f579fe..6023d0b7c542 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -217,6 +217,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list }
dev->iommu->iommu_dev = iommu_dev; + dev->iommu->ops = iommu_dev->ops;
group = iommu_group_get_for_dev(dev); if (IS_ERR(group)) { @@ -2865,10 +2866,8 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids); */ int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat) { - const struct iommu_ops *ops = dev->bus->iommu_ops; - - if (ops && ops->dev_enable_feat) - return ops->dev_enable_feat(dev, feat); + if (dev->iommu && dev->iommu->ops->dev_enable_feat) + return dev->iommu->ops->dev_enable_feat(dev, feat);
return -ENODEV; } @@ -2881,10 +2880,8 @@ EXPORT_SYMBOL_GPL(iommu_dev_enable_feature); */ int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat) { - const struct iommu_ops *ops = dev->bus->iommu_ops; - - if (ops && ops->dev_disable_feat) - return ops->dev_disable_feat(dev, feat); + if (dev->iommu && dev->iommu->ops->dev_disable_feat) + return dev->iommu->ops->dev_disable_feat(dev, feat);
return -EBUSY; } @@ -2892,10 +2889,8 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features feat) { - const struct iommu_ops *ops = dev->bus->iommu_ops; - - if (ops && ops->dev_feat_enabled) - return ops->dev_feat_enabled(dev, feat); + if (dev->iommu && dev->iommu->ops->dev_feat_enabled) + return dev->iommu->ops->dev_feat_enabled(dev, feat);
return false; } diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 524ffc2ff64f..ff0c76bdfb67 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -354,6 +354,7 @@ struct iommu_fault_param { * @fault_param: IOMMU detected device fault reporting data * @fwspec: IOMMU fwspec data * @iommu_dev: IOMMU device this device is linked to + * @ops: iommu-ops for talking to the iommu_dev * @priv: IOMMU Driver private data * * TODO: migrate other per device data pointers under iommu_dev_data, e.g. @@ -364,6 +365,7 @@ struct dev_iommu { struct iommu_fault_param *fault_param; struct iommu_fwspec *fwspec; struct iommu_device *iommu_dev; + const struct iommu_ops *ops; void *priv; };
-- 2.17.1 _______________________________________________ Linuxarm mailing list -- linuxarm@openeuler.org To unsubscribe send an email to linuxarm-leave@openeuler.org
On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote:
Hi Robin/Joerg,
-----Original Message----- From: Shameer Kolothum [mailto:shameerali.kolothum.thodi@huawei.com] Sent: 01 February 2021 12:41 To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org; will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>; linuxarm@openeuler.org Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
The device iommu probe/attach might have failed leaving dev->iommu to NULL and device drivers may still invoke these functions resulting in a crash in iommu vendor driver code. Hence make sure we check that.
Also added iommu_ops to the "struct dev_iommu" and set it if the dev is successfully associated with an iommu.
Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device") Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> --- v1 --> v2: -Added iommu_ops to struct dev_iommu based on the discussion with Robin. -Rebased against iommu-tree core branch.
A gentle ping on this...
Is there a convincing justification for maintaining yet another copy of the ops pointer rather than simply dereferencing iommu_dev->ops at point of use? Robin.
Thanks, Shameer
--- drivers/iommu/iommu.c | 19 +++++++------------ include/linux/iommu.h | 2 ++ 2 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index fd76e2f579fe..6023d0b7c542 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -217,6 +217,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list }
dev->iommu->iommu_dev = iommu_dev; + dev->iommu->ops = iommu_dev->ops;
group = iommu_group_get_for_dev(dev); if (IS_ERR(group)) { @@ -2865,10 +2866,8 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids); */ int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat) { - const struct iommu_ops *ops = dev->bus->iommu_ops; - - if (ops && ops->dev_enable_feat) - return ops->dev_enable_feat(dev, feat); + if (dev->iommu && dev->iommu->ops->dev_enable_feat) + return dev->iommu->ops->dev_enable_feat(dev, feat);
return -ENODEV; } @@ -2881,10 +2880,8 @@ EXPORT_SYMBOL_GPL(iommu_dev_enable_feature); */ int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat) { - const struct iommu_ops *ops = dev->bus->iommu_ops; - - if (ops && ops->dev_disable_feat) - return ops->dev_disable_feat(dev, feat); + if (dev->iommu && dev->iommu->ops->dev_disable_feat) + return dev->iommu->ops->dev_disable_feat(dev, feat);
return -EBUSY; } @@ -2892,10 +2889,8 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features feat) { - const struct iommu_ops *ops = dev->bus->iommu_ops; - - if (ops && ops->dev_feat_enabled) - return ops->dev_feat_enabled(dev, feat); + if (dev->iommu && dev->iommu->ops->dev_feat_enabled) + return dev->iommu->ops->dev_feat_enabled(dev, feat);
return false; } diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 524ffc2ff64f..ff0c76bdfb67 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -354,6 +354,7 @@ struct iommu_fault_param { * @fault_param: IOMMU detected device fault reporting data * @fwspec: IOMMU fwspec data * @iommu_dev: IOMMU device this device is linked to + * @ops: iommu-ops for talking to the iommu_dev * @priv: IOMMU Driver private data * * TODO: migrate other per device data pointers under iommu_dev_data, e.g. @@ -364,6 +365,7 @@ struct dev_iommu { struct iommu_fault_param *fault_param; struct iommu_fwspec *fwspec; struct iommu_device *iommu_dev; + const struct iommu_ops *ops; void *priv; };
-- 2.17.1 _______________________________________________ Linuxarm mailing list -- linuxarm@openeuler.org To unsubscribe send an email to linuxarm-leave@openeuler.org
-----Original Message----- From: Robin Murphy [mailto:robin.murphy@arm.com] Sent: 12 February 2021 16:39 To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>; linuxarm@openeuler.org Subject: Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote:
Hi Robin/Joerg,
-----Original Message----- From: Shameer Kolothum [mailto:shameerali.kolothum.thodi@huawei.com] Sent: 01 February 2021 12:41 To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org; will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>; linuxarm@openeuler.org Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
The device iommu probe/attach might have failed leaving dev->iommu to NULL and device drivers may still invoke these functions resulting in a crash in iommu vendor driver code. Hence make sure we check that.
Also added iommu_ops to the "struct dev_iommu" and set it if the dev is successfully associated with an iommu.
Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device") Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> --- v1 --> v2: -Added iommu_ops to struct dev_iommu based on the discussion with Robin. -Rebased against iommu-tree core branch.
A gentle ping on this...
Is there a convincing justification for maintaining yet another copy of the ops pointer rather than simply dereferencing iommu_dev->ops at point of use?
TBH, nothing I can think of now. That was mainly the way I interpreted your suggestion from the v1. Now it looks like you didn’t mean it :). I am Ok to rework it to dereference it from iommu_dev. Please let me know. Thanks, Shameer
Robin.
Thanks, Shameer
--- drivers/iommu/iommu.c | 19 +++++++------------ include/linux/iommu.h | 2 ++ 2 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index fd76e2f579fe..6023d0b7c542 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -217,6 +217,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list }
dev->iommu->iommu_dev = iommu_dev; + dev->iommu->ops = iommu_dev->ops;
group = iommu_group_get_for_dev(dev); if (IS_ERR(group)) { @@ -2865,10 +2866,8 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids); */ int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat) { - const struct iommu_ops *ops = dev->bus->iommu_ops; - - if (ops && ops->dev_enable_feat) - return ops->dev_enable_feat(dev, feat); + if (dev->iommu && dev->iommu->ops->dev_enable_feat) + return dev->iommu->ops->dev_enable_feat(dev, feat);
return -ENODEV; } @@ -2881,10 +2880,8 @@ EXPORT_SYMBOL_GPL(iommu_dev_enable_feature); */ int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat) { - const struct iommu_ops *ops = dev->bus->iommu_ops; - - if (ops && ops->dev_disable_feat) - return ops->dev_disable_feat(dev, feat); + if (dev->iommu && dev->iommu->ops->dev_disable_feat) + return dev->iommu->ops->dev_disable_feat(dev, feat);
return -EBUSY; } @@ -2892,10 +2889,8 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features feat) { - const struct iommu_ops *ops = dev->bus->iommu_ops; - - if (ops && ops->dev_feat_enabled) - return ops->dev_feat_enabled(dev, feat); + if (dev->iommu && dev->iommu->ops->dev_feat_enabled) + return dev->iommu->ops->dev_feat_enabled(dev, feat);
return false; } diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 524ffc2ff64f..ff0c76bdfb67 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -354,6 +354,7 @@ struct iommu_fault_param { * @fault_param: IOMMU detected device fault reporting data * @fwspec: IOMMU fwspec data * @iommu_dev: IOMMU device this device is linked to + * @ops: iommu-ops for talking to the iommu_dev * @priv: IOMMU Driver private data * * TODO: migrate other per device data pointers under iommu_dev_data, e.g. @@ -364,6 +365,7 @@ struct dev_iommu { struct iommu_fault_param *fault_param; struct iommu_fwspec *fwspec; struct iommu_device *iommu_dev; + const struct iommu_ops *ops; void *priv; };
-- 2.17.1 _______________________________________________ Linuxarm mailing list -- linuxarm@openeuler.org To unsubscribe send an email to linuxarm-leave@openeuler.org
-----Original Message----- From: Shameerali Kolothum Thodi Sent: 12 February 2021 16:45 To: 'Robin Murphy' <robin.murphy@arm.com>; linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>; linuxarm@openeuler.org Subject: RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
-----Original Message----- From: Robin Murphy [mailto:robin.murphy@arm.com] Sent: 12 February 2021 16:39 To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>; linuxarm@openeuler.org Subject: Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote:
Hi Robin/Joerg,
-----Original Message----- From: Shameer Kolothum [mailto:shameerali.kolothum.thodi@huawei.com] Sent: 01 February 2021 12:41 To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org; will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>; linuxarm@openeuler.org Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
The device iommu probe/attach might have failed leaving dev->iommu to NULL and device drivers may still invoke these functions resulting in a crash in iommu vendor driver code. Hence make sure we check that.
Also added iommu_ops to the "struct dev_iommu" and set it if the dev is successfully associated with an iommu.
Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device") Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> --- v1 --> v2: -Added iommu_ops to struct dev_iommu based on the discussion with Robin. -Rebased against iommu-tree core branch.
A gentle ping on this...
Is there a convincing justification for maintaining yet another copy of the ops pointer rather than simply dereferencing iommu_dev->ops at point of use?
TBH, nothing I can think of now. That was mainly the way I interpreted your suggestion from the v1. Now it looks like you didn’t mean it :). I am Ok to rework it to dereference it from iommu_dev. Please let me know.
So we can do something like this, index fd76e2f579fe..5fd31a3cec18 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2865,10 +2865,12 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids); */ int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat) { - const struct iommu_ops *ops = dev->bus->iommu_ops; + if (dev->iommu && dev->iommu->iommu_dev && dev->iommu->iommu_dev->ops) + struct iommu_ops *ops = dev->iommu->iommu_dev->ops; - if (ops && ops->dev_enable_feat) - return ops->dev_enable_feat(dev, feat); + if (ops->dev_enable_feat) + return ops->dev_enable_feat(dev, feat); + } return -ENODEV; } Again, not sure we need to do the checking for iommu->dev and ops here. If the dev->iommu is set, is it safe to assume that we have a valid iommu->iommu_dev and ops always? (May be it is safer to do the checking in case something else breaks this assumption in future). Please let me know your thoughts. Thanks, Shameer
On 2021-02-12 17:28, Shameerali Kolothum Thodi wrote:
-----Original Message----- From: Shameerali Kolothum Thodi Sent: 12 February 2021 16:45 To: 'Robin Murphy' <robin.murphy@arm.com>; linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>; linuxarm@openeuler.org Subject: RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
-----Original Message----- From: Robin Murphy [mailto:robin.murphy@arm.com] Sent: 12 February 2021 16:39 To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>; linuxarm@openeuler.org Subject: Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote:
Hi Robin/Joerg,
-----Original Message----- From: Shameer Kolothum [mailto:shameerali.kolothum.thodi@huawei.com] Sent: 01 February 2021 12:41 To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org; will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>; linuxarm@openeuler.org Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
The device iommu probe/attach might have failed leaving dev->iommu to NULL and device drivers may still invoke these functions resulting in a crash in iommu vendor driver code. Hence make sure we check that.
Also added iommu_ops to the "struct dev_iommu" and set it if the dev is successfully associated with an iommu.
Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device") Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> --- v1 --> v2: -Added iommu_ops to struct dev_iommu based on the discussion with Robin. -Rebased against iommu-tree core branch.
A gentle ping on this...
Is there a convincing justification for maintaining yet another copy of the ops pointer rather than simply dereferencing iommu_dev->ops at point of use?
TBH, nothing I can think of now. That was mainly the way I interpreted your suggestion from the v1. Now it looks like you didn’t mean it :). I am Ok to rework it to dereference it from iommu_dev. Please let me know.
So we can do something like this,
index fd76e2f579fe..5fd31a3cec18 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2865,10 +2865,12 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids); */ int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat) { - const struct iommu_ops *ops = dev->bus->iommu_ops; + if (dev->iommu && dev->iommu->iommu_dev && dev->iommu->iommu_dev->ops) + struct iommu_ops *ops = dev->iommu->iommu_dev->ops;
- if (ops && ops->dev_enable_feat) - return ops->dev_enable_feat(dev, feat); + if (ops->dev_enable_feat) + return ops->dev_enable_feat(dev, feat); + }
return -ENODEV; }
Again, not sure we need to do the checking for iommu->dev and ops here. If the dev->iommu is set, is it safe to assume that we have a valid iommu->iommu_dev and ops always? (May be it is safer to do the checking in case something else breaks this assumption in future). Please let me know your thoughts.
I think it *could* happen that dev->iommu is set by iommu_fwspec_init() but iommu_probe_device() later refuses the device for whatever reason, so we would still need to check iommu->iommu_dev to be completely safe. We can assume iommu_dev->ops is valid, since if the IOMMU driver has returned something bogus from .probe_device then it's a major bug in that driver and crashing is the best indicator :) Robin.
Thanks, Shameer
_______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
participants (2)
-
Robin Murphy -
Shameerali Kolothum Thodi