[PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

[PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

[Huacai Chen]

In non-coherent DMA mode, kernel uses cache flushing operations to
maintain I/O coherency, so the dmapool objects should be aligned to
ARCH_DMA_MINALIGN. Otherwise, it will cause data corruption, at least
on MIPS:

	Step 1, dma_map_single
	Step 2, cache_invalidate (no writeback)
	Step 3, dma_from_device
	Step 4, dma_unmap_single

If a DMA buffer and a kernel structure share a same cache line, and if
the kernel structure has dirty data, cache_invalidate (no writeback)
will cause data lost.

Cc: stable@vger.kernel.org
Signed-off-by: Huacai Chen <chenhc@lemote.com>
---
 mm/dmapool.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index 4d90a64..6263905 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -140,6 +140,9 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
 	else if (align & (align - 1))
 		return NULL;
 
+	if (!device_is_coherent(dev))
+		align = max_t(size_t, align, dma_get_cache_alignment());
+
 	if (size == 0)
 		return NULL;
 	else if (size < 4)
-- 
2.7.0

[PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

[Huacai Chen]

In non-coherent DMA mode, kernel uses cache flushing operations to
maintain I/O coherency, so the dmapool objects should be aligned to
ARCH_DMA_MINALIGN. Otherwise, it will cause data corruption, at least
on MIPS:

	Step 1, dma_map_single
	Step 2, cache_invalidate (no writeback)
	Step 3, dma_from_device
	Step 4, dma_unmap_single

If a DMA buffer and a kernel structure share a same cache line, and if
the kernel structure has dirty data, cache_invalidate (no writeback)
will cause data lost.

Cc: stable@vger.kernel.org
Signed-off-by: Huacai Chen <chenhc@lemote.com>
---
 mm/dmapool.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index 4d90a64..6263905 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -140,6 +140,9 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
 	else if (align & (align - 1))
 		return NULL;
 
+	if (!device_is_coherent(dev))
+		align = max_t(size_t, align, dma_get_cache_alignment());
+
 	if (size == 0)
 		return NULL;
 	else if (size < 4)
-- 
2.7.0



--
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 V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

[Christoph Hellwig]

The dmapool code uses dma_alloc_coherent to allocate each element,
and dma_alloc_coherent must align to ARCH_DMA_MINALIGN already.
If you implementation doesn't do that it needs to be fixed.

Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

[Christoph Hellwig]

The dmapool code uses dma_alloc_coherent to allocate each element,
and dma_alloc_coherent must align to ARCH_DMA_MINALIGN already.
If you implementation doesn't do that it needs to be fixed.

--
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 V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN innon-coherent DMA mode

[陈华才]

Hi, Christoph,

Maybe you missed something.
1, pool_alloc_page() use dma_alloc_coherent() to allocate pool pages, and of course these pages are aligned to  ARCH_DMA_MINALIGN.
2, dma_pool_alloc() is the element allocator, but it doesn't use dma_alloc_coherent(). Elements only align to pool->size, but pool->size is usually less than ARCH_DMA_MINALIGN.
3, ARCH_DMA_MINALIGN is now only used in serveral drivers, no dma_ops use it.

Huacai
 
------------------ Original ------------------
From:  "Christoph Hellwig"<hch@infradead.org>;
Date:  Mon, Sep 18, 2017 01:22 PM
To:  "Huacai Chen"<chenhc@lemote.com>;
Cc:  "Andrew Morton"<akpm@linux-foundation.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "linux-mm"<linux-mm@kvack.org>; "linux-kernel"<linux-kernel@vger.kernel.org>; "stable"<stable@vger.kernel.org>;
Subject:  Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN innon-coherent DMA mode
 
The dmapool code uses dma_alloc_coherent to allocate each element,
and dma_alloc_coherent must align to ARCH_DMA_MINALIGN already.
If you implementation doesn't do that it needs to be fixed.

Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

[Robin Murphy]

On 18/09/17 05:22, Huacai Chen wrote:
> In non-coherent DMA mode, kernel uses cache flushing operations to
> maintain I/O coherency, so the dmapool objects should be aligned to
> ARCH_DMA_MINALIGN. Otherwise, it will cause data corruption, at least
> on MIPS:
> 
> 	Step 1, dma_map_single
> 	Step 2, cache_invalidate (no writeback)
> 	Step 3, dma_from_device
> 	Step 4, dma_unmap_single

This is a massive red warning flag for the whole series, because DMA
pools don't work like that. At best, this will do nothing, and at worst
it is papering over egregious bugs elsewhere. Streaming mappings of
coherent allocations means completely broken code.

> If a DMA buffer and a kernel structure share a same cache line, and if
> the kernel structure has dirty data, cache_invalidate (no writeback)
> will cause data lost.

DMA pools are backed by coherent allocations, and those should already
be at *page* granularity, so this doubly cannot happen for correct code.

More generally, the whole point of having the DMA APIs is that drivers
and subsystems should not have to be aware of details like hardware
coherency. Besides, cache line sharing that could pose a correctness
issue for non-hardware-coherent systems could still be a performance
issue in the presence of hardware coherency (due to unnecessary line
migration), so there's still an argument for not treating them differently.

Robin.

> Cc: stable@vger.kernel.org
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
>  mm/dmapool.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/dmapool.c b/mm/dmapool.c
> index 4d90a64..6263905 100644
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -140,6 +140,9 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
>  	else if (align & (align - 1))
>  		return NULL;
>  
> +	if (!device_is_coherent(dev))
> +		align = max_t(size_t, align, dma_get_cache_alignment());
> +
>  	if (size == 0)
>  		return NULL;
>  	else if (size < 4)
> 

Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

[Robin Murphy]

On 18/09/17 05:22, Huacai Chen wrote:
> In non-coherent DMA mode, kernel uses cache flushing operations to
> maintain I/O coherency, so the dmapool objects should be aligned to
> ARCH_DMA_MINALIGN. Otherwise, it will cause data corruption, at least
> on MIPS:
> 
> 	Step 1, dma_map_single
> 	Step 2, cache_invalidate (no writeback)
> 	Step 3, dma_from_device
> 	Step 4, dma_unmap_single

This is a massive red warning flag for the whole series, because DMA
pools don't work like that. At best, this will do nothing, and at worst
it is papering over egregious bugs elsewhere. Streaming mappings of
coherent allocations means completely broken code.

> If a DMA buffer and a kernel structure share a same cache line, and if
> the kernel structure has dirty data, cache_invalidate (no writeback)
> will cause data lost.

DMA pools are backed by coherent allocations, and those should already
be at *page* granularity, so this doubly cannot happen for correct code.

More generally, the whole point of having the DMA APIs is that drivers
and subsystems should not have to be aware of details like hardware
coherency. Besides, cache line sharing that could pose a correctness
issue for non-hardware-coherent systems could still be a performance
issue in the presence of hardware coherency (due to unnecessary line
migration), so there's still an argument for not treating them differently.

Robin.

> Cc: stable@vger.kernel.org
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
>  mm/dmapool.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/dmapool.c b/mm/dmapool.c
> index 4d90a64..6263905 100644
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -140,6 +140,9 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
>  	else if (align & (align - 1))
>  		return NULL;
>  
> +	if (!device_is_coherent(dev))
> +		align = max_t(size_t, align, dma_get_cache_alignment());
> +
>  	if (size == 0)
>  		return NULL;
>  	else if (size < 4)
> 

--
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 V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

[Christoph Hellwig]

Ok, it looks like adding the dma_get_cache_alignment constraint here
looks good - but again it should be unconditional.

Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

[Christoph Hellwig]

Ok, it looks like adding the dma_get_cache_alignment constraint here
looks good - but again it should be unconditional.

--
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: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

[Christoph Hellwig]

On Mon, Sep 18, 2017 at 10:44:54AM +0100, Robin Murphy wrote:
> On 18/09/17 05:22, Huacai Chen wrote:
> > In non-coherent DMA mode, kernel uses cache flushing operations to
> > maintain I/O coherency, so the dmapool objects should be aligned to
> > ARCH_DMA_MINALIGN. Otherwise, it will cause data corruption, at least
> > on MIPS:
> > 
> > 	Step 1, dma_map_single
> > 	Step 2, cache_invalidate (no writeback)
> > 	Step 3, dma_from_device
> > 	Step 4, dma_unmap_single
> 
> This is a massive red warning flag for the whole series, because DMA
> pools don't work like that. At best, this will do nothing, and at worst
> it is papering over egregious bugs elsewhere. Streaming mappings of
> coherent allocations means completely broken code.

Oh, I hadn't even seen that part.  Yes, dma coherent (and pool)
allocations must never be used for streaming mappings.  I wish we'd
have some debug infrastructure to warn on such uses.

Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

[Christoph Hellwig]

On Mon, Sep 18, 2017 at 10:44:54AM +0100, Robin Murphy wrote:
> On 18/09/17 05:22, Huacai Chen wrote:
> > In non-coherent DMA mode, kernel uses cache flushing operations to
> > maintain I/O coherency, so the dmapool objects should be aligned to
> > ARCH_DMA_MINALIGN. Otherwise, it will cause data corruption, at least
> > on MIPS:
> > 
> > 	Step 1, dma_map_single
> > 	Step 2, cache_invalidate (no writeback)
> > 	Step 3, dma_from_device
> > 	Step 4, dma_unmap_single
> 
> This is a massive red warning flag for the whole series, because DMA
> pools don't work like that. At best, this will do nothing, and at worst
> it is papering over egregious bugs elsewhere. Streaming mappings of
> coherent allocations means completely broken code.

Oh, I hadn't even seen that part.  Yes, dma coherent (and pool)
allocations must never be used for streaming mappings.  I wish we'd
have some debug infrastructure to warn on such uses.

--
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: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN innon-coherent DMA mode

[陈华才]

Oh, I know, I've make a mistake, dmapool doesn't need to change.

Huacai
 
 
------------------ Original ------------------
From:  "Christoph Hellwig"<hch@infradead.org>;
Date:  Mon, Sep 18, 2017 11:51 PM
To:  "Robin Murphy"<robin.murphy@arm.com>; 
Cc:  "Huacai Chen"<chenhc@lemote.com>; "Andrew Morton"<akpm@linux-foundation.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "linux-mm"<linux-mm@kvack.org>; "linux-kernel"<linux-kernel@vger.kernel.org>; "stable"<stable@vger.kernel.org>; 
Subject:  Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN innon-coherent DMA mode

 
On Mon, Sep 18, 2017 at 10:44:54AM +0100, Robin Murphy wrote:
> On 18/09/17 05:22, Huacai Chen wrote:
> > In non-coherent DMA mode, kernel uses cache flushing operations to
> > maintain I/O coherency, so the dmapool objects should be aligned to
> > ARCH_DMA_MINALIGN. Otherwise, it will cause data corruption, at least
> > on MIPS:
> > 
> > 	Step 1, dma_map_single
> > 	Step 2, cache_invalidate (no writeback)
> > 	Step 3, dma_from_device
> > 	Step 4, dma_unmap_single
> 
> This is a massive red warning flag for the whole series, because DMA
> pools don't work like that. At best, this will do nothing, and at worst
> it is papering over egregious bugs elsewhere. Streaming mappings of
> coherent allocations means completely broken code.

Oh, I hadn't even seen that part.  Yes, dma coherent (and pool)
allocations must never be used for streaming mappings.  I wish we'd
have some debug infrastructure to warn on such uses.