From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Luis R. Rodriguez" Subject: Re: [RFC] libxc: check return values on mmap() and madvise() on xc_alloc_hypercall_buffer() Date: Thu, 8 May 2014 07:03:20 +0200 Message-ID: <20140508050320.GD13289@wotan.suse.de> References: <1399499244-4421-1-git-send-email-mcgrof@do-not-panic.com> <536AAF3F.3070700@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WiGUN-0007c1-BO for xen-devel@lists.xenproject.org; Thu, 08 May 2014 05:03:23 +0000 Content-Disposition: inline In-Reply-To: <536AAF3F.3070700@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: xen-devel@lists.xenproject.org, Stefano Stabellini , Ian Jackson , Ian Campbell , "Luis R. Rodriguez" List-Id: xen-devel@lists.xenproject.org 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" > > > > 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 > > > > 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 > > Cc: Stefano Stabellini > > Cc: Ian Campbell > > Signed-off-by: Luis R. Rodriguez > > --- > > 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 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 3892 rt_sigaction(SIGPIPE, {SIG_IGN, [], SA_RESTORER, 0x7fdd47457880}, 3905 <... write resumed> ) = 1 3892 <... rt_sigaction resumed> {SIG_IGN, ~[KILL STOP RTMIN RT_1], SA_RESTORER, 0x7fdd47457880}, 8) = 0 3905 read(20, 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 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, 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