LKML Archive on lore.kernel.org
 help / Atom feed
* [RFC PATCH 0/3] dma-mapping: clear dangling pointers consistently
@ 2018-09-13 15:17 Wolfram Sang
  2018-09-13 15:17 ` [RFC PATCH 1/3] ARM: dma-mapping: update comment about handling dma_ops when detaching from IOMMU Wolfram Sang
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Wolfram Sang @ 2018-09-13 15:17 UTC (permalink / raw)
  To: iommu, Robin Murphy, Christoph Hellwig
  Cc: linux-renesas-soc, Marek Szyprowski, linux-arm-kernel,
	linux-kernel, Wolfram Sang

When working with dma_set_max_seg_size(), I noticed issues with a
dangling dma_parms pointer. I saw Christoph just worked on handling
something similar for the dma_ops pointer, too. I came up with three
patches on top of Christoph's work which I send out for discussion here:

Patch 1 fixes a meanwhile stale comment.

Patch 2 makes clearing the dma_ops pointer more consistent because it
was missed on the custom ARM implementation to the best of my knowledge.

Patch 3 generalizes teardown_dma_ops to teardown_dma, so that clearing
dma_parms can be added there.

All these patches are based on the dma-mapping for-next branch. They are
build tested and runtime tested on a Renesas Salvator-XS board (R-Car
M3-N, ARM64) and Renesas Lager board (R-Car H2, ARM32).

A branch containing these (and dma_parms additions for the above boards)
can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/sdhi/set_max_seg

Looking forward to opinions.

Regards,

   Wolfram


Wolfram Sang (3):
  ARM: dma-mapping: update comment about handling dma_ops when detaching
    from IOMMU
  dma-mapping: clear dma_ops pointer also on ARM
  dma-mapping: clear dma_parms on teardown, too

 arch/arm/mm/dma-mapping.c   | 8 ++++----
 include/linux/dma-mapping.h | 6 ++++--
 2 files changed, 8 insertions(+), 6 deletions(-)

-- 
2.18.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [RFC PATCH 1/3] ARM: dma-mapping: update comment about handling dma_ops when detaching from IOMMU
  2018-09-13 15:17 [RFC PATCH 0/3] dma-mapping: clear dangling pointers consistently Wolfram Sang
@ 2018-09-13 15:17 ` Wolfram Sang
  2018-09-14 12:15   ` Geert Uytterhoeven
  2018-09-13 15:17 ` [RFC PATCH 2/3] dma-mapping: clear dma_ops pointer also on ARM Wolfram Sang
  2018-09-13 15:17 ` [RFC PATCH 3/3] dma-mapping: clear dma_parms on teardown, too Wolfram Sang
  2 siblings, 1 reply; 10+ messages in thread
From: Wolfram Sang @ 2018-09-13 15:17 UTC (permalink / raw)
  To: iommu, Robin Murphy, Christoph Hellwig
  Cc: linux-renesas-soc, Marek Szyprowski, linux-arm-kernel,
	linux-kernel, Wolfram Sang

Update the comment because we don't set the pointer to NULL anymore.
Also use the correct pointer name 'dma_ops' instead of 'dma_map_ops'.

Fixes: 1874619a7df4 ("ARM: dma-mapping: Set proper DMA ops in arm_iommu_detach_device()")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 arch/arm/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 66566472c153..e3b04786838f 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2287,7 +2287,7 @@ EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
  * @dev: valid struct device pointer
  *
  * Detaches the provided device from a previously attached map.
- * This voids the dma operations (dma_map_ops pointer)
+ * This overwrites the dma_ops pointer with appropriate non-IOMMU ops.
  */
 void arm_iommu_detach_device(struct device *dev)
 {
-- 
2.18.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [RFC PATCH 2/3] dma-mapping: clear dma_ops pointer also on ARM
  2018-09-13 15:17 [RFC PATCH 0/3] dma-mapping: clear dangling pointers consistently Wolfram Sang
  2018-09-13 15:17 ` [RFC PATCH 1/3] ARM: dma-mapping: update comment about handling dma_ops when detaching from IOMMU Wolfram Sang
