All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
       [not found] <CGME20180613085851eucas1p20337d050face8ff8ea87674e16a9ccd2@eucas1p2.samsung.com>
@ 2018-06-13  8:58   ` Marek Szyprowski
  0 siblings, 0 replies; 22+ messages in thread
From: Marek Szyprowski @ 2018-06-13  8:58 UTC (permalink / raw)
  To: linux-mm, linux-kernel, linux-arm-kernel
  Cc: Marek Szyprowski, Andrew Morton, Michal Nazarewicz, Joonsoo Kim,
	Vlastimil Babka

cma_alloc() function has gfp mask parameter, so users expect that it
honors typical memory allocation related flags. The most imporant from
the security point of view is handling of __GFP_ZERO flag, because memory
allocated by this function usually can be directly remapped to userspace
by device drivers as a part of multimedia processing and ignoring this
flag might lead to leaking some kernel structures to userspace.
Some callers of this function (for example arm64 dma-iommu glue code)
already assumed that the allocated buffers are cleared when this flag
is set. To avoid such issues, add simple code for clearing newly
allocated buffer when __GFP_ZERO flag is set. Callers will be then
updated to skip implicit clearing or adjust passed gfp flags.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 mm/cma.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/mm/cma.c b/mm/cma.c
index 5809bbe360d7..1106d5aef2cc 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -464,6 +464,13 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
 		start = bitmap_no + mask + 1;
 	}
 
+	if (ret == 0 && gfp_mask & __GFP_ZERO) {
+		int i;
+
+		for (i = 0; i < count; i++)
+			clear_highpage(page + i);
+	}
+
 	trace_cma_alloc(pfn, page, count, align);
 
 	if (ret && !(gfp_mask & __GFP_NOWARN)) {
-- 
2.17.1


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

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
@ 2018-06-13  8:58   ` Marek Szyprowski
  0 siblings, 0 replies; 22+ messages in thread
From: Marek Szyprowski @ 2018-06-13  8:58 UTC (permalink / raw)
  To: linux-arm-kernel

cma_alloc() function has gfp mask parameter, so users expect that it
honors typical memory allocation related flags. The most imporant from
the security point of view is handling of __GFP_ZERO flag, because memory
allocated by this function usually can be directly remapped to userspace
by device drivers as a part of multimedia processing and ignoring this
flag might lead to leaking some kernel structures to userspace.
Some callers of this function (for example arm64 dma-iommu glue code)
already assumed that the allocated buffers are cleared when this flag
is set. To avoid such issues, add simple code for clearing newly
allocated buffer when __GFP_ZERO flag is set. Callers will be then
updated to skip implicit clearing or adjust passed gfp flags.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 mm/cma.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/mm/cma.c b/mm/cma.c
index 5809bbe360d7..1106d5aef2cc 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -464,6 +464,13 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
 		start = bitmap_no + mask + 1;
 	}
 
