[PATCH 0/4] Assorted DMA mapping tweaks

[PATCH 0/4] Assorted DMA mapping tweaks

[Robin Murphy]

Hi Andrew,

This is a miscellany of fixes and tweaks to the common DMA mapping code
which I've been collecting. They don't strictly depend on each other,
but I figure I may as well send them all together for the sake of
explaining them in one place:

#1 is a straightforward and hopefully obvious bugfix.
#2 has been posted before but I subsequently forgot to follow up on it.
   It's still possible to hit this at least on arm64 systems, with the
   SWIOTLB/pl330 combination blowing up just running dmatest.
#3 follows on from a recent discussion about dma_sync_sg[0]; there's
   already a related patch in -next from Sakari clarifying the docs.
#4 seemed worth posting now that the recent rework means it no longer
   has to be a sprawling touch-all-the-architectures patch for something
   so small.

Thanks,
Robin.

[0]:http://thread.gmane.org/gmane.linux.kernel/2043117

Robin Murphy (4):
  dmapool: Fix overflow condition in pool_find_page
  dma-mapping: Tidy up dma_parms default handling
  dma-debug: Check nents in dma_sync_sg*
  dma-debug: Allow poisoning nonzero allocations

 include/asm-generic/dma-mapping-common.h |  2 +-
 include/linux/dma-debug.h                |  6 ++++--
 include/linux/dma-mapping.h              | 17 ++++++++++-------
 include/linux/poison.h                   |  3 +++
 lib/Kconfig.debug                        | 10 ++++++++++
 lib/dma-debug.c                          | 14 +++++++++++++-
 mm/dmapool.c                             |  2 +-
 7 files changed, 42 insertions(+), 12 deletions(-)

-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/4] dmapool: Fix overflow condition in pool_find_page

[Robin Murphy]

If a DMA pool lies at the very top of the dma_addr_t range (as may
happen with an IOMMU involved), the calculated end address of the pool
wraps around to zero, and page lookup always fails. Tweak the relevant
calculation to be overflow-proof.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 mm/dmapool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index 71a8998..312a716 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -394,7 +394,7 @@ static struct dma_page *pool_find_page(struct dma_pool *pool, dma_addr_t dma)
 	list_for_each_entry(page, &pool->page_list, page_list) {
 		if (dma < page->dma)
 			continue;
-		if (dma < (page->dma + pool->allocation))
+		if ((dma - page->dma) < pool->allocation)
 			return page;
 	}
 	return NULL;
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/4] dma-mapping: Tidy up dma_parms default handling

[Robin Murphy]

Many DMA controllers and other devices set max_segment_size to
indicate their scatter-gather capability, but have no interest in
segment_boundary_mask. However, the existence of a dma_parms structure
precludes the use of any default value, leaving them as zeros (assuming
a properly kzalloc'ed structure). If a well-behaved IOMMU (or SWIOTLB)
then tries to respect this by ensuring a mapped segment does not cross
a zero-byte boundary, hilarity ensues.

Since zero is a nonsensical value for either parameter, treat it as an
indicator for "default", as might be expected. In the process, clean up
a bit by replacing the bare constants with slightly more meaningful
macros and removing the superfluous "else" statements.

Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Sumit Semwal <sumit.semwal@linaro.org>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 include/linux/dma-mapping.h | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index ac07ff0..ac9af22 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -145,7 +145,9 @@ static inline void arch_teardown_dma_ops(struct device *dev) { }
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
 {
-	return dev->dma_parms ? dev->dma_parms->max_segment_size : 65536;
+	if (dev->dma_parms && dev->dma_parms->max_segment_size)
+		return dev->dma_parms->max_segment_size;
+	return SZ_64K;
 }
 
 static inline unsigned int dma_set_max_seg_size(struct device *dev,
@@ -154,14 +156,15 @@ static inline unsigned int dma_set_max_seg_size(struct device *dev,
 	if (dev->dma_parms) {
 		dev->dma_parms->max_segment_size = size;
 		return 0;
-	} else
-		return -EIO;
+	}
+	return -EIO;
 }
 
 static inline unsigned long dma_get_seg_boundary(struct device *dev)
 {
-	return dev->dma_parms ?
-		dev->dma_parms->segment_boundary_mask : 0xffffffff;
+	if (dev->dma_parms && dev->dma_parms->segment_boundary_mask)
+		return dev->dma_parms->segment_boundary_mask;
+	return DMA_BIT_MASK(32);
 }
 
 static inline int dma_set_seg_boundary(struct device *dev, unsigned long mask)
@@ -169,8 +172,8 @@ static inline int dma_set_seg_boundary(struct device *dev, unsigned long mask)
 	if (dev->dma_parms) {
 		dev->dma_parms->segment_boundary_mask = mask;
 		return 0;
-	} else
-		return -EIO;
+	}
+	return -EIO;
 }
 
 #ifndef dma_max_pfn
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/4] dma-debug: Check nents in dma_sync_sg*

