Re: [RFC] libxc: check return values on mmap() and madvise() on xc_alloc_hypercall_buffer()

["Luis R. Rodriguez" <mcgrof@suse.com>]

On Wed, May 07, 2014 at 11:10:07PM +0100, Andrew Cooper wrote:
> On 07/05/2014 22:47, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >
> > On a Thinkpad T4440p with OpenSUSE tumbleweed with v3.15-rc4
> > and today's latest xen tip from the git tree strace -f reveals
> > we end up on a never ending wait shortly after
> >
> > write(20, "backend/console/5\0", 18 <unfinished ...>
> >
> > I've tracked this down to a lack of error return values on mmap() and
> > madvise() on xc_alloc_hypercall_buffer(). This moves us forward.
> >
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> > ---
> 
> Good catch.  I am supprised this hasn't blown up in someones face .

Tell me about it.

>  Out of interest, which bit starts failing given correct error handling here?

The next part that fails is:

1742  write(22, "b", 1)                 = 1                                     
1742  read(20, "\4\0\0\0\0\0\0\0\0\0\0\0\3\0\0\0", 16) = 16                     
1742  read(20, "OK\0", 3)               = 3                                     
1735  futex(0x7f0f65173770, FUTEX_WAKE_PRIVATE, 1) = 0                          
1735  rt_sigaction(SIGPIPE, {SIG_IGN, ~[KILL STOP RTMIN RT_1], SA_RESTORER, 0x7f0f5f1cb9f0}, NULL, 8) = 0
1735  rt_sigaction(SIGPIPE, {SIG_IGN, [], SA_RESTORER, 0x7f0f5f1cb9f0}, {SIG_IGN, ~[KILL STOP RTMIN RT_1], SA_RESTORER, 0x7f0f5f1cb9f0}, 8) = 0
1735  write(20, "\1\0\0\0\0\0\0\0\0\0\0\0\22\0\0\0", 16 <unfinished ...>        
1734  <... wait4 resumed> 0x7fffa73a89ac, 0, NULL) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
1734  --- SIGWINCH {si_signo=SIGWINCH, si_code=SI_KERNEL} ---                   
1734  wait4(1735,  

On a working system this looks like:

3905  write(22, "b", 1 <unfinished ...>                                         
3892  rt_sigaction(SIGPIPE, {SIG_IGN, [], SA_RESTORER, 0x7fdd47457880},  <unfinished ...>
3905  <... write resumed> )             = 1                                     
3892  <... rt_sigaction resumed> {SIG_IGN, ~[KILL STOP RTMIN RT_1], SA_RESTORER, 0x7fdd47457880}, 8) = 0
3905  read(20,  <unfinished ...>                                                
3892  write(20, "\1\0\0\0\0\0\0\0\0\0\0\0\22\0\0\0", 16) = 16                   
3892  write(20, "backend/console/1\0", 18) = 18                                 
3905  <... read resumed> "\1\0\0\0\0\0\0\0\0\0\0\0\2\0\0\0", 16) = 16           
3892  futex(0x7fdd4dbc213c, FUTEX_WAIT_PRIVATE, 3, NULL <unfinished ...>        
3905  read(20, "0\0", 2)                = 2                                     
3905  futex(0x7fdd4dbc213c, FUTEX_WAKE_OP_PRIVATE, 1, 1, 0x7fdd4dbc2138, {FUTEX_OP_SET, 0, FUTEX_OP_CMP_GT, 1}) = 1
3892  <... futex resumed> )             = 0                                     
3905  read(20,  <unfinished ...>                                                
3892  futex(0x7fdd4dbc2110, FUTEX_WAKE_PRIVATE, 1) = 0                          
3892  rt_sigaction(SIGPIPE, {SIG_IGN, ~[KILL STOP RTMIN RT_1], SA_RESTORER, 0x7fdd47457880}, NULL, 8) = 0
3892  ioctl(2, SNDCTL_TMR_TIMEBASE or TCGETS, 0x7fff1c0e1b10) = -1 ENOTTY (Inappropriate ioctl for device)

The 1735 process should be qemu, so it ends up getting stuck on the infinite loop
on libxl__spawn_spawn() waiting for qemu. Its unclear why it doesn't end.

> > BTW I see no ldconfig run after make install, where do we want to put it
> > given we have a few libs ?

How about this?

> >  tools/libxc/xc_linux_osdep.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> >
> > diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
> > index 73860a2..32e5332 100644
> > --- a/tools/libxc/xc_linux_osdep.c
> > +++ b/tools/libxc/xc_linux_osdep.c
> > @@ -92,14 +92,29 @@ static void *linux_privcmd_alloc_hypercall_buffer(xc_interface *xch, xc_osdep_ha
> >  {
> >      size_t size = npages * XC_PAGE_SIZE;
> >      void *p;
> > +    int rc, saved_errno;
> >  
> >      /* Address returned by mmap is page aligned. */
> >      p = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_LOCKED, -1, 0);
> > +    if ( p == MAP_FAILED )
> > +    {
> > +        PERROR("xc_alloc_hypercall_buffer: mmap failed");
> > +        return NULL;
> > +    }
> >  
> >      /* Do not copy the VMA to child process on fork. Avoid the page being COW
> >          on hypercall. */
> > -    madvise(p, npages * XC_PAGE_SIZE, MADV_DONTFORK);
> > +    rc = madvise(p, npages * XC_PAGE_SIZE, MADV_DONTFORK);
> > +    if ( rc < 0 )
> > +	    goto out;
> > +
> 
> This might be cleaner like:
> 
> if ( rc == 0 )
>     return p;
> 
> error cleanup;
>
> 
> as it is small enough to do without an out; label.

Sure, that makes sesnse.

> Also, you can do without 'rc' if you are happy putting the madvise() in the if statement
> itself.  'rc' isn't subsequently referenced.

OK.

  Luis