+	if (ret == 0 && gfp_mask & __GFP_ZERO) {
+		int i;
+
+		for (i = 0; i < count; i++)
+			clear_highpage(page + i);
+	}
+
 	trace_cma_alloc(pfn, page, count, align);
 
 	if (ret && !(gfp_mask & __GFP_NOWARN)) {
-- 
2.17.1

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

* Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
  2018-06-13  8:58   ` Marek Szyprowski
@ 2018-06-13 12:24     ` Matthew Wilcox
  -1 siblings, 0 replies; 22+ messages in thread
From: Matthew Wilcox @ 2018-06-13 12:24 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-mm, linux-kernel, linux-arm-kernel, Andrew Morton,
	Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka

On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
> cma_alloc() function has gfp mask parameter, so users expect that it
> honors typical memory allocation related flags. The most imporant from
> the security point of view is handling of __GFP_ZERO flag, because memory
> allocated by this function usually can be directly remapped to userspace
> by device drivers as a part of multimedia processing and ignoring this
> flag might lead to leaking some kernel structures to userspace.
> Some callers of this function (for example arm64 dma-iommu glue code)
> already assumed that the allocated buffers are cleared when this flag
> is set. To avoid such issues, add simple code for clearing newly
> allocated buffer when __GFP_ZERO flag is set. Callers will be then
> updated to skip implicit clearing or adjust passed gfp flags.

I think the documentation for this function needs improving.  For example,
GFP_ATOMIC does not work (it takes a mutex lock, so it can sleep).
At the very least, the kernel-doc needs:

 * Context: Process context (may sleep even if GFP flags indicate otherwise).

Unless someone wants to rework this allocator to use spinlocks instead
of mutexes ...

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

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
@ 2018-06-13 12:24     ` Matthew Wilcox
  0 siblings, 0 replies; 22+ messages in thread
From: Matthew Wilcox @ 2018-06-13 12:24 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
> cma_alloc() function has gfp mask parameter, so users expect that it
> honors typical memory allocation related flags. The most imporant from
> the security point of view is handling of __GFP_ZERO flag, because memory
> allocated by this function usually can be directly remapped to userspace
> by device drivers as a part of multimedia processing and ignoring this
> flag might lead to leaking some kernel structures to userspace.
> Some callers of this function (for example arm64 dma-iommu glue code)
> already assumed that the allocated buffers are cleared when this flag
> is set. To avoid such issues, add simple code for clearing newly
> allocated buffer when __GFP_ZERO flag is set. Callers will be then
> updated to skip implicit clearing or adjust passed gfp flags.

I think the documentation for this function needs improving.  For example,
GFP_ATOMIC does not work (it takes a mutex lock, so it can sleep).
At the very least, the kernel-doc needs:

 * Context: Process context (may sleep even if GFP flags indicate otherwise).

Unless someone wants to rework this allocator to use spinlocks instead
of mutexes ...

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

* Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
  2018-06-13 12:24     ` Matthew Wilcox
@ 2018-06-13 12:40       ` Marek Szyprowski
  -1 siblings, 0 replies; 22+ messages in thread
From: Marek Szyprowski @ 2018-06-13 12:40 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: linux-mm, linux-kernel, linux-arm-kernel, Andrew Morton,
	Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka

Hi Matthew,

On 2018-06-13 14:24, Matthew Wilcox wrote:
> On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
>> cma_alloc() function has gfp mask parameter, so users expect that it
>> honors typical memory allocation related flags. The most imporant from
>> the security point of view is handling of __GFP_ZERO flag, because memory
>> allocated by this function usually can be directly remapped to userspace
>> by device drivers as a part of multimedia processing and ignoring this
>> flag might lead to leaking some kernel structures to userspace.
>> Some callers of this function (for example arm64 dma-iommu glue code)
>> already assumed that the allocated buffers are cleared when this flag
>> is set. To avoid such issues, add simple code for clearing newly
>> allocated buffer when __GFP_ZERO flag is set. Callers will be then
>> updated to skip implicit clearing or adjust passed gfp flags.
> I think the documentation for this function needs improving.  For example,
> GFP_ATOMIC does not work (it takes a mutex lock, so it can sleep).
> At the very least, the kernel-doc needs:
>
>   * Context: Process context (may sleep even if GFP flags indicate otherwise).
>
> Unless someone wants to rework this allocator to use spinlocks instead
> of mutexes ...

It is not only the matter of the spinlocks. GFP_ATOMIC is not supported 
by the
memory compaction code, which is used in alloc_contig_range(). Right, this
should be also noted in the documentation.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


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

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
@ 2018-06-13 12:40       ` Marek Szyprowski
  0 siblings, 0 replies; 22+ messages in thread
From: Marek Szyprowski @ 2018-06-13 12:40 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Matthew,

On 2018-06-13 14:24, Matthew Wilcox wrote:
> On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
>> cma_alloc() function has gfp mask parameter, so users expect that it
>> honors typical memory allocation related flags. The most imporant from
>> the security point of view is handling of __GFP_ZERO flag, because memory
>> allocated by this function usually can be directly remapped to userspace
>> by device drivers as a part of multimedia processing and ignoring this
>> flag might lead to leaking some kernel structures to userspace.
>> Some callers of this function (for example arm64 dma-iommu glue code)
>> already assumed that the allocated buffers are cleared when this flag
>> is set. To avoid such issues, add simple code for clearing newly
>> allocated buffer when __GFP_ZERO flag is set. Callers will be then
>> updated to skip implicit clearing or adjust passed gfp flags.
> I think the documentation for this function needs improving.  For example,
> GFP_ATOMIC does not work (it takes a mutex lock, so it can sleep).
> At the very least, the kernel-doc needs:
>
>   * Context: Process context (may sleep even if GFP flags indicate otherwise).
>
> Unless someone wants to rework this allocator to use spinlocks instead
> of mutexes ...

It is not only the matter of the spinlocks. GFP_ATOMIC is not supported 
by the
memory compaction code, which is used in alloc_contig_range(). Right, this
should be also noted in the documentation.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

* Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
  2018-06-13  8:58   ` Marek Szyprowski
@ 2018-06-13 12:52     ` Christoph Hellwig
  -1 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2018-06-13 12:52 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-mm, linux-kernel, linux-arm-kernel, Andrew Morton,
	Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka

On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
> cma_alloc() function has gfp mask parameter, so users expect that it
> honors typical memory allocation related flags. The most imporant from
> the security point of view is handling of __GFP_ZERO flag, because memory
> allocated by this function usually can be directly remapped to userspace
> by device drivers as a part of multimedia processing and ignoring this
> flag might lead to leaking some kernel structures to userspace.
> Some callers of this function (for example arm64 dma-iommu glue code)
> already assumed that the allocated buffers are cleared when this flag
> is set. To avoid such issues, add simple code for clearing newly
> allocated buffer when __GFP_ZERO flag is set. Callers will be then
> updated to skip implicit clearing or adjust passed gfp flags.

dma mapping implementations need to zero all memory returned anyway
(even if a few implementation don't do that yet).

I'd rather keep the zeroing in the common callers.

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

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
@ 2018-06-13 12:52     ` Christoph Hellwig
  0 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2018-06-13 12:52 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
> cma_alloc() function has gfp mask parameter, so users expect that it
> honors typical memory allocation related flags. The most imporant from
> the security point of view is handling of __GFP_ZERO flag, because memory
> allocated by this function usually can be directly remapped to userspace
> by device drivers as a part of multimedia processing and ignoring this
> flag might lead to leaking some kernel structures to userspace.
> Some callers of this function (for example arm64 dma-iommu glue code)
> already assumed that the allocated buffers are cleared when this flag
> is set. To avoid such issues, add simple code for clearing newly
> allocated buffer when __GFP_ZERO flag is set. Callers will be then
> updated to skip implicit clearing or adjust passed gfp flags.

dma mapping implementations need to zero all memory returned anyway
(even if a few implementation don't do that yet).

I'd rather keep the zeroing in the common callers.

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

* Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
  2018-06-13 12:40       ` Marek Szyprowski
@ 2018-06-13 12:55         ` Christoph Hellwig
  -1 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2018-06-13 12:55 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Matthew Wilcox, linux-mm, linux-kernel, linux-arm-kernel,
	Andrew Morton, Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka

On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:
> It is not only the matter of the spinlocks. GFP_ATOMIC is not supported 
> by the
> memory compaction code, which is used in alloc_contig_range(). Right, this
> should be also noted in the documentation.

Documentation is good, asserts are better.  The code should reject any
flag not explicitly supported, or even better have its own flags type
with the few actually supported flags.

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

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
@ 2018-06-13 12:55         ` Christoph Hellwig
  0 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2018-06-13 12:55 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:
> It is not only the matter of the spinlocks. GFP_ATOMIC is not supported 
> by the
> memory compaction code, which is used in alloc_contig_range(). Right, this
> should be also noted in the documentation.

Documentation is good, asserts are better.  The code should reject any
flag not explicitly supported, or even better have its own flags type
with the few actually supported flags.

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

* Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
  2018-06-13 12:55         ` Christoph Hellwig
@ 2018-06-13 13:39           ` Michal Hocko
  -1 siblings, 0 replies; 22+ messages in thread
From: Michal Hocko @ 2018-06-13 13:39 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Marek Szyprowski, Matthew Wilcox, linux-mm, linux-kernel,
	linux-arm-kernel, Andrew Morton, Michal Nazarewicz, Joonsoo Kim,
	Vlastimil Babka

On Wed 13-06-18 05:55:46, Christoph Hellwig wrote:
> On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:
> > It is not only the matter of the spinlocks. GFP_ATOMIC is not supported 
> > by the
> > memory compaction code, which is used in alloc_contig_range(). Right, this
> > should be also noted in the documentation.
> 
> Documentation is good, asserts are better.  The code should reject any
> flag not explicitly supported, or even better have its own flags type
> with the few actually supported flags.

Agreed. Is the cma allocator used for anything other than GFP_KERNEL
btw.? If not then, shouldn't we simply drop the gfp argument altogether
rather than give users a false hope for differen gfp modes that are not
really supported and grow broken code?

-- 
Michal Hocko
SUSE Labs

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

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
@ 2018-06-13 13:39           ` Michal Hocko
  0 siblings, 0 replies; 22+ messages in thread
From: Michal Hocko @ 2018-06-13 13:39 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed 13-06-18 05:55:46, Christoph Hellwig wrote:
> On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:
> > It is not only the matter of the spinlocks. GFP_ATOMIC is not supported 
> > by the
> > memory compaction code, which is used in alloc_contig_range(). Right, this
> > should be also noted in the documentation.
> 
> Documentation is good, asserts are better.  The code should reject any
> flag not explicitly supported, or even better have its own flags type
> with the few actually supported flags.

Agreed. Is the cma allocator used for anything other than GFP_KERNEL
btw.? If not then, shouldn't we simply drop the gfp argument altogether
rather than give users a false hope for differen gfp modes that are not
really supported and grow broken code?

-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
  2018-06-13 13:39           ` Michal Hocko
@ 2018-07-02 13:23             ` Marek Szyprowski
  -1 siblings, 0 replies; 22+ messages in thread
From: Marek Szyprowski @ 2018-07-02 13:23 UTC (permalink / raw)
  To: Michal Hocko, Christoph Hellwig
  Cc: Matthew Wilcox, linux-mm, linux-kernel, linux-arm-kernel,
	Andrew Morton, Michal Nazarewicz, Joonsoo Kim, Vlastimil Babka

Hi Michal,

On 2018-06-13 15:39, Michal Hocko wrote:
> On Wed 13-06-18 05:55:46, Christoph Hellwig wrote:
>> On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:
>>> It is not only the matter of the spinlocks. GFP_ATOMIC is not supported
>>> by the
>>> memory compaction code, which is used in alloc_contig_range(). Right, this
>>> should be also noted in the documentation.
>> Documentation is good, asserts are better.  The code should reject any
>> flag not explicitly supported, or even better have its own flags type
>> with the few actually supported flags.
> Agreed. Is the cma allocator used for anything other than GFP_KERNEL
> btw.? If not then, shouldn't we simply drop the gfp argument altogether
> rather than give users a false hope for differen gfp modes that are not
> really supported and grow broken code?

Nope, all cma_alloc() callers are expected to use it with GFP_KERNEL gfp 
mask.
The only flag which is now checked is __GFP_NOWARN. I can change the 
function
signature of cma_alloc to:
struct page *cma_alloc(struct cma *cma, size_t count, unsigned int 
align, bool no_warn);

What about clearing the allocated buffer? Should it be another bool 
parameter,
done unconditionally or moved to the callers?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


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

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
@ 2018-07-02 13:23             ` Marek Szyprowski
  0 siblings, 0 replies; 22+ messages in thread
From: Marek Szyprowski @ 2018-07-02 13:23 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Michal,

On 2018-06-13 15:39, Michal Hocko wrote:
> On Wed 13-06-18 05:55:46, Christoph Hellwig wrote:
>> On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:
>>> It is not only the matter of the spinlocks. GFP_ATOMIC is not supported
>>> by the
>>> memory compaction code, which is used in alloc_contig_range(). Right, this
>>> should be also noted in the documentation.
>> Documentation is good, asserts are better.  The code should reject any
>> flag not explicitly supported, or even better have its own flags type
>> with the few actually supported flags.
> Agreed. Is the cma allocator used for anything other than GFP_KERNEL
> btw.? If not then, shouldn't we simply drop the gfp argument altogether
> rather than give users a false hope for differen gfp modes that are not
> really supported and grow broken code?

Nope, all cma_alloc() callers are expected to use it with GFP_KERNEL gfp 
mask.
The only flag which is now checked is __GFP_NOWARN. I can change the 
function
signature of cma_alloc to:
struct page *cma_alloc(struct cma *cma, size_t count, unsigned int 
align, bool no_warn);

What about clearing the allocated buffer? Should it be another bool 
parameter,
done unconditionally or moved to the callers?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

* Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
  2018-07-02 13:23             ` Marek Szyprowski
@ 2018-07-02 13:30               ` Christoph Hellwig
  -1 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2018-07-02 13:30 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Michal Hocko, Christoph Hellwig, Matthew Wilcox, linux-mm,
	linux-kernel, linux-arm-kernel, Andrew Morton, Michal Nazarewicz,
	Joonsoo Kim, Vlastimil Babka

On Mon, Jul 02, 2018 at 03:23:34PM +0200, Marek Szyprowski wrote:
> What about clearing the allocated buffer? Should it be another bool 
> parameter,
> done unconditionally or moved to the callers?

Please keep it in the callers.  I plan to push it up even higher
from the current callers short to midterm.

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

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
@ 2018-07-02 13:30               ` Christoph Hellwig
  0 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2018-07-02 13:30 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 02, 2018 at 03:23:34PM +0200, Marek Szyprowski wrote:
> What about clearing the allocated buffer? Should it be another bool 
> parameter,
> done unconditionally or moved to the callers?

Please keep it in the callers.  I plan to push it up even higher
from the current callers short to midterm.

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

* Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
  2018-07-02 13:23             ` Marek Szyprowski
@ 2018-07-02 13:32               ` Michal Hocko
  -1 siblings, 0 replies; 22+ messages in thread
From: Michal Hocko @ 2018-07-02 13:32 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Christoph Hellwig, Matthew Wilcox, linux-mm, linux-kernel,
	linux-arm-kernel, Andrew Morton, Michal Nazarewicz, Joonsoo Kim,
	Vlastimil Babka

On Mon 02-07-18 15:23:34, Marek Szyprowski wrote:
> Hi Michal,
> 
> On 2018-06-13 15:39, Michal Hocko wrote:
> > On Wed 13-06-18 05:55:46, Christoph Hellwig wrote:
> >> On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:
> >>> It is not only the matter of the spinlocks. GFP_ATOMIC is not supported
> >>> by the
> >>> memory compaction code, which is used in alloc_contig_range(). Right, this
> >>> should be also noted in the documentation.
> >> Documentation is good, asserts are better.  The code should reject any
> >> flag not explicitly supported, or even better have its own flags type
> >> with the few actually supported flags.
> > Agreed. Is the cma allocator used for anything other than GFP_KERNEL
> > btw.? If not then, shouldn't we simply drop the gfp argument altogether
> > rather than give users a false hope for differen gfp modes that are not
> > really supported and grow broken code?
> 
> Nope, all cma_alloc() callers are expected to use it with GFP_KERNEL gfp 
> mask.
> The only flag which is now checked is __GFP_NOWARN. I can change the 
> function
> signature of cma_alloc to:
> struct page *cma_alloc(struct cma *cma, size_t count, unsigned int 
> align, bool no_warn);

Are there any __GFP_NOWARN users? I have quickly hit the indirection
trap and searching for alloc callback didn't tell me really much.

> What about clearing the allocated buffer? Should it be another bool
> parameter, done unconditionally or moved to the callers?

That really depends on callers. I have no idea what they actually ask
for.
-- 
Michal Hocko
SUSE Labs

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

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
@ 2018-07-02 13:32               ` Michal Hocko
  0 siblings, 0 replies; 22+ messages in thread
From: Michal Hocko @ 2018-07-02 13:32 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon 02-07-18 15:23:34, Marek Szyprowski wrote:
> Hi Michal,
> 
> On 2018-06-13 15:39, Michal Hocko wrote:
> > On Wed 13-06-18 05:55:46, Christoph Hellwig wrote:
> >> On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:
> >>> It is not only the matter of the spinlocks. GFP_ATOMIC is not supported
> >>> by the
> >>> memory compaction code, which is used in alloc_contig_range(). Right, this
> >>> should be also noted in the documentation.
> >> Documentation is good, asserts are better.  The code should reject any
> >> flag not explicitly supported, or even better have its own flags type
> >> with the few actually supported flags.
> > Agreed. Is the cma allocator used for anything other than GFP_KERNEL
> > btw.? If not then, shouldn't we simply drop the gfp argument altogether
> > rather than give users a false hope for differen gfp modes that are not
> > really supported and grow broken code?
> 
> Nope, all cma_alloc() callers are expected to use it with GFP_KERNEL gfp 
> mask.
> The only flag which is now checked is __GFP_NOWARN. I can change the 
> function
> signature of cma_alloc to:
> struct page *cma_alloc(struct cma *cma, size_t count, unsigned int 
> align, bool no_warn);

Are there any __GFP_NOWARN users? I have quickly hit the indirection
trap and searching for alloc callback didn't tell me really much.

> What about clearing the allocated buffer? Should it be another bool
> parameter, done unconditionally or moved to the callers?

That really depends on callers. I have no idea what they actually ask
for.
-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
  2018-07-02 13:32               ` Michal Hocko
@ 2018-07-09 10:31                 ` Marek Szyprowski
  -1 siblings, 0 replies; 22+ messages in thread
From: Marek Szyprowski @ 2018-07-09 10:31 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Christoph Hellwig, Matthew Wilcox, linux-mm, linux-kernel,
	linux-arm-kernel, Andrew Morton, Michal Nazarewicz, Joonsoo Kim,
	Vlastimil Babka

Hi Michal,

On 2018-07-02 15:32, Michal Hocko wrote:
> On Mon 02-07-18 15:23:34, Marek Szyprowski wrote:
>> On 2018-06-13 15:39, Michal Hocko wrote:
>>> On Wed 13-06-18 05:55:46, Christoph Hellwig wrote:
>>>> On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:
>>>>> It is not only the matter of the spinlocks. GFP_ATOMIC is not supported
>>>>> by the
>>>>> memory compaction code, which is used in alloc_contig_range(). Right, this
>>>>> should be also noted in the documentation.
>>>> Documentation is good, asserts are better.  The code should reject any
>>>> flag not explicitly supported, or even better have its own flags type
>>>> with the few actually supported flags.
>>> Agreed. Is the cma allocator used for anything other than GFP_KERNEL
>>> btw.? If not then, shouldn't we simply drop the gfp argument altogether
>>> rather than give users a false hope for differen gfp modes that are not
>>> really supported and grow broken code?
>> Nope, all cma_alloc() callers are expected to use it with GFP_KERNEL gfp
>> mask.
>> The only flag which is now checked is __GFP_NOWARN. I can change the
>> function
>> signature of cma_alloc to:
>> struct page *cma_alloc(struct cma *cma, size_t count, unsigned int
>> align, bool no_warn);
> Are there any __GFP_NOWARN users? I have quickly hit the indirection
> trap and searching for alloc callback didn't tell me really much.

They might be via dma_alloc_from_contiguous() and dma_alloc_*() path.

>> What about clearing the allocated buffer? Should it be another bool
>> parameter, done unconditionally or moved to the callers?
> That really depends on callers. I have no idea what they actually ask
> for.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


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

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
@ 2018-07-09 10:31                 ` Marek Szyprowski
  0 siblings, 0 replies; 22+ messages in thread
From: Marek Szyprowski @ 2018-07-09 10:31 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Michal,

On 2018-07-02 15:32, Michal Hocko wrote:
> On Mon 02-07-18 15:23:34, Marek Szyprowski wrote:
>> On 2018-06-13 15:39, Michal Hocko wrote:
>>> On Wed 13-06-18 05:55:46, Christoph Hellwig wrote:
>>>> On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:
>>>>> It is not only the matter of the spinlocks. GFP_ATOMIC is not supported
>>>>> by the
>>>>> memory compaction code, which is used in alloc_contig_range(). Right, this
>>>>> should be also noted in the documentation.
>>>> Documentation is good, asserts are better.  The code should reject any
>>>> flag not explicitly supported, or even better have its own flags type
>>>> with the few actually supported flags.
>>> Agreed. Is the cma allocator used for anything other than GFP_KERNEL
>>> btw.? If not then, shouldn't we simply drop the gfp argument altogether
>>> rather than give users a false hope for differen gfp modes that are not
>>> really supported and grow broken code?
>> Nope, all cma_alloc() callers are expected to use it with GFP_KERNEL gfp
>> mask.
>> The only flag which is now checked is __GFP_NOWARN. I can change the
>> function
>> signature of cma_alloc to:
>> struct page *cma_alloc(struct cma *cma, size_t count, unsigned int
>> align, bool no_warn);
> Are there any __GFP_NOWARN users? I have quickly hit the indirection
> trap and searching for alloc callback didn't tell me really much.

They might be via dma_alloc_from_contiguous() and dma_alloc_*() path.

>> What about clearing the allocated buffer? Should it be another bool
>> parameter, done unconditionally or moved to the callers?
> That really depends on callers. I have no idea what they actually ask
> for.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

* Re: [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
  2018-07-02 13:30               ` Christoph Hellwig
@ 2018-07-09 10:32                 ` Marek Szyprowski
  -1 siblings, 0 replies; 22+ messages in thread
From: Marek Szyprowski @ 2018-07-09 10:32 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Michal Hocko, Matthew Wilcox, linux-mm, linux-kernel,
	linux-arm-kernel, Andrew Morton, Michal Nazarewicz, Joonsoo Kim,
	Vlastimil Babka

Hi Christoph,

On 2018-07-02 15:30, Christoph Hellwig wrote:
> On Mon, Jul 02, 2018 at 03:23:34PM +0200, Marek Szyprowski wrote:
>> What about clearing the allocated buffer? Should it be another bool
>> parameter,
>> done unconditionally or moved to the callers?
> Please keep it in the callers.  I plan to push it up even higher
> from the current callers short to midterm.

Okay, I will post a patch with this approach then.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


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

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
@ 2018-07-09 10:32                 ` Marek Szyprowski
  0 siblings, 0 replies; 22+ messages in thread
From: Marek Szyprowski @ 2018-07-09 10:32 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Christoph,

On 2018-07-02 15:30, Christoph Hellwig wrote:
> On Mon, Jul 02, 2018 at 03:23:34PM +0200, Marek Szyprowski wrote:
>> What about clearing the allocated buffer? Should it be another bool
>> parameter,
>> done unconditionally or moved to the callers?
> Please keep it in the callers.  I plan to push it up even higher
> from the current callers short to midterm.

Okay, I will post a patch with this approach then.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

end of thread, other threads:[~2018-07-09 10:33 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20180613085851eucas1p20337d050face8ff8ea87674e16a9ccd2@eucas1p2.samsung.com>
2018-06-13  8:58 ` [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc() Marek Szyprowski
2018-06-13  8:58   ` Marek Szyprowski
2018-06-13 12:24   ` Matthew Wilcox
2018-06-13 12:24     ` Matthew Wilcox
2018-06-13 12:40     ` Marek Szyprowski
2018-06-13 12:40       ` Marek Szyprowski
2018-06-13 12:55       ` Christoph Hellwig
2018-06-13 12:55         ` Christoph Hellwig
2018-06-13 13:39         ` Michal Hocko
2018-06-13 13:39           ` Michal Hocko
2018-07-02 13:23           ` Marek Szyprowski
2018-07-02 13:23             ` Marek Szyprowski
2018-07-02 13:30             ` Christoph Hellwig
2018-07-02 13:30               ` Christoph Hellwig
2018-07-09 10:32               ` Marek Szyprowski
2018-07-09 10:32                 ` Marek Szyprowski
2018-07-02 13:32             ` Michal Hocko
2018-07-02 13:32               ` Michal Hocko
2018-07-09 10:31               ` Marek Szyprowski
2018-07-09 10:31                 ` Marek Szyprowski
2018-06-13 12:52   ` Christoph Hellwig
2018-06-13 12:52     ` Christoph Hellwig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.