[Robin Murphy]

Like dma_unmap_sg, dma_sync_sg* should be called with the original
number of entries passed to dma_map_sg, so do the same check in the sync
path as we do in the unmap path.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 lib/dma-debug.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index dace71f..908fb35 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -1249,6 +1249,14 @@ static void check_sync(struct device *dev,
 				dir2name[entry->direction],
 				dir2name[ref->direction]);
 
+	if (ref->sg_call_ents && ref->type == dma_debug_sg &&
+	    ref->sg_call_ents != entry->sg_call_ents) {
+		err_printk(ref->dev, entry, "DMA-API: device driver syncs "
+			   "DMA sg list with different entry count "
+			   "[map count=%d] [sync count=%d]\n",
+			   entry->sg_call_ents, ref->sg_call_ents);
+	}
+
 out:
 	put_hash_bucket(bucket, &flags);
 }
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 4/4] dma-debug: Allow poisoning nonzero allocations

[Robin Murphy]

Since some dma_alloc_coherent implementations return a zeroed buffer
regardless of whether __GFP_ZERO is passed, there exist drivers which
are implicitly dependent on this and pass otherwise uninitialised
buffers to hardware. This can lead to subtle and awkward-to-debug issues
using those drivers on different platforms, where nonzero uninitialised
junk may for instance occasionally look like a valid command which
causes the hardware to start misbehaving. To help with debugging such
issues, add the option to make uninitialised buffers much more obvious.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 include/asm-generic/dma-mapping-common.h |  2 +-
 include/linux/dma-debug.h                |  6 ++++--
 include/linux/poison.h                   |  3 +++
 lib/Kconfig.debug                        | 10 ++++++++++
 lib/dma-debug.c                          |  6 +++++-
 5 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/asm-generic/dma-mapping-common.h b/include/asm-generic/dma-mapping-common.h
index b1bc954..0f3e16b 100644
--- a/include/asm-generic/dma-mapping-common.h
+++ b/include/asm-generic/dma-mapping-common.h
@@ -260,7 +260,7 @@ static inline void *dma_alloc_attrs(struct device *dev, size_t size,
 		return NULL;
 
 	cpu_addr = ops->alloc(dev, size, dma_handle, flag, attrs);
-	debug_dma_alloc_coherent(dev, size, *dma_handle, cpu_addr);
+	debug_dma_alloc_coherent(dev, size, *dma_handle, cpu_addr, flag);
 	return cpu_addr;
 }
 
diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index fe8cb61..e5f539d 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -51,7 +51,8 @@ extern void debug_dma_unmap_sg(struct device *dev, struct scatterlist *sglist,
 			       int nelems, int dir);
 
 extern void debug_dma_alloc_coherent(struct device *dev, size_t size,
-				     dma_addr_t dma_addr, void *virt);
+				     dma_addr_t dma_addr, void *virt,
+				     gfp_t flags);
 
 extern void debug_dma_free_coherent(struct device *dev, size_t size,
 				    void *virt, dma_addr_t addr);
@@ -132,7 +133,8 @@ static inline void debug_dma_unmap_sg(struct device *dev,
 }
 
 static inline void debug_dma_alloc_coherent(struct device *dev, size_t size,
-					    dma_addr_t dma_addr, void *virt)
+					    dma_addr_t dma_addr, void *virt,
+					    gfp_t flags)
 {
 }
 