@ 2018-09-13 15:17 ` Wolfram Sang
  2018-09-14 13:12   ` Christoph Hellwig
  2018-09-13 15:17 ` [RFC PATCH 3/3] dma-mapping: clear dma_parms on teardown, too Wolfram Sang
  2 siblings, 1 reply; 10+ messages in thread
From: Wolfram Sang @ 2018-09-13 15:17 UTC (permalink / raw)
  To: iommu, Robin Murphy, Christoph Hellwig
  Cc: linux-renesas-soc, Marek Szyprowski, linux-arm-kernel,
	linux-kernel, Wolfram Sang

The generic fallback of arch_teardown_dma_ops() clears the dma_ops
pointer but the ARM specific version does not. Rename the generic one,
so it can be called from ARM specific one, too. This will ensure
consistent behaviour.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 arch/arm/mm/dma-mapping.c   | 6 +++---
 include/linux/dma-mapping.h | 5 +++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index e3b04786838f..466b0242e8af 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2396,8 +2396,8 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 
 void arch_teardown_dma_ops(struct device *dev)
 {
-	if (!dev->archdata.dma_ops_setup)
-		return;
+	if (dev->archdata.dma_ops_setup)
+		arm_teardown_iommu_dma_ops(dev);
 
-	arm_teardown_iommu_dma_ops(dev);
+	generic_teardown_dma_ops(dev);
 }
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index eafd6f318e78..020512cb7f0e 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -663,11 +663,12 @@ static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
 				      bool coherent) { }
 #endif
 
-#ifndef arch_teardown_dma_ops
-static inline void arch_teardown_dma_ops(struct device *dev)
+static inline void generic_teardown_dma_ops(struct device *dev)
 {
 	dev->dma_ops = NULL;
 }
+#ifndef arch_teardown_dma_ops
+#define arch_teardown_dma_ops generic_teardown_dma_ops
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
-- 
2.18.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [RFC PATCH 3/3] dma-mapping: clear dma_parms on teardown, too
  2018-09-13 15:17 [RFC PATCH 0/3] dma-mapping: clear dangling pointers consistently Wolfram Sang
  2018-09-13 15:17 ` [RFC PATCH 1/3] ARM: dma-mapping: update comment about handling dma_ops when detaching from IOMMU Wolfram Sang
  2018-09-13 15:17 ` [RFC PATCH 2/3] dma-mapping: clear dma_ops pointer also on ARM Wolfram Sang
@ 2018-09-13 15:17 ` Wolfram Sang
  2018-09-14 13:12   ` Christoph Hellwig
  2018-09-14 13:23   ` Robin Murphy
  2 siblings, 2 replies; 10+ messages in thread
From: Wolfram Sang @ 2018-09-13 15:17 UTC (permalink / raw)
  To: iommu, Robin Murphy, Christoph Hellwig
  Cc: linux-renesas-soc, Marek Szyprowski, linux-arm-kernel,
	linux-kernel, Wolfram Sang

While sanitizig the pointer for dma_ops on teardown, do the same for
dma_parms, too. Rename the function to have a more generic name.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 arch/arm/mm/dma-mapping.c   | 2 +-
 include/linux/dma-mapping.h | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 466b0242e8af..bcf77bc0423f 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2399,5 +2399,5 @@ void arch_teardown_dma_ops(struct device *dev)
 	if (dev->archdata.dma_ops_setup)
 		arm_teardown_iommu_dma_ops(dev);
 
-	generic_teardown_dma_ops(dev);
+	generic_teardown_dma(dev);
 }
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 020512cb7f0e..6a2d8779b1d8 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -663,12 +663,13 @@ static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
 				      bool coherent) { }
 #endif
 
-static inline void generic_teardown_dma_ops(struct device *dev)
+static inline void generic_teardown_dma(struct device *dev)
 {
 	dev->dma_ops = NULL;
+	dev->dma_parms = NULL;
 }
 #ifndef arch_teardown_dma_ops
