[PATCH 0/4] linux-user: Fix shmat(NULL) for h != g

[PATCH 0/4] linux-user: Fix shmat(NULL) for h != g

[Ilya Leoshkevich]

Hi,

I noticed that while shmat() now works with /proc/self/maps,
shmat(NULL) got broken. This series fixes that along with two related
strace issues, and adds a test.

Best regards,
Ilya

Ilya Leoshkevich (4):
  linux-user: Fix semctl() strace
  linux-user: Fix shmat() strace
  linux-user: Fix shmat(NULL) for h != g
  tests/tcg: Check that shmat(NULL)

 linux-user/mmap.c                            |  2 +-
 linux-user/strace.c                          | 10 +++---
 tests/tcg/multiarch/linux/linux-shmat-null.c | 38 ++++++++++++++++++++
 3 files changed, 43 insertions(+), 7 deletions(-)
 create mode 100644 tests/tcg/multiarch/linux/linux-shmat-null.c

-- 
2.44.0

[PATCH 1/4] linux-user: Fix semctl() strace

[Ilya Leoshkevich]

The indices of arguments used with semctl() are all off-by-1, because
arg1 is the ipc() command. Fix them. While at it, reuse print_semctl().

New output (for a small test program):

    3540333 semctl(999,888,SEM_INFO,0x00007fe5051ee9a0) = -1 errno=14 (Bad address)

Fixes: 7ccfb2eb5f9d ("Fix warnings that would be caused by gcc flag -Wwrite-strings")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/strace.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/linux-user/strace.c b/linux-user/strace.c
index 9934e2208e2..9be71af4016 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -657,7 +657,7 @@ print_newselect(CPUArchState *cpu_env, const struct syscallname *name,
 }
 #endif
 