diff --git a/include/linux/poison.h b/include/linux/poison.h
index 317e16d..174104e 100644
--- a/include/linux/poison.h
+++ b/include/linux/poison.h
@@ -73,6 +73,9 @@
 #define MUTEX_DEBUG_INIT	0x11
 #define MUTEX_DEBUG_FREE	0x22
 
+/********** lib/dma_debug.c **********/
+#define DMA_ALLOC_POISON	0xee
+
 /********** lib/flex_array.c **********/
 #define FLEX_ARRAY_FREE	0x6c	/* for use-after-free poisoning */
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index ab76b99..f2da7a1 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1752,6 +1752,16 @@ config DMA_API_DEBUG
 
 	  If unsure, say N.
 
+config DMA_API_DEBUG_POISON
+	bool "Poison coherent DMA buffers"
+	depends on DMA_API_DEBUG && EXPERT
+	help
+	  Poison DMA buffers returned by dma_alloc_coherent unless __GFP_ZERO
+	  is explicitly specified, to catch drivers depending on zeroed buffers
+	  without passing the correct flags.
+
+	  Only say Y if you're prepared for almost everything to break.
+
 config TEST_LKM
 	tristate "Test module loading with 'hello world' module"
 	default n
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 908fb35..40514ed 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -30,6 +30,7 @@
 #include <linux/sched.h>
 #include <linux/ctype.h>
 #include <linux/list.h>
+#include <linux/poison.h>
 #include <linux/slab.h>
 
 #include <asm/sections.h>
