Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

2021-02-16 Thread Robin Murphy 

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' ; linux-kernel@vger.kernel.org;
io...@lists.linux-foundation.org
Cc: j...@8bytes.org; jean-phili...@linaro.org; w...@kernel.org; Zengtao (B)
; linux...@openeuler.org
Subject: RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions




-Original Message-
From: Robin Murphy [mailto:robin.mur...@arm.com]
Sent: 12 February 2021 16:39
To: Shameerali Kolothum Thodi ;
linux-kernel@vger.kernel.org; io...@lists.linux-foundation.org
Cc: j...@8bytes.org; jean-phili...@linaro.org; w...@kernel.org; Zengtao (B)
; linux...@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.th...@huawei.com]

Sent: 01 February 2021 12:41
To: linux-kernel@vger.kernel.org; io...@lists.linux-foundation.org
Cc: j...@8bytes.org; robin.mur...@arm.com; jean-phili...@linaro.org;
w...@kernel.org; Zengtao (B) ;
linux...@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



---
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
io...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

2021-02-12 Thread Shameerali Kolothum Thodi 


> -Original Message-
> From: Shameerali Kolothum Thodi
> Sent: 12 February 2021 16:45
> To: 'Robin Murphy' ; linux-kernel@vger.kernel.org;
> io...@lists.linux-foundation.org
> Cc: j...@8bytes.org; jean-phili...@linaro.org; w...@kernel.org; Zengtao (B)
> ; linux...@openeuler.org
> Subject: RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions
> 
> 
> 
> > -Original Message-
> > From: Robin Murphy [mailto:robin.mur...@arm.com]
> > Sent: 12 February 2021 16:39
> > To: Shameerali Kolothum Thodi ;
> > linux-kernel@vger.kernel.org; io...@lists.linux-foundation.org
> > Cc: j...@8bytes.org; jean-phili...@linaro.org; w...@kernel.org; Zengtao (B)
> > ; linux...@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.th...@huawei.com]
> > >> Sent: 01 February 2021 12:41
> > >> To: linux-kernel@vger.kernel.org; io...@lists.linux-foundation.org
> > >> Cc: j...@8bytes.org; robin.mur...@arm.com; jean-phili...@linaro.org;
> > >> w...@kernel.org; Zengtao (B) ;
> > >> linux...@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
> > 
> > >> ---
> > >> 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

RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

2021-02-12 Thread Shameerali Kolothum Thodi 


> -Original Message-
> From: Robin Murphy [mailto:robin.mur...@arm.com]
> Sent: 12 February 2021 16:39
> To: Shameerali Kolothum Thodi ;
> linux-kernel@vger.kernel.org; io...@lists.linux-foundation.org
> Cc: j...@8bytes.org; jean-phili...@linaro.org; w...@kernel.org; Zengtao (B)
> ; linux...@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.th...@huawei.com]
> >> Sent: 01 February 2021 12:41
> >> To: linux-kernel@vger.kernel.org; io...@lists.linux-foundation.org
> >> Cc: j...@8bytes.org; robin.mur...@arm.com; jean-phili...@linaro.org;
> >> w...@kernel.org; Zengtao (B) ;
> >> linux...@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
> 
> >> ---
> >> 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)
> >> -

Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

2021-02-12 Thread Robin Murphy 

On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote:

Hi Robin/Joerg,


-Original Message-
From: Shameer Kolothum [mailto:shameerali.kolothum.th...@huawei.com]
Sent: 01 February 2021 12:41
To: linux-kernel@vger.kernel.org; io...@lists.linux-foundation.org
Cc: j...@8bytes.org; robin.mur...@arm.com; jean-phili...@linaro.org;
w...@kernel.org; Zengtao (B) ;
linux...@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 
---
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 -- linux...@openeuler.org
To unsubscribe send an email to linuxarm-le...@openeuler.org

RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions

2021-02-12 Thread Shameerali Kolothum Thodi 
Hi Robin/Joerg,

> -Original Message-
> From: Shameer Kolothum [mailto:shameerali.kolothum.th...@huawei.com]
> Sent: 01 February 2021 12:41
> To: linux-kernel@vger.kernel.org; io...@lists.linux-foundation.org
> Cc: j...@8bytes.org; robin.mur...@arm.com; jean-phili...@linaro.org;
> w...@kernel.org; Zengtao (B) ;
> linux...@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 
> ---
> 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 -- linux...@openeuler.org
> To unsubscribe send an email to linuxarm-le...@openeuler.org