[PATCH] mm/cma: Fix crash on CMA allocation if bitmap allocation fails

[PATCH] mm/cma: Fix crash on CMA allocation if bitmap allocation fails

[Yue Hu]

From: Yue Hu <huyue2@yulong.com>

A previous commit f022d8cb7ec7 ("mm: cma: Don't crash on allocation
if CMA area can't be activated") fixes the crash issue when activation
fails via setting cma->count as 0, same logic exists if bitmap
allocation fails.

Signed-off-by: Yue Hu <huyue2@yulong.com>
---
 mm/cma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/cma.c b/mm/cma.c
index f5bf819..991a6ce 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -106,8 +106,10 @@ static int __init cma_activate_area(struct cma *cma)
 
 	cma->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
 
-	if (!cma->bitmap)
+	if (!cma->bitmap) {
+		cma->count = 0;
 		return -ENOMEM;
+	}
 
 	WARN_ON_ONCE(!pfn_valid(pfn));
 	zone = page_zone(pfn_to_page(pfn));
-- 
1.9.1

Re: [PATCH] mm/cma: Fix crash on CMA allocation if bitmap allocation fails

[Anshuman Khandual]

On 03/25/2019 01:43 PM, Yue Hu wrote:
> From: Yue Hu <huyue2@yulong.com>
> 
> A previous commit f022d8cb7ec7 ("mm: cma: Don't crash on allocation
> if CMA area can't be activated") fixes the crash issue when activation
> fails via setting cma->count as 0, same logic exists if bitmap
> allocation fails.
> 
> Signed-off-by: Yue Hu <huyue2@yulong.com>

Looks good to me. Just wondering if cma->count =  0 should be wrapped around in
a helper which explicitly states that this cma cannot be used for allocation.

Nonetheless.

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Re: [PATCH] mm/cma: Fix crash on CMA allocation if bitmap allocation fails

[Andrew Morton]

On Mon, 25 Mar 2019 16:13:09 +0800 Yue Hu <zbestahu@gmail.com> wrote:

> From: Yue Hu <huyue2@yulong.com>
> 
> A previous commit f022d8cb7ec7 ("mm: cma: Don't crash on allocation
> if CMA area can't be activated") fixes the crash issue when activation
> fails via setting cma->count as 0, same logic exists if bitmap
> allocation fails.
> 
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -106,8 +106,10 @@ static int __init cma_activate_area(struct cma *cma)
>  
>  	cma->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
>  
> -	if (!cma->bitmap)
> +	if (!cma->bitmap) {
> +		cma->count = 0;
>  		return -ENOMEM;
> +	}
>  
>  	WARN_ON_ONCE(!pfn_valid(pfn));
>  	zone = page_zone(pfn_to_page(pfn));

I'm unsure whether this is needed.

kmalloc() within __init code is generally considered to be a "can't
fail".

If this was the only issue then I guess I'd take the patch if only for
documentation/clarity purposes.  But cma_areas[] is in bss and is
guaranteed to be all-zeroes, so I suspect this bug is a can't-happen. 
And we could revert f022d8cb7ec7 if we could be bothered (I can't).

Re: [PATCH] mm/cma: Fix crash on CMA allocation if bitmap allocation fails

[Yue Hu]

On Mon, 25 Mar 2019 15:15:41 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 25 Mar 2019 16:13:09 +0800 Yue Hu <zbestahu@gmail.com> wrote:
> 
> > From: Yue Hu <huyue2@yulong.com>
> > 
> > A previous commit f022d8cb7ec7 ("mm: cma: Don't crash on allocation
> > if CMA area can't be activated") fixes the crash issue when activation
> > fails via setting cma->count as 0, same logic exists if bitmap
> > allocation fails.
> > 
> > --- a/mm/cma.c
> > +++ b/mm/cma.c
> > @@ -106,8 +106,10 @@ static int __init cma_activate_area(struct cma *cma)
> >  
> >  	cma->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> >  
> > -	if (!cma->bitmap)
> > +	if (!cma->bitmap) {
> > +		cma->count = 0;
> >  		return -ENOMEM;
> > +	}
> >  
> >  	WARN_ON_ONCE(!pfn_valid(pfn));
> >  	zone = page_zone(pfn_to_page(pfn));  
> 
> I'm unsure whether this is needed.
> 
> kmalloc() within __init code is generally considered to be a "can't
> fail".
> 
> If this was the only issue then I guess I'd take the patch if only for
> documentation/clarity purposes.  But cma_areas[] is in bss and is
> guaranteed to be all-zeroes, so I suspect this bug is a can't-happen. 

However, firstly cma->count will be assigned to size >> PAGE_SHIFT in
cma_init_reserved_mem().

> And we could revert f022d8cb7ec7 if we could be bothered (I can't).
> 
>