-#define arch_teardown_dma_ops generic_teardown_dma_ops
+#define arch_teardown_dma_ops generic_teardown_dma
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
-- 
2.18.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 1/3] ARM: dma-mapping: update comment about handling dma_ops when detaching from IOMMU
  2018-09-13 15:17 ` [RFC PATCH 1/3] ARM: dma-mapping: update comment about handling dma_ops when detaching from IOMMU Wolfram Sang
@ 2018-09-14 12:15   ` Geert Uytterhoeven
  2018-09-14 16:11     ` Wolfram Sang
  0 siblings, 1 reply; 10+ messages in thread
From: Geert Uytterhoeven @ 2018-09-14 12:15 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Linux IOMMU, Robin Murphy, Christoph Hellwig, Linux-Renesas,
	Marek Szyprowski, Linux ARM, Linux Kernel Mailing List

On Thu, Sep 13, 2018 at 5:18 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Update the comment because we don't set the pointer to NULL anymore.
> Also use the correct pointer name 'dma_ops' instead of 'dma_map_ops'.
>
> Fixes: 1874619a7df4 ("ARM: dma-mapping: Set proper DMA ops in arm_iommu_detach_device()")
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Nice catch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 2/3] dma-mapping: clear dma_ops pointer also on ARM
  2018-09-13 15:17 ` [RFC PATCH 2/3] dma-mapping: clear dma_ops pointer also on ARM Wolfram Sang
@ 2018-09-14 13:12   ` Christoph Hellwig
  0 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2018-09-14 13:12 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: iommu, Robin Murphy, Christoph Hellwig, linux-renesas-soc,
	Marek Szyprowski, linux-arm-kernel, linux-kernel

On Thu, Sep 13, 2018 at 05:17:15PM +0200, Wolfram Sang wrote:
> The generic fallback of arch_teardown_dma_ops() clears the dma_ops
> pointer but the ARM specific version does not. Rename the generic one,
> so it can be called from ARM specific one, too. This will ensure
> consistent behaviour.

Hmm.  I'd rather remove your new generic version entirely and move
the ops clearing into the two callers.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 3/3] dma-mapping: clear dma_parms on teardown, too
  2018-09-13 15:17 ` [RFC PATCH 3/3] dma-mapping: clear dma_parms on teardown, too Wolfram Sang
@ 2018-09-14 13:12   ` Christoph Hellwig
  2018-09-14 13:23   ` Robin Murphy
  1 sibling, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2018-09-14 13:12 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: iommu, Robin Murphy, Christoph Hellwig, linux-renesas-soc,
	Marek Szyprowski, linux-arm-kernel, linux-kernel

Same here.  I don't even think we really need to clear the ops to
start with, but we if we want to do it I'd just do it directly in
the two callers.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 3/3] dma-mapping: clear dma_parms on teardown, too
  2018-09-13 15:17 ` [RFC PATCH 3/3] dma-mapping: clear dma_parms on teardown, too Wolfram Sang
  2018-09-14 13:12   ` Christoph Hellwig
@ 2018-09-14 13:23   ` Robin Murphy
  2018-09-14 15:57     ` Wolfram Sang
  1 sibling, 1 reply; 10+ messages in thread
From: Robin Murphy @ 2018-09-14 13:23 UTC (permalink / raw)
  To: Wolfram Sang, iommu, Christoph Hellwig
  Cc: linux-renesas-soc, Marek Szyprowski, linux-arm-kernel, linux-kernel

On 13/09/18 16:17, Wolfram Sang wrote:
> While sanitizig the pointer for dma_ops on teardown, do the same for
> dma_parms, too. Rename the function to have a more generic name.

Upon closer inspection, it looks like there are some cases (at least PCI 
and MFD) where dma_parms is installed by the parent/bus at device 
creation, and therefore remains valid and *would* be expected to persist 
across the child device's driver unbinding and rebinding - seems this is 
more complex than I first thought, sorry.

Robin.

> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>   arch/arm/mm/dma-mapping.c   | 2 +-
>   include/linux/dma-mapping.h | 5 +++--
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 466b0242e8af..bcf77bc0423f 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -2399,5 +2399,5 @@ void arch_teardown_dma_ops(struct device *dev)
>   	if (dev->archdata.dma_ops_setup)
>   		arm_teardown_iommu_dma_ops(dev);
>   
> -	generic_teardown_dma_ops(dev);
> +	generic_teardown_dma(dev);
>   }
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 020512cb7f0e..6a2d8779b1d8 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -663,12 +663,13 @@ static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
>   				      bool coherent) { }
>   #endif
>   
> -static inline void generic_teardown_dma_ops(struct device *dev)
> +static inline void generic_teardown_dma(struct device *dev)
>   {
>   	dev->dma_ops = NULL;
> +	dev->dma_parms = NULL;
>   }
>   #ifndef arch_teardown_dma_ops
> -#define arch_teardown_dma_ops generic_teardown_dma_ops
> +#define arch_teardown_dma_ops generic_teardown_dma
>   #endif
>   
>   static inline unsigned int dma_get_max_seg_size(struct device *dev)
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 3/3] dma-mapping: clear dma_parms on teardown, too
  2018-09-14 13:23   ` Robin Murphy
@ 2018-09-14 15:57     ` Wolfram Sang
  0 siblings, 0 replies; 10+ messages in thread
From: Wolfram Sang @ 2018-09-14 15:57 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Wolfram Sang, iommu, Christoph Hellwig, linux-renesas-soc,
	Marek Szyprowski, linux-arm-kernel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

On Fri, Sep 14, 2018 at 02:23:51PM +0100, Robin Murphy wrote:
> On 13/09/18 16:17, Wolfram Sang wrote:
> > While sanitizig the pointer for dma_ops on teardown, do the same for
> > dma_parms, too. Rename the function to have a more generic name.
> 
> Upon closer inspection, it looks like there are some cases (at least PCI and
> MFD) where dma_parms is installed by the parent/bus at device creation, and
> therefore remains valid and *would* be expected to persist across the child
> device's driver unbinding and rebinding - seems this is more complex than I
> first thought, sorry.

No problem. I am thankful I understand more details. So, the life cycle
of dma_parms is truly that of the device. Which means that the drivers
using devm_kzalloc in their probe, should ideally clear the pointer on
unbind. Otherwise, it is not possible to say if the non-NULL dma_parms
is intended or dangling. Which makes my initial dmam_set_dma_parms()
approach look not too bad, I'd think?

However, I don't want to push hard with this one. If you think this is
too academic, I'll just leave it for now...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 1/3] ARM: dma-mapping: update comment about handling dma_ops when detaching from IOMMU
  2018-09-14 12:15   ` Geert Uytterhoeven
@ 2018-09-14 16:11     ` Wolfram Sang
  0 siblings, 0 replies; 10+ messages in thread
From: Wolfram Sang @ 2018-09-14 16:11 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Wolfram Sang, Linux IOMMU, Robin Murphy, Christoph Hellwig,
	Linux-Renesas, Marek Szyprowski, Linux ARM,
	Linux Kernel Mailing List

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On Fri, Sep 14, 2018 at 02:15:01PM +0200, Geert Uytterhoeven wrote:
> On Thu, Sep 13, 2018 at 5:18 PM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > Update the comment because we don't set the pointer to NULL anymore.
> > Also use the correct pointer name 'dma_ops' instead of 'dma_map_ops'.
> >
> > Fixes: 1874619a7df4 ("ARM: dma-mapping: Set proper DMA ops in arm_iommu_detach_device()")
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Nice catch!
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks, so I sent this seperately via ALKML now.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-13 15:17 [RFC PATCH 0/3] dma-mapping: clear dangling pointers consistently Wolfram Sang
2018-09-13 15:17 ` [RFC PATCH 1/3] ARM: dma-mapping: update comment about handling dma_ops when detaching from IOMMU Wolfram Sang
2018-09-14 12:15   ` Geert Uytterhoeven
2018-09-14 16:11     ` Wolfram Sang
2018-09-13 15:17 ` [RFC PATCH 2/3] dma-mapping: clear dma_ops pointer also on ARM Wolfram Sang
2018-09-14 13:12   ` Christoph Hellwig
2018-09-13 15:17 ` [RFC PATCH 3/3] dma-mapping: clear dma_parms on teardown, too Wolfram Sang
2018-09-14 13:12   ` Christoph Hellwig
2018-09-14 13:23   ` Robin Murphy
2018-09-14 15:57     ` Wolfram Sang

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox