[PATCH] mm/cma: cma_declare_contiguous: correct err handling

[PATCH] mm/cma: cma_declare_contiguous: correct err handling

[Peng Fan]

In case cma_init_reserved_mem failed, need to free the memblock allocated
by memblock_reserve or memblock_alloc_range.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V1:
 code inspection, I do not met failure in cma_init_reserved_mem.

 mm/cma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/cma.c b/mm/cma.c
index c7b39dd3b4f6..f4f3a8a57d86 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -353,12 +353,14 @@ int __init cma_declare_contiguous(phys_addr_t base,
 
 	ret = cma_init_reserved_mem(base, size, order_per_bit, name, res_cma);
 	if (ret)
-		goto err;
+		goto free_mem;
 
 	pr_info("Reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
 		&base);
 	return 0;
 
+free_mem:
+	memblock_free(base, size);
 err:
 	pr_err("Failed to reserve %ld MiB\n", (unsigned long)size / SZ_1M);
 	return ret;
-- 
2.16.4

Re: [PATCH] mm/cma: cma_declare_contiguous: correct err handling

[Andrew Morton]

On Thu, 14 Feb 2019 12:45:51 +0000 Peng Fan <peng.fan@nxp.com> wrote:

> In case cma_init_reserved_mem failed, need to free the memblock allocated
> by memblock_reserve or memblock_alloc_range.
> 
> ...
>
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -353,12 +353,14 @@ int __init cma_declare_contiguous(phys_addr_t base,
>  
>  	ret = cma_init_reserved_mem(base, size, order_per_bit, name, res_cma);
>  	if (ret)
> -		goto err;
> +		goto free_mem;
>  
>  	pr_info("Reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
>  		&base);
>  	return 0;
>  
> +free_mem:
> +	memblock_free(base, size);
>  err:
>  	pr_err("Failed to reserve %ld MiB\n", (unsigned long)size / SZ_1M);
>  	return ret;

This doesn't look right to me.  In the `fixed==true' case we didn't
actually allocate anything and in the `fixed==false' case, the
allocated memory is at `addr', not at `base'.

RE: [PATCH] mm/cma: cma_declare_contiguous: correct err handling

[Peng Fan]

Hi Andrew

> -----Original Message-----
> From: Andrew Morton [mailto:akpm@linux-foundation.org]
> Sent: 2019年2月15日 4:38
> To: Peng Fan <peng.fan@nxp.com>
> Cc: labbott@redhat.com; mhocko@suse.com; vbabka@suse.cz;
> iamjoonsoo.kim@lge.com; rppt@linux.vnet.ibm.com;
> m.szyprowski@samsung.com; rdunlap@infradead.org;
> andreyknvl@google.com; linux-mm@kvack.org; linux-kernel@vger.kernel.org;
> van.freenix@gmail.com; Mike Rapoport <rppt@linux.ibm.com>
> Subject: Re: [PATCH] mm/cma: cma_declare_contiguous: correct err handling
> 
> On Thu, 14 Feb 2019 12:45:51 +0000 Peng Fan <peng.fan@nxp.com> wrote:
> 
> > In case cma_init_reserved_mem failed, need to free the memblock
> > allocated by memblock_reserve or memblock_alloc_range.
> >
> > ...
> >
> > --- a/mm/cma.c
> > +++ b/mm/cma.c
> > @@ -353,12 +353,14 @@ int __init cma_declare_contiguous(phys_addr_t
> > base,
> >
> >  	ret = cma_init_reserved_mem(base, size, order_per_bit, name,
> res_cma);
> >  	if (ret)
> > -		goto err;
> > +		goto free_mem;
> >
> >  	pr_info("Reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
> >  		&base);
> >  	return 0;
> >
> > +free_mem:
> > +	memblock_free(base, size);
> >  err:
> >  	pr_err("Failed to reserve %ld MiB\n", (unsigned long)size / SZ_1M);
> >  	return ret;
> 
> This doesn't look right to me.  In the `fixed==true' case we didn't actually
> allocate anything and in the `fixed==false' case, the allocated memory is at
> `addr', not at `base'.

My code base is 5.0.0-rc6, in mm/cma.c
313         /* Reserve memory */
314         if (fixed) {
315                 if (memblock_is_region_reserved(base, size) ||
316                     memblock_reserve(base, size) < 0) {
317                         ret = -EBUSY;
318                         goto err;
319                 }
320         } else {

When fixed is true, memblock_is_region_reserved will check whether the [base, base + size)
is reserved, if reserved, return -EBUSY, if not reserved, it will call memblock_reserve,
if memblock_reserve fail, it will return -EBUSY.

When fixed is false, after memblock_alloc_range, there is one line code `base = addr;`.

Thanks,
Peng.

Re: [PATCH] mm/cma: cma_declare_contiguous: correct err handling

[Vlastimil Babka]

On 2/14/19 9:38 PM, Andrew Morton wrote:
> On Thu, 14 Feb 2019 12:45:51 +0000 Peng Fan <peng.fan@nxp.com> wrote:
> 
>> In case cma_init_reserved_mem failed, need to free the memblock allocated
>> by memblock_reserve or memblock_alloc_range.
>>
>> ...
>>
>> --- a/mm/cma.c
>> +++ b/mm/cma.c
>> @@ -353,12 +353,14 @@ int __init cma_declare_contiguous(phys_addr_t base,
>>  
>>  	ret = cma_init_reserved_mem(base, size, order_per_bit, name, res_cma);
>>  	if (ret)
>> -		goto err;
>> +		goto free_mem;
>>  
>>  	pr_info("Reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
>>  		&base);
>>  	return 0;
>>  
>> +free_mem:
>> +	memblock_free(base, size);
>>  err:
>>  	pr_err("Failed to reserve %ld MiB\n", (unsigned long)size / SZ_1M);
>>  	return ret;
> 
> This doesn't look right to me.  In the `fixed==true' case we didn't
> actually allocate anything and in the `fixed==false' case, the
> allocated memory is at `addr', not at `base'.

I think it's ok as the fixed==true path has "memblock_reserve()", but
better leave this to the memblock maintainer :)

There's also 'kmemleak_ignore_phys(addr)' which should probably be
undone (or not called at all) in the failure case. But it seems to be
missing from the fixed==true path?

Re: [PATCH] mm/cma: cma_declare_contiguous: correct err handling

[Mike Rapoport]

On Tue, Feb 19, 2019 at 05:55:33PM +0100, Vlastimil Babka wrote:
> On 2/14/19 9:38 PM, Andrew Morton wrote:
> > On Thu, 14 Feb 2019 12:45:51 +0000 Peng Fan <peng.fan@nxp.com> wrote:
> > 
> >> In case cma_init_reserved_mem failed, need to free the memblock allocated
> >> by memblock_reserve or memblock_alloc_range.
> >>
> >> ...
> >>
> >> --- a/mm/cma.c
> >> +++ b/mm/cma.c
> >> @@ -353,12 +353,14 @@ int __init cma_declare_contiguous(phys_addr_t base,
> >>  
> >>  	ret = cma_init_reserved_mem(base, size, order_per_bit, name, res_cma);
> >>  	if (ret)
> >> -		goto err;
> >> +		goto free_mem;
> >>  
> >>  	pr_info("Reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
> >>  		&base);
> >>  	return 0;
> >>  
> >> +free_mem:
> >> +	memblock_free(base, size);
> >>  err:
> >>  	pr_err("Failed to reserve %ld MiB\n", (unsigned long)size / SZ_1M);
> >>  	return ret;
> > 
> > This doesn't look right to me.  In the `fixed==true' case we didn't
> > actually allocate anything and in the `fixed==false' case, the
> > allocated memory is at `addr', not at `base'.
> 
> I think it's ok as the fixed==true path has "memblock_reserve()", but
> better leave this to the memblock maintainer :)

As Peng Fan noted in the other e-mail, fixed==true has memblock_reserve()
and fixed==false resets base = addr, so this is Ok.
 
> There's also 'kmemleak_ignore_phys(addr)' which should probably be
> undone (or not called at all) in the failure case. But it seems to be
> missing from the fixed==true path?

Well, memblock and kmemleak interaction does not seem to have clear
semantics anyway. memblock_free() calls kmemleak_free_part_phys() which
does not seem to care about ignored objects.
As for the fixed==true path, memblock_reserve() does not register the area
with kmemleak, so there would be no object to free in memblock_free().
AFAIU, kmemleak simply ignores this.

Catalin, can you comment please?

-- 
Sincerely yours,
Mike.

RE: [PATCH] mm/cma: cma_declare_contiguous: correct err handling

[Peng Fan]

> -----Original Message-----
> From: Mike Rapoport [mailto:rppt@linux.ibm.com]
> Sent: 2019年2月20日 1:46
> To: Vlastimil Babka <vbabka@suse.cz>
> Cc: Andrew Morton <akpm@linux-foundation.org>; Peng Fan
> <peng.fan@nxp.com>; labbott@redhat.com; mhocko@suse.com;
> iamjoonsoo.kim@lge.com; rppt@linux.vnet.ibm.com;
> m.szyprowski@samsung.com; rdunlap@infradead.org;
> andreyknvl@google.com; linux-mm@kvack.org; linux-kernel@vger.kernel.org;
> van.freenix@gmail.com; Catalin Marinas <catalin.marinas@arm.com>
> Subject: Re: [PATCH] mm/cma: cma_declare_contiguous: correct err handling
> 
> On Tue, Feb 19, 2019 at 05:55:33PM +0100, Vlastimil Babka wrote:
> > On 2/14/19 9:38 PM, Andrew Morton wrote:
> > > On Thu, 14 Feb 2019 12:45:51 +0000 Peng Fan <peng.fan@nxp.com>
> wrote:
> > >
> > >> In case cma_init_reserved_mem failed, need to free the memblock
> > >> allocated by memblock_reserve or memblock_alloc_range.
> > >>
> > >> ...
> > >>
> > >> --- a/mm/cma.c
> > >> +++ b/mm/cma.c
> > >> @@ -353,12 +353,14 @@ int __init
> cma_declare_contiguous(phys_addr_t
> > >> base,
> > >>
> > >>  	ret = cma_init_reserved_mem(base, size, order_per_bit, name,
> res_cma);
> > >>  	if (ret)
> > >> -		goto err;
> > >> +		goto free_mem;
> > >>
> > >>  	pr_info("Reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
> > >>  		&base);
> > >>  	return 0;
> > >>
> > >> +free_mem:
> > >> +	memblock_free(base, size);
> > >>  err:
> > >>  	pr_err("Failed to reserve %ld MiB\n", (unsigned long)size / SZ_1M);
> > >>  	return ret;
> > >
> > > This doesn't look right to me.  In the `fixed==true' case we didn't
> > > actually allocate anything and in the `fixed==false' case, the
> > > allocated memory is at `addr', not at `base'.
> >
> > I think it's ok as the fixed==true path has "memblock_reserve()", but
> > better leave this to the memblock maintainer :)
> 
> As Peng Fan noted in the other e-mail, fixed==true has memblock_reserve()
> and fixed==false resets base = addr, so this is Ok.
> 
> > There's also 'kmemleak_ignore_phys(addr)' which should probably be
> > undone (or not called at all) in the failure case. But it seems to be
> > missing from the fixed==true path?
> 
> Well, memblock and kmemleak interaction does not seem to have clear
> semantics anyway. memblock_free() calls kmemleak_free_part_phys() which
> does not seem to care about ignored objects.
> As for the fixed==true path, memblock_reserve() does not register the area
> with kmemleak, so there would be no object to free in memblock_free().
> AFAIU, kmemleak simply ignores this.

I also go through the memblock_free flow, and agree with Mike
memblock_free 
    -> kmemleak_free_part_phys 
          -> kmemleak_free_part
                 |-> delete_object_part
                         |-> object = find_and_remove_object(ptr, 1);

memblock_reserve not register the area in kmemleak, so find_and_remove_object
will not be able to find a valid area and just return.

What should I do next with this patch?

Thanks,
Peng.

> 
> Catalin, can you comment please?
> 
> --
> Sincerely yours,
> Mike.

Re: [PATCH] mm/cma: cma_declare_contiguous: correct err handling

[Mike Rapoport]

On Fri, Feb 22, 2019 at 12:55:41PM +0000, Peng Fan wrote:
> 
> 
> > -----Original Message-----
> > From: Mike Rapoport [mailto:rppt@linux.ibm.com]
> > Sent: 2019年2月20日 1:46
> > To: Vlastimil Babka <vbabka@suse.cz>
> > Cc: Andrew Morton <akpm@linux-foundation.org>; Peng Fan
> > <peng.fan@nxp.com>; labbott@redhat.com; mhocko@suse.com;
> > iamjoonsoo.kim@lge.com; rppt@linux.vnet.ibm.com;
> > m.szyprowski@samsung.com; rdunlap@infradead.org;
> > andreyknvl@google.com; linux-mm@kvack.org; linux-kernel@vger.kernel.org;
> > van.freenix@gmail.com; Catalin Marinas <catalin.marinas@arm.com>
> > Subject: Re: [PATCH] mm/cma: cma_declare_contiguous: correct err handling
> > 
> > On Tue, Feb 19, 2019 at 05:55:33PM +0100, Vlastimil Babka wrote:
> > > On 2/14/19 9:38 PM, Andrew Morton wrote:
> > > > On Thu, 14 Feb 2019 12:45:51 +0000 Peng Fan <peng.fan@nxp.com>
> > wrote:
> > > >
> > > >> In case cma_init_reserved_mem failed, need to free the memblock
> > > >> allocated by memblock_reserve or memblock_alloc_range.
> > > >>
> > > >> ...
> > > >>
> > > >> --- a/mm/cma.c
> > > >> +++ b/mm/cma.c
> > > >> @@ -353,12 +353,14 @@ int __init
> > cma_declare_contiguous(phys_addr_t
> > > >> base,
> > > >>
> > > >>  	ret = cma_init_reserved_mem(base, size, order_per_bit, name,
> > res_cma);
> > > >>  	if (ret)
> > > >> -		goto err;
> > > >> +		goto free_mem;
> > > >>
> > > >>  	pr_info("Reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
> > > >>  		&base);
> > > >>  	return 0;
> > > >>
> > > >> +free_mem:
> > > >> +	memblock_free(base, size);
> > > >>  err:
> > > >>  	pr_err("Failed to reserve %ld MiB\n", (unsigned long)size / SZ_1M);
> > > >>  	return ret;
> > > >
> > > > This doesn't look right to me.  In the `fixed==true' case we didn't
> > > > actually allocate anything and in the `fixed==false' case, the
> > > > allocated memory is at `addr', not at `base'.
> > >
> > > I think it's ok as the fixed==true path has "memblock_reserve()", but
> > > better leave this to the memblock maintainer :)
> > 
> > As Peng Fan noted in the other e-mail, fixed==true has memblock_reserve()
> > and fixed==false resets base = addr, so this is Ok.
> > 
> > > There's also 'kmemleak_ignore_phys(addr)' which should probably be
> > > undone (or not called at all) in the failure case. But it seems to be
> > > missing from the fixed==true path?
> > 
> > Well, memblock and kmemleak interaction does not seem to have clear
> > semantics anyway. memblock_free() calls kmemleak_free_part_phys() which
> > does not seem to care about ignored objects.
> > As for the fixed==true path, memblock_reserve() does not register the area
> > with kmemleak, so there would be no object to free in memblock_free().
> > AFAIU, kmemleak simply ignores this.
> 
> I also go through the memblock_free flow, and agree with Mike
> memblock_free 
>     -> kmemleak_free_part_phys 
>           -> kmemleak_free_part
>                  |-> delete_object_part
>                          |-> object = find_and_remove_object(ptr, 1);
> 
> memblock_reserve not register the area in kmemleak, so find_and_remove_object
> will not be able to find a valid area and just return.
> 
> What should I do next with this patch?
 
I'd suggest to wait for Catalin to review it.

I think it's also worth making the changelog more elaborate and include the
details we've discussed in this thread.

> Thanks,
> Peng.
> 
> > 
> > Catalin, can you comment please?
> > 
> > --
> > Sincerely yours,
> > Mike.
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH] mm/cma: cma_declare_contiguous: correct err handling

[Catalin Marinas]

On Tue, Feb 19, 2019 at 07:46:11PM +0200, Mike Rapoport wrote:
> On Tue, Feb 19, 2019 at 05:55:33PM +0100, Vlastimil Babka wrote:
> > On 2/14/19 9:38 PM, Andrew Morton wrote:
> > > On Thu, 14 Feb 2019 12:45:51 +0000 Peng Fan <peng.fan@nxp.com> wrote:
> > > 
> > >> In case cma_init_reserved_mem failed, need to free the memblock allocated
> > >> by memblock_reserve or memblock_alloc_range.
> > >>
> > >> ...
> > >>
> > >> --- a/mm/cma.c
> > >> +++ b/mm/cma.c
> > >> @@ -353,12 +353,14 @@ int __init cma_declare_contiguous(phys_addr_t base,
> > >>  
> > >>  	ret = cma_init_reserved_mem(base, size, order_per_bit, name, res_cma);
> > >>  	if (ret)
> > >> -		goto err;
> > >> +		goto free_mem;
> > >>  
> > >>  	pr_info("Reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
> > >>  		&base);
> > >>  	return 0;
> > >>  
> > >> +free_mem:
> > >> +	memblock_free(base, size);
> > >>  err:
> > >>  	pr_err("Failed to reserve %ld MiB\n", (unsigned long)size / SZ_1M);
> > >>  	return ret;
> > > 
> > > This doesn't look right to me.  In the `fixed==true' case we didn't
> > > actually allocate anything and in the `fixed==false' case, the
> > > allocated memory is at `addr', not at `base'.
> > 
> > I think it's ok as the fixed==true path has "memblock_reserve()", but
> > better leave this to the memblock maintainer :)
> 
> As Peng Fan noted in the other e-mail, fixed==true has memblock_reserve()
> and fixed==false resets base = addr, so this is Ok.
>  
> > There's also 'kmemleak_ignore_phys(addr)' which should probably be
> > undone (or not called at all) in the failure case. But it seems to be
> > missing from the fixed==true path?
> 
> Well, memblock and kmemleak interaction does not seem to have clear
> semantics anyway. memblock_free() calls kmemleak_free_part_phys() which
> does not seem to care about ignored objects.
> As for the fixed==true path, memblock_reserve() does not register the area
> with kmemleak, so there would be no object to free in memblock_free().
> AFAIU, kmemleak simply ignores this.

Kmemleak is supposed to work with the memblock_{alloc,free} pair and it
ignores the memblock_reserve() as a memblock_alloc() implementation
detail. It is, however, tolerant to memblock_free() being called on a
sub-range or just a different range from a previous memblock_alloc(). So
the original patch looks fine to me. FWIW:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>