-#ifdef TARGET_NR_semctl
+#if defined(TARGET_NR_semctl) || defined(TARGET_NR_ipc)
 static void
 print_semctl(CPUArchState *cpu_env, const struct syscallname *name,
              abi_long arg1, abi_long arg2, abi_long arg3,
@@ -698,10 +698,8 @@ print_ipc(CPUArchState *cpu_env, const struct syscallname *name,
 {
     switch(arg1) {
     case IPCOP_semctl:
-        qemu_log("semctl(" TARGET_ABI_FMT_ld "," TARGET_ABI_FMT_ld ",",
-                 arg1, arg2);
-        print_ipc_cmd(arg3);
-        qemu_log(",0x" TARGET_ABI_FMT_lx ")", arg4);
+        print_semctl(cpu_env, &(const struct syscallname){ .name = "semctl" },
+                     arg2, arg3, arg4, arg5, 0, 0);
         break;
     case IPCOP_shmat:
         print_shmat(cpu_env, &(const struct syscallname){ .name = "shmat" },
-- 
2.44.0

Re: [PATCH 1/4] linux-user: Fix semctl() strace

[Richard Henderson]

On 3/25/24 05:07, Ilya Leoshkevich wrote:
> The indices of arguments used with semctl() are all off-by-1, because
> arg1 is the ipc() command. Fix them. While at it, reuse print_semctl().
> 
> New output (for a small test program):
> 
>      3540333 semctl(999,888,SEM_INFO,0x00007fe5051ee9a0) = -1 errno=14 (Bad address)
> 
> Fixes: 7ccfb2eb5f9d ("Fix warnings that would be caused by gcc flag -Wwrite-strings")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   linux-user/strace.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 9934e2208e2..9be71af4016 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -657,7 +657,7 @@ print_newselect(CPUArchState *cpu_env, const struct syscallname *name,
>   }
>   #endif
>   
> -#ifdef TARGET_NR_semctl
> +#if defined(TARGET_NR_semctl) || defined(TARGET_NR_ipc)
>   static void
>   print_semctl(CPUArchState *cpu_env, const struct syscallname *name,
>                abi_long arg1, abi_long arg2, abi_long arg3,

You can remove this ifdef, because one of the two is always defined.
Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

> @@ -698,10 +698,8 @@ print_ipc(CPUArchState *cpu_env, const struct syscallname *name,
>   {
>       switch(arg1) {
>       case IPCOP_semctl:
> -        qemu_log("semctl(" TARGET_ABI_FMT_ld "," TARGET_ABI_FMT_ld ",",
> -                 arg1, arg2);
> -        print_ipc_cmd(arg3);
> -        qemu_log(",0x" TARGET_ABI_FMT_lx ")", arg4);
> +        print_semctl(cpu_env, &(const struct syscallname){ .name = "semctl" },
> +                     arg2, arg3, arg4, arg5, 0, 0);
>           break;
>       case IPCOP_shmat:
>           print_shmat(cpu_env, &(const struct syscallname){ .name = "shmat" },

[PATCH 2/4] linux-user: Fix shmat() strace

[Ilya Leoshkevich]

The indices of arguments passed to print_shmat() are all off-by-1,
because arg1 is the ipc() command. Fix them.

New output for linux-shmat-maps test:

    3501769 shmat(4784214,0x0000000000800000,SHM_RND) = 0

Fixes: 9f7c97324c27 ("linux-user: Add strace for shmat")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/strace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/strace.c b/linux-user/strace.c
index 9be71af4016..3b4ccd9fa04 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -703,7 +703,7 @@ print_ipc(CPUArchState *cpu_env, const struct syscallname *name,
         break;
     case IPCOP_shmat:
         print_shmat(cpu_env, &(const struct syscallname){ .name = "shmat" },
-                    arg1, arg4, arg2, 0, 0, 0);
+                    arg2, arg5, arg3, 0, 0, 0);
         break;
     default:
         qemu_log(("%s("
-- 
2.44.0

Re: [PATCH 2/4] linux-user: Fix shmat() strace

[Richard Henderson]

On 3/25/24 05:07, Ilya Leoshkevich wrote:
> The indices of arguments passed to print_shmat() are all off-by-1,
> because arg1 is the ipc() command. Fix them.
> 
> New output for linux-shmat-maps test:
> 
>      3501769 shmat(4784214,0x0000000000800000,SHM_RND) = 0
> 
> Fixes: 9f7c97324c27 ("linux-user: Add strace for shmat")
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   linux-user/strace.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

r~

[PATCH 3/4] linux-user: Fix shmat(NULL) for h != g

[Ilya Leoshkevich]

In the h != g && shmaddr == NULL && !reserved_va case, target_shmat()
incorrectly mmap()s the initial anonymous range with
MAP_FIXED_NOREPLACE, even though the earlier mmap_find_vma() has
already reserved the respective address range.

Fix by using MAP_FIXED when "mapped", which is set after
mmap_find_vma(), is true.

Fixes: 78bc8ed9a8f0 ("linux-user: Rewrite target_shmat")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/mmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index e88faf1ab3d..681b6db1b67 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -1358,7 +1358,7 @@ abi_ulong target_shmat(CPUArchState *cpu_env, int shmid,
         if (h_len != t_len) {
             int mmap_p = PROT_READ | (shmflg & SHM_RDONLY ? 0 : PROT_WRITE);
             int mmap_f = MAP_PRIVATE | MAP_ANONYMOUS
-                       | (reserved_va || (shmflg & SHM_REMAP)
+                       | (reserved_va || mapped || (shmflg & SHM_REMAP)
                           ? MAP_FIXED : MAP_FIXED_NOREPLACE);
 
             test = mmap(want, m_len, mmap_p, mmap_f, -1, 0);
-- 
2.44.0

Re: [PATCH 3/4] linux-user: Fix shmat(NULL) for h != g

[Richard Henderson]

On 3/25/24 05:07, Ilya Leoshkevich wrote:
> In the h != g && shmaddr == NULL && !reserved_va case, target_shmat()
> incorrectly mmap()s the initial anonymous range with
> MAP_FIXED_NOREPLACE, even though the earlier mmap_find_vma() has
> already reserved the respective address range.
> 
> Fix by using MAP_FIXED when "mapped", which is set after
> mmap_find_vma(), is true.
> 
> Fixes: 78bc8ed9a8f0 ("linux-user: Rewrite target_shmat")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   linux-user/mmap.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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


r~

[PATCH 4/4] tests/tcg: Test shmat(NULL)

[Ilya Leoshkevich]

Add a small test to prevent regressions.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/multiarch/linux/linux-shmat-null.c | 38 ++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 tests/tcg/multiarch/linux/linux-shmat-null.c

diff --git a/tests/tcg/multiarch/linux/linux-shmat-null.c b/tests/tcg/multiarch/linux/linux-shmat-null.c
new file mode 100644
index 00000000000..94eaaec371a
--- /dev/null
+++ b/tests/tcg/multiarch/linux/linux-shmat-null.c
@@ -0,0 +1,38 @@
+/*
+ * Test shmat(NULL).
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <stdlib.h>
+#include <sys/ipc.h>
+#include <sys/shm.h>
+
+int main(void)
+{
+    int shmid;
+    char *p;
+    int err;
+
+    /* Create, attach and intialize shared memory. */
+    shmid = shmget(IPC_PRIVATE, 1, IPC_CREAT | 0600);
+    assert(shmid != -1);
+    p = shmat(shmid, NULL, 0);
+    assert(p != (void *)-1);
+    *p = 42;
+
+    /* Reattach, check that the value is still there. */
+    err = shmdt(p);
+    assert(err == 0);
+    p = shmat(shmid, NULL, 0);
+    assert(p != (void *)-1);
+    assert(*p == 42);
+
+    /* Detach. */
+    err = shmdt(p);
+    assert(err == 0);
+    err = shmctl(shmid, IPC_RMID, NULL);
+    assert(err == 0);
+
+    return EXIT_SUCCESS;
+}
-- 
2.44.0

Re: [PATCH 4/4] tests/tcg: Test shmat(NULL)

[Richard Henderson]

On 3/25/24 05:07, Ilya Leoshkevich wrote:
> Add a small test to prevent regressions.
> 
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   tests/tcg/multiarch/linux/linux-shmat-null.c | 38 ++++++++++++++++++++
>   1 file changed, 38 insertions(+)
>   create mode 100644 tests/tcg/multiarch/linux/linux-shmat-null.c

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

r~