[PATCH] ARM: DMA: Fix kzalloc flags in __iommu_alloc_buffer()

[PATCH] ARM: DMA: Fix kzalloc flags in __iommu_alloc_buffer()

[Alexandre Courbot]

There doesn't seem to be any valid reason to allocate the pages array
with the same flags as the buffer itself. Doing so can eventually lead
to the following safeguard in mm/slab.c to be hit:

BUG_ON(flags & GFP_SLAB_BUG_MASK);

This happens when buffers are allocated with __GFP_DMA32 or
__GFP_HIGHMEM.

Fix this by allocating the pages array with GFP_KERNEL to follow what is
done elsewhere in this file. Using GFP_KERNEL in __iommu_alloc_buffer()
is safe because atomic allocations are handled by __iommu_alloc_atomic().

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.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 e8907117861e..bc495354c802 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1106,7 +1106,7 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
 	int i = 0;
 
 	if (array_size <= PAGE_SIZE)
-		pages = kzalloc(array_size, gfp);
+		pages = kzalloc(array_size, GFP_KERNEL);
 	else
 		pages = vzalloc(array_size);
 	if (!pages)
-- 
2.1.3

[PATCH] ARM: DMA: Fix kzalloc flags in __iommu_alloc_buffer()

[Arnd Bergmann]

On Monday 08 December 2014 17:39:27 Alexandre Courbot wrote:
> There doesn't seem to be any valid reason to allocate the pages array
> with the same flags as the buffer itself. Doing so can eventually lead
> to the following safeguard in mm/slab.c to be hit:
> 
> BUG_ON(flags & GFP_SLAB_BUG_MASK);
> 
> This happens when buffers are allocated with __GFP_DMA32 or
> __GFP_HIGHMEM.
> 
> Fix this by allocating the pages array with GFP_KERNEL to follow what is
> done elsewhere in this file. Using GFP_KERNEL in __iommu_alloc_buffer()
> is safe because atomic allocations are handled by __iommu_alloc_atomic().
> 

I think you need to carry over the GFP_ATOMIC flag if that is set by the
caller, but not the GFP_HIGHMEM or GFP_DMA32. Not sure if it's better
to mask out flags from the caller mask, or to start with GFP_KERNEL
and adding in extra bits.

	Arnd

[PATCH] ARM: DMA: Fix kzalloc flags in __iommu_alloc_buffer()

[Alexandre Courbot]

On Mon, Dec 8, 2014 at 7:24 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 08 December 2014 17:39:27 Alexandre Courbot wrote:
>> There doesn't seem to be any valid reason to allocate the pages array
>> with the same flags as the buffer itself. Doing so can eventually lead
>> to the following safeguard in mm/slab.c to be hit:
>>
>> BUG_ON(flags & GFP_SLAB_BUG_MASK);
>>
>> This happens when buffers are allocated with __GFP_DMA32 or
>> __GFP_HIGHMEM.
>>
>> Fix this by allocating the pages array with GFP_KERNEL to follow what is
>> done elsewhere in this file. Using GFP_KERNEL in __iommu_alloc_buffer()
>> is safe because atomic allocations are handled by __iommu_alloc_atomic().
>>
>
> I think you need to carry over the GFP_ATOMIC flag if that is set by the
> caller, but not the GFP_HIGHMEM or GFP_DMA32. Not sure if it's better
> to mask out flags from the caller mask, or to start with GFP_KERNEL
> and adding in extra bits.

I thought the issue of atomicity is already handled by
__iommu_alloc_buffer's caller (arm_iommu_alloc_attrs):

    if (!(gfp & __GFP_WAIT))
        return __iommu_alloc_atomic(dev, size, handle);
    ....
    pages = __iommu_alloc_buffer(dev, size, gfp, attrs);

Isn't the interesting property about GFP_ATOMIC that it does not
include __GFP_WAIT? I may very well misunderstand the issue, sorry if
that's the case.

[PATCH] ARM: DMA: Fix kzalloc flags in __iommu_alloc_buffer()

[Arnd Bergmann]

