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

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

[Luis R. Rodriguez]

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>
---

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

 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;
+
     return p;
+out:
+    saved_errno = errno;
+    PERROR("xc_alloc_hypercall_buffer: madvise failed");
+    (void)munmap(p, size);
+    errno = saved_errno;
+    return NULL;
 }
 
 static void linux_privcmd_free_hypercall_buffer(xc_interface *xch, xc_osdep_handle h, void *ptr, int npages)
-- 
1.8.4.5

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

[Andrew Cooper]

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 .  Out
of interest, which bit starts failing given correct error handling here?

>
> BTW I see no ldconfig run after make install, where do we want to put it
> given we have a few libs ?
>
>  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.  Also, you can do
without 'rc' if you are happy putting the madvise() in the if statement
itself.  'rc' isn't subsequently referenced.

~Andrew

>      return p;
> +out:
> +    saved_errno = errno;
> +    PERROR("xc_alloc_hypercall_buffer: madvise failed");
> +    (void)munmap(p, size);
> +    errno = saved_errno;
> +    return NULL;
>  }
>  
>  static void linux_privcmd_free_hypercall_buffer(xc_interface *xch, xc_osdep_handle h, void *ptr, int npages)

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

[Luis R. Rodriguez]

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

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

[Ian Campbell]

On Thu, 2014-05-08 at 07:03 +0200, Luis R. Rodriguez wrote:
> > > 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?

Isn't this conventionally the admin's responsibility?

> 
> > >  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.

The out label idiom you have used is pretty conventional and will save
faff if some other operation gets added here. I'm happy with the patch
as it is.

> > 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

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

[Luis R. Rodriguez]

On Thu, May 8, 2014 at 3:15 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2014-05-08 at 07:03 +0200, Luis R. Rodriguez wrote:
>> > > 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?
>
> Isn't this conventionally the admin's responsibility?

If that is part of the documentation of requirements :)

>> > error cleanup;
>> >
>> >
>> > as it is small enough to do without an out; label.
>>
>> Sure, that makes sesnse.
>
> The out label idiom you have used is pretty conventional and will save
> faff if some other operation gets added here. I'm happy with the patch
> as it is.

OK feel free to take it then.

BTW any idea where in code we get the trigger for what strace picks up as:

1742  write(22, "b", 1)                 = 1

I'm now in a state where instead of b I get "/" written and I just
can't explain this yet.

  Luis

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

[Ian Campbell]

On Thu, 2014-05-08 at 03:34 -0700, Luis R. Rodriguez wrote:
> On Thu, May 8, 2014 at 3:15 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Thu, 2014-05-08 at 07:03 +0200, Luis R. Rodriguez wrote:
> >> > > 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?
> >
> > Isn't this conventionally the admin's responsibility?
> 
> If that is part of the documentation of requirements :)

http://wiki.xen.org/wiki/Compiling_Xen_From_Source
Ctrl-F
ldconfig
Profit.

> 
> >> > error cleanup;
> >> >
> >> >
> >> > as it is small enough to do without an out; label.
> >>
> >> Sure, that makes sesnse.
> >
> > The out label idiom you have used is pretty conventional and will save
> > faff if some other operation gets added here. I'm happy with the patch
> > as it is.
> 
> OK feel free to take it then.
> 
> BTW any idea where in code we get the trigger for what strace picks up as:
> 
> 1742  write(22, "b", 1)                 = 1

No idea. /proc/$pid/fds might tell you what fd 22 is, which might start
to give a clue.

> I'm now in a state where instead of b I get "/" written and I just
> can't explain this yet.
> 
>   Luis

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

[Ian Campbell]

On Thu, 2014-05-08 at 03:34 -0700, Luis R. Rodriguez wrote:
> On Thu, May 8, 2014 at 3:15 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > The out label idiom you have used is pretty conventional and will save
> > faff if some other operation gets added here. I'm happy with the patch
> > as it is.
> 
> OK feel free to take it then.

I hadn't noticed that there was a failure specific PERROR at the out
label location. For this to make sense it would need to be:

    rc = madvise(p, npages * XC_PAGE_SIZE, MADV_DONTFORK);
    if ( rc < 0 )
    {
        PERROR("xc_alloc_hypercall_buffer: madvise failed");
         goto out;
    }

    ....
 out:
    the cleanup

Ian.

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

[Luis R. Rodriguez]

On Mon, May 12, 2014 at 5:03 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> I hadn't noticed that there was a failure specific PERROR at the out
> label location. For this to make sense it would need to be:
>
>     rc = madvise(p, npages * XC_PAGE_SIZE, MADV_DONTFORK);
>     if ( rc < 0 )
>     {
>         PERROR("xc_alloc_hypercall_buffer: madvise failed");
>          goto out;
>     }
>
>     ....
>  out:
>     the cleanup


OK I'll move the PERROR() above, and will fold this into my series of
systemd patches as I have a second spin coming up.

  Luis