[PATCH 0/4] linux-user: Fix fcntl64() and accept4() for 32-bit targets

[PATCH 0/4] linux-user: Fix fcntl64() and accept4() for 32-bit targets

[Helge Deller]

A set of 4 patches:
The first two patches fix fcntl64() and accept4() for 32-bit targets.

Patch 3 and 4 are resent here with ask to get review.
They improve mmap2() for 32-bit targets and strace of pread64/pwrite64().

Helge

Helge Deller (4):
  linux-user: Fix fcntl64() to return O_LARGEFILE for 32-bit targets
  linux-user: Fix accept4(SOCK_NONBLOCK) syscall
  linux-user: Fix mmap2() syscall on 32-bit targets to allow file
    mapping beyond 4GB
  linux-user: Improve strace output of pread64() and pwrite64()

 linux-user/mmap.c      |  9 +++++----
 linux-user/strace.c    | 19 +++++++++++++++++++
 linux-user/strace.list |  4 ++--
 linux-user/syscall.c   | 24 ++++++++++++++++++++++--
 linux-user/user-mmap.h |  2 +-
 5 files changed, 49 insertions(+), 9 deletions(-)

--
2.41.0

[PATCH 1/4] linux-user: Fix fcntl64() to return O_LARGEFILE for 32-bit targets

[Helge Deller]

On a 64-bit host, O_LARGEFILE has the value 0.
When running a 32-bit guest on a 64-bit host, fcntl64(F_GETFL) should
return with the O_LARGEFILE flag set, because the 64-bit host supports
large files unconditionally.

The flag translation should have happened in do_fcntl(), but since O_LARGEFILE
is zero for 64-bit hosts, the translation can't be done with the
translation table.

Fix it by setting the TARGET_O_LARGEFILE flag unconditionally for
32-bit guests on 64-bit hosts when fcntl64() is called.

