A memory leak problem in xen-blkback module

A memory leak problem in xen-blkback module

[Chentao(Boby)]

Hi Konrad,

    I find a memory leak problem in xen-blkback module of linux-3.14.4 release, and the newest 3.17-rc4 also has the same problem. The problem will occur in below condition.

    In xen_blkbk_map function, first get_free_page from balloon or the list of blkif free pages, then map this page. If get_free_page succeed, but map failed,
the grant handle corresponding to this page will be assigned to BLKBACK_INVALID_HANDLE. Because map failed, it will execute xen_blkbk_unmap to retrieve resources.
But in xen_blkbk_unmap function, if the grant handle of a page is BLKBACK_INVALID_HANDLE, it will continue to next loop to execute unmap and put_free_pages.
Only executes put_free_pages, these pages will be returned to the list of blkif free pages and at last be returned to balloon.

    Make a summary, in the condition of get_free_page succeed but map failed, the page will be leaked from balloon or the list of blkif free pages. I have a immature thought,
in xen_blkbk_unmap funtion, when judge the grant handle of a page is BLKBACK_INVALID_HANDLE, can we execute put_free_pages to retrieve this one page?

    Just like below:
    if (pages[i]->handle == BLKBACK_INVALID_HANDLE) {
        put_free_pages(blkif, pages[i]->page, 1);
        continue;
    }

    I'm looking forward to your reply. Any reply is appreciated.

    Best wishes.

    Tao Chen

Re: A memory leak problem in xen-blkback module

[Roger Pau Monné]

