linux-kernel - Re: [PATCH] xen-blkback: use balloon pages for persistent grants

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <511CFC93.9070205@citrix.com>
Date:	Thu, 14 Feb 2013 16:02:43 +0100
From:	Roger Pau Monné <roger.pau@...rix.com>
To:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"xen-devel@...ts.xen.org" <xen-devel@...ts.xen.org>
Subject: Re: [PATCH] xen-blkback: use balloon pages for persistent grants

On 14/02/13 14:23, Konrad Rzeszutek Wilk wrote:
> On Thu, Feb 14, 2013 at 11:12:09AM +0100, Roger Pau Monne wrote:
>> With current persistent grants implementation we are not freeing the
>> persistent grants after we disconnect the device. Since grant map
> 
> Can you explain this in more details please? Isn't gnttab_set_unmap_op
> in free_persistent_gnts doing the right job of putting in the right
> mfn in? And then we could free the page?

No, after gnttab_unmap_refs the page still points to the grant frame
mfn. All the users of grant frames either have an internal buffer of
pages that are reused (like blkback), or use balloon pages (like gntdev).

I could probably modify gnttab_map to store the mfn and set it back at
gnttab_unmap, but that will surely require more work than this simple fix.

> 
> 
>> operations change the mfn of the allocated page, and we can no longer
>> pass it to __free_page without setting the mfn to a sane value, use
>> balloon grant pages instead, as the gntdev device does.
> 
> 
> Wow. I did not realize that we leaving such a huge memory leak behind!
> But I guess that was never an issue as we would recycle those persistent
> grants to other domains.

No, we didn't recycle them AFAIK, we allocated them using alloc_page,
passed them to gnttab_map, used them, and when closing the backend we
only unmapped them, but they where never freed.

>>
>> Signed-off-by: Roger Pau Monné <roger.pau@...rix.com>
>> Cc: xen-devel@...ts.xen.org
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
>> ---
>>  drivers/block/xen-blkback/blkback.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
>> index c46824f..e6c2f6a 100644
>> --- a/drivers/block/xen-blkback/blkback.c
>> +++ b/drivers/block/xen-blkback/blkback.c
>> @@ -46,6 +46,7 @@
>>  #include <xen/xen.h>
>>  #include <asm/xen/hypervisor.h>
>>  #include <asm/xen/hypercall.h>
>> +#include <xen/balloon.h>
>>  #include "common.h"
>>  
>>  /*
>> @@ -239,6 +240,7 @@ static void free_persistent_gnts(struct rb_root *root, unsigned int num)
>>  			ret = gnttab_unmap_refs(unmap, NULL, pages,
>>  				segs_to_unmap);
>>  			BUG_ON(ret);
>> +			free_xenballooned_pages(segs_to_unmap, pages);
>>  			segs_to_unmap = 0;
>>  		}
>>  
>> @@ -527,8 +529,8 @@ static int xen_blkbk_map(struct blkif_request *req,
>>  				GFP_KERNEL);
>>  			if (!persistent_gnt)
>>  				return -ENOMEM;
>> -			persistent_gnt->page = alloc_page(GFP_KERNEL);
>> -			if (!persistent_gnt->page) {
>> +			if (alloc_xenballooned_pages(1, &persistent_gnt->page,
>> +			    false)) {
>>  				kfree(persistent_gnt);
>>  				return -ENOMEM;
>>  			}
>> -- 
>> 1.7.7.5 (Apple Git-26)
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/