[PATCH] linux_gntshr_munmap: munmap takes a length, not a page count

[PATCH] linux_gntshr_munmap: munmap takes a length, not a page count

[David Scott]

This fixes a bug where if a client shares more than 1 page, the
munmap call fails to clean up everything. A process which does
a lot of sharing and unsharing can run out of resources.

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 tools/libxc/xc_linux_osdep.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
index 86bff3e..a19e4b6 100644
--- a/tools/libxc/xc_linux_osdep.c
+++ b/tools/libxc/xc_linux_osdep.c
@@ -847,7 +847,7 @@ static void *linux_gntshr_share_pages(xc_gntshr *xch, xc_osdep_handle h,
 static int linux_gntshr_munmap(xc_gntshr *xcg, xc_osdep_handle h,
                                void *start_address, uint32_t count)
 {
-    return munmap(start_address, count);
+    return munmap(start_address, count * XC_PAGE_SIZE);
 }
 
 static struct xc_osdep_ops linux_gntshr_ops = {
-- 
1.7.10.4

Re: [PATCH] linux_gntshr_munmap: munmap takes a length, not a page count

[Stefano Stabellini]

On Mon, 1 Sep 2014, David Scott wrote:
> This fixes a bug where if a client shares more than 1 page, the
> munmap call fails to clean up everything. A process which does
> a lot of sharing and unsharing can run out of resources.
> 
> Signed-off-by: David Scott <dave.scott@citrix.com>

Great catch!

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  tools/libxc/xc_linux_osdep.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
> index 86bff3e..a19e4b6 100644
> --- a/tools/libxc/xc_linux_osdep.c
> +++ b/tools/libxc/xc_linux_osdep.c
> @@ -847,7 +847,7 @@ static void *linux_gntshr_share_pages(xc_gntshr *xch, xc_osdep_handle h,
>  static int linux_gntshr_munmap(xc_gntshr *xcg, xc_osdep_handle h,
>                                 void *start_address, uint32_t count)
>  {
> -    return munmap(start_address, count);
> +    return munmap(start_address, count * XC_PAGE_SIZE);
>  }
>  
>  static struct xc_osdep_ops linux_gntshr_ops = {
> -- 
> 1.7.10.4
> 

Re: [PATCH] linux_gntshr_munmap: munmap takes a length, not a page count

[Ian Campbell]

On Mon, 2014-09-01 at 13:16 +0100, David Scott wrote:
> This fixes a bug where if a client shares more than 1 page, the
> munmap call fails to clean up everything. A process which does
> a lot of sharing and unsharing can run out of resources.

The doc comment on xc_gntshr_munmap does say count is in pages, so your
change is correct but all of the in tree callers seem to pass a number
of bytes not a number of pages. i.e. everything in tools/libvchan passes
n*PAGE_SIZE.

So I don't think this change is complete without also updating those.

Ian.

> 
> Signed-off-by: David Scott <dave.scott@citrix.com>
> ---
>  tools/libxc/xc_linux_osdep.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
> index 86bff3e..a19e4b6 100644
> --- a/tools/libxc/xc_linux_osdep.c
> +++ b/tools/libxc/xc_linux_osdep.c
> @@ -847,7 +847,7 @@ static void *linux_gntshr_share_pages(xc_gntshr *xch, xc_osdep_handle h,
>  static int linux_gntshr_munmap(xc_gntshr *xcg, xc_osdep_handle h,
>                                 void *start_address, uint32_t count)
>  {
> -    return munmap(start_address, count);
> +    return munmap(start_address, count * XC_PAGE_SIZE);
>  }
>  
>  static struct xc_osdep_ops linux_gntshr_ops = {

Re: [PATCH] linux_gntshr_munmap: munmap takes a length, not a page count

[David Vrabel]

On 03/09/14 15:01, Ian Campbell wrote:
> On Mon, 2014-09-01 at 13:16 +0100, David Scott wrote:
>> This fixes a bug where if a client shares more than 1 page, the
>> munmap call fails to clean up everything. A process which does
>> a lot of sharing and unsharing can run out of resources.
> 
> The doc comment on xc_gntshr_munmap does say count is in pages, so your
> change is correct but all of the in tree callers seem to pass a number
> of bytes not a number of pages. i.e. everything in tools/libvchan passes
> n*PAGE_SIZE.
> 
> So I don't think this change is complete without also updating those.

Would the corrent non-ABI/API-breaking fix be to just update the
documentation?

David

Re: [PATCH] linux_gntshr_munmap: munmap takes a length, not a page count

[Ian Campbell]

On Wed, 2014-09-03 at 15:15 +0100, David Vrabel wrote:
> On 03/09/14 15:01, Ian Campbell wrote:
> > On Mon, 2014-09-01 at 13:16 +0100, David Scott wrote:
> >> This fixes a bug where if a client shares more than 1 page, the
> >> munmap call fails to clean up everything. A process which does
> >> a lot of sharing and unsharing can run out of resources.
> > 
> > The doc comment on xc_gntshr_munmap does say count is in pages, so your
> > change is correct but all of the in tree callers seem to pass a number
> > of bytes not a number of pages. i.e. everything in tools/libvchan passes
> > n*PAGE_SIZE.
> > 
> > So I don't think this change is complete without also updating those.
> 
> Would the corrent non-ABI/API-breaking fix be to just update the
> documentation?

libxc doesn't have a stable API or ABI.

My thinking in this case was that @count being pages was consistent with
other related functions in the API and also avoids issues of what to do
if count%4096 != 0.

Ian.

Re: [PATCH] linux_gntshr_munmap: munmap takes a length, not a page count

[David Vrabel]

On 03/09/14 15:17, Ian Campbell wrote:
> On Wed, 2014-09-03 at 15:15 +0100, David Vrabel wrote:
>> On 03/09/14 15:01, Ian Campbell wrote:
>>> On Mon, 2014-09-01 at 13:16 +0100, David Scott wrote:
>>>> This fixes a bug where if a client shares more than 1 page, the
>>>> munmap call fails to clean up everything. A process which does
>>>> a lot of sharing and unsharing can run out of resources.
>>>
>>> The doc comment on xc_gntshr_munmap does say count is in pages, so your
>>> change is correct but all of the in tree callers seem to pass a number
>>> of bytes not a number of pages. i.e. everything in tools/libvchan passes
>>> n*PAGE_SIZE.
>>>
>>> So I don't think this change is complete without also updating those.
>>
>> Would the corrent non-ABI/API-breaking fix be to just update the
>> documentation?
> 
> libxc doesn't have a stable API or ABI.

xc_gntshr_*() really should be part of a separate libxc-for-domu that
does have a stable ABI.  But since it isn't yet...

> My thinking in this case was that @count being pages was consistent with
> other related functions in the API and also avoids issues of what to do
> if count%4096 != 0.

...this sounds reasonable.

David

Re: [PATCH] linux_gntshr_munmap: munmap takes a length, not a page count

[Ian Campbell]

On Wed, 2014-09-03 at 15:21 +0100, David Vrabel wrote:
> On 03/09/14 15:17, Ian Campbell wrote:
> > On Wed, 2014-09-03 at 15:15 +0100, David Vrabel wrote:
> >> On 03/09/14 15:01, Ian Campbell wrote:
> >>> On Mon, 2014-09-01 at 13:16 +0100, David Scott wrote:
> >>>> This fixes a bug where if a client shares more than 1 page, the
> >>>> munmap call fails to clean up everything. A process which does
> >>>> a lot of sharing and unsharing can run out of resources.
> >>>
> >>> The doc comment on xc_gntshr_munmap does say count is in pages, so your
> >>> change is correct but all of the in tree callers seem to pass a number
> >>> of bytes not a number of pages. i.e. everything in tools/libvchan passes
> >>> n*PAGE_SIZE.
> >>>
> >>> So I don't think this change is complete without also updating those.
> >>
> >> Would the corrent non-ABI/API-breaking fix be to just update the
> >> documentation?
> > 
> > libxc doesn't have a stable API or ABI.
> 
> xc_gntshr_*() really should be part of a separate libxc-for-domu that
> does have a stable ABI.

This hadn't occurred to me, but yes, this does sound sensible.

>   But since it isn't yet...
> 
> > My thinking in this case was that @count being pages was consistent with
> > other related functions in the API and also avoids issues of what to do
> > if count%4096 != 0.
> 
> ...this sounds reasonable.

Thanks.

Ian

Re: [PATCH] linux_gntshr_munmap: munmap takes a length, not a page count

[Andrew Cooper]

On 03/09/14 15:31, Ian Campbell wrote:
> On Wed, 2014-09-03 at 15:21 +0100, David Vrabel wrote:
>> On 03/09/14 15:17, Ian Campbell wrote:
>>> On Wed, 2014-09-03 at 15:15 +0100, David Vrabel wrote:
>>>> On 03/09/14 15:01, Ian Campbell wrote:
>>>>> On Mon, 2014-09-01 at 13:16 +0100, David Scott wrote:
>>>>>> This fixes a bug where if a client shares more than 1 page, the
>>>>>> munmap call fails to clean up everything. A process which does
>>>>>> a lot of sharing and unsharing can run out of resources.
>>>>> The doc comment on xc_gntshr_munmap does say count is in pages, so your
>>>>> change is correct but all of the in tree callers seem to pass a number
>>>>> of bytes not a number of pages. i.e. everything in tools/libvchan passes
>>>>> n*PAGE_SIZE.
>>>>>
>>>>> So I don't think this change is complete without also updating those.
>>>> Would the corrent non-ABI/API-breaking fix be to just update the
>>>> documentation?
>>> libxc doesn't have a stable API or ABI.
>> xc_gntshr_*() really should be part of a separate libxc-for-domu that
>> does have a stable ABI.
> This hadn't occurred to me, but yes, this does sound sensible.

Currently, all domU event and grant operations require libxc, including
the use of libxenstore.

I have raised this before, and believe there is a line item defered
until 4.6 to cleave libxc into several pieces, to avoid needing to put
libxc into all domUs to use the xenstore-* binaries.  libxenevent and
libxengrant seem like plausible library names.

~Andrew

Re: [PATCH] linux_gntshr_munmap: munmap takes a length, not a page count

[Dave Scott]

On 3 Sep 2014, at 15:01, Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Mon, 2014-09-01 at 13:16 +0100, David Scott wrote:
>> This fixes a bug where if a client shares more than 1 page, the
>> munmap call fails to clean up everything. A process which does
>> a lot of sharing and unsharing can run out of resources.
> 
> The doc comment on xc_gntshr_munmap does say count is in pages, so your
> change is correct but all of the in tree callers seem to pass a number
> of bytes not a number of pages. i.e. everything in tools/libvchan passes
> n*PAGE_SIZE.

Good point.

> So I don't think this change is complete without also updating those.

I’ll resubmit with the in-tree callers fixed.

Thanks,
Dave

> 
> Ian.
> 
>> 
>> Signed-off-by: David Scott <dave.scott@citrix.com>
>> ---
>> tools/libxc/xc_linux_osdep.c |    2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
>> index 86bff3e..a19e4b6 100644
>> --- a/tools/libxc/xc_linux_osdep.c
>> +++ b/tools/libxc/xc_linux_osdep.c
>> @@ -847,7 +847,7 @@ static void *linux_gntshr_share_pages(xc_gntshr *xch, xc_osdep_handle h,
>> static int linux_gntshr_munmap(xc_gntshr *xcg, xc_osdep_handle h,
>>                                void *start_address, uint32_t count)
>> {
>> -    return munmap(start_address, count);
>> +    return munmap(start_address, count * XC_PAGE_SIZE);
>> }
>> 
>> static struct xc_osdep_ops linux_gntshr_ops = {
> 
> 

Re: [PATCH] linux_gntshr_munmap: munmap takes a length, not a page count

[Dave Scott]

On 3 Sep 2014, at 16:48, David Scott <dave.scott@citrix.com> wrote:

> 
> On 3 Sep 2014, at 15:01, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
>> On Mon, 2014-09-01 at 13:16 +0100, David Scott wrote:
>>> This fixes a bug where if a client shares more than 1 page, the
>>> munmap call fails to clean up everything. A process which does
>>> a lot of sharing and unsharing can run out of resources.
>> 
>> The doc comment on xc_gntshr_munmap does say count is in pages, so your
>> change is correct but all of the in tree callers seem to pass a number
>> of bytes not a number of pages. i.e. everything in tools/libvchan passes
>> n*PAGE_SIZE.
> 
> Good point.
> 
>> So I don't think this change is complete without also updating those.
> 
> I’ll resubmit with the in-tree callers fixed.

Also it looks like xc_gnttab_munmap (gnttab not gntshr) uses pages in both the API and implementation, but libvchan is supplying bytes. I’ll fix vchan here too.

Dave

> 
> Thanks,
> Dave
> 
>> 
>> Ian.
>> 
>>> 
>>> Signed-off-by: David Scott <dave.scott@citrix.com>
>>> ---
>>> tools/libxc/xc_linux_osdep.c |    2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
>>> index 86bff3e..a19e4b6 100644
>>> --- a/tools/libxc/xc_linux_osdep.c
>>> +++ b/tools/libxc/xc_linux_osdep.c
>>> @@ -847,7 +847,7 @@ static void *linux_gntshr_share_pages(xc_gntshr *xch, xc_osdep_handle h,
>>> static int linux_gntshr_munmap(xc_gntshr *xcg, xc_osdep_handle h,
>>>                               void *start_address, uint32_t count)
>>> {
>>> -    return munmap(start_address, count);
>>> +    return munmap(start_address, count * XC_PAGE_SIZE);
>>> }
>>> 
>>> static struct xc_osdep_ops linux_gntshr_ops = {
>> 
>> 
> 

[PATCH v2] linux_gntshr_munmap: munmap takes a length, not a page count

[David Scott]

This fixes a bug where if a client shares more than 1 page, the
munmap call fails to clean up everything. A process which does
a lot of sharing and unsharing can run out of resources.

This patch also fixes in-tree callers of
  - xc_gntshr_munmap
  - xc_gnttab_munmap
to supply page counts rather than lengths.

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 tools/libvchan/init.c        |    8 ++++----
 tools/libvchan/io.c          |    4 ++--
 tools/libxc/xc_linux_osdep.c |    2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index c080a1c..de10817 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -129,9 +129,9 @@ out:
 	return ring_ref;
 out_unmap_left:
 	if (pages_left)
-		xc_gntshr_munmap(ctrl->gntshr, ctrl->read.buffer, pages_left * PAGE_SIZE);
+		xc_gntshr_munmap(ctrl->gntshr, ctrl->read.buffer, pages_left);
 out_ring:
-	xc_gntshr_munmap(ctrl->gntshr, ring, PAGE_SIZE);
+	xc_gntshr_munmap(ctrl->gntshr, ring, 1);
 	ring_ref = -1;
 	ctrl->ring = NULL;
 	ctrl->write.order = ctrl->read.order = 0;
@@ -204,9 +204,9 @@ static int init_gnt_cli(struct libxenvchan *ctrl, int domain, uint32_t ring_ref)
  out_unmap_left:
 	if (ctrl->write.order >= PAGE_SHIFT)
 		xc_gnttab_munmap(ctrl->gnttab, ctrl->write.buffer,
-		                 1 << ctrl->write.order);
+		                 1 << (ctrl->write.order - PAGE_SHIFT));
  out_unmap_ring:
-	xc_gnttab_munmap(ctrl->gnttab, ctrl->ring, PAGE_SIZE);
+	xc_gnttab_munmap(ctrl->gnttab, ctrl->ring, 1);
 	ctrl->ring = 0;
 	ctrl->write.order = ctrl->read.order = 0;
 	rv = -1;
diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c
index 6e6e239..e66bc4e 100644
--- a/tools/libvchan/io.c
+++ b/tools/libvchan/io.c
@@ -365,10 +365,10 @@ void libxenvchan_close(struct libxenvchan *ctrl)
 	if (ctrl->ring) {
 		if (ctrl->is_server) {
 			ctrl->ring->srv_live = 0;
-			xc_gntshr_munmap(ctrl->gntshr, ctrl->ring, PAGE_SIZE);
+			xc_gntshr_munmap(ctrl->gntshr, ctrl->ring, 1);
 		} else {
 			ctrl->ring->cli_live = 0;
-			xc_gnttab_munmap(ctrl->gnttab, ctrl->ring, PAGE_SIZE);
+			xc_gnttab_munmap(ctrl->gnttab, ctrl->ring, 1);
 		}
 	}
 	if (ctrl->event) {
diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
index 86bff3e..a19e4b6 100644
--- a/tools/libxc/xc_linux_osdep.c
+++ b/tools/libxc/xc_linux_osdep.c
@@ -847,7 +847,7 @@ static void *linux_gntshr_share_pages(xc_gntshr *xch, xc_osdep_handle h,
 static int linux_gntshr_munmap(xc_gntshr *xcg, xc_osdep_handle h,
                                void *start_address, uint32_t count)
 {
-    return munmap(start_address, count);
+    return munmap(start_address, count * XC_PAGE_SIZE);
 }
 
 static struct xc_osdep_ops linux_gntshr_ops = {
-- 
1.7.10.4

Re: [PATCH v2] linux_gntshr_munmap: munmap takes a length, not a page count

[Ian Campbell]

On Wed, 2014-09-03 at 18:34 +0100, David Scott wrote:
> This fixes a bug where if a client shares more than 1 page, the
> munmap call fails to clean up everything. A process which does
> a lot of sharing and unsharing can run out of resources.
> 
> This patch also fixes in-tree callers of
>   - xc_gntshr_munmap
>   - xc_gnttab_munmap
> to supply page counts rather than lengths.
> 
> Signed-off-by: David Scott <dave.scott@citrix.com>

Acked + applied, thanks.