[Qemu-devel] [PATCH] linux-user: implement pipe2 syscall

[Qemu-devel] [PATCH] linux-user: implement pipe2 syscall

[Riku Voipio]

implement pipe2 syscall. instead of calling pipe2 directly
(which was introduced in 2.6.27), emulate the flag functionality
with fcntl.

Signed-off-by: Riku Voipio <riku.voipio@iki.fi>
---
 linux-user/syscall.c |   71 +++++++++++++++++++++++++++++++++++++------------
 1 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1096bb1..12ae5dc 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -943,6 +943,53 @@ static abi_long do_select(int n,
 
     return ret;
 }
+static abi_long pipe_set_flag(int fd, int readcmd, int writecmd, long newflag)
+{
+    int flags = fcntl(fd, readcmd);
+    if (flags<0)
+        return get_errno(flags);
+    flags |= newflag;
+    flags = fcntl(fd, writecmd, flags);
+    return get_errno(flags);
+}
+
+static abi_long do_pipe(int pipedes, int flags)
+{
+    int host_pipe[2];
+    abi_long ret;
+    ret = pipe(host_pipe);
+    if (is_error(ret))
+        return get_errno(ret);
+#if defined(TARGET_MIPS)
+    CPUMIPSState *env = (CPUMIPSState*)cpu_env;
+    env->active_tc.gpr[3] = host_pipe[1];
+    ret = host_pipe[0];
+#elif defined(TARGET_SH4)
+    ((CPUSH4State*)cpu_env)->gregs[1] = host_pipe[1];
+    ret = host_pipe[0];
+#else
+    if (put_user_s32(host_pipe[0], pipedes)
+        || put_user_s32(host_pipe[1], pipedes + sizeof(host_pipe[0])))
+        return -TARGET_EFAULT;
+#endif
+    if (flags & O_NONBLOCK) {
+        ret = pipe_set_flag(host_pipe[0], F_GETFL, F_SETFL, O_NONBLOCK);
+        if (is_error(ret))
+            return get_errno(ret);
+        ret = pipe_set_flag(host_pipe[1], F_GETFL, F_SETFL, O_NONBLOCK);
+        if (is_error(ret))
+            return get_errno(ret);
+    }
+    if (flags & O_CLOEXEC) {
+        ret = pipe_set_flag(host_pipe[0], F_GETFD, F_SETFD, FD_CLOEXEC);
+        if (is_error(ret))
+            return get_errno(ret);
+        ret = pipe_set_flag(host_pipe[1], F_GETFD, F_SETFD, FD_CLOEXEC);
+        if (is_error(ret))
+            return get_errno(ret);
+    }
+    return get_errno(ret);
+}
 
 static inline abi_long target_to_host_ip_mreq(struct ip_mreqn *mreqn,
                                               abi_ulong target_addr,
@@ -4525,25 +4572,13 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         ret = get_errno(dup(arg1));
         break;
     case TARGET_NR_pipe:
-        {
-            int host_pipe[2];
-            ret = get_errno(pipe(host_pipe));
-            if (!is_error(ret)) {
-#if defined(TARGET_MIPS)
-                CPUMIPSState *env = (CPUMIPSState*)cpu_env;
-		env->active_tc.gpr[3] = host_pipe[1];
-		ret = host_pipe[0];
-#elif defined(TARGET_SH4)
-		((CPUSH4State*)cpu_env)->gregs[1] = host_pipe[1];
-		ret = host_pipe[0];
-#else
-                if (put_user_s32(host_pipe[0], arg1)
-                    || put_user_s32(host_pipe[1], arg1 + sizeof(host_pipe[0])))
-                    goto efault;
-#endif
-            }
-        }
+        ret = do_pipe(arg1, 0);
+        break;
+#ifdef TARGET_NR_pipe2
+    case TARGET_NR_pipe2:
+        ret = do_pipe(arg1, arg2);
         break;
+#endif
     case TARGET_NR_times:
         {
             struct target_tms *tmsp;
-- 
1.6.2.1

[Qemu-devel] [PATCH] linux-user: implement pipe2 syscall [v2]

[Riku Voipio]

On Tue, May 05, 2009 at 04:30:48PM +0300, Riku Voipio wrote:
> implement pipe2 syscall. instead of calling pipe2 directly
> (which was introduced in 2.6.27), emulate the flag functionality
> with fcntl.

and badly tested from me. failed to build on mips and sh4 targets.

>From bf49a705ddbec1821163c54f326782b924729310 Mon Sep 17 00:00:00 2001
From: Riku Voipio <riku.voipio@iki.fi>
Date: Tue, 5 May 2009 12:10:04 +0300
Subject: [PATCH] linux-user: implement pipe2 [v2]

implement pipe2 syscall. instead of calling pipe2 directly
(which was introduced in 2.6.27), emulate the flag functionality
with fcntl.

[v2] fix do_pipe build on mips and sh4

Signed-off-by: Riku Voipio <riku.voipio@iki.fi>
---
 linux-user/syscall.c |   70 +++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1096bb1..e3cd185 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -943,6 +943,52 @@ static abi_long do_select(int n,
 
     return ret;
 }
+static abi_long pipe_set_flag(int fd, int readcmd, int writecmd, long newflag)
+{
+    int flags = fcntl(fd, readcmd);
+    if (flags<0)
+        return get_errno(flags);
+    flags |= newflag;
+    flags = fcntl(fd, writecmd, flags);
+    return get_errno(flags);
+}
+
+static abi_long do_pipe(void *cpu_env, int pipedes, int flags)
+{
+    int host_pipe[2];
+    abi_long ret;
+    ret = pipe(host_pipe);
+    if (is_error(ret))
+        return get_errno(ret);
+#if defined(TARGET_MIPS)
+    ((CPUMIPSState*)cpu_env)->active_tc.gpr[3] = host_pipe[1];
+    ret = host_pipe[0];
+#elif defined(TARGET_SH4)
+    ((CPUSH4State*)cpu_env)->gregs[1] = host_pipe[1];
+    ret = host_pipe[0];
+#else
+    if (put_user_s32(host_pipe[0], pipedes)
+        || put_user_s32(host_pipe[1], pipedes + sizeof(host_pipe[0])))
+        return -TARGET_EFAULT;
+#endif
+    if (flags & O_NONBLOCK) {
+        ret = pipe_set_flag(host_pipe[0], F_GETFL, F_SETFL, O_NONBLOCK);
+        if (is_error(ret))
+            return get_errno(ret);
+        ret = pipe_set_flag(host_pipe[1], F_GETFL, F_SETFL, O_NONBLOCK);
+        if (is_error(ret))
+            return get_errno(ret);
+    }
+    if (flags & O_CLOEXEC) {
+        ret = pipe_set_flag(host_pipe[0], F_GETFD, F_SETFD, FD_CLOEXEC);
+        if (is_error(ret))
+            return get_errno(ret);
+        ret = pipe_set_flag(host_pipe[1], F_GETFD, F_SETFD, FD_CLOEXEC);
+        if (is_error(ret))
+            return get_errno(ret);
+    }
+    return get_errno(ret);
+}
 
 static inline abi_long target_to_host_ip_mreq(struct ip_mreqn *mreqn,
                                               abi_ulong target_addr,
@@ -4525,25 +4571,13 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         ret = get_errno(dup(arg1));
         break;
     case TARGET_NR_pipe:
-        {
-            int host_pipe[2];
-            ret = get_errno(pipe(host_pipe));
-            if (!is_error(ret)) {
-#if defined(TARGET_MIPS)
-                CPUMIPSState *env = (CPUMIPSState*)cpu_env;
-		env->active_tc.gpr[3] = host_pipe[1];
-		ret = host_pipe[0];
-#elif defined(TARGET_SH4)
-		((CPUSH4State*)cpu_env)->gregs[1] = host_pipe[1];
-		ret = host_pipe[0];
-#else
-                if (put_user_s32(host_pipe[0], arg1)
-                    || put_user_s32(host_pipe[1], arg1 + sizeof(host_pipe[0])))
-                    goto efault;
-#endif
-            }
-        }
+        ret = do_pipe(cpu_env, arg1, 0);
+        break;
+#ifdef TARGET_NR_pipe2
+    case TARGET_NR_pipe2:
+        ret = do_pipe(cpu_env, arg1, arg2);
         break;
+#endif
     case TARGET_NR_times:
         {
             struct target_tms *tmsp;
-- 
1.6.2.1

Re: [Qemu-devel] [PATCH] linux-user: implement pipe2 syscall

[Jamie Lokier]

Riku Voipio wrote:
> implement pipe2 syscall. instead of calling pipe2 directly
> (which was introduced in 2.6.27), emulate the flag functionality
> with fcntl.

This is wrong with multiple threads if the flag contains FD_CLOEXEC.
If that situation is possible, please don't do this.

The point of pipe2() with FD_CLOEXEC is to be atomic: make sure
another thread can never see the file descriptor with FD_CLOEXEC not set.

If you can't guarantee that, it's better to return ENOSYS as every
application using pipe2() like this has a fallback to use pipe() and
FD_CLOEXEC itself, and probably has application logic to protect
against the race condition.

If there's only one thread, or if you can arrange to block any
concurrent clone/fork/execve calls in other threads (in QEMU) during
the race window, then it's fine to emulate it with fcntl.

-- Jamie

Re: [Qemu-devel] [PATCH] linux-user: implement pipe2 syscall

[Riku Voipio]

On Tue, May 05, 2009 at 11:58:09PM +0100, Jamie Lokier wrote:
> Riku Voipio wrote:
> > implement pipe2 syscall. instead of calling pipe2 directly
> > (which was introduced in 2.6.27), emulate the flag functionality
> > with fcntl.

> This is wrong with multiple threads if the flag contains FD_CLOEXEC.
> If that situation is possible, please don't do this.

> The point of pipe2() with FD_CLOEXEC is to be atomic: make sure
> another thread can never see the file descriptor with FD_CLOEXEC not set.

> If you can't guarantee that, it's better to return ENOSYS as every
> application using pipe2() like this has a fallback to use pipe() and
> FD_CLOEXEC itself, and probably has application logic to protect
> against the race condition.

> If there's only one thread, or if you can arrange to block any
> concurrent clone/fork/execve calls in other threads (in QEMU) during
> the race window, then it's fine to emulate it with fcntl.

We haven't returned from the pipe2 syscall when setting the flag with fcntl.
Before returning from the syscall, the pipe file descriptors could point
to anything (unitialized memory, zeros, ...)

Re: [Qemu-devel] [PATCH] linux-user: implement pipe2 syscall

[Martin Mohring]

Riku Voipio wrote:
> On Tue, May 05, 2009 at 11:58:09PM +0100, Jamie Lokier wrote:
>   
>> Riku Voipio wrote:
>>     
>>> implement pipe2 syscall. instead of calling pipe2 directly
>>> (which was introduced in 2.6.27), emulate the flag functionality
>>> with fcntl.
>>>       
>
>   
>> This is wrong with multiple threads if the flag contains FD_CLOEXEC.
>> If that situation is possible, please don't do this.
>>     
>
>   
>> The point of pipe2() with FD_CLOEXEC is to be atomic: make sure
>> another thread can never see the file descriptor with FD_CLOEXEC not set.
>>     
>
>   
How do we want to handle if "O_CLOEXEC" is not defined on the host OS,
because kernel too old (Debian Etch, CentOS 5)? Should it then return
ENOSYS?
>> If you can't guarantee that, it's better to return ENOSYS as every
>> application using pipe2() like this has a fallback to use pipe() and
>> FD_CLOEXEC itself, and probably has application logic to protect
>> against the race condition.
>>     
>
>   
>> If there's only one thread, or if you can arrange to block any
>> concurrent clone/fork/execve calls in other threads (in QEMU) during
>> the race window, then it's fine to emulate it with fcntl.
>>     
>
> We haven't returned from the pipe2 syscall when setting the flag with fcntl.
> Before returning from the syscall, the pipe file descriptors could point
> to anything (unitialized memory, zeros, ...)
>
>
>
>   

Re: [Qemu-devel] [PATCH] linux-user: implement pipe2 syscall

[Jamie Lokier]

Martin Mohring wrote:
> >> The point of pipe2() with FD_CLOEXEC is to be atomic: make sure
> >> another thread can never see the file descriptor with FD_CLOEXEC not set.
> >   
> How do we want to handle if "O_CLOEXEC" is not defined on the host OS,
> because kernel too old (Debian Etch, CentOS 5)? Should it then return
> ENOSYS?

You can emulate it on any host OS using a mutex or read-write lock:

    - Acquire the lock for reading before calling the hosts's
      open/pipe/socket/etc.
    - Release the lock after calling the host's fcntl to set FD_CLOEXEC.

    - Acquire the lock for writing around emulation of fork, exec,
      clone and unshare.

There are ways to do it more efficiently without a lock, but I don't
think they are worth bothering with here.

-- Jamie

Re: [Qemu-devel] [PATCH] linux-user: implement pipe2 syscall

[Riku Voipio]

On Wed, May 06, 2009 at 11:18:46AM +0200, Martin Mohring wrote:
> How do we want to handle if "O_CLOEXEC" is not defined on the host OS,
> because kernel too old (Debian Etch, CentOS 5)? Should it then return
> ENOSYS?

Unless you fake the uname, the glibc on the target system won't call
pipe2, and thus won't ask for O_CLOEXEC.

Re: [Qemu-devel] [PATCH] linux-user: implement pipe2 syscall

[Jamie Lokier]

Riku Voipio wrote:
> > The point of pipe2() with FD_CLOEXEC is to be atomic: make sure
> > another thread can never see the file descriptor with FD_CLOEXEC not set.
> 
> > If you can't guarantee that, it's better to return ENOSYS as every
> > application using pipe2() like this has a fallback to use pipe() and
> > FD_CLOEXEC itself, and probably has application logic to protect
> > against the race condition.
> 
> > If there's only one thread, or if you can arrange to block any
> > concurrent clone/fork/execve calls in other threads (in QEMU) during
> > the race window, then it's fine to emulate it with fcntl.
> 
> We haven't returned from the pipe2 syscall when setting the flag with fcntl.
> Before returning from the syscall, the pipe file descriptors could point
> to anything (unitialized memory, zeros, ...)

That's not possible with file descriptors.  A user program never sees
an uninitialized descriptor - because descriptors aren't visible to
the user program (in any threads) until they are stored into the file
descriptor table for the process.  That happens once the descriptor is
completely initialised, and for pipe2() that means _after_ FD_CLOEXEC
is set.

Of course it's usually an application bug to use a specific file
descriptor from another thread, when that descriptor is still being
created :-)

But it's not a bug to call execve(), or fork() then execve(), in
another thread at the same time as descriptors are being created.
Those calls scan the whole file descriptor table, and look at the
FD_CLOEXEC flags.

The bug is that execve() in parallel with pipe()+fcntl() can result in
the file descriptor getting copied to a child process, because
execve() scans it.  That's why pipe2() exists, to fix that bug
properly by making it impossible.

I haven't looked too closely at how guest file descriptors are handled
in QEMU these days.  In an older version I'm looking at, guest file
descriptors are simply host file descriptors so the pipe2 emulation is
broken in this way.

If QEMU maintained a guest file descriptor table internally, emulating
what a kernel does, this would be solved automatically, but it doesn't.

You can solve it quite simply for any host kernel with the lock
solution I just posted in another mail on this thread.  The same
method works for all the other syscalls taking *_CLOEXEC flags, so
it's probably a good idea :-)

-- Jamie

Re: [Qemu-devel] [PATCH] linux-user: implement pipe2 syscall

[Riku Voipio]

On Wed, May 06, 2009 at 12:08:32PM +0100, Jamie Lokier wrote:
> But it's not a bug to call execve(), or fork() then execve(), in
> another thread at the same time as descriptors are being created.
> Those calls scan the whole file descriptor table, and look at the
> FD_CLOEXEC flags.

Now this discussion would be much more useful if qemu was actually
properly threadsafe to begin with...

> I haven't looked too closely at how guest file descriptors are handled
> in QEMU these days.  In an older version I'm looking at, guest file
> descriptors are simply host file descriptors so the pipe2 emulation is
> broken in this way.

> If QEMU maintained a guest file descriptor table internally, emulating
> what a kernel does, this would be solved automatically, but it doesn't.

> You can solve it quite simply for any host kernel with the lock
> solution I just posted in another mail on this thread.  The same
> method works for all the other syscalls taking *_CLOEXEC flags, so
> it's probably a good idea :-)

Either of these suggested changes are separate new features, and
would thus need to be implemented as separate patches. Thanks
for patches in advance :-)

Re: [Qemu-devel] [PATCH] linux-user: implement pipe2 syscall

[Paul Brook]

On Wednesday 06 May 2009, Riku Voipio wrote:
> On Wed, May 06, 2009 at 12:08:32PM +0100, Jamie Lokier wrote:
> > But it's not a bug to call execve(), or fork() then execve(), in
> > another thread at the same time as descriptors are being created.
> > Those calls scan the whole file descriptor table, and look at the
> > FD_CLOEXEC flags.
>
> Now this discussion would be much more useful if qemu was actually
> properly threadsafe to begin with...

For usermode emulation it is.

Paul

Re: [Qemu-devel] [PATCH] linux-user: implement pipe2 syscall

[Riku Voipio]

On Wed, May 06, 2009 at 01:23:28PM +0100, Paul Brook wrote:
> On Wednesday 06 May 2009, Riku Voipio wrote:
> > On Wed, May 06, 2009 at 12:08:32PM +0100, Jamie Lokier wrote:
> > > But it's not a bug to call execve(), or fork() then execve(), in
> > > another thread at the same time as descriptors are being created.
> > > Those calls scan the whole file descriptor table, and look at the
> > > FD_CLOEXEC flags.
> >
> > Now this discussion would be much more useful if qemu was actually
> > properly threadsafe to begin with...

> For usermode emulation it is.

Ok, I was under impression that threads in qemu linux-user were still
considered broken.

I'll see what would be the best way handle the CLOEXEC flags in a safe
manner.

Re: [Qemu-devel] [PATCH] linux-user: implement pipe2 syscall

[Martin Mohring]

Riku Voipio wrote:
> On Wed, May 06, 2009 at 12:08:32PM +0100, Jamie Lokier wrote:
>   
>> But it's not a bug to call execve(), or fork() then execve(), in
>> another thread at the same time as descriptors are being created.
>> Those calls scan the whole file descriptor table, and look at the
>> FD_CLOEXEC flags.
>>     
>
> Now this discussion would be much more useful if qemu was actually
> properly threadsafe to begin with...
>
>   
>> I haven't looked too closely at how guest file descriptors are handled
>> in QEMU these days.  In an older version I'm looking at, guest file
>> descriptors are simply host file descriptors so the pipe2 emulation is
>> broken in this way.
>>     
>
>   
>> If QEMU maintained a guest file descriptor table internally, emulating
>> what a kernel does, this would be solved automatically, but it doesn't.
>>     
>
>   
>> You can solve it quite simply for any host kernel with the lock
>> solution I just posted in another mail on this thread.  The same
>> method works for all the other syscalls taking *_CLOEXEC flags, so
>> it's probably a good idea :-)
>>     
>
> Either of these suggested changes are separate new features, and
> would thus need to be implemented as separate patches. Thanks
> for patches in advance :-)
>   
What do we now discuss? A reimplementation of a complete threadsafe user
mode for qemu now? I dont think that this is the scope of the two
patches. What do the architects of user mode say about that? I thought
we have even the problem of a missing qemu user mode Maintainer?