bug in xc_gntshr_munmap?

bug in xc_gntshr_munmap?

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 851 bytes --]

Hi,

Header says:
/*
 * Unmaps the @count pages starting at @start_address, which were mapped by a
 * call to xc_gntshr_share_*. Never logs.
 */
int xc_gntshr_munmap(xc_gntshr *xcg, void *start_address, uint32_t count);

But implementation calls:
static int linux_gntshr_munmap(xc_gntshr *xcg, xc_osdep_handle h,
                               void *start_address, uint32_t count)
{
    return munmap(start_address, count);
}

munmap(2) expect second argument to be size of mapped area (in bytes), not
pages count.

Users of xc_gntshr_munmap (the only one I'm aware of is libxenvchan) already
uses that broken semantic.

Is it going to be fixed (I can send trivial patch for both libxc and
libxenvchan), or the comment in header should be updated?

-- 
Best Regards / Pozdrawiam,
Marek Marczykowski
Invisible Things Lab


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 553 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH] libxc: fix xc_gntshr_munmap semantic

[Marek Marczykowski]

"count" parameter should be pages count (as stated in comment in
xenctrl.h), not bytes count.
This patch fixes also the only user of this function (in xen sources) -
libvchan.

Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
---
 tools/libvchan/init.c        |  4 ++--
 tools/libvchan/io.c          | 10 ++++++----
 tools/libxc/xc_linux_osdep.c |  2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index 0c7cff6..f0d2505 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;
diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c
index 3c8d236..3040099 100644
--- a/tools/libvchan/io.c
+++ b/tools/libvchan/io.c
@@ -324,16 +324,18 @@ void libxenvchan_close(struct libxenvchan *ctrl)
 	if (!ctrl)
 		return;
 	if (ctrl->read.order >= PAGE_SHIFT)
-		munmap(ctrl->read.buffer, 1 << ctrl->read.order);
+		xc_gntshr_munmap(ctrl->gntshr, ctrl->read.buffer,
+				1 << (ctrl->read.order - PAGE_SHIFT));
 	if (ctrl->write.order >= PAGE_SHIFT)
-		munmap(ctrl->write.buffer, 1 << ctrl->write.order);
+		xc_gntshr_munmap(ctrl->gntshr, ctrl->write.buffer,
+				1 << (ctrl->write.order - PAGE_SHIFT));
 	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 36832b6..3c43fca 100644
--- a/tools/libxc/xc_linux_osdep.c
+++ b/tools/libxc/xc_linux_osdep.c
@@ -825,7 +825,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.8.1.4

Re: bug in xc_gntshr_munmap?

[Ian Campbell]

On Fri, 2013-04-26 at 12:07 +0100, Marek Marczykowski wrote:
> Hi,
> 
> Header says:
> /*
>  * Unmaps the @count pages starting at @start_address, which were mapped by a
>  * call to xc_gntshr_share_*. Never logs.
>  */
> int xc_gntshr_munmap(xc_gntshr *xcg, void *start_address, uint32_t count);
> 
> But implementation calls:
> static int linux_gntshr_munmap(xc_gntshr *xcg, xc_osdep_handle h,
>                                void *start_address, uint32_t count)
> {
>     return munmap(start_address, count);
> }
> 
> munmap(2) expect second argument to be size of mapped area (in bytes), not
> pages count.
> 
> Users of xc_gntshr_munmap (the only one I'm aware of is libxenvchan) already
> uses that broken semantic.
> 
> Is it going to be fixed (I can send trivial patch for both libxc and
> libxenvchan), or the comment in header should be updated?

I think the function should behave the same as the map side, whichever
that is.

Ian.

Re: bug in xc_gntshr_munmap?

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 1375 bytes --]