@@ -1447,7 +1448,7 @@ void debug_dma_unmap_sg(struct device *dev, struct scatterlist *sglist,
 EXPORT_SYMBOL(debug_dma_unmap_sg);
 
 void debug_dma_alloc_coherent(struct device *dev, size_t size,
-			      dma_addr_t dma_addr, void *virt)
+			      dma_addr_t dma_addr, void *virt, gfp_t flags)
 {
 	struct dma_debug_entry *entry;
 
@@ -1457,6 +1458,9 @@ void debug_dma_alloc_coherent(struct device *dev, size_t size,
 	if (unlikely(virt == NULL))
 		return;
 
+	if (IS_ENABLED(CONFIG_DMA_API_DEBUG_POISON) && !(flags & __GFP_ZERO))
+		memset(virt, DMA_ALLOC_POISON, size);
+
 	entry = dma_entry_alloc();
 	if (!entry)
 		return;
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] dma-debug: Allow poisoning nonzero allocations

[Russell King - ARM Linux]

On Fri, Sep 25, 2015 at 01:15:46PM +0100, Robin Murphy wrote:
> Since some dma_alloc_coherent implementations return a zeroed buffer
> regardless of whether __GFP_ZERO is passed, there exist drivers which
> are implicitly dependent on this and pass otherwise uninitialised
> buffers to hardware. This can lead to subtle and awkward-to-debug issues
> using those drivers on different platforms, where nonzero uninitialised
> junk may for instance occasionally look like a valid command which
> causes the hardware to start misbehaving. To help with debugging such
> issues, add the option to make uninitialised buffers much more obvious.

The reason people started to do this is to stop a security leak in the
ALSA code: ALSA allocates the ring buffer with dma_alloc_coherent()
which used to grab pages and return them uninitialised.  These pages
could contain anything - including the contents of /etc/shadow, or
your bank details.

ALSA then lets userspace mmap() that memory, which means any user process
which has access to the sound devices can read data leaked from kernel
memory.

I think I did bring it up at the time I found it, and decided that the
safest thing to do was to always return an initialised buffer - short of
constantly auditing every dma_alloc_coherent() user which also mmap()s
the buffer into userspace, I couldn't convince myself that it was safe
to avoid initialising the buffer.

I don't know whether the original problem still exists in ALSA or not,
but I do know that there are dma_alloc_coherent() implementations out
there which do not initialise prior to returning memory.

> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index 908fb35..40514ed 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -30,6 +30,7 @@
>  #include <linux/sched.h>
>  #include <linux/ctype.h>
>  #include <linux/list.h>
> +#include <linux/poison.h>
>  #include <linux/slab.h>
>  
>  #include <asm/sections.h>
> @@ -1447,7 +1448,7 @@ void debug_dma_unmap_sg(struct device *dev, struct scatterlist *sglist,
>  EXPORT_SYMBOL(debug_dma_unmap_sg);
>  
>  void debug_dma_alloc_coherent(struct device *dev, size_t size,
> -			      dma_addr_t dma_addr, void *virt)
> +			      dma_addr_t dma_addr, void *virt, gfp_t flags)
>  {
>  	struct dma_debug_entry *entry;
>  
> @@ -1457,6 +1458,9 @@ void debug_dma_alloc_coherent(struct device *dev, size_t size,
>  	if (unlikely(virt == NULL))
>  		return;
>  
> +	if (IS_ENABLED(CONFIG_DMA_API_DEBUG_POISON) && !(flags & __GFP_ZERO))
> +		memset(virt, DMA_ALLOC_POISON, size);
> +

This is likely to be slow in the case of non-cached memory and large
allocations.  The config option should come with a warning.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] dma-debug: Allow poisoning nonzero allocations

[Robin Murphy]

Hi Russell,

On 25/09/15 13:44, Russell King - ARM Linux wrote:
> On Fri, Sep 25, 2015 at 01:15:46PM +0100, Robin Murphy wrote:
>> Since some dma_alloc_coherent implementations return a zeroed buffer
>> regardless of whether __GFP_ZERO is passed, there exist drivers which
>> are implicitly dependent on this and pass otherwise uninitialised
>> buffers to hardware. This can lead to subtle and awkward-to-debug issues
>> using those drivers on different platforms, where nonzero uninitialised
>> junk may for instance occasionally look like a valid command which
>> causes the hardware to start misbehaving. To help with debugging such
>> issues, add the option to make uninitialised buffers much more obvious.
>
> The reason people started to do this is to stop a security leak in the
> ALSA code: ALSA allocates the ring buffer with dma_alloc_coherent()
> which used to grab pages and return them uninitialised.  These pages
> could contain anything - including the contents of /etc/shadow, or
> your bank details.
>
> ALSA then lets userspace mmap() that memory, which means any user process
> which has access to the sound devices can read data leaked from kernel
> memory.
>
> I think I did bring it up at the time I found it, and decided that the
> safest thing to do was to always return an initialised buffer - short of
> constantly auditing every dma_alloc_coherent() user which also mmap()s
> the buffer into userspace, I couldn't convince myself that it was safe
> to avoid initialising the buffer.
>
> I don't know whether the original problem still exists in ALSA or not,
> but I do know that there are dma_alloc_coherent() implementations out
> there which do not initialise prior to returning memory.

Indeed, I think we've discussed this before, and I don't imagine we'll 
be changing the actual behaviour of the existing allocators any time soon.

[ I still don't see that as an excuse for callers not to be fixed, 
though - anyone allocating something that may be exposed to userspace 
has a responsibility to initialise it appropriately. After all, the DMA 
API is just one source, what do we do if such a careless subsystem got 
some uninitialised pages of leftover sensitive data from, say, 
alloc_pages() instead? ]

That's a bit of a separate issue though. If a driver itself _needs_ a 
zeroed buffer but doesn't specifically request one, or doesn't get one 
even if it did, then that's just a regular bug, and it's what this patch 
is intended to help weed out. We've no need for a special poison value 
for data protection in the general case; zero is just fine for that.

>> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
>> index 908fb35..40514ed 100644
>> --- a/lib/dma-debug.c
>> +++ b/lib/dma-debug.c
>> @@ -30,6 +30,7 @@
>>   #include <linux/sched.h>
>>   #include <linux/ctype.h>
>>   #include <linux/list.h>
>> +#include <linux/poison.h>
>>   #include <linux/slab.h>
>>
>>   #include <asm/sections.h>
>> @@ -1447,7 +1448,7 @@ void debug_dma_unmap_sg(struct device *dev, struct scatterlist *sglist,
>>   EXPORT_SYMBOL(debug_dma_unmap_sg);
>>
>>   void debug_dma_alloc_coherent(struct device *dev, size_t size,
>> -			      dma_addr_t dma_addr, void *virt)
>> +			      dma_addr_t dma_addr, void *virt, gfp_t flags)
>>   {
>>   	struct dma_debug_entry *entry;
>>
>> @@ -1457,6 +1458,9 @@ void debug_dma_alloc_coherent(struct device *dev, size_t size,
>>   	if (unlikely(virt == NULL))
>>   		return;
>>
>> +	if (IS_ENABLED(CONFIG_DMA_API_DEBUG_POISON) && !(flags & __GFP_ZERO))
>> +		memset(virt, DMA_ALLOC_POISON, size);
>> +
>
> This is likely to be slow in the case of non-cached memory and large
> allocations.  The config option should come with a warning.

It depends on DMA_API_DEBUG, which already has a stern performance 
warning, is additionally hidden behind EXPERT, and carries a slightly 
flippant yet largely truthful warning that actually using it could break 
pretty much every driver in your system; is that not enough?

If I was feeling particularly antagonistic, I'd also point out that as 
discussed above you've already taken the hit of a memset(0) and cache 
flush that you _didn't_ ask for, and there was no warning on that ;)

The intent is a specific troubleshooting tool - realistically it's 
probably only usable at all when restricting DMA debug to a per-driver 
basis. My hunch is that nobody's too fussed about the performance of a 
driver that doesn't work properly, especially once they've reached the 
point of dumping buffers in an attempt to figure out why, when seeing 
the presence or not of uniform poison values could be helpful.

(Of course, sometimes you end up debugging the allocator itself - see 
commit 7132813c3845 - which was one of the motivating factors for this 
patch).

Robin.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] dma-debug: Allow poisoning nonzero allocations

[Andrew Morton]

On Fri, 25 Sep 2015 18:35:39 +0100 Robin Murphy <robin.murphy@arm.com> wrote:

> Hi Russell,
> 
> On 25/09/15 13:44, Russell King - ARM Linux wrote:
> > On Fri, Sep 25, 2015 at 01:15:46PM +0100, Robin Murphy wrote:
> >> Since some dma_alloc_coherent implementations return a zeroed buffer
> >> regardless of whether __GFP_ZERO is passed, there exist drivers which
> >> are implicitly dependent on this and pass otherwise uninitialised
> >> buffers to hardware. This can lead to subtle and awkward-to-debug issues
> >> using those drivers on different platforms, where nonzero uninitialised
> >> junk may for instance occasionally look like a valid command which
> >> causes the hardware to start misbehaving. To help with debugging such
> >> issues, add the option to make uninitialised buffers much more obvious.
> >
> > The reason people started to do this is to stop a security leak in the
> > ALSA code: ALSA allocates the ring buffer with dma_alloc_coherent()
> > which used to grab pages and return them uninitialised.  These pages
> > could contain anything - including the contents of /etc/shadow, or
> > your bank details.
> >
> > ALSA then lets userspace mmap() that memory, which means any user process
> > which has access to the sound devices can read data leaked from kernel
> > memory.
> >
> > I think I did bring it up at the time I found it, and decided that the
> > safest thing to do was to always return an initialised buffer - short of
> > constantly auditing every dma_alloc_coherent() user which also mmap()s
> > the buffer into userspace, I couldn't convince myself that it was safe
> > to avoid initialising the buffer.
> >
> > I don't know whether the original problem still exists in ALSA or not,
> > but I do know that there are dma_alloc_coherent() implementations out
> > there which do not initialise prior to returning memory.
> 
> Indeed, I think we've discussed this before, and I don't imagine we'll 
> be changing the actual behaviour of the existing allocators any time soon.

If I'm understanding things correctly, some allocators zero the memory
by default and others do not.  And we have an unknown number of drivers
which are assuming that the memory is zeroed.

Correct?

If so, our options are

a) audit all callers, find the ones which expect zeroed memory but
   aren't passing __GFP_ZERO and fix them.

b) convert all allocators to zero the memory by default.

Obviously, a) is better.  How big a job is it?