El 12/09/14 a les 8.58, Chentao(Boby) ha escrit:
> Hi Konrad,
> 
>     I find a memory leak problem in xen-blkback module of linux-3.14.4 release, and the newest 3.17-rc4 also has the same problem. The problem will occur in below condition.
> 
>     In xen_blkbk_map function, first get_free_page from balloon or the list of blkif free pages, then map this page. If get_free_page succeed, but map failed,
> the grant handle corresponding to this page will be assigned to BLKBACK_INVALID_HANDLE. Because map failed, it will execute xen_blkbk_unmap to retrieve resources.
> But in xen_blkbk_unmap function, if the grant handle of a page is BLKBACK_INVALID_HANDLE, it will continue to next loop to execute unmap and put_free_pages.
> Only executes put_free_pages, these pages will be returned to the list of blkif free pages and at last be returned to balloon.
> 
>     Make a summary, in the condition of get_free_page succeed but map failed, the page will be leaked from balloon or the list of blkif free pages. I have a immature thought,
> in xen_blkbk_unmap funtion, when judge the grant handle of a page is BLKBACK_INVALID_HANDLE, can we execute put_free_pages to retrieve this one page?
> 
>     Just like below:
>     if (pages[i]->handle == BLKBACK_INVALID_HANDLE) {
>         put_free_pages(blkif, pages[i]->page, 1);

This is not correct, and will fail to compile AFAICT. put_free_pages 
expects an array of pointers to page structs and you are passing a 
pointer to a page struct.

I have the following patch, which I think solves the problem. I've 
placed the free_pages call in xen_blkbk_map itself, but it could also 
be done in xen_blkbk_map_unmap.

---
commit 879ebb502e2f72279553bb0a85cc885ec492a0c1
Author: Roger Pau Monne <roger.pau@citrix.com>
Date:   Mon Sep 15 11:01:40 2014 +0200

    xen-blkback: fix leak on grant map error path
    
    Fix leaking a page when a grant mapping has failed.
    
    Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
    Reported by: Tao Chen <boby.chen@huawei.com>
    ---
    This patch should be backported to stable branches.

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 64c60ed..63fc7f0 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -763,6 +763,7 @@ again:
 			BUG_ON(new_map_idx >= segs_to_map);
 			if (unlikely(map[new_map_idx].status != 0)) {
 				pr_debug(DRV_PFX "invalid buffer -- could not remap it\n");
+				put_free_pages(blkif, &pages[seg_idx]->page, 1);
 				pages[seg_idx]->handle = BLKBACK_INVALID_HANDLE;
 				ret |= 1;
 				goto next;

Re: A memory leak problem in xen-blkback module

[Chentao(Boby)]

On 2014/9/15 17:55, Roger Pau Monné wrote:
> El 12/09/14 a les 8.58, Chentao(Boby) ha escrit:
>> Hi Konrad,
>>
>>     I find a memory leak problem in xen-blkback module of linux-3.14.4 release, and the newest 3.17-rc4 also has the same problem. The problem will occur in below condition.
>>
>>     In xen_blkbk_map function, first get_free_page from balloon or the list of blkif free pages, then map this page. If get_free_page succeed, but map failed,
>> the grant handle corresponding to this page will be assigned to BLKBACK_INVALID_HANDLE. Because map failed, it will execute xen_blkbk_unmap to retrieve resources.
>> But in xen_blkbk_unmap function, if the grant handle of a page is BLKBACK_INVALID_HANDLE, it will continue to next loop to execute unmap and put_free_pages.
>> Only executes put_free_pages, these pages will be returned to the list of blkif free pages and at last be returned to balloon.
>>
>>     Make a summary, in the condition of get_free_page succeed but map failed, the page will be leaked from balloon or the list of blkif free pages. I have a immature thought,
>> in xen_blkbk_unmap funtion, when judge the grant handle of a page is BLKBACK_INVALID_HANDLE, can we execute put_free_pages to retrieve this one page?
>>
>>     Just like below:
>>     if (pages[i]->handle == BLKBACK_INVALID_HANDLE) {
>>         put_free_pages(blkif, pages[i]->page, 1);
> 
> This is not correct, and will fail to compile AFAICT. put_free_pages 
> expects an array of pointers to page structs and you are passing a 
> pointer to a page struct.
> 
Thanks for pointing out my code mistake. And I'm very appreciated for your reply.

> I have the following patch, which I think solves the problem. I've 
> placed the free_pages call in xen_blkbk_map itself, but it could also 
> be done in xen_blkbk_map_unmap.
> 
I think the funtion is xen_blkbk_unmap, not xen_blkbk_map_unmap. Am I right?

> ---
> commit 879ebb502e2f72279553bb0a85cc885ec492a0c1
> Author: Roger Pau Monne <roger.pau@citrix.com>
> Date:   Mon Sep 15 11:01:40 2014 +0200
> 
>     xen-blkback: fix leak on grant map error path
>     
>     Fix leaking a page when a grant mapping has failed.
>     
>     Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>     Reported by: Tao Chen <boby.chen@huawei.com>
>     ---
>     This patch should be backported to stable branches.
> 
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index 64c60ed..63fc7f0 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -763,6 +763,7 @@ again:
>  			BUG_ON(new_map_idx >= segs_to_map);
>  			if (unlikely(map[new_map_idx].status != 0)) {
>  				pr_debug(DRV_PFX "invalid buffer -- could not remap it\n");
> +				put_free_pages(blkif, &pages[seg_idx]->page, 1);
>  				pages[seg_idx]->handle = BLKBACK_INVALID_HANDLE;
>  				ret |= 1;
>  				goto next;
> 
> 
> 
> .
>

Re: A memory leak problem in xen-blkback module

[Roger Pau Monné]

El 15/09/14 a les 14.54, Chentao(Boby) ha escrit:
> 
> 
> On 2014/9/15 17:55, Roger Pau Monné wrote:
>> I have the following patch, which I think solves the problem. I've 
>> placed the free_pages call in xen_blkbk_map itself, but it could also 
>> be done in xen_blkbk_map_unmap.
>>
> I think the funtion is xen_blkbk_unmap, not xen_blkbk_map_unmap. Am I right?

Yes, sorry for the typo. Could you confirm that this fix works for you?

Roger.

Re: A memory leak problem in xen-blkback module

[Chentao(Boby)]

On 2014/9/15 21:15, Roger Pau Monné wrote:
> El 15/09/14 a les 14.54, Chentao(Boby) ha escrit:
>>
>>
>> On 2014/9/15 17:55, Roger Pau Monné wrote:
>>> I have the following patch, which I think solves the problem. I've 
>>> placed the free_pages call in xen_blkbk_map itself, but it could also 
>>> be done in xen_blkbk_map_unmap.
>>>
>> I think the funtion is xen_blkbk_unmap, not xen_blkbk_map_unmap. Am I right?
> 
> Yes, sorry for the typo. Could you confirm that this fix works for you?
> 
> Roger.
> 
> .
> 
Yes, this fix works for me. Thanks one more time.

Best wishes.

Tao Chen

Re: A memory leak problem in xen-blkback module

[Roger Pau Monné]

El 15/09/14 a les 11.55, Roger Pau Monné ha escrit:
> El 12/09/14 a les 8.58, Chentao(Boby) ha escrit:
>> Hi Konrad,
>>
>>     I find a memory leak problem in xen-blkback module of linux-3.14.4 release, and the newest 3.17-rc4 also has the same problem. The problem will occur in below condition.
>>
>>     In xen_blkbk_map function, first get_free_page from balloon or the list of blkif free pages, then map this page. If get_free_page succeed, but map failed,
>> the grant handle corresponding to this page will be assigned to BLKBACK_INVALID_HANDLE. Because map failed, it will execute xen_blkbk_unmap to retrieve resources.
>> But in xen_blkbk_unmap function, if the grant handle of a page is BLKBACK_INVALID_HANDLE, it will continue to next loop to execute unmap and put_free_pages.
>> Only executes put_free_pages, these pages will be returned to the list of blkif free pages and at last be returned to balloon.
>>
>>     Make a summary, in the condition of get_free_page succeed but map failed, the page will be leaked from balloon or the list of blkif free pages. I have a immature thought,
>> in xen_blkbk_unmap funtion, when judge the grant handle of a page is BLKBACK_INVALID_HANDLE, can we execute put_free_pages to retrieve this one page?
>>
>>     Just like below:
>>     if (pages[i]->handle == BLKBACK_INVALID_HANDLE) {
>>         put_free_pages(blkif, pages[i]->page, 1);
> 
> This is not correct, and will fail to compile AFAICT. put_free_pages 
> expects an array of pointers to page structs and you are passing a 
> pointer to a page struct.
> 
> I have the following patch, which I think solves the problem. I've 
> placed the free_pages call in xen_blkbk_map itself, but it could also 
> be done in xen_blkbk_map_unmap.
> 
> ---
> commit 879ebb502e2f72279553bb0a85cc885ec492a0c1
> Author: Roger Pau Monne <roger.pau@citrix.com>
> Date:   Mon Sep 15 11:01:40 2014 +0200
> 
>     xen-blkback: fix leak on grant map error path
>     
>     Fix leaking a page when a grant mapping has failed.
>     
>     Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>     Reported by: Tao Chen <boby.chen@huawei.com>
>     ---
>     This patch should be backported to stable branches.

Ping?

Re: A memory leak problem in xen-blkback module

[Konrad Rzeszutek Wilk]

On Tue, Sep 30, 2014 at 05:23:13PM +0200, Roger Pau Monné wrote:
> El 15/09/14 a les 11.55, Roger Pau Monné ha escrit:
> > El 12/09/14 a les 8.58, Chentao(Boby) ha escrit:
> >> Hi Konrad,
> >>
> >>     I find a memory leak problem in xen-blkback module of linux-3.14.4 release, and the newest 3.17-rc4 also has the same problem. The problem will occur in below condition.
> >>
> >>     In xen_blkbk_map function, first get_free_page from balloon or the list of blkif free pages, then map this page. If get_free_page succeed, but map failed,
> >> the grant handle corresponding to this page will be assigned to BLKBACK_INVALID_HANDLE. Because map failed, it will execute xen_blkbk_unmap to retrieve resources.
> >> But in xen_blkbk_unmap function, if the grant handle of a page is BLKBACK_INVALID_HANDLE, it will continue to next loop to execute unmap and put_free_pages.
> >> Only executes put_free_pages, these pages will be returned to the list of blkif free pages and at last be returned to balloon.
> >>
> >>     Make a summary, in the condition of get_free_page succeed but map failed, the page will be leaked from balloon or the list of blkif free pages. I have a immature thought,
> >> in xen_blkbk_unmap funtion, when judge the grant handle of a page is BLKBACK_INVALID_HANDLE, can we execute put_free_pages to retrieve this one page?
> >>
> >>     Just like below:
> >>     if (pages[i]->handle == BLKBACK_INVALID_HANDLE) {
> >>         put_free_pages(blkif, pages[i]->page, 1);
> > 
> > This is not correct, and will fail to compile AFAICT. put_free_pages 
> > expects an array of pointers to page structs and you are passing a 
> > pointer to a page struct.
> > 
> > I have the following patch, which I think solves the problem. I've 
> > placed the free_pages call in xen_blkbk_map itself, but it could also 
> > be done in xen_blkbk_map_unmap.
> > 
> > ---
> > commit 879ebb502e2f72279553bb0a85cc885ec492a0c1
> > Author: Roger Pau Monne <roger.pau@citrix.com>
> > Date:   Mon Sep 15 11:01:40 2014 +0200
> > 
> >     xen-blkback: fix leak on grant map error path
> >     
> >     Fix leaking a page when a grant mapping has failed.
> >     
> >     Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >     Reported by: Tao Chen <boby.chen@huawei.com>
> >     ---
> >     This patch should be backported to stable branches.
> 
> Ping?
> 

Reviewed. Testing it and once that is done will send it for Jens. Thanks!