Signed-off-by: Helge Deller <deller@gmx.de>
---
 linux-user/syscall.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 08162cc966..3f1e8e7ad9 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -12328,6 +12328,15 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
         }

         switch(arg2) {
+#if HOST_LONG_BITS == 64 && TARGET_LONG_BITS == 32 && \
+    O_LARGEFILE == 0     && TARGET_O_LARGEFILE != 0
+        case TARGET_F_GETFL:
+            ret = do_fcntl(arg1, arg2, arg3);
+            if (ret > 0) {
+                ret |= TARGET_O_LARGEFILE;
+            }
+	    break;
+#endif
         case TARGET_F_GETLK64:
             ret = copyfrom(&fl, arg3);
             if (ret) {
--
2.41.0

Re: [PATCH 1/4] linux-user: Fix fcntl64() to return O_LARGEFILE for 32-bit targets

[Richard Henderson]

On 7/7/23 14:19, Helge Deller wrote:
> On a 64-bit host, O_LARGEFILE has the value 0.
> When running a 32-bit guest on a 64-bit host, fcntl64(F_GETFL) should
> return with the O_LARGEFILE flag set, because the 64-bit host supports
> large files unconditionally.
> 
> The flag translation should have happened in do_fcntl(), but since O_LARGEFILE
> is zero for 64-bit hosts, the translation can't be done with the
> translation table.

But surely add the code to do_fcntl, right after the host_to_target_bitmask, so that it's 
present for fcntl64 as well?


r~

> 
> Fix it by setting the TARGET_O_LARGEFILE flag unconditionally for
> 32-bit guests on 64-bit hosts when fcntl64() is called.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> ---
>   linux-user/syscall.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 08162cc966..3f1e8e7ad9 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -12328,6 +12328,15 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
>           }
> 
>           switch(arg2) {
> +#if HOST_LONG_BITS == 64 && TARGET_LONG_BITS == 32 && \
> +    O_LARGEFILE == 0     && TARGET_O_LARGEFILE != 0
> +        case TARGET_F_GETFL:
> +            ret = do_fcntl(arg1, arg2, arg3);
> +            if (ret > 0) {
> +                ret |= TARGET_O_LARGEFILE;
> +            }
> +	    break;
> +#endif
>           case TARGET_F_GETLK64:
>               ret = copyfrom(&fl, arg3);
>               if (ret) {
> --
> 2.41.0
> 

[PATCH 2/4] linux-user: Fix accept4(SOCK_NONBLOCK) syscall

[Helge Deller]

The accept4() syscall takes two flags only: SOCK_NONBLOCK and
SOCK_CLOEXEC.
Even the real Linux kernel returns -EINVAL if any other bits
have been set.

Change the implementation of accept4() to recognize those two values
only, instead of using the fcntl_flags_tbl[] bitmask translation.

Beside this correction in behaviour, it actually fixes the accept4()
emulation for hppa, mips and alpha targets for which SOCK_NONBLOCK is
different than TARGET_SOCK_NONBLOCK.

I noticed this wrong behaviour with the testcase of the debian lwt package
which failed (by timeout while hanging in the read() syscall) in qemu but
succeeded on real hardware.

Signed-off-by: Helge Deller <deller@gmx.de>
---
 linux-user/syscall.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 3f1e8e7ad9..9e9317237d 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3440,7 +3440,18 @@ static abi_long do_accept4(int fd, abi_ulong target_addr,
     abi_long ret;
     int host_flags;

-    host_flags = target_to_host_bitmask(flags, fcntl_flags_tbl);
+    host_flags = 0;
+#if defined(SOCK_NONBLOCK)
+    if (flags & ~(TARGET_SOCK_CLOEXEC | TARGET_SOCK_NONBLOCK)) {
+        return -TARGET_EINVAL;
+    }
+    if (flags & TARGET_SOCK_NONBLOCK) {
+        host_flags |= SOCK_NONBLOCK;
+    }
+#endif
+    if (flags & TARGET_SOCK_CLOEXEC) {
+        host_flags |= SOCK_CLOEXEC;
+    }

     if (target_addr == 0) {
         return get_errno(safe_accept4(fd, NULL, NULL, host_flags));
--
2.41.0

Re: [PATCH 2/4] linux-user: Fix accept4(SOCK_NONBLOCK) syscall

[Richard Henderson]

On 7/7/23 14:19, Helge Deller wrote:
> The accept4() syscall takes two flags only: SOCK_NONBLOCK and
> SOCK_CLOEXEC.
> Even the real Linux kernel returns -EINVAL if any other bits
> have been set.
> 
> Change the implementation of accept4() to recognize those two values
> only, instead of using the fcntl_flags_tbl[] bitmask translation.
> 
> Beside this correction in behaviour, it actually fixes the accept4()
> emulation for hppa, mips and alpha targets for which SOCK_NONBLOCK is
> different than TARGET_SOCK_NONBLOCK.
> 
> I noticed this wrong behaviour with the testcase of the debian lwt package
> which failed (by timeout while hanging in the read() syscall) in qemu but
> succeeded on real hardware.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> ---
>   linux-user/syscall.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 3f1e8e7ad9..9e9317237d 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3440,7 +3440,18 @@ static abi_long do_accept4(int fd, abi_ulong target_addr,
>       abi_long ret;
>       int host_flags;
> 
> -    host_flags = target_to_host_bitmask(flags, fcntl_flags_tbl);
> +    host_flags = 0;
> +#if defined(SOCK_NONBLOCK)
> +    if (flags & ~(TARGET_SOCK_CLOEXEC | TARGET_SOCK_NONBLOCK)) {
> +        return -TARGET_EINVAL;
> +    }
> +    if (flags & TARGET_SOCK_NONBLOCK) {
> +        host_flags |= SOCK_NONBLOCK;
> +    }
> +#endif

Can we avoid the ifdef?  Anyway, surely the TARGET bit check should not be protected by 
the #ifdef.


r~

Re: [PATCH 2/4] linux-user: Fix accept4(SOCK_NONBLOCK) syscall

[Helge Deller]

On 7/7/23 22:15, Richard Henderson wrote:
> On 7/7/23 14:19, Helge Deller wrote:
>> The accept4() syscall takes two flags only: SOCK_NONBLOCK and
>> SOCK_CLOEXEC.
>> Even the real Linux kernel returns -EINVAL if any other bits
>> have been set.
>>
>> Change the implementation of accept4() to recognize those two values
>> only, instead of using the fcntl_flags_tbl[] bitmask translation.
>>
>> Beside this correction in behaviour, it actually fixes the accept4()
>> emulation for hppa, mips and alpha targets for which SOCK_NONBLOCK is
>> different than TARGET_SOCK_NONBLOCK.
>>
>> I noticed this wrong behaviour with the testcase of the debian lwt package
>> which failed (by timeout while hanging in the read() syscall) in qemu but
>> succeeded on real hardware.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> ---
>>   linux-user/syscall.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 3f1e8e7ad9..9e9317237d 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -3440,7 +3440,18 @@ static abi_long do_accept4(int fd, abi_ulong target_addr,
>>       abi_long ret;
>>       int host_flags;
>>
>> -    host_flags = target_to_host_bitmask(flags, fcntl_flags_tbl);
>> +    host_flags = 0;
>> +#if defined(SOCK_NONBLOCK)
>> +    if (flags & ~(TARGET_SOCK_CLOEXEC | TARGET_SOCK_NONBLOCK)) {
>> +        return -TARGET_EINVAL;
>> +    }
>> +    if (flags & TARGET_SOCK_NONBLOCK) {
>> +        host_flags |= SOCK_NONBLOCK;
>> +    }
>> +#endif
>
> Can we avoid the ifdef?

I don't know. There are multiple such SOCK_NONBLOCK checks in the code.

> Anyway, surely the TARGET bit check should not be protected by the #ifdef.

Ok.

Helge

[PATCH 3/4] linux-user: Fix mmap2() syscall on 32-bit targets to allow file mapping beyond 4GB

[Helge Deller]

The mmap2() syscall allows 32-bit guests to specify the offset into a
file in page units (instead of bytes, as done by mmap(2)).
On physical machines this allows 32-bit applications to map such parts
of large files which are stored beyond the 4GB limit.

Allow the same behaviour when emulating 32-bit guests with qemu.

For that switch the mmap2() function to always take an abi_ullong
(64-bit) offset parameter for target_mmap() and mmap_frag() to avoid an
arithmetical overflow when shifing a 32-bit offset parameter by
12 bits (=PAGE_SHIFT) and thus possibly overflow the abi_ulong (32-bit)
type.

Signed-off-by: Helge Deller <deller@gmx.de>
---
 linux-user/mmap.c      | 9 +++++----
 linux-user/syscall.c   | 2 +-
 linux-user/user-mmap.h | 2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 2692936773..2750146758 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -192,7 +192,7 @@ error:
 /* map an incomplete host page */
 static int mmap_frag(abi_ulong real_start,
                      abi_ulong start, abi_ulong end,
-                     int prot, int flags, int fd, abi_ulong offset)
+                     int prot, int flags, int fd, abi_ullong offset)
 {
     abi_ulong real_end, addr;
     void *host_start;
@@ -436,10 +436,11 @@ abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size, abi_ulong align)

 /* NOTE: all the constants are the HOST ones */
 abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
-                     int flags, int fd, abi_ulong offset)
+                     int flags, int fd, abi_ullong offset)
 {
-    abi_ulong ret, end, real_start, real_end, retaddr, host_offset, host_len,
+    abi_ulong ret, end, real_start, real_end, retaddr, host_len,
               passthrough_start = -1, passthrough_end = -1;
+    abi_ullong host_offset;
     int page_flags, host_prot;

     mmap_lock();
@@ -627,7 +628,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
         /* map the middle (easier) */
         if (real_start < real_end) {
             void *p;
-            unsigned long offset1;
+            off_t offset1;
             if (flags & MAP_ANONYMOUS)
                 offset1 = 0;
             else
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9e9317237d..5ebc502f71 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -10427,7 +10427,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
 #endif
         ret = target_mmap(arg1, arg2, arg3,
                           target_to_host_bitmask(arg4, mmap_flags_tbl),
-                          arg5, arg6 << MMAP_SHIFT);
+                          arg5, ((abi_ullong)arg6) << MMAP_SHIFT);
         return get_errno(ret);
 #endif
     case TARGET_NR_munmap:
diff --git a/linux-user/user-mmap.h b/linux-user/user-mmap.h
index 480ce1c114..72e99000d9 100644
--- a/linux-user/user-mmap.h
+++ b/linux-user/user-mmap.h
@@ -20,7 +20,7 @@

 int target_mprotect(abi_ulong start, abi_ulong len, int prot);
 abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
-                     int flags, int fd, abi_ulong offset);
+                     int flags, int fd, abi_ullong offset);
 int target_munmap(abi_ulong start, abi_ulong len);
 abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
                        abi_ulong new_size, unsigned long flags,
--
2.41.0

Re: [PATCH 3/4] linux-user: Fix mmap2() syscall on 32-bit targets to allow file mapping beyond 4GB

[Richard Henderson]

On 7/7/23 14:19, Helge Deller wrote:
> The mmap2() syscall allows 32-bit guests to specify the offset into a
> file in page units (instead of bytes, as done by mmap(2)).
> On physical machines this allows 32-bit applications to map such parts
> of large files which are stored beyond the 4GB limit.
> 
> Allow the same behaviour when emulating 32-bit guests with qemu.
> 
> For that switch the mmap2() function to always take an abi_ullong
> (64-bit) offset parameter for target_mmap() and mmap_frag() to avoid an
> arithmetical overflow when shifing a 32-bit offset parameter by
> 12 bits (=PAGE_SHIFT) and thus possibly overflow the abi_ulong (32-bit)
> type.
> 
> Signed-off-by: Helge Deller<deller@gmx.de>
> ---
>   linux-user/mmap.c      | 9 +++++----
>   linux-user/syscall.c   | 2 +-
>   linux-user/user-mmap.h | 2 +-
>   3 files changed, 7 insertions(+), 6 deletions(-)

https://patchew.org/QEMU/20230630132159.376995-1-richard.henderson@linaro.org/20230630132159.376995-12-richard.henderson@linaro.org/

Wherein I use the host off_t (which must be 64-bits).
(I'm pretty sure there's an older similar patch, but I couldn't find it.)


r~

Re: [PATCH 3/4] linux-user: Fix mmap2() syscall on 32-bit targets to allow file mapping beyond 4GB

[Helge Deller]

On 7/7/23 21:47, Richard Henderson wrote:
> On 7/7/23 14:19, Helge Deller wrote:
>> The mmap2() syscall allows 32-bit guests to specify the offset into a
>> file in page units (instead of bytes, as done by mmap(2)).
>> On physical machines this allows 32-bit applications to map such parts
>> of large files which are stored beyond the 4GB limit.
>>
>> Allow the same behaviour when emulating 32-bit guests with qemu.
>>
>> For that switch the mmap2() function to always take an abi_ullong
>> (64-bit) offset parameter for target_mmap() and mmap_frag() to avoid an
>> arithmetical overflow when shifing a 32-bit offset parameter by
>> 12 bits (=PAGE_SHIFT) and thus possibly overflow the abi_ulong (32-bit)
>> type.
>>
>> Signed-off-by: Helge Deller<deller@gmx.de>
>> ---
>>   linux-user/mmap.c      | 9 +++++----
>>   linux-user/syscall.c   | 2 +-
>>   linux-user/user-mmap.h | 2 +-
>>   3 files changed, 7 insertions(+), 6 deletions(-)
>
> https://patchew.org/QEMU/20230630132159.376995-1-richard.henderson@linaro.org/20230630132159.376995-12-richard.henderson@linaro.org/
>
> Wherein I use the host off_t (which must be 64-bits).

I like your patch.
But wouldn't it be better to use off64_t instead of off_t just to make
clear that this is a 64bit int?

And this part:
-                          arg5, arg6 << MMAP_SHIFT);
+                          arg5, (off_t)(abi_ulong)arg6 << MMAP_SHIFT);
maybe should become (with brackets): ?
+                          arg5, ((off64_t)(abi_ulong)arg6) << MMAP_SHIFT);

In any case I'm fine if your or my patch could be appled.

Helge

Re: [PATCH 3/4] linux-user: Fix mmap2() syscall on 32-bit targets to allow file mapping beyond 4GB

[Richard Henderson]

On 7/7/23 21:04, Helge Deller wrote:
> On 7/7/23 21:47, Richard Henderson wrote:
>> On 7/7/23 14:19, Helge Deller wrote:
>>> The mmap2() syscall allows 32-bit guests to specify the offset into a
>>> file in page units (instead of bytes, as done by mmap(2)).
>>> On physical machines this allows 32-bit applications to map such parts
>>> of large files which are stored beyond the 4GB limit.
>>>
>>> Allow the same behaviour when emulating 32-bit guests with qemu.
>>>
>>> For that switch the mmap2() function to always take an abi_ullong
>>> (64-bit) offset parameter for target_mmap() and mmap_frag() to avoid an
>>> arithmetical overflow when shifing a 32-bit offset parameter by
>>> 12 bits (=PAGE_SHIFT) and thus possibly overflow the abi_ulong (32-bit)
>>> type.
>>>
>>> Signed-off-by: Helge Deller<deller@gmx.de>
>>> ---
>>>   linux-user/mmap.c      | 9 +++++----
>>>   linux-user/syscall.c   | 2 +-
>>>   linux-user/user-mmap.h | 2 +-
>>>   3 files changed, 7 insertions(+), 6 deletions(-)
>>
>> https://patchew.org/QEMU/20230630132159.376995-1-richard.henderson@linaro.org/20230630132159.376995-12-richard.henderson@linaro.org/
>>
>> Wherein I use the host off_t (which must be 64-bits).
> 
> I like your patch.
> But wouldn't it be better to use off64_t instead of off_t just to make
> clear that this is a 64bit int?

No, I don't think so.  That's the point of _FILE_OFFSET_BITS=64.

> And this part:
> -                          arg5, arg6 << MMAP_SHIFT);
> +                          arg5, (off_t)(abi_ulong)arg6 << MMAP_SHIFT);
> maybe should become (with brackets): ?
> +                          arg5, ((off64_t)(abi_ulong)arg6) << MMAP_SHIFT);

