* [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions
@ 2021-01-26 13:06 Shameer Kolothum
2021-01-26 13:50 ` Robin Murphy
2021-01-28 12:54 ` Joerg Roedel
0 siblings, 2 replies; 5+ messages in thread
From: Shameer Kolothum @ 2021-01-26 13:06 UTC (permalink / raw)
To: linux-kernel, iommu; +Cc: jean-philippe, will, linuxarm, prime.zeng
The device iommu probe/attach might have failed leaving dev->iommu
to NULL and device drivers may still invoke these functions resulting
a crash in iommu vendor driver code. Hence make sure we check that.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
drivers/iommu/iommu.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ffeebda8d6de..cb68153c5cc0 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2867,7 +2867,7 @@ bool iommu_dev_has_feature(struct device *dev, enum iommu_dev_features feat)
{
const struct iommu_ops *ops = dev->bus->iommu_ops;
- if (ops && ops->dev_has_feat)
+ if (dev->iommu && ops && ops->dev_has_feat)
return ops->dev_has_feat(dev, feat);
return false;
@@ -2878,7 +2878,7 @@ 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)
+ if (dev->iommu && ops && ops->dev_enable_feat)
return ops->dev_enable_feat(dev, feat);
return -ENODEV;
@@ -2894,7 +2894,7 @@ 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)
+ if (dev->iommu && ops && ops->dev_disable_feat)
return ops->dev_disable_feat(dev, feat);
return -EBUSY;
@@ -2905,7 +2905,7 @@ 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)
+ if (dev->iommu && ops && ops->dev_feat_enabled)
return ops->dev_feat_enabled(dev, feat);
return false;
--
2.17.1
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions
2021-01-26 13:06 [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions Shameer Kolothum
@ 2021-01-26 13:50 ` Robin Murphy
2021-01-26 16:40 ` Shameerali Kolothum Thodi
2021-01-28 12:54 ` Joerg Roedel
1 sibling, 1 reply; 5+ messages in thread
From: Robin Murphy @ 2021-01-26 13:50 UTC (permalink / raw)
To: Shameer Kolothum
Cc: jean-philippe, linux-kernel, linuxarm, iommu, prime.zeng, will
On Tue, 26 Jan 2021 13:06:29 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> The device iommu probe/attach might have failed leaving dev->iommu
> to NULL and device drivers may still invoke these functions resulting
> a crash in iommu vendor driver code. Hence make sure we check that.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> drivers/iommu/iommu.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index ffeebda8d6de..cb68153c5cc0 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2867,7 +2867,7 @@ bool iommu_dev_has_feature(struct device *dev,
> enum iommu_dev_features feat) {
> const struct iommu_ops *ops = dev->bus->iommu_ops;
>
> - if (ops && ops->dev_has_feat)
> + if (dev->iommu && ops && ops->dev_has_feat)
> return ops->dev_has_feat(dev, feat);
Might make sense to make these more self-contained, e.g.:
if (dev->iommu && dev->iommu->ops->foo)
dev->iommu->ops->foo()
Robin.
> return false;
> @@ -2878,7 +2878,7 @@ 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)
> + if (dev->iommu && ops && ops->dev_enable_feat)
> return ops->dev_enable_feat(dev, feat);
>
> return -ENODEV;
> @@ -2894,7 +2894,7 @@ 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)
> + if (dev->iommu && ops && ops->dev_disable_feat)
> return ops->dev_disable_feat(dev, feat);
>
> return -EBUSY;
> @@ -2905,7 +2905,7 @@ 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)
> + if (dev->iommu && ops && ops->dev_feat_enabled)
> return ops->dev_feat_enabled(dev, feat);
>
> return false;
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions
2021-01-26 13:50 ` Robin Murphy
@ 2021-01-26 16:40 ` Shameerali Kolothum Thodi
2021-01-26 17:24 ` Robin Murphy
0 siblings, 1 reply; 5+ messages in thread
From: Shameerali Kolothum Thodi @ 2021-01-26 16:40 UTC (permalink / raw)
To: Robin Murphy
Cc: jean-philippe, linux-kernel, linuxarm, iommu, Zengtao (B), will
Hi Robin,
> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 26 January 2021 13:51
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org;
> jean-philippe@linaro.org; will@kernel.org; linuxarm@openeuler.org; Zengtao
> (B) <prime.zeng@hisilicon.com>
> Subject: Re: [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions
>
> On Tue, 26 Jan 2021 13:06:29 +0000
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>
> > The device iommu probe/attach might have failed leaving dev->iommu to
> > NULL and device drivers may still invoke these functions resulting a
> > crash in iommu vendor driver code. Hence make sure we check that.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> > drivers/iommu/iommu.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
> > ffeebda8d6de..cb68153c5cc0 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -2867,7 +2867,7 @@ bool iommu_dev_has_feature(struct device *dev,
> > enum iommu_dev_features feat) {
> > const struct iommu_ops *ops = dev->bus->iommu_ops;
> >
> > - if (ops && ops->dev_has_feat)
> > + if (dev->iommu && ops && ops->dev_has_feat)
> > return ops->dev_has_feat(dev, feat);
>
> Might make sense to make these more self-contained, e.g.:
>
> if (dev->iommu && dev->iommu->ops->foo)
> dev->iommu->ops->foo()
Right. Does that mean adding ops to "struct dev_iommu" or retrieve ops like
below,
if (dev->iommu && dev->iommu->iommu_dev->ops->foo)
dev->iommu->iommu_dev->ops->foo()
Sorry, not clear to me.
Thanks,
Shameer
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions
2021-01-26 16:40 ` Shameerali Kolothum Thodi
@ 2021-01-26 17:24 ` Robin Murphy
0 siblings, 0 replies; 5+ messages in thread
From: Robin Murphy @ 2021-01-26 17:24 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: jean-philippe, linux-kernel, linuxarm, iommu, Zengtao (B), will
On 2021-01-26 16:40, Shameerali Kolothum Thodi wrote:
> Hi Robin,
>
>> -----Original Message-----
>> From: Robin Murphy [mailto:robin.murphy@arm.com]
>> Sent: 26 January 2021 13:51
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
>> Cc: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org;
>> jean-philippe@linaro.org; will@kernel.org; linuxarm@openeuler.org; Zengtao
>> (B) <prime.zeng@hisilicon.com>
>> Subject: Re: [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions
>>
>> On Tue, 26 Jan 2021 13:06:29 +0000
>> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>>
>>> The device iommu probe/attach might have failed leaving dev->iommu to
>>> NULL and device drivers may still invoke these functions resulting a
>>> crash in iommu vendor driver code. Hence make sure we check that.
>>>
>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>>> ---
>>> drivers/iommu/iommu.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
>>> ffeebda8d6de..cb68153c5cc0 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -2867,7 +2867,7 @@ bool iommu_dev_has_feature(struct device *dev,
>>> enum iommu_dev_features feat) {
>>> const struct iommu_ops *ops = dev->bus->iommu_ops;
>>>
>>> - if (ops && ops->dev_has_feat)
>>> + if (dev->iommu && ops && ops->dev_has_feat)
>>> return ops->dev_has_feat(dev, feat);
>>
>> Might make sense to make these more self-contained, e.g.:
>>
>> if (dev->iommu && dev->iommu->ops->foo)
>> dev->iommu->ops->foo()
>
> Right. Does that mean adding ops to "struct dev_iommu" or retrieve ops like
> below,
>
> if (dev->iommu && dev->iommu->iommu_dev->ops->foo)
> dev->iommu->iommu_dev->ops->foo()
>
> Sorry, not clear to me.
Bleh, I was thinking that dev->iommu pointed directly to a struct
iommu_device there, sorry. There are too many things and not enough
distinct names for the things.
But yeah, basically that if the device's "I am associated with an IOMMU"
data is set, then by construction it must lead to a set of ops which are
definitely valid. Conceptually it's cleaner than combining two different
data sources (the per-device info plus the bus ops which may or may not
be relevant to a given device), even if cosmetically we have to juggle
through practically every possible permutation of the names "iommu" and
"device" to get there :/
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions
2021-01-26 13:06 [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions Shameer Kolothum
2021-01-26 13:50 ` Robin Murphy
@ 2021-01-28 12:54 ` Joerg Roedel
1 sibling, 0 replies; 5+ messages in thread
From: Joerg Roedel @ 2021-01-28 12:54 UTC (permalink / raw)
To: Shameer Kolothum
Cc: jean-philippe, linux-kernel, linuxarm, iommu, prime.zeng, will
On Tue, Jan 26, 2021 at 01:06:29PM +0000, Shameer Kolothum wrote:
> The device iommu probe/attach might have failed leaving dev->iommu
> to NULL and device drivers may still invoke these functions resulting
> a crash in iommu vendor driver code. Hence make sure we check that.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> drivers/iommu/iommu.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index ffeebda8d6de..cb68153c5cc0 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2867,7 +2867,7 @@ bool iommu_dev_has_feature(struct device *dev, enum iommu_dev_features feat)
This function has been removed from the iommu-tree. Can you please
rebase this patch against the latest 'core' branch when I pushed it
later this week (maybe even later today)?
A Fixes tag would be nice too.
Regards,
Joerg
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-28 12:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-26 13:06 [PATCH] iommu: Check dev->iommu in iommu_dev_xxx functions Shameer Kolothum
2021-01-26 13:50 ` Robin Murphy
2021-01-26 16:40 ` Shameerali Kolothum Thodi
2021-01-26 17:24 ` Robin Murphy
2021-01-28 12:54 ` Joerg Roedel
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).