On 26.04.2013 15:34, Ian Campbell wrote:
> On Fri, 2013-04-26 at 12:07 +0100, Marek Marczykowski wrote:
>> Hi,
>>
>> Header says:
>> /*
>>  * Unmaps the @count pages starting at @start_address, which were mapped by a
>>  * call to xc_gntshr_share_*. Never logs.
>>  */
>> int xc_gntshr_munmap(xc_gntshr *xcg, void *start_address, uint32_t count);
>>
>> But implementation calls:
>> static int linux_gntshr_munmap(xc_gntshr *xcg, xc_osdep_handle h,
>>                                void *start_address, uint32_t count)
>> {
>>     return munmap(start_address, count);
>> }
>>
>> munmap(2) expect second argument to be size of mapped area (in bytes), not
>> pages count.
>>
>> Users of xc_gntshr_munmap (the only one I'm aware of is libxenvchan) already
>> uses that broken semantic.
>>
>> Is it going to be fixed (I can send trivial patch for both libxc and
>> libxenvchan), or the comment in header should be updated?
> 
> I think the function should behave the same as the map side, whichever
> that is.

Map side uses pages count.
Also xc_gnttab_{grant_map,munmap} both uses pages count. So I assume it is a
bug. The question is can it be simply changed - some software can already
depend on that broken semantic...

Anyway I will send a patch in a moment.

-- 
Best Regards / Pozdrawiam,
Marek Marczykowski
Invisible Things Lab


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 553 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: bug in xc_gntshr_munmap?

[Ian Campbell]

On Fri, 2013-04-26 at 14:41 +0100, Marek Marczykowski wrote:
> On 26.04.2013 15:34, Ian Campbell wrote:
> > On Fri, 2013-04-26 at 12:07 +0100, Marek Marczykowski wrote:
> >> Hi,
> >>
> >> Header says:
> >> /*
> >>  * Unmaps the @count pages starting at @start_address, which were mapped by a
> >>  * call to xc_gntshr_share_*. Never logs.
> >>  */
> >> int xc_gntshr_munmap(xc_gntshr *xcg, void *start_address, uint32_t count);
> >>
> >> But implementation calls:
> >> static int linux_gntshr_munmap(xc_gntshr *xcg, xc_osdep_handle h,
> >>                                void *start_address, uint32_t count)
> >> {
> >>     return munmap(start_address, count);
> >> }
> >>
> >> munmap(2) expect second argument to be size of mapped area (in bytes), not
> >> pages count.
> >>
> >> Users of xc_gntshr_munmap (the only one I'm aware of is libxenvchan) already
> >> uses that broken semantic.
> >>
> >> Is it going to be fixed (I can send trivial patch for both libxc and
> >> libxenvchan), or the comment in header should be updated?
> > 
> > I think the function should behave the same as the map side, whichever
> > that is.
> 
> Map side uses pages count.
> Also xc_gnttab_{grant_map,munmap} both uses pages count. So I assume it is a
> bug. The question is can it be simply changed - some software can already
> depend on that broken semantic...

libxc does not provide a stable API/ABI so it is OK to change as long as
you update the intree callers. We'll bump the SONAMEs before release if
we haven't already this cycle...


> Anyway I will send a patch in a moment.
> 

Re: [PATCH] libxc: fix xc_gntshr_munmap semantic

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 2869 bytes --]

CC'ed Ian.

On 26.04.2013 14:40, Marek Marczykowski wrote:
> "count" parameter should be pages count (as stated in comment in
> xenctrl.h), not bytes count.
> This patch fixes also the only user of this function (in xen sources) -
> libvchan.
> 
> Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
> ---
>  tools/libvchan/init.c        |  4 ++--
>  tools/libvchan/io.c          | 10 ++++++----
>  tools/libxc/xc_linux_osdep.c |  2 +-
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
> index 0c7cff6..f0d2505 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;
> diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c
> index 3c8d236..3040099 100644
> --- a/tools/libvchan/io.c
> +++ b/tools/libvchan/io.c
> @@ -324,16 +324,18 @@ void libxenvchan_close(struct libxenvchan *ctrl)
>  	if (!ctrl)
>  		return;
>  	if (ctrl->read.order >= PAGE_SHIFT)
> -		munmap(ctrl->read.buffer, 1 << ctrl->read.order);
> +		xc_gntshr_munmap(ctrl->gntshr, ctrl->read.buffer,
> +				1 << (ctrl->read.order - PAGE_SHIFT));
>  	if (ctrl->write.order >= PAGE_SHIFT)
> -		munmap(ctrl->write.buffer, 1 << ctrl->write.order);
> +		xc_gntshr_munmap(ctrl->gntshr, ctrl->write.buffer,
> +				1 << (ctrl->write.order - PAGE_SHIFT));
>  	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 36832b6..3c43fca 100644
> --- a/tools/libxc/xc_linux_osdep.c
> +++ b/tools/libxc/xc_linux_osdep.c
> @@ -825,7 +825,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 = {
> 


-- 
Best Regards / Pozdrawiam,
Marek Marczykowski
Invisible Things Lab


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 553 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] libxc: fix xc_gntshr_munmap semantic

[Ian Campbell]

On Fri, 2013-04-26 at 13:40 +0100, Marek Marczykowski wrote:
> "count" parameter should be pages count (as stated in comment in
> xenctrl.h), not bytes count.
> This patch fixes also the only user of this function (in xen sources) -
> libvchan.

Looks ok to me but Daniel De Graaf wrote all this stuff, Ccing him.

> 
> Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
> ---
>  tools/libvchan/init.c        |  4 ++--
>  tools/libvchan/io.c          | 10 ++++++----
>  tools/libxc/xc_linux_osdep.c |  2 +-
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
> index 0c7cff6..f0d2505 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;
> diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c
> index 3c8d236..3040099 100644
> --- a/tools/libvchan/io.c
> +++ b/tools/libvchan/io.c
> @@ -324,16 +324,18 @@ void libxenvchan_close(struct libxenvchan *ctrl)
>  	if (!ctrl)
>  		return;
>  	if (ctrl->read.order >= PAGE_SHIFT)
> -		munmap(ctrl->read.buffer, 1 << ctrl->read.order);
> +		xc_gntshr_munmap(ctrl->gntshr, ctrl->read.buffer,
> +				1 << (ctrl->read.order - PAGE_SHIFT));
>  	if (ctrl->write.order >= PAGE_SHIFT)
> -		munmap(ctrl->write.buffer, 1 << ctrl->write.order);
> +		xc_gntshr_munmap(ctrl->gntshr, ctrl->write.buffer,
> +				1 << (ctrl->write.order - PAGE_SHIFT));
>  	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 36832b6..3c43fca 100644
> --- a/tools/libxc/xc_linux_osdep.c
> +++ b/tools/libxc/xc_linux_osdep.c
> @@ -825,7 +825,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] libxc: fix xc_gntshr_munmap semantic