Why would you add useless parenthesis?
At some point everyone should know C operator precedence...


r~

[PATCH 4/4] linux-user: Improve strace output of pread64() and pwrite64()

[Helge Deller]

Make the strace look nicer for those two syscalls.

Signed-off-by: Helge Deller <deller@gmx.de>
---
 linux-user/strace.c    | 19 +++++++++++++++++++
 linux-user/strace.list |  4 ++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/linux-user/strace.c b/linux-user/strace.c
index aad2b62ca4..669200c4a4 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -3999,6 +3999,25 @@ print_tgkill(CPUArchState *cpu_env, const struct syscallname *name,
 }
 #endif

+#if defined(TARGET_NR_pread64) || defined(TARGET_NR_pwrite64)
+static void
+print_pread64(CPUArchState *cpu_env, const struct syscallname *name,
+        abi_long arg0, abi_long arg1, abi_long arg2,
+        abi_long arg3, abi_long arg4, abi_long arg5)
+{
+    if (regpairs_aligned(cpu_env, TARGET_NR_pread64)) {
+        arg3 = arg4;
+        arg4 = arg5;
+    }
+    print_syscall_prologue(name);
+    print_raw_param("%d", arg0, 0);
+    print_pointer(arg1, 0);
+    print_raw_param("%d", arg2, 0);
+    print_raw_param("%" PRIu64, target_offset64(arg3, arg4), 1);
+    print_syscall_epilogue(name);
+}
+#endif
+
 #ifdef TARGET_NR_statx
 static void
 print_statx(CPUArchState *cpu_env, const struct syscallname *name,
diff --git a/linux-user/strace.list b/linux-user/strace.list
index c7808ea118..6655d4f26d 100644
--- a/linux-user/strace.list
+++ b/linux-user/strace.list
@@ -1068,7 +1068,7 @@
 { TARGET_NR_prctl, "prctl" , NULL, NULL, NULL },
 #endif
 #ifdef TARGET_NR_pread64
-{ TARGET_NR_pread64, "pread64" , NULL, NULL, NULL },
+{ TARGET_NR_pread64, "pread64" , NULL, print_pread64, NULL },
 #endif
 #ifdef TARGET_NR_preadv
 { TARGET_NR_preadv, "preadv" , NULL, NULL, NULL },
@@ -1099,7 +1099,7 @@
 { TARGET_NR_putpmsg, "putpmsg" , NULL, NULL, NULL },
 #endif
 #ifdef TARGET_NR_pwrite64
-{ TARGET_NR_pwrite64, "pwrite64" , NULL, NULL, NULL },
+{ TARGET_NR_pwrite64, "pwrite64" , NULL, print_pread64, NULL },
 #endif
 #ifdef TARGET_NR_pwritev
 { TARGET_NR_pwritev, "pwritev" , NULL, NULL, NULL },
--
2.41.0

Re: [PATCH 4/4] linux-user: Improve strace output of pread64() and pwrite64()

[Richard Henderson]

On 7/7/23 14:19, Helge Deller wrote:
> Make the strace look nicer for those two syscalls.
> 
> Signed-off-by: Helge Deller<deller@gmx.de>
> ---
>   linux-user/strace.c    | 19 +++++++++++++++++++
>   linux-user/strace.list |  4 ++--
>   2 files changed, 21 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~