On Tuesday 09 December 2014 11:57:22 Alexandre Courbot wrote:
> On Mon, Dec 8, 2014 at 7:24 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Monday 08 December 2014 17:39:27 Alexandre Courbot wrote:
> >> There doesn't seem to be any valid reason to allocate the pages array
> >> with the same flags as the buffer itself. Doing so can eventually lead
> >> to the following safeguard in mm/slab.c to be hit:
> >>
> >> BUG_ON(flags & GFP_SLAB_BUG_MASK);
> >>
> >> This happens when buffers are allocated with __GFP_DMA32 or
> >> __GFP_HIGHMEM.
> >>
> >> Fix this by allocating the pages array with GFP_KERNEL to follow what is
> >> done elsewhere in this file. Using GFP_KERNEL in __iommu_alloc_buffer()
> >> is safe because atomic allocations are handled by __iommu_alloc_atomic().
> >>
> >
> > I think you need to carry over the GFP_ATOMIC flag if that is set by the
> > caller, but not the GFP_HIGHMEM or GFP_DMA32. Not sure if it's better
> > to mask out flags from the caller mask, or to start with GFP_KERNEL
> > and adding in extra bits.
> 
> I thought the issue of atomicity is already handled by
> __iommu_alloc_buffer's caller (arm_iommu_alloc_attrs):
> 
>     if (!(gfp & __GFP_WAIT))
>         return __iommu_alloc_atomic(dev, size, handle);
>     ....
>     pages = __iommu_alloc_buffer(dev, size, gfp, attrs);
> 
> Isn't the interesting property about GFP_ATOMIC that it does not
> include __GFP_WAIT? I may very well misunderstand the issue, sorry if
> that's the case.

No, I think you are right, I wasn't looking at the whole call chain,
just at your patch, and it looks good to me now.

	Arnd

[PATCH] ARM: DMA: Fix kzalloc flags in __iommu_alloc_buffer()

[Marek Szyprowski]

On 2014-12-08 09:39, Alexandre Courbot wrote:
> There doesn't seem to be any valid reason to allocate the pages array
> with the same flags as the buffer itself. Doing so can eventually lead
> to the following safeguard in mm/slab.c to be hit:
>
> BUG_ON(flags & GFP_SLAB_BUG_MASK);
>
> This happens when buffers are allocated with __GFP_DMA32 or
> __GFP_HIGHMEM.
>
> Fix this by allocating the pages array with GFP_KERNEL to follow what is
> done elsewhere in this file. Using GFP_KERNEL in __iommu_alloc_buffer()
> is safe because atomic allocations are handled by __iommu_alloc_atomic().
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Marek Szyprowski <m.szyprowski@samsung.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 e8907117861e..bc495354c802 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1106,7 +1106,7 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
>   	int i = 0;
>   
>   	if (array_size <= PAGE_SIZE)
> -		pages = kzalloc(array_size, gfp);
> +		pages = kzalloc(array_size, GFP_KERNEL);
>   	else
>   		pages = vzalloc(array_size);
>   	if (!pages)

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

[PATCH] ARM: DMA: Fix kzalloc flags in __iommu_alloc_buffer()

[Alexandre Courbot]

Ping? This patch still seems to be needed as of today...