[Daniel De Graaf]

On 04/26/2013 10:44 AM, Ian Campbell wrote:
> On Fri, 2013-04-26 at 13:40 +0100, Marek Marczykowski wrote:
>> "count" parameter should be pages count (as stated in comment in
>> xenctrl.h), not bytes count.
>> This patch fixes also the only user of this function (in xen sources) -
>> libvchan.
>
> Looks ok to me but Daniel De Graaf wrote all this stuff, Ccing him.

This also looks good to me.

>>
>> Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
>> ---
>>   tools/libvchan/init.c        |  4 ++--
>>   tools/libvchan/io.c          | 10 ++++++----
>>   tools/libxc/xc_linux_osdep.c |  2 +-
>>   3 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
>> index 0c7cff6..f0d2505 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;
>> diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c
>> index 3c8d236..3040099 100644
>> --- a/tools/libvchan/io.c
>> +++ b/tools/libvchan/io.c
>> @@ -324,16 +324,18 @@ void libxenvchan_close(struct libxenvchan *ctrl)
>>   	if (!ctrl)
>>   		return;
>>   	if (ctrl->read.order >= PAGE_SHIFT)
>> -		munmap(ctrl->read.buffer, 1 << ctrl->read.order);
>> +		xc_gntshr_munmap(ctrl->gntshr, ctrl->read.buffer,
>> +				1 << (ctrl->read.order - PAGE_SHIFT));
>>   	if (ctrl->write.order >= PAGE_SHIFT)
>> -		munmap(ctrl->write.buffer, 1 << ctrl->write.order);
>> +		xc_gntshr_munmap(ctrl->gntshr, ctrl->write.buffer,
>> +				1 << (ctrl->write.order - PAGE_SHIFT));
>>   	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 36832b6..3c43fca 100644
>> --- a/tools/libxc/xc_linux_osdep.c
>> +++ b/tools/libxc/xc_linux_osdep.c
>> @@ -825,7 +825,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 = {
>


-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] libxc: fix xc_gntshr_munmap semantic

