Re: [PATCH] linux-user: Fix pipe() vs. pipe2() usage for ALPHA, MIPS, SH4 and SPARC

[Helge Deller <deller@gmx.de>]

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