Re: [Xen-devel] [PATCH RFC 03/14] libxc: Fix xc_domain_get_tsc_info returning -Exx instead of -1.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Sat, Mar 14, 2015 at 07:55:39AM +0000, Ian Campbell wrote:
> On Fri, 2015-03-13 at 15:40 -0400, Konrad Rzeszutek Wilk wrote:
> > On Fri, Mar 13, 2015 at 03:31:12PM -0400, Konrad Rzeszutek Wilk wrote:
> > > Instead put the -Exx in errno.
> > > 
> > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > > ---
> > >  tools/libxc/xc_domain.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> > > index 845d1d7..eb3a5f2 100644
> > > --- a/tools/libxc/xc_domain.c
> > > +++ b/tools/libxc/xc_domain.c
> > > @@ -771,8 +771,10 @@ int xc_domain_get_tsc_info(xc_interface *xch,
> > >  
> > >      info = xc_hypercall_buffer_alloc(xch, info, sizeof(*info));
> > >      if ( info == NULL )
> > > -        return -ENOMEM;
> > > -
> > > +    {
> > > +        errno = ENOMEM;
> > > +        return -1;
> > > +    }
> > 
> > While I was doing that I saw a lot of other code doing:
> > 
> > 
> > if ( xc_hypercall_bounce_pre (..) )
> >     return -1;
> > 
> > Should they all be fixed up to do:
> >     'errno = ENOMEM'
> > 
> > before returning -1?
> 
> I think xc_hypercall_bounce_pre should probably set an appropriate errno
> on failure rather than making all the callers have to think about it. It

Doh!

> may indirectly already do so via mmap? I didn't actually go look...

Ugh. We seem to be doing pthread manipulation which can return values
(which we don't check) and put values in errno.

Then the 'mmap' which can fail with an variety of errno values.
Ditto for 'madvise'.

I think we are actually safe - we do put in errno values in.


This patch should be more of removing the 'errno' and just return -1.


Hm, the pthread manipulation is worrying. We do a lot of:

 rc = some_hypercall[]
 (Linux stashes the errnor value in)

 xc_hypercall_bounce_post(..)
 [if hte pthread fails it will stash an errno value in!]


And we end up over-writting the proper errno value (say EACCESS!).

This ought to do it:

commit 45dc0277aed59a891e8bae7bab52dfaf96b5b308
Author: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date:   Mon Mar 16 09:57:05 2015 -0400

    libxl: Propagate errno from hypercall instead of anything else.
    
    After we have done the hypercall - the errno has the failure
    code. However our usage of pthread and munmap can trigger them
    to manipulate the errno with their failure values. That would
    be bad as what we care about is just the hypercall error value.
    
    Another solution to this would be to save the 'errno' from
    pthread/munmap/madvise as an extra parameter to be analyzed
    later. However the call-sites above us do not care about it.
    
    Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

diff --git a/tools/libxc/xc_freebsd_osdep.c b/tools/libxc/xc_freebsd_osdep.c
index 151d3bf..5ada50d 100644
--- a/tools/libxc/xc_freebsd_osdep.c
+++ b/tools/libxc/xc_freebsd_osdep.c
@@ -125,10 +125,12 @@ static void 
freebsd_privcmd_free_hypercall_buffer(xc_interface *xch,
                                                   int npages)
 {
 
+    int saved_errno = errno;
     /* Unlock pages */
     munlock(ptr, npages * XC_PAGE_SIZE);
 
     munmap(ptr, npages * XC_PAGE_SIZE);
+    errno = saved_errno;
 }
 
 static int freebsd_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h,
diff --git a/tools/libxc/xc_hcall_buf.c b/tools/libxc/xc_hcall_buf.c
index e762a93..b3b5416 100644
--- a/tools/libxc/xc_hcall_buf.c
+++ b/tools/libxc/xc_hcall_buf.c
@@ -33,16 +33,24 @@ pthread_mutex_t hypercall_buffer_cache_mutex = 
PTHREAD_MUTEX_INITIALIZER;
 
 static void hypercall_buffer_cache_lock(xc_interface *xch)
 {
+    int saved_errno;
     if ( xch->flags & XC_OPENFLAG_NON_REENTRANT )
         return;
+    saved_errno = errno;
     pthread_mutex_lock(&hypercall_buffer_cache_mutex);
+    /* Ignore the pthread errors. */
+    errno = saved_errno;
 }
 
 static void hypercall_buffer_cache_unlock(xc_interface *xch)
 {
+    int saved_errno;
     if ( xch->flags & XC_OPENFLAG_NON_REENTRANT )
         return;
+    saved_errno = errno;
     pthread_mutex_unlock(&hypercall_buffer_cache_mutex);
+    /* Ignore the pthread errors. */
+    errno = saved_errno;
 }
 
 static void *hypercall_buffer_cache_alloc(xc_interface *xch, int nr_pages)
diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
index a19e4b6..15d772b 100644
--- a/tools/libxc/xc_linux_osdep.c
+++ b/tools/libxc/xc_linux_osdep.c
@@ -122,10 +122,13 @@ out:
 
 static void linux_privcmd_free_hypercall_buffer(xc_interface *xch, 
xc_osdep_handle h, void *ptr, int npages)
 {
+    int saved_errno = errno;
     /* Recover the VMA flags. Maybe it's not necessary */
     madvise(ptr, npages * XC_PAGE_SIZE, MADV_DOFORK);
 
     munmap(ptr, npages * XC_PAGE_SIZE);
+    /* We MUST propagate the hypercall errno, not unmap calls. */
+    errno = saved_errno;
 }
 
 static int linux_privcmd_hypercall(xc_interface *xch, xc_osdep_handle h, 
privcmd_hypercall_t *hypercall)
> 
> 
> Ian.
> 

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