[Ian Campbell]

On Fri, 2013-04-26 at 16:15 +0100, Daniel De Graaf wrote:
> On 04/26/2013 10:44 AM, Ian Campbell wrote:
> > On Fri, 2013-04-26 at 13:40 +0100, Marek Marczykowski wrote:
> >> "count" parameter should be pages count (as stated in comment in
> >> xenctrl.h), not bytes count.
> >> This patch fixes also the only user of this function (in xen sources) -
> >> libvchan.
> >
> > Looks ok to me but Daniel De Graaf wrote all this stuff, Ccing him.
> 
> This also looks good to me.

May I take that as an Ack (or a Reviewed-by if you prefer)?

> 
> >>
> >> Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
> >> ---
> >>   tools/libvchan/init.c        |  4 ++--
> >>   tools/libvchan/io.c          | 10 ++++++----
> >>   tools/libxc/xc_linux_osdep.c |  2 +-
> >>   3 files changed, 9 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
> >> index 0c7cff6..f0d2505 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;
> >> diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c
> >> index 3c8d236..3040099 100644
> >> --- a/tools/libvchan/io.c
> >> +++ b/tools/libvchan/io.c
> >> @@ -324,16 +324,18 @@ void libxenvchan_close(struct libxenvchan *ctrl)
> >>   	if (!ctrl)
> >>   		return;
> >>   	if (ctrl->read.order >= PAGE_SHIFT)
> >> -		munmap(ctrl->read.buffer, 1 << ctrl->read.order);
> >> +		xc_gntshr_munmap(ctrl->gntshr, ctrl->read.buffer,
> >> +				1 << (ctrl->read.order - PAGE_SHIFT));
> >>   	if (ctrl->write.order >= PAGE_SHIFT)
> >> -		munmap(ctrl->write.buffer, 1 << ctrl->write.order);
> >> +		xc_gntshr_munmap(ctrl->gntshr, ctrl->write.buffer,
> >> +				1 << (ctrl->write.order - PAGE_SHIFT));
> >>   	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 36832b6..3c43fca 100644
> >> --- a/tools/libxc/xc_linux_osdep.c
> >> +++ b/tools/libxc/xc_linux_osdep.c
> >> @@ -825,7 +825,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] libxc: fix xc_gntshr_munmap semantic

[Daniel De Graaf]

On 04/26/2013 11:26 AM, Ian Campbell wrote:
> On Fri, 2013-04-26 at 16:15 +0100, Daniel De Graaf wrote:
>> On 04/26/2013 10:44 AM, Ian Campbell wrote:
>>> On Fri, 2013-04-26 at 13:40 +0100, Marek Marczykowski wrote:
>>>> "count" parameter should be pages count (as stated in comment in
>>>> xenctrl.h), not bytes count.
>>>> This patch fixes also the only user of this function (in xen sources) -
>>>> libvchan.
>>>
>>> Looks ok to me but Daniel De Graaf wrote all this stuff, Ccing him.
>>
>> This also looks good to me.
>
> May I take that as an Ack (or a Reviewed-by if you prefer)?

Yes, either one is fine.

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