On Thu, Dec 11, 2014 at 8:12 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 2014-12-08 09:39, Alexandre Courbot wrote:
>>
>> There doesn't seem to be any valid reason to allocate the pages array
>> with the same flags as the buffer itself. Doing so can eventually lead
>> to the following safeguard in mm/slab.c to be hit:
>>
>> BUG_ON(flags & GFP_SLAB_BUG_MASK);
>>
>> This happens when buffers are allocated with __GFP_DMA32 or
>> __GFP_HIGHMEM.
>>
>> Fix this by allocating the pages array with GFP_KERNEL to follow what is
>> done elsewhere in this file. Using GFP_KERNEL in __iommu_alloc_buffer()
>> is safe because atomic allocations are handled by __iommu_alloc_atomic().
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Thierry Reding <treding@nvidia.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
>
> Acked-by: Marek Szyprowski <m.szyprowski@samsung.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 e8907117861e..bc495354c802 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -1106,7 +1106,7 @@ static struct page **__iommu_alloc_buffer(struct
>> device *dev, size_t size,
>>         int i = 0;
>>         if (array_size <= PAGE_SIZE)
>> -               pages = kzalloc(array_size, gfp);
>> +               pages = kzalloc(array_size, GFP_KERNEL);
>>         else
>>                 pages = vzalloc(array_size);
>>         if (!pages)
>
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

[PATCH] ARM: DMA: Fix kzalloc flags in __iommu_alloc_buffer()

[Marek Szyprowski]

Hello,

On 2015-01-13 09:45, Alexandre Courbot wrote:
> Ping? This patch still seems to be needed as of today...

Arnd, could you take this patch together with your other pending 
dma-mapping.h changes?

> On Thu, Dec 11, 2014 at 8:12 PM, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 2014-12-08 09:39, Alexandre Courbot wrote:
>>> There doesn't seem to be any valid reason to allocate the pages array
>>> with the same flags as the buffer itself. Doing so can eventually lead
>>> to the following safeguard in mm/slab.c to be hit:
>>>
>>> BUG_ON(flags & GFP_SLAB_BUG_MASK);
>>>
>>> This happens when buffers are allocated with __GFP_DMA32 or
>>> __GFP_HIGHMEM.
>>>
>>> Fix this by allocating the pages array with GFP_KERNEL to follow what is
>>> done elsewhere in this file. Using GFP_KERNEL in __iommu_alloc_buffer()
>>> is safe because atomic allocations are handled by __iommu_alloc_atomic().
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>> Cc: Russell King <linux@arm.linux.org.uk>
>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Thierry Reding <treding@nvidia.com>
>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Acked-by: Marek Szyprowski <m.szyprowski@samsung.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 e8907117861e..bc495354c802 100644
>>> --- a/arch/arm/mm/dma-mapping.c
>>> +++ b/arch/arm/mm/dma-mapping.c
>>> @@ -1106,7 +1106,7 @@ static struct page **__iommu_alloc_buffer(struct
>>> device *dev, size_t size,
>>>          int i = 0;
>>>          if (array_size <= PAGE_SIZE)
>>> -               pages = kzalloc(array_size, gfp);
>>> +               pages = kzalloc(array_size, GFP_KERNEL);
>>>          else
>>>                  pages = vzalloc(array_size);
>>>          if (!pages)
>>>

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

[PATCH] ARM: DMA: Fix kzalloc flags in __iommu_alloc_buffer()

[Alexandre Courbot]

On Tue, Jan 13, 2015 at 8:20 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hello,
>
> On 2015-01-13 09:45, Alexandre Courbot wrote:
>>
>> Ping? This patch still seems to be needed as of today...
>
>
> Arnd, could you take this patch together with your other pending
> dma-mapping.h changes?

Arnd, gentle ping on this?

>
>
>> On Thu, Dec 11, 2014 at 8:12 PM, Marek Szyprowski
>> <m.szyprowski@samsung.com> wrote:
>>>
>>> On 2014-12-08 09:39, Alexandre Courbot wrote:
>>>>
>>>> There doesn't seem to be any valid reason to allocate the pages array
>>>> with the same flags as the buffer itself. Doing so can eventually lead
>>>> to the following safeguard in mm/slab.c to be hit:
>>>>
>>>> BUG_ON(flags & GFP_SLAB_BUG_MASK);
>>>>
>>>> This happens when buffers are allocated with __GFP_DMA32 or
>>>> __GFP_HIGHMEM.
>>>>
>>>> Fix this by allocating the pages array with GFP_KERNEL to follow what is
>>>> done elsewhere in this file. Using GFP_KERNEL in __iommu_alloc_buffer()
>>>> is safe because atomic allocations are handled by
>>>> __iommu_alloc_atomic().
>>>>
>>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>>> Cc: Russell King <linux@arm.linux.org.uk>
>>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>> Cc: Thierry Reding <treding@nvidia.com>
>>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>
>>> Acked-by: Marek Szyprowski <m.szyprowski@samsung.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 e8907117861e..bc495354c802 100644
>>>> --- a/arch/arm/mm/dma-mapping.c
>>>> +++ b/arch/arm/mm/dma-mapping.c
>>>> @@ -1106,7 +1106,7 @@ static struct page **__iommu_alloc_buffer(struct
>>>> device *dev, size_t size,
>>>>          int i = 0;
>>>>          if (array_size <= PAGE_SIZE)
>>>> -               pages = kzalloc(array_size, gfp);
>>>> +               pages = kzalloc(array_size, GFP_KERNEL);
>>>>          else
>>>>                  pages = vzalloc(array_size);
>>>>          if (!pages)
>>>>
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>