| Message ID | YtQzMUuBOfBiMNlY@p100 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-user: Fix pipe() vs. pipe2() usage for ALPHA, MIPS, SH4 and SPARC | expand |
On 7/17/22 18:08, Helge Deller wrote: > In 2010, the commit b41a66edd0c added a thrird parameter "is_pipe2" to the > internal do_pipe() function, but missed to actually use this parameter to > decide if the pipe() or pipe2() syscall should be used. > Instead it just continued to check the flags parameter and used pipe2() > unconditionally if flags is non-zero. > > This change should make a difference for the ALPHA, MIPS, SH4 and SPARC > targets if the emulated code calls pipe2() with a flags value of 0. > > Fixes: fb41a66edd0c ("alpha-linux-user: Fix pipe return mechanism.") Signed-off-by: Helge Deller <deller@gmx.de> > Cc: Richard Henderson <rth@twiddle.net> > Cc: Aurelien Jarno <aurelien@aurel32.net> > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 991b85e6b4..1e6e814871 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -1600,7 +1600,7 @@ static abi_long do_pipe(CPUArchState *cpu_env, abi_ulong pipedes, > { > int host_pipe[2]; > abi_long ret; > - ret = flags ? do_pipe2(host_pipe, flags) : pipe(host_pipe); > + ret = is_pipe2 ? do_pipe2(host_pipe, flags) : pipe(host_pipe); > > if (is_error(ret)) > return get_errno(ret);
On Sun, 17 Jul 2022 at 17:10, Helge Deller <deller@gmx.de> wrote: > > In 2010, the commit b41a66edd0c added a thrird parameter "is_pipe2" to the typo in commit hash (lost the first letter). Should be fb41a66edd0c7bd6 ("alpha-linux-user: Fix pipe return mechanism." I think ? > internal do_pipe() function, but missed to actually use this parameter to > decide if the pipe() or pipe2() syscall should be used. > Instead it just continued to check the flags parameter and used pipe2() > unconditionally if flags is non-zero. > > This change should make a difference for the ALPHA, MIPS, SH4 and SPARC > targets if the emulated code calls pipe2() with a flags value of 0. > > Fixes: fb41a66edd0c ("alpha-linux-user: Fix pipe return mechanism.") > Cc: Richard Henderson <rth@twiddle.net> > Cc: Aurelien Jarno <aurelien@aurel32.net> > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 991b85e6b4..1e6e814871 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -1600,7 +1600,7 @@ static abi_long do_pipe(CPUArchState *cpu_env, abi_ulong pipedes, > { > int host_pipe[2]; > abi_long ret; > - ret = flags ? do_pipe2(host_pipe, flags) : pipe(host_pipe); > + ret = is_pipe2 ? do_pipe2(host_pipe, flags) : pipe(host_pipe); Why do we need to do this? If the flags argument is 0, then pipe2() is the same as pipe(), so we can safely emulate it with the host pipe() call. It is, or at least was, worth doing that, so that guests that use pipe2(fds, 0) can still run on hosts that don't implement the pipe2 syscall. There's probably a reasonable argument to be made that we don't care any more about hosts so old they don't have pipe2 and that we could just dump do_pipe2() and the CONFIG_PIPE2 check, and have do_pipe() unconditionally call pipe2(). Would just need somebody to check what kernel/glibc versions pipe2() came in. The architecture specific bit of target behaviour that we need the is_pipe2 flag for is purely for handling the weird return code that only the pipe syscall has, and for that we are correctly looking at the is_pipe2 argument. (The bug that fb41a66edd0c7bd6 fixes is that we used to incorrectly give the pipe syscall return argument behaviour for pipe2-with-flags-zero.) thanks -- PMM
On 7/18/22 14:51, Peter Maydell wrote: > On Sun, 17 Jul 2022 at 17:10, Helge Deller <deller@gmx.de> wrote: >> >> In 2010, the commit b41a66edd0c added a thrird parameter "is_pipe2" to the > > typo in commit hash (lost the first letter). Should be > fb41a66edd0c7bd6 ("alpha-linux-user: Fix pipe return mechanism." > I think ? Yes. I missed that "f" although I had it in the Fixes: tag. >> internal do_pipe() function, but missed to actually use this parameter to >> decide if the pipe() or pipe2() syscall should be used. >> Instead it just continued to check the flags parameter and used pipe2() >> unconditionally if flags is non-zero. >> >> This change should make a difference for the ALPHA, MIPS, SH4 and SPARC >> targets if the emulated code calls pipe2() with a flags value of 0. >> >> Fixes: fb41a66edd0c ("alpha-linux-user: Fix pipe return mechanism.") >> Cc: Richard Henderson <rth@twiddle.net> >> Cc: Aurelien Jarno <aurelien@aurel32.net> >> >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> index 991b85e6b4..1e6e814871 100644 >> --- a/linux-user/syscall.c >> +++ b/linux-user/syscall.c >> @@ -1600,7 +1600,7 @@ static abi_long do_pipe(CPUArchState *cpu_env, abi_ulong pipedes, >> { >> int host_pipe[2]; >> abi_long ret; >> - ret = flags ? do_pipe2(host_pipe, flags) : pipe(host_pipe); >> + ret = is_pipe2 ? do_pipe2(host_pipe, flags) : pipe(host_pipe); > > Why do we need to do this? Yep, we don't *need* to... > If the flags argument is 0, > then pipe2() is the same as pipe(), so we can safely > emulate it with the host pipe() call. It is, or at > least was, worth doing that, so that guests that use > pipe2(fds, 0) can still run on hosts that don't implement > the pipe2 syscall. True, but only for pipe2(fds,0), not e.g. for pipe2(fds,1). On the other side if a guest explicitly calls pipe2() and if it isn't available, then IMHO -ENOSYS should be returned. Let's assume userspace checks in configure/make scripts if pipe2() is available and succeeds due to pipe2(fds,0), it will assume pipe2() is generally available which isn't necessarily true. > There's probably a reasonable argument to be made that we don't > care any more about hosts so old they don't have pipe2 and that > we could just dump do_pipe2() and the CONFIG_PIPE2 check, and > have do_pipe() unconditionally call pipe2(). Would just need > somebody to check what kernel/glibc versions pipe2() came in. Yes. > The architecture specific bit of target behaviour that > we need the is_pipe2 flag for is purely for handling the > weird return code that only the pipe syscall has, and > for that we are correctly looking at the is_pipe2 argument. > (The bug that fb41a66edd0c7bd6 fixes is that we used to > incorrectly give the pipe syscall return argument behaviour > for pipe2-with-flags-zero.) Yes, that part is ok. In summary, IMHO the patch isn't necessary, but probably more correct. Helge
On Mon, 18 Jul 2022 at 15:21, Helge Deller <deller@gmx.de> wrote: > On 7/18/22 14:51, Peter Maydell wrote: > > Why do we need to do this? > > Yep, we don't *need* to... > > > If the flags argument is 0, > > then pipe2() is the same as pipe(), so we can safely > > emulate it with the host pipe() call. It is, or at > > least was, worth doing that, so that guests that use > > pipe2(fds, 0) can still run on hosts that don't implement > > the pipe2 syscall. > > True, but only for pipe2(fds,0), not e.g. for pipe2(fds,1). > On the other side if a guest explicitly calls pipe2() > and if it isn't available, then IMHO -ENOSYS should be returned. > Let's assume userspace checks in configure/make scripts if pipe2() > is available and succeeds due to pipe2(fds,0), it will assume pipe2() > is generally available which isn't necessarily true. Fair point. Did you run into this in practice, or is it just a theoretical concern ? NB that any probing code that does that will also get the wrong answer on musl libc, though, because musl's pipe2() implementation always calls pipe() for a zero-flags call: https://git.musl-libc.org/cgit/musl/tree/src/unistd/pipe2.c > > There's probably a reasonable argument to be made that we don't > > care any more about hosts so old they don't have pipe2 and that > > we could just dump do_pipe2() and the CONFIG_PIPE2 check, and > > have do_pipe() unconditionally call pipe2(). Would just need > > somebody to check what kernel/glibc versions pipe2() came in. > > Yes. I just had a look, and the pipe2 syscall came in in Linux 2.6.27. musl has implemented pipe2() since at least 2013, and glibc must have had it for at least that long. In fact current glibc always implements pipe() in terms of pipe2(): https://sourceware.org/git/?p=glibc.git;a=commit;h=efc6b2dbc47231dee7a7ac39beec808deb4e4d1f So my preference would be that we should just say "we can assume that pipe2 is always available and implemented on linux hosts" and remove the code that handles the possibility that it isn't. thanks -- PMM
On 7/18/22 16:33, Peter Maydell wrote: > On Mon, 18 Jul 2022 at 15:21, Helge Deller <deller@gmx.de> wrote: >> On 7/18/22 14:51, Peter Maydell wrote: >>> Why do we need to do this? >> >> Yep, we don't *need* to... >> >>> If the flags argument is 0, >>> then pipe2() is the same as pipe(), so we can safely >>> emulate it with the host pipe() call. It is, or at >>> least was, worth doing that, so that guests that use >>> pipe2(fds, 0) can still run on hosts that don't implement >>> the pipe2 syscall. >> >> True, but only for pipe2(fds,0), not e.g. for pipe2(fds,1). >> On the other side if a guest explicitly calls pipe2() >> and if it isn't available, then IMHO -ENOSYS should be returned. >> Let's assume userspace checks in configure/make scripts if pipe2() >> is available and succeeds due to pipe2(fds,0), it will assume pipe2() >> is generally available which isn't necessarily true. > > Fair point. Did you run into this in practice, or is it just a > theoretical concern ? > > NB that any probing code that does that will also get the wrong > answer on musl libc, though, because musl's pipe2() implementation > always calls pipe() for a zero-flags call: > https://git.musl-libc.org/cgit/musl/tree/src/unistd/pipe2.c > >>> There's probably a reasonable argument to be made that we don't >>> care any more about hosts so old they don't have pipe2 and that >>> we could just dump do_pipe2() and the CONFIG_PIPE2 check, and >>> have do_pipe() unconditionally call pipe2(). Would just need >>> somebody to check what kernel/glibc versions pipe2() came in. >> >> Yes. > > I just had a look, and the pipe2 syscall came in in Linux 2.6.27. > musl has implemented pipe2() since at least 2013, and glibc must > have had it for at least that long. > > In fact current glibc always implements pipe() in terms of pipe2(): > https://sourceware.org/git/?p=glibc.git;a=commit;h=efc6b2dbc47231dee7a7ac39beec808deb4e4d1f > > So my preference would be that we should just say "we can assume > that pipe2 is always available and implemented on linux hosts" and > remove the code that handles the possibility that it isn't. Ok for me. Do you want me to send a patch or will you do? Helge
On Mon, 18 Jul 2022 at 16:50, Helge Deller <deller@gmx.de> wrote: > On 7/18/22 16:33, Peter Maydell wrote: > > So my preference would be that we should just say "we can assume > > that pipe2 is always available and implemented on linux hosts" and > > remove the code that handles the possibility that it isn't. > > Ok for me. > Do you want me to send a patch or will you do? If you'd like to write the patch that would be great. You can remove the meson.build line that sets CONFIG_PIPE2 as well, since we have no other places that check it. -- PMM
On 7/18/22 18:17, Peter Maydell wrote: > On Mon, 18 Jul 2022 at 16:50, Helge Deller <deller@gmx.de> wrote: >> On 7/18/22 16:33, Peter Maydell wrote: >>> So my preference would be that we should just say "we can assume >>> that pipe2 is always available and implemented on linux hosts" and >>> remove the code that handles the possibility that it isn't. >> >> Ok for me. >> Do you want me to send a patch or will you do? > > If you'd like to write the patch that would be great. > You can remove the meson.build line that sets CONFIG_PIPE2 > as well, since we have no other places that check it. Ok, I'll do. Btw, you asked: > Did you run into this in practice, or is it just a > theoretical concern ? I didn't faced the problem in practice. I'm trying to solve another issue and while debugging I stumbled over that code. Helge
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 991b85e6b4..1e6e814871 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -1600,7 +1600,7 @@ static abi_long do_pipe(CPUArchState *cpu_env, abi_ulong pipedes, { int host_pipe[2]; abi_long ret; - ret = flags ? do_pipe2(host_pipe, flags) : pipe(host_pipe); + ret = is_pipe2 ? do_pipe2(host_pipe, flags) : pipe(host_pipe); if (is_error(ret)) return get_errno(ret);
In 2010, the commit b41a66edd0c added a thrird parameter "is_pipe2" to the internal do_pipe() function, but missed to actually use this parameter to decide if the pipe() or pipe2() syscall should be used. Instead it just continued to check the flags parameter and used pipe2() unconditionally if flags is non-zero. This change should make a difference for the ALPHA, MIPS, SH4 and SPARC targets if the emulated code calls pipe2() with a flags value of 0. Fixes: fb41a66edd0c ("alpha-linux-user: Fix pipe return mechanism.") Cc: Richard Henderson <rth@twiddle.net> Cc: Aurelien Jarno <aurelien@aurel32.net>