>>
>>>>
>>>> Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
>>>> ---
>>>>    tools/libvchan/init.c        |  4 ++--
>>>>    tools/libvchan/io.c          | 10 ++++++----
>>>>    tools/libxc/xc_linux_osdep.c |  2 +-
>>>>    3 files changed, 9 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
>>>> index 0c7cff6..f0d2505 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;
>>>> diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c
>>>> index 3c8d236..3040099 100644
>>>> --- a/tools/libvchan/io.c
>>>> +++ b/tools/libvchan/io.c
>>>> @@ -324,16 +324,18 @@ void libxenvchan_close(struct libxenvchan *ctrl)
>>>>    	if (!ctrl)
>>>>    		return;
>>>>    	if (ctrl->read.order >= PAGE_SHIFT)
>>>> -		munmap(ctrl->read.buffer, 1 << ctrl->read.order);
>>>> +		xc_gntshr_munmap(ctrl->gntshr, ctrl->read.buffer,
>>>> +				1 << (ctrl->read.order - PAGE_SHIFT));
>>>>    	if (ctrl->write.order >= PAGE_SHIFT)
>>>> -		munmap(ctrl->write.buffer, 1 << ctrl->write.order);
>>>> +		xc_gntshr_munmap(ctrl->gntshr, ctrl->write.buffer,
>>>> +				1 << (ctrl->write.order - PAGE_SHIFT));
>>>>    	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 36832b6..3c43fca 100644
>>>> --- a/tools/libxc/xc_linux_osdep.c
>>>> +++ b/tools/libxc/xc_linux_osdep.c
>>>> @@ -825,7 +825,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] libxc: fix xc_gntshr_munmap semantic

[Ian Campbell]

On Fri, 2013-04-26 at 17:17 +0100, Daniel De Graaf wrote:
> On 04/26/2013 11:26 AM, Ian Campbell wrote:
> > On Fri, 2013-04-26 at 16:15 +0100, Daniel De Graaf wrote:
> >> On 04/26/2013 10:44 AM, Ian Campbell wrote:
> >>> On Fri, 2013-04-26 at 13:40 +0100, Marek Marczykowski wrote:
> >>>> "count" parameter should be pages count (as stated in comment in
> >>>> xenctrl.h), not bytes count.
> >>>> This patch fixes also the only user of this function (in xen sources) -
> >>>> libvchan.
> >>>
> >>> Looks ok to me but Daniel De Graaf wrote all this stuff, Ccing him.
> >>
> >> This also looks good to me.
> >
> > May I take that as an Ack (or a Reviewed-by if you prefer)?
> 
> Yes, either one is fine.
> 
> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Is the change from munmap to xc_gntshr_munmap, which wasn't mentioned in
the changelog description (tut tut), correct? It seems like these
mappings can either be establish with xc_gntshr_share_pages or with "=
((void*)ctrl->ring) + LARGE_RING_OFFSET", with the second one being the
case I'm concerned about... Should it not duplicate the switch used at
mapping time?

Ian.

Re: [PATCH] libxc: fix xc_gntshr_munmap semantic

[Daniel De Graaf]

On 04/30/2013 06:39 AM, Ian Campbell wrote:
> On Fri, 2013-04-26 at 17:17 +0100, Daniel De Graaf wrote:
>> On 04/26/2013 11:26 AM, Ian Campbell wrote:
>>> On Fri, 2013-04-26 at 16:15 +0100, Daniel De Graaf wrote:
>>>> On 04/26/2013 10:44 AM, Ian Campbell wrote:
>>>>> On Fri, 2013-04-26 at 13:40 +0100, Marek Marczykowski wrote:
>>>>>> "count" parameter should be pages count (as stated in comment in
>>>>>> xenctrl.h), not bytes count.
>>>>>> This patch fixes also the only user of this function (in xen sources) -
>>>>>> libvchan.
>>>>>
>>>>> Looks ok to me but Daniel De Graaf wrote all this stuff, Ccing him.
>>>>
>>>> This also looks good to me.
>>>
>>> May I take that as an Ack (or a Reviewed-by if you prefer)?
>>
>> Yes, either one is fine.
>>
>> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>
> Is the change from munmap to xc_gntshr_munmap, which wasn't mentioned in
> the changelog description (tut tut), correct? It seems like these
> mappings can either be establish with xc_gntshr_share_pages or with "=
> ((void*)ctrl->ring) + LARGE_RING_OFFSET", with the second one being the
> case I'm concerned about... Should it not duplicate the switch used at
> mapping time?
>
> Ian.
>

The unmap is only done if (ctrl->read.order >= PAGE_SHIFT), which is a
functional duplicate of the switch statement. However, this does bring
up a different issue: the munmap() should be replaced with the correct
one of xc_gntshr_munmap and xc_gnttab_munmap depending on ctrl->is_server,
in the same way as ctrl->ring is unmapped.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] libxc: fix xc_gntshr_munmap semantic

[Ian Campbell]

On Tue, 2013-04-30 at 14:21 +0100, Daniel De Graaf wrote:
> On 04/30/2013 06:39 AM, Ian Campbell wrote:
> > On Fri, 2013-04-26 at 17:17 +0100, Daniel De Graaf wrote:
> >> On 04/26/2013 11:26 AM, Ian Campbell wrote:
> >>> On Fri, 2013-04-26 at 16:15 +0100, Daniel De Graaf wrote:
> >>>> On 04/26/2013 10:44 AM, Ian Campbell wrote:
> >>>>> On Fri, 2013-04-26 at 13:40 +0100, Marek Marczykowski wrote:
> >>>>>> "count" parameter should be pages count (as stated in comment in
> >>>>>> xenctrl.h), not bytes count.
> >>>>>> This patch fixes also the only user of this function (in xen sources) -
> >>>>>> libvchan.
> >>>>>
> >>>>> Looks ok to me but Daniel De Graaf wrote all this stuff, Ccing him.
> >>>>
> >>>> This also looks good to me.
> >>>
> >>> May I take that as an Ack (or a Reviewed-by if you prefer)?
> >>
> >> Yes, either one is fine.
> >>
> >> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> >
> > Is the change from munmap to xc_gntshr_munmap, which wasn't mentioned in
> > the changelog description (tut tut), correct? It seems like these
> > mappings can either be establish with xc_gntshr_share_pages or with "=
> > ((void*)ctrl->ring) + LARGE_RING_OFFSET", with the second one being the
> > case I'm concerned about... Should it not duplicate the switch used at
> > mapping time?
> >
> > Ian.
> >
> 
> The unmap is only done if (ctrl->read.order >= PAGE_SHIFT), which is a
> functional duplicate of the switch statement.

It's a pretty opaque transformation ;-)

But for e.g. order 9, it isn't the same is it? The switch on alloc will
hit the default case while the free won't hit this if statement.

>  However, this does bring
> up a different issue: the munmap() should be replaced with the correct
> one of xc_gntshr_munmap and xc_gnttab_munmap depending on ctrl->is_server,
> in the same way as ctrl->ring is unmapped.

OK, I take it the Ack is rescinded for the time being?

Ian.

Re: [PATCH] libxc: fix xc_gntshr_munmap semantic

[Daniel De Graaf]

On 04/30/2013 10:00 AM, Ian Campbell wrote:
> On Tue, 2013-04-30 at 14:21 +0100, Daniel De Graaf wrote:
>> On 04/30/2013 06:39 AM, Ian Campbell wrote:
>>> On Fri, 2013-04-26 at 17:17 +0100, Daniel De Graaf wrote:
>>>> On 04/26/2013 11:26 AM, Ian Campbell wrote:
>>>>> On Fri, 2013-04-26 at 16:15 +0100, Daniel De Graaf wrote:
>>>>>> On 04/26/2013 10:44 AM, Ian Campbell wrote:
>>>>>>> On Fri, 2013-04-26 at 13:40 +0100, Marek Marczykowski wrote:
>>>>>>>> "count" parameter should be pages count (as stated in comment in
>>>>>>>> xenctrl.h), not bytes count.
>>>>>>>> This patch fixes also the only user of this function (in xen sources) -
>>>>>>>> libvchan.
>>>>>>>
>>>>>>> Looks ok to me but Daniel De Graaf wrote all this stuff, Ccing him.
>>>>>>
>>>>>> This also looks good to me.
>>>>>
>>>>> May I take that as an Ack (or a Reviewed-by if you prefer)?
>>>>
>>>> Yes, either one is fine.
>>>>
>>>> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>>
>>> Is the change from munmap to xc_gntshr_munmap, which wasn't mentioned in
>>> the changelog description (tut tut), correct? It seems like these
>>> mappings can either be establish with xc_gntshr_share_pages or with "=
>>> ((void*)ctrl->ring) + LARGE_RING_OFFSET", with the second one being the
>>> case I'm concerned about... Should it not duplicate the switch used at
>>> mapping time?
>>>
>>> Ian.
>>>
>>
>> The unmap is only done if (ctrl->read.order >= PAGE_SHIFT), which is a
>> functional duplicate of the switch statement.
>
> It's a pretty opaque transformation ;-)
>
> But for e.g. order 9, it isn't the same is it? The switch on alloc will
> hit the default case while the free won't hit this if statement.

