* bug in xc_gntshr_munmap?
@ 2013-04-26 11:07 Marek Marczykowski
2013-04-26 13:34 ` Ian Campbell
0 siblings, 1 reply; 15+ messages in thread
From: Marek Marczykowski @ 2013-04-26 11:07 UTC (permalink / raw)
To: xen-devel@lists.xen.org
[-- 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: bug in xc_gntshr_munmap? 2013-04-26 11:07 bug in xc_gntshr_munmap? Marek Marczykowski @ 2013-04-26 13:34 ` Ian Campbell 2013-04-26 13:41 ` Marek Marczykowski 0 siblings, 1 reply; 15+ messages in thread From: Ian Campbell @ 2013-04-26 13:34 UTC (permalink / raw) To: Marek Marczykowski; +Cc: xen-devel@lists.xen.org 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: bug in xc_gntshr_munmap? 2013-04-26 13:34 ` Ian Campbell @ 2013-04-26 13:41 ` Marek Marczykowski 2013-04-26 12:40 ` [PATCH] libxc: fix xc_gntshr_munmap semantic Marek Marczykowski 2013-04-26 13:49 ` bug in xc_gntshr_munmap? Ian Campbell 0 siblings, 2 replies; 15+ messages in thread From: Marek Marczykowski @ 2013-04-26 13:41 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel@lists.xen.org [-- 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] libxc: fix xc_gntshr_munmap semantic 2013-04-26 13:41 ` Marek Marczykowski @ 2013-04-26 12:40 ` Marek Marczykowski 2013-04-26 14:01 ` Marek Marczykowski 2013-04-26 14:44 ` Ian Campbell 2013-04-26 13:49 ` bug in xc_gntshr_munmap? Ian Campbell 1 sibling, 2 replies; 15+ messages in thread From: Marek Marczykowski @ 2013-04-26 12:40 UTC (permalink / raw) To: xen-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] libxc: fix xc_gntshr_munmap semantic 2013-04-26 12:40 ` [PATCH] libxc: fix xc_gntshr_munmap semantic Marek Marczykowski @ 2013-04-26 14:01 ` Marek Marczykowski 2013-04-26 14:44 ` Ian Campbell 1 sibling, 0 replies; 15+ messages in thread From: Marek Marczykowski @ 2013-04-26 14:01 UTC (permalink / raw) To: Marek Marczykowski; +Cc: Ian Campbell, xen-devel [-- 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libxc: fix xc_gntshr_munmap semantic 2013-04-26 12:40 ` [PATCH] libxc: fix xc_gntshr_munmap semantic Marek Marczykowski 2013-04-26 14:01 ` Marek Marczykowski @ 2013-04-26 14:44 ` Ian Campbell 2013-04-26 15:15 ` Daniel De Graaf 1 sibling, 1 reply; 15+ messages in thread From: Ian Campbell @ 2013-04-26 14:44 UTC (permalink / raw) To: Marek Marczykowski; +Cc: Daniel De Graaf, xen-devel@lists.xen.org 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 = { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libxc: fix xc_gntshr_munmap semantic 2013-04-26 14:44 ` Ian Campbell @ 2013-04-26 15:15 ` Daniel De Graaf 2013-04-26 15:26 ` Ian Campbell 0 siblings, 1 reply; 15+ messages in thread From: Daniel De Graaf @ 2013-04-26 15:15 UTC (permalink / raw) To: Ian Campbell, Marek Marczykowski; +Cc: xen-devel@lists.xen.org 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libxc: fix xc_gntshr_munmap semantic 2013-04-26 15:15 ` Daniel De Graaf @ 2013-04-26 15:26 ` Ian Campbell 2013-04-26 16:17 ` Daniel De Graaf 0 siblings, 1 reply; 15+ messages in thread From: Ian Campbell @ 2013-04-26 15:26 UTC (permalink / raw) To: Daniel De Graaf; +Cc: Marek Marczykowski, xen-devel@lists.xen.org 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 = { > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libxc: fix xc_gntshr_munmap semantic 2013-04-26 15:26 ` Ian Campbell @ 2013-04-26 16:17 ` Daniel De Graaf 2013-04-30 10:39 ` Ian Campbell 0 siblings, 1 reply; 15+ messages in thread From: Daniel De Graaf @ 2013-04-26 16:17 UTC (permalink / raw) To: Ian Campbell; +Cc: Marek Marczykowski, xen-devel@lists.xen.org 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 = { >>> >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libxc: fix xc_gntshr_munmap semantic 2013-04-26 16:17 ` Daniel De Graaf @ 2013-04-30 10:39 ` Ian Campbell 2013-04-30 13:21 ` Daniel De Graaf 0 siblings, 1 reply; 15+ messages in thread From: Ian Campbell @ 2013-04-30 10:39 UTC (permalink / raw) To: Daniel De Graaf; +Cc: Marek Marczykowski, xen-devel@lists.xen.org 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libxc: fix xc_gntshr_munmap semantic 2013-04-30 10:39 ` Ian Campbell @ 2013-04-30 13:21 ` Daniel De Graaf 2013-04-30 14:00 ` Ian Campbell 0 siblings, 1 reply; 15+ messages in thread From: Daniel De Graaf @ 2013-04-30 13:21 UTC (permalink / raw) To: Ian Campbell; +Cc: Marek Marczykowski, xen-devel@lists.xen.org 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libxc: fix xc_gntshr_munmap semantic 2013-04-30 13:21 ` Daniel De Graaf @ 2013-04-30 14:00 ` Ian Campbell 2013-04-30 14:19 ` Daniel De Graaf 0 siblings, 1 reply; 15+ messages in thread From: Ian Campbell @ 2013-04-30 14:00 UTC (permalink / raw) To: Daniel De Graaf; +Cc: Marek Marczykowski, xen-devel@lists.xen.org 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libxc: fix xc_gntshr_munmap semantic 2013-04-30 14:00 ` Ian Campbell @ 2013-04-30 14:19 ` Daniel De Graaf 2013-04-30 14:29 ` Ian Campbell 0 siblings, 1 reply; 15+ messages in thread From: Daniel De Graaf @ 2013-04-30 14:19 UTC (permalink / raw) To: Ian Campbell; +Cc: Marek Marczykowski, xen-devel@lists.xen.org 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] libxc: fix xc_gntshr_munmap semantic 2013-04-30 14:19 ` Daniel De Graaf @ 2013-04-30 14:29 ` Ian Campbell 0 siblings, 0 replies; 15+ messages in thread From: Ian Campbell @ 2013-04-30 14:29 UTC (permalink / raw) To: Daniel De Graaf; +Cc: Marek Marczykowski, xen-devel@lists.xen.org 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: bug in xc_gntshr_munmap? 2013-04-26 13:41 ` Marek Marczykowski 2013-04-26 12:40 ` [PATCH] libxc: fix xc_gntshr_munmap semantic Marek Marczykowski @ 2013-04-26 13:49 ` Ian Campbell 1 sibling, 0 replies; 15+ messages in thread From: Ian Campbell @ 2013-04-26 13:49 UTC (permalink / raw) To: Marek Marczykowski; +Cc: xen-devel@lists.xen.org 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. > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2013-04-30 14:29 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-26 11:07 bug in xc_gntshr_munmap? Marek Marczykowski 2013-04-26 13:34 ` Ian Campbell 2013-04-26 13:41 ` Marek Marczykowski 2013-04-26 12:40 ` [PATCH] libxc: fix xc_gntshr_munmap semantic Marek Marczykowski 2013-04-26 14:01 ` Marek Marczykowski 2013-04-26 14:44 ` Ian Campbell 2013-04-26 15:15 ` Daniel De Graaf 2013-04-26 15:26 ` Ian Campbell 2013-04-26 16:17 ` Daniel De Graaf 2013-04-30 10:39 ` Ian Campbell 2013-04-30 13:21 ` Daniel De Graaf 2013-04-30 14:00 ` Ian Campbell 2013-04-30 14:19 ` Daniel De Graaf 2013-04-30 14:29 ` Ian Campbell 2013-04-26 13:49 ` bug in xc_gntshr_munmap? Ian Campbell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.