| Message ID | 1490181925-8026-1-git-send-email-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 22, 2017 at 11:25:25AM +0000, Paul Durrant wrote: > Commit 8ef5f344d061 "tools/libxendevicemodel: add a call to restrict the > handle" added a function to the devicemodel interface to restrict > operations through the API to a specific domain, where a capable under- > lying privcmd driver exists. > > This patch adds similar functionality to the xenforeignmemory API. This > will be necessary (as much as xendevicemodel restriction) for limiting > the scope of device models to specific domains. > > NOTE: My patch to the linux kernel [1] added the appropriate checks to > the foreign memory ioctls. > > [1] https://git.kernel.org/cgit/linux/kernel/git/ostr/linux.git/commit/?id=4610d240 > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Acked-by: Wei Liu <wei.liu2@citrix.com>
On 22/03/17 11:25, Paul Durrant wrote: > diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map b/tools/libs/foreignmemory/libxenforeignmemory.map > index df206b3..5c9806c 100644 > --- a/tools/libs/foreignmemory/libxenforeignmemory.map > +++ b/tools/libs/foreignmemory/libxenforeignmemory.map > @@ -4,5 +4,6 @@ VERS_1.0 { > xenforeignmemory_close; > xenforeignmemory_map; > xenforeignmemory_unmap; > + xenforeignmemory_restrict; > local: *; /* Do not expose anything by default */ > }; This isn't correct. New functions like this need to bump at least the SO minor version, like c/s f1446de4ba Fixing this is a blocker to releasing 4.9 Also, we should really get automatic ABI checking built into the release process, to prevent mistakes like this from happening accidentally. ~Andrew
> -----Original Message----- > From: Andrew Cooper > Sent: 24 March 2017 20:10 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org > Cc: Wei Liu <wei.liu2@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; > Julien Grall <julien.grall@arm.com> > Subject: Re: [Xen-devel] [PATCH] tools/libxenforeignmemory: add a call to > restrict the handle > > On 22/03/17 11:25, Paul Durrant wrote: > > diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map > b/tools/libs/foreignmemory/libxenforeignmemory.map > > index df206b3..5c9806c 100644 > > --- a/tools/libs/foreignmemory/libxenforeignmemory.map > > +++ b/tools/libs/foreignmemory/libxenforeignmemory.map > > @@ -4,5 +4,6 @@ VERS_1.0 { > > xenforeignmemory_close; > > xenforeignmemory_map; > > xenforeignmemory_unmap; > > + xenforeignmemory_restrict; > > local: *; /* Do not expose anything by default */ > > }; > > This isn't correct. > > New functions like this need to bump at least the SO minor version, like > c/s f1446de4ba > Sorry, yes, I forgot these libraries are supposed to be stable now. > Fixing this is a blocker to releasing 4.9 > I'll submit a patch on Monday. > Also, we should really get automatic ABI checking built into the release > process, to prevent mistakes like this from happening accidentally. > Yes, definitely. Paul > ~Andrew
> -----Original Message----- > From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of > Paul Durrant > Sent: 24 March 2017 20:24 > To: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > devel@lists.xenproject.org > Cc: Ian Jackson <Ian.Jackson@citrix.com>; Julien Grall > <julien.grall@arm.com>; Wei Liu <wei.liu2@citrix.com> > Subject: Re: [Xen-devel] [PATCH] tools/libxenforeignmemory: add a call to > restrict the handle > > > -----Original Message----- > > From: Andrew Cooper > > Sent: 24 March 2017 20:10 > > To: Paul Durrant <Paul.Durrant@citrix.com>; xen- > devel@lists.xenproject.org > > Cc: Wei Liu <wei.liu2@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; > > Julien Grall <julien.grall@arm.com> > > Subject: Re: [Xen-devel] [PATCH] tools/libxenforeignmemory: add a call to > > restrict the handle > > > > On 22/03/17 11:25, Paul Durrant wrote: > > > diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map > > b/tools/libs/foreignmemory/libxenforeignmemory.map > > > index df206b3..5c9806c 100644 > > > --- a/tools/libs/foreignmemory/libxenforeignmemory.map > > > +++ b/tools/libs/foreignmemory/libxenforeignmemory.map > > > @@ -4,5 +4,6 @@ VERS_1.0 { > > > xenforeignmemory_close; > > > xenforeignmemory_map; > > > xenforeignmemory_unmap; > > > + xenforeignmemory_restrict; > > > local: *; /* Do not expose anything by default */ > > > }; > > > > This isn't correct. > > > > New functions like this need to bump at least the SO minor version, like > > c/s f1446de4ba > > > > Sorry, yes, I forgot these libraries are supposed to be stable now. > > > Fixing this is a blocker to releasing 4.9 > > > > I'll submit a patch on Monday. Actually, on second thoughts, do we want to bump the version for all changes or just incompatible ones? I added a call here so it shouldn't break any older application compiling against this library. Paul > > > Also, we should really get automatic ABI checking built into the release > > process, to prevent mistakes like this from happening accidentally. > > > > Yes, definitely. > > Paul > > > ~Andrew > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
On 27/03/2017 09:06, Paul Durrant wrote: >> -----Original Message----- >> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of >> Paul Durrant >> Sent: 24 March 2017 20:24 >> To: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- >> devel@lists.xenproject.org >> Cc: Ian Jackson <Ian.Jackson@citrix.com>; Julien Grall >> <julien.grall@arm.com>; Wei Liu <wei.liu2@citrix.com> >> Subject: Re: [Xen-devel] [PATCH] tools/libxenforeignmemory: add a call to >> restrict the handle >> >>> -----Original Message----- >>> From: Andrew Cooper >>> Sent: 24 March 2017 20:10 >>> To: Paul Durrant <Paul.Durrant@citrix.com>; xen- >> devel@lists.xenproject.org >>> Cc: Wei Liu <wei.liu2@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; >>> Julien Grall <julien.grall@arm.com> >>> Subject: Re: [Xen-devel] [PATCH] tools/libxenforeignmemory: add a call to >>> restrict the handle >>> >>> On 22/03/17 11:25, Paul Durrant wrote: >>>> diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map >>> b/tools/libs/foreignmemory/libxenforeignmemory.map >>>> index df206b3..5c9806c 100644 >>>> --- a/tools/libs/foreignmemory/libxenforeignmemory.map >>>> +++ b/tools/libs/foreignmemory/libxenforeignmemory.map >>>> @@ -4,5 +4,6 @@ VERS_1.0 { >>>> xenforeignmemory_close; >>>> xenforeignmemory_map; >>>> xenforeignmemory_unmap; >>>> + xenforeignmemory_restrict; >>>> local: *; /* Do not expose anything by default */ >>>> }; >>> This isn't correct. >>> >>> New functions like this need to bump at least the SO minor version, like >>> c/s f1446de4ba >>> >> Sorry, yes, I forgot these libraries are supposed to be stable now. >> >>> Fixing this is a blocker to releasing 4.9 >>> >> I'll submit a patch on Monday. > Actually, on second thoughts, do we want to bump the version for all changes or just incompatible ones? I added a call here so it shouldn't break any older application compiling against this library. You must bump the versions; the version information gets embedded as part of the dynamic linkage of the application built against libxenforeignmemory. In particular, you need to prevent an application built now from referencing xenforeignmemory_restrict@1.0 because the version 1.0 shipped in all earlier versions of Xen lacked this function. Not bumping the version now will result in newer applications being built which the dynamic loader thinks will work with older versions of the library, rather than correctly identifying that a newer version of the library is required. As with other things in staging, it can get away with only being bumped once per Xen release, but it must always be bumped at the first change. ~Andrew
> -----Original Message----- > From: Andrew Cooper [mailto:amc96@hermes.cam.ac.uk] On Behalf Of > Andrew Cooper > Sent: 27 March 2017 09:16 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org > Cc: Ian Jackson <Ian.Jackson@citrix.com>; Julien Grall > <julien.grall@arm.com>; Wei Liu <wei.liu2@citrix.com> > Subject: Re: [Xen-devel] [PATCH] tools/libxenforeignmemory: add a call to > restrict the handle > > On 27/03/2017 09:06, Paul Durrant wrote: > >> -----Original Message----- > >> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of > >> Paul Durrant > >> Sent: 24 March 2017 20:24 > >> To: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > >> devel@lists.xenproject.org > >> Cc: Ian Jackson <Ian.Jackson@citrix.com>; Julien Grall > >> <julien.grall@arm.com>; Wei Liu <wei.liu2@citrix.com> > >> Subject: Re: [Xen-devel] [PATCH] tools/libxenforeignmemory: add a call > to > >> restrict the handle > >> > >>> -----Original Message----- > >>> From: Andrew Cooper > >>> Sent: 24 March 2017 20:10 > >>> To: Paul Durrant <Paul.Durrant@citrix.com>; xen- > >> devel@lists.xenproject.org > >>> Cc: Wei Liu <wei.liu2@citrix.com>; Ian Jackson > <Ian.Jackson@citrix.com>; > >>> Julien Grall <julien.grall@arm.com> > >>> Subject: Re: [Xen-devel] [PATCH] tools/libxenforeignmemory: add a call > to > >>> restrict the handle > >>> > >>> On 22/03/17 11:25, Paul Durrant wrote: > >>>> diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map > >>> b/tools/libs/foreignmemory/libxenforeignmemory.map > >>>> index df206b3..5c9806c 100644 > >>>> --- a/tools/libs/foreignmemory/libxenforeignmemory.map > >>>> +++ b/tools/libs/foreignmemory/libxenforeignmemory.map > >>>> @@ -4,5 +4,6 @@ VERS_1.0 { > >>>> xenforeignmemory_close; > >>>> xenforeignmemory_map; > >>>> xenforeignmemory_unmap; > >>>> + xenforeignmemory_restrict; > >>>> local: *; /* Do not expose anything by default */ > >>>> }; > >>> This isn't correct. > >>> > >>> New functions like this need to bump at least the SO minor version, like > >>> c/s f1446de4ba > >>> > >> Sorry, yes, I forgot these libraries are supposed to be stable now. > >> > >>> Fixing this is a blocker to releasing 4.9 > >>> > >> I'll submit a patch on Monday. > > Actually, on second thoughts, do we want to bump the version for all > changes or just incompatible ones? I added a call here so it shouldn't break > any older application compiling against this library. > > You must bump the versions; the version information gets embedded as > part of the dynamic linkage of the application built against > libxenforeignmemory. > > In particular, you need to prevent an application built now from > referencing xenforeignmemory_restrict@1.0 because the version 1.0 > shipped in all earlier versions of Xen lacked this function. > > Not bumping the version now will result in newer applications being > built which the dynamic loader thinks will work with older versions of > the library, rather than correctly identifying that a newer version of > the library is required. > > As with other things in staging, it can get away with only being bumped > once per Xen release, but it must always be bumped at the first change. > Ok. I'll send a patch today then. Paul > ~Andrew
Hi Andre, On 24/03/17 20:10, Andrew Cooper wrote: > On 22/03/17 11:25, Paul Durrant wrote: >> diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map b/tools/libs/foreignmemory/libxenforeignmemory.map >> index df206b3..5c9806c 100644 >> --- a/tools/libs/foreignmemory/libxenforeignmemory.map >> +++ b/tools/libs/foreignmemory/libxenforeignmemory.map >> @@ -4,5 +4,6 @@ VERS_1.0 { >> xenforeignmemory_close; >> xenforeignmemory_map; >> xenforeignmemory_unmap; >> + xenforeignmemory_restrict; >> local: *; /* Do not expose anything by default */ >> }; > > This isn't correct. > > New functions like this need to bump at least the SO minor version, like > c/s f1446de4ba > > Fixing this is a blocker to releasing 4.9 I saw a patch (commit 9a7fbdd692) merged today to fix this. Let me know if there are more patches coming to address this. > > Also, we should really get automatic ABI checking built into the release > process, to prevent mistakes like this from happening accidentally. That's a good idea. I will create a task on Jira for that. Cheers,
On 27/03/2017 19:03, Julien Grall wrote: > Hi Andre, > > On 24/03/17 20:10, Andrew Cooper wrote: >> On 22/03/17 11:25, Paul Durrant wrote: >>> diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map >>> b/tools/libs/foreignmemory/libxenforeignmemory.map >>> index df206b3..5c9806c 100644 >>> --- a/tools/libs/foreignmemory/libxenforeignmemory.map >>> +++ b/tools/libs/foreignmemory/libxenforeignmemory.map >>> @@ -4,5 +4,6 @@ VERS_1.0 { >>> xenforeignmemory_close; >>> xenforeignmemory_map; >>> xenforeignmemory_unmap; >>> + xenforeignmemory_restrict; >>> local: *; /* Do not expose anything by default */ >>> }; >> >> This isn't correct. >> >> New functions like this need to bump at least the SO minor version, like >> c/s f1446de4ba >> >> Fixing this is a blocker to releasing 4.9 > > I saw a patch (commit 9a7fbdd692) merged today to fix this. Let me > know if there are more patches coming to address this. That should be all to fix this specific issue. > >> >> Also, we should really get automatic ABI checking built into the release >> process, to prevent mistakes like this from happening accidentally. > > That's a good idea. I will create a task on Jira for that. https://xenproject.atlassian.net/projects/XEN/issues/XEN-33 is way ahead of you :) ~Andrew
diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c index a872b95..0ebd429 100644 --- a/tools/libs/foreignmemory/core.c +++ b/tools/libs/foreignmemory/core.c @@ -106,6 +106,12 @@ int xenforeignmemory_unmap(xenforeignmemory_handle *fmem, return osdep_xenforeignmemory_unmap(fmem, addr, num); } +int xenforeignmemory_restrict(xenforeignmemory_handle *fmem, + domid_t domid) +{ + return osdep_xenforeignmemory_restrict(fmem, domid); +} + /* * Local variables: * mode: C diff --git a/tools/libs/foreignmemory/freebsd.c b/tools/libs/foreignmemory/freebsd.c index ef08b6c..f6cd08c 100644 --- a/tools/libs/foreignmemory/freebsd.c +++ b/tools/libs/foreignmemory/freebsd.c @@ -96,6 +96,13 @@ int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem, return munmap(addr, num << PAGE_SHIFT); } +int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem, + domid_t domid) +{ + errno = -EOPNOTSUPP; + return -1; +} + /* * Local variables: * mode: C diff --git a/tools/libs/foreignmemory/include/xenforeignmemory.h b/tools/libs/foreignmemory/include/xenforeignmemory.h index 92b9277..d5be648 100644 --- a/tools/libs/foreignmemory/include/xenforeignmemory.h +++ b/tools/libs/foreignmemory/include/xenforeignmemory.h @@ -115,6 +115,17 @@ void *xenforeignmemory_map(xenforeignmemory_handle *fmem, uint32_t dom, int xenforeignmemory_unmap(xenforeignmemory_handle *fmem, void *addr, size_t pages); +/** + * This function restricts the use of this handle to the specified + * domain. + * + * @parm fmem handle to the open foreignmemory interface + * @parm domid the domain id + * @return 0 on success, -1 on failure. + */ +int xenforeignmemory_restrict(xenforeignmemory_handle *fmem, + domid_t domid); + #endif /* diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map b/tools/libs/foreignmemory/libxenforeignmemory.map index df206b3..5c9806c 100644 --- a/tools/libs/foreignmemory/libxenforeignmemory.map +++ b/tools/libs/foreignmemory/libxenforeignmemory.map @@ -4,5 +4,6 @@ VERS_1.0 { xenforeignmemory_close; xenforeignmemory_map; xenforeignmemory_unmap; + xenforeignmemory_restrict; local: *; /* Do not expose anything by default */ }; diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c index 423c744..320bb21 100644 --- a/tools/libs/foreignmemory/linux.c +++ b/tools/libs/foreignmemory/linux.c @@ -272,6 +272,12 @@ int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem, return munmap(addr, num << PAGE_SHIFT); } +int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem, + domid_t domid) +{ + return ioctl(fmem->fd, IOCTL_PRIVCMD_RESTRICT, &domid); +} + /* * Local variables: * mode: C diff --git a/tools/libs/foreignmemory/minios.c b/tools/libs/foreignmemory/minios.c index 6dc97bd..2dd4910 100644 --- a/tools/libs/foreignmemory/minios.c +++ b/tools/libs/foreignmemory/minios.c @@ -58,6 +58,13 @@ int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem, return munmap(addr, num << PAGE_SHIFT); } +int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem, + domid_t domid) +{ + errno = -EOPNOTSUPP; + return -1; +} + /* * Local variables: * mode: C diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c index 08f4964..af3a1a4 100644 --- a/tools/libs/foreignmemory/netbsd.c +++ b/tools/libs/foreignmemory/netbsd.c @@ -100,6 +100,13 @@ int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem, return munmap(addr, num*XC_PAGE_SIZE); } +int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem, + domid_t domid) +{ + errno = -EOPNOTSUPP; + return -1; +} + /* * Local variables: * mode: C diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h index 9cc7814..ed7ec7a 100644 --- a/tools/libs/foreignmemory/private.h +++ b/tools/libs/foreignmemory/private.h @@ -32,6 +32,9 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem, int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem, void *addr, size_t num); +int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem, + domid_t domid); + #if defined(__NetBSD__) || defined(__sun__) /* Strictly compat for those two only only */ void *compat_mapforeign_batch(xenforeignmem_handle *fmem, uint32_t dom, diff --git a/tools/libs/foreignmemory/solaris.c b/tools/libs/foreignmemory/solaris.c index e925a29..fe7bb45 100644 --- a/tools/libs/foreignmemory/solaris.c +++ b/tools/libs/foreignmemory/solaris.c @@ -98,6 +98,13 @@ int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem, return munmap(addr, num*XC_PAGE_SIZE); } +int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem, + domid_t domid) +{ + errno = -EOPNOTSUPP; + return -1; +} + /* * Local variables: * mode: C
Commit 8ef5f344d061 "tools/libxendevicemodel: add a call to restrict the handle" added a function to the devicemodel interface to restrict operations through the API to a specific domain, where a capable under- lying privcmd driver exists. This patch adds similar functionality to the xenforeignmemory API. This will be necessary (as much as xendevicemodel restriction) for limiting the scope of device models to specific domains. NOTE: My patch to the linux kernel [1] added the appropriate checks to the foreign memory ioctls. [1] https://git.kernel.org/cgit/linux/kernel/git/ostr/linux.git/commit/?id=4610d240 Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> --- tools/libs/foreignmemory/core.c | 6 ++++++ tools/libs/foreignmemory/freebsd.c | 7 +++++++ tools/libs/foreignmemory/include/xenforeignmemory.h | 11 +++++++++++ tools/libs/foreignmemory/libxenforeignmemory.map | 1 + tools/libs/foreignmemory/linux.c | 6 ++++++ tools/libs/foreignmemory/minios.c | 7 +++++++ tools/libs/foreignmemory/netbsd.c | 7 +++++++ tools/libs/foreignmemory/private.h | 3 +++ tools/libs/foreignmemory/solaris.c | 7 +++++++ 9 files changed, 55 insertions(+)