Order 9 isn't valid: the only permitted values are 10, 11, and 12+, all of
which are handled correctly here.

>>   However, this does bring
>> up a different issue: the munmap() should be replaced with the correct
>> one of xc_gntshr_munmap and xc_gnttab_munmap depending on ctrl->is_server,
>> in the same way as ctrl->ring is unmapped.
>
> OK, I take it the Ack is rescinded for the time being?
>

The change is still a strict improvement over the old code, but since more
changes are needed to complete the fix, it is rescinded for now.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] libxc: fix xc_gntshr_munmap semantic

[Ian Campbell]

On Tue, 2013-04-30 at 15:19 +0100, Daniel De Graaf wrote:
> On 04/30/2013 10:00 AM, Ian Campbell wrote:
> > On Tue, 2013-04-30 at 14:21 +0100, Daniel De Graaf wrote:
> >> On 04/30/2013 06:39 AM, Ian Campbell wrote:
> >>> On Fri, 2013-04-26 at 17:17 +0100, Daniel De Graaf wrote:
> >>>> On 04/26/2013 11:26 AM, Ian Campbell wrote:
> >>>>> On Fri, 2013-04-26 at 16:15 +0100, Daniel De Graaf wrote:
> >>>>>> On 04/26/2013 10:44 AM, Ian Campbell wrote:
> >>>>>>> On Fri, 2013-04-26 at 13:40 +0100, Marek Marczykowski wrote:
> >>>>>>>> "count" parameter should be pages count (as stated in comment in
> >>>>>>>> xenctrl.h), not bytes count.
> >>>>>>>> This patch fixes also the only user of this function (in xen sources) -
> >>>>>>>> libvchan.
> >>>>>>>
> >>>>>>> Looks ok to me but Daniel De Graaf wrote all this stuff, Ccing him.
> >>>>>>
> >>>>>> This also looks good to me.
> >>>>>
> >>>>> May I take that as an Ack (or a Reviewed-by if you prefer)?
> >>>>
> >>>> Yes, either one is fine.
> >>>>
> >>>> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> >>>
> >>> Is the change from munmap to xc_gntshr_munmap, which wasn't mentioned in
> >>> the changelog description (tut tut), correct? It seems like these
> >>> mappings can either be establish with xc_gntshr_share_pages or with "=
> >>> ((void*)ctrl->ring) + LARGE_RING_OFFSET", with the second one being the
> >>> case I'm concerned about... Should it not duplicate the switch used at
> >>> mapping time?
> >>>
> >>> Ian.
> >>>
> >>
> >> The unmap is only done if (ctrl->read.order >= PAGE_SHIFT), which is a
> >> functional duplicate of the switch statement.
> >
> > It's a pretty opaque transformation ;-)
> >
> > But for e.g. order 9, it isn't the same is it? The switch on alloc will
> > hit the default case while the free won't hit this if statement.
> 
> Order 9 isn't valid: the only permitted values are 10, 11, and 12+, all of
> which are handled correctly here.

Ah, I missed the limit check just above, good.

> >>   However, this does bring
> >> up a different issue: the munmap() should be replaced with the correct
> >> one of xc_gntshr_munmap and xc_gnttab_munmap depending on ctrl->is_server,
> >> in the same way as ctrl->ring is unmapped.
> >
> > OK, I take it the Ack is rescinded for the time being?
> >
> 
> The change is still a strict improvement over the old code, but since more
> changes are needed to complete the fix, it is rescinded for now.

It might be best to get the mechanical API change as a separate patch
(e.g. without adding new callers of the function, correct or otherwise)
and handle the correction of the munmap calls separately.

Ian.