This patch will help the process, if people use it.

> >> +	if (IS_ENABLED(CONFIG_DMA_API_DEBUG_POISON) && !(flags & __GFP_ZERO))
> >> +		memset(virt, DMA_ALLOC_POISON, size);
> >> +
> >
> > This is likely to be slow in the case of non-cached memory and large
> > allocations.  The config option should come with a warning.
> 
> It depends on DMA_API_DEBUG, which already has a stern performance 
> warning, is additionally hidden behind EXPERT, and carries a slightly 
> flippant yet largely truthful warning that actually using it could break 
> pretty much every driver in your system; is that not enough?

It might be helpful to provide a runtime knob as well - having to
rebuild&reinstall just to enable/disable this feature is a bit painful.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] dma-debug: Allow poisoning nonzero allocations

[Robin Murphy]

On 29/09/15 22:27, Andrew Morton wrote:
[...]
> If I'm understanding things correctly, some allocators zero the memory
> by default and others do not.  And we have an unknown number of drivers
> which are assuming that the memory is zeroed.
>
> Correct?

That's precisely the motivation here, yes.

> If so, our options are
>
> a) audit all callers, find the ones which expect zeroed memory but
>     aren't passing __GFP_ZERO and fix them.
>
> b) convert all allocators to zero the memory by default.
>
> Obviously, a) is better.  How big a job is it?

This I'm not so sure of, hence the very tentative first step. For a very 
crude guess at an an upper bound:

$ git grep -E '(dma|pci)_alloc_co(her|nsist)ent' drivers/ | wc -l
1148

vs.

$ git grep -E '(dma|pci)_zalloc_co(her|nsist)ent' drivers/ | wc -l
234

noting that the vast majority of the former are still probably benign, 
but picking out those which aren't from the code alone without knowledge 
of and/or access to the hardware might be non-trivial.

> This patch will help the process, if people use it.
>
>>>> +	if (IS_ENABLED(CONFIG_DMA_API_DEBUG_POISON) && !(flags & __GFP_ZERO))
>>>> +		memset(virt, DMA_ALLOC_POISON, size);
>>>> +
>>>
>>> This is likely to be slow in the case of non-cached memory and large
>>> allocations.  The config option should come with a warning.
>>
>> It depends on DMA_API_DEBUG, which already has a stern performance
>> warning, is additionally hidden behind EXPERT, and carries a slightly
>> flippant yet largely truthful warning that actually using it could break
>> pretty much every driver in your system; is that not enough?
>
> It might be helpful to provide a runtime knob as well - having to
> rebuild&reinstall just to enable/disable this feature is a bit painful.

Good point - there's always the global DMA debug disable knob, but this 
particular feature probably does warrant finer-grained control to be 
really practical. Having thought about it some more, it's also probably 
wrong that this doesn't respect the dma_debug_driver filter, given that 
it is actually invasive; in fixing that, how about if it also *only* 
applied when a specific driver is filtered? Then there would be no 
problematic "break anything and everything" mode, and the existing 
debugfs controls should suffice.

Robin.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] dma-debug: Allow poisoning nonzero allocations

[Andrew Morton]

On Wed, 7 Oct 2015 20:17:03 +0100 Robin Murphy <robin.murphy@arm.com> wrote:

> > It might be helpful to provide a runtime knob as well - having to
> > rebuild&reinstall just to enable/disable this feature is a bit painful.
> 
> Good point - there's always the global DMA debug disable knob, but this 
> particular feature probably does warrant finer-grained control to be 
> really practical. Having thought about it some more, it's also probably 
> wrong that this doesn't respect the dma_debug_driver filter, given that 
> it is actually invasive; in fixing that, how about if it also *only* 
> applied when a specific driver is filtered? Then there would be no 
> problematic "break anything and everything" mode, and the existing 
> debugfs controls should suffice.

Yes, this should respect the driver filtering.

On reflection...

The patch poisons dma buffers if CONFIG_DMA_API_DEBUG and if __GFP_ZERO
wasn't explicitly used.  I'm rather surprised that the dma-debug code
didn't do this from day one.

I'd be inclined to enable this buffer-poisoning by default.  Do you
have a feeling for how much overhead that will add?  Presumably not
much, if __GFP_ZERO is acceptable.

Also, how about we remove CONFIG_DMA_API_DEBUG_POISON and switch to a
debugfs knob?


btw, the documentation could do with a bit of a tune-up.  The comments
in dma-debug.c regarding driver filtering are non-existent. 
Documentation/kernel-parameters.txt says "The filter can be disabled or
changed to another driver later using sysfs" but
Documentation/DMA-API.txt talks about debugfs.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>