[PATCH v2 0/4] linux-user/flatload: Take mmap_lock in load_flt_binary()

[PATCH v2 0/4] linux-user/flatload: Take mmap_lock in load_flt_binary()

[Philippe Mathieu-Daudé]

Fix for https://gitlab.com/qemu-project/qemu/-/issues/2525

Supersedes: <20240821153836.67987-1-philmd@linaro.org>

Philippe Mathieu-Daudé (4):
  accel/tcg: Make page_set_flags() documentation public
  linux-user/flatload: Take mmap_lock in load_flt_binary()
  tests/avocado: Allow running user-mode tests
  tests/avocado: Run STM32 bFLT busybox binary in current directory

 include/exec/cpu-all.h                 | 13 +++++++++++++
 accel/tcg/user-exec.c                  |  5 -----
 linux-user/flatload.c                  |  3 +++
 tests/avocado/avocado_qemu/__init__.py |  2 +-
 tests/avocado/load_bflt.py             |  2 +-
 5 files changed, 18 insertions(+), 7 deletions(-)

-- 
2.45.2

[PATCH v2 1/4] accel/tcg: Make page_set_flags() documentation public

[Philippe Mathieu-Daudé]

Commit e505a063ba ("translate-all: Add assert_(memory|tb)_lock
annotations") states page_set_flags() is "public APIs and [is]
documented as needing them held for linux-user mode".
Document the prototype.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/cpu-all.h | 13 +++++++++++++
 accel/tcg/user-exec.c  |  5 -----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 6f09b86e7f..45e6676938 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -166,7 +166,20 @@ typedef int (*walk_memory_regions_fn)(void *, target_ulong,
 int walk_memory_regions(void *, walk_memory_regions_fn);
 
 int page_get_flags(target_ulong address);
+
+/**
+ * page_set_flags:
+ * @start: first byte of range
+ * @last: last byte of range
+ * @flags: flags to set
+ * Context: holding mmap lock
+ *
+ * Modify the flags of a page and invalidate the code if necessary.
+ * The flag PAGE_WRITE_ORG is positioned automatically depending
+ * on PAGE_WRITE.  The mmap_lock should already be held.
+ */
 void page_set_flags(target_ulong start, target_ulong last, int flags);
+
 void page_reset_target_data(target_ulong start, target_ulong last);
 
 /**
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 7ddc47b0ba..11b6d45e90 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -485,11 +485,6 @@ static bool pageflags_set_clear(target_ulong start, target_ulong last,
     return inval_tb;
 }
 
-/*
- * Modify the flags of a page and invalidate the code if necessary.
- * The flag PAGE_WRITE_ORG is positioned automatically depending
- * on PAGE_WRITE.  The mmap_lock should already be held.
- */
 void page_set_flags(target_ulong start, target_ulong last, int flags)
 {
     bool reset = false;
-- 
2.45.2

[PATCH v2 2/4] linux-user/flatload: Take mmap_lock in load_flt_binary()

[Philippe Mathieu-Daudé]

load_flt_binary() calls load_flat_file() -> page_set_flags().

page_set_flags() must be called with the mmap_lock held,
otherwise it aborts:

  $ qemu-arm -L stm32/lib/ stm32/bin/busybox
  qemu-arm: ../accel/tcg/user-exec.c:505: page_set_flags: Assertion `have_mmap_lock()' failed.
  Aborted (core dumped)

Fix by taking the lock in load_flt_binary().

Fixes: fbd3c4cff6 ("linux-user/arm: Mark the commpage executable")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2525
Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 linux-user/flatload.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/linux-user/flatload.c b/linux-user/flatload.c
index 04d8138d12..0e4be5bf44 100644
--- a/linux-user/flatload.c
+++ b/linux-user/flatload.c
@@ -487,7 +487,10 @@ int load_flt_binary(struct linux_binprm *bprm, struct image_info *info)
     stack_len += (bprm->envc + 1) * 4; /* the envp array */
 
 
+    mmap_lock();
     res = load_flat_file(bprm, libinfo, 0, &stack_len);
+    mmap_unlock();
+
     if (is_error(res)) {
             return res;
     }
-- 
2.45.2

[PATCH v2 3/4] tests/avocado: Allow running user-mode tests

[Philippe Mathieu-Daudé]

Commit 816d4201ea ("tests/avocado: Move LinuxTest related
code into a separate file") removed the Avocado 'process'
import which is used by the QemuUserTest class, restore it.

Fixes: 816d4201ea ("tests/avocado: Move LinuxTest ...")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 tests/avocado/avocado_qemu/__init__.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
index ef935614cf..0d57addfea 100644
--- a/tests/avocado/avocado_qemu/__init__.py
+++ b/tests/avocado/avocado_qemu/__init__.py
@@ -17,7 +17,7 @@
 import uuid
 
 import avocado
-from avocado.utils import ssh
+from avocado.utils import process, ssh
 from avocado.utils.path import find_command
 
 from qemu.machine import QEMUMachine
-- 
2.45.2

[PATCH v2 4/4] tests/avocado: Run STM32 bFLT busybox binary in current directory

[Philippe Mathieu-Daudé]

When this test was added in commit 8011837a01, self.workdir was
set to the test directory. As of this commit, it is not set
anymore. Rather than using a full path to the busybox binary,
we can run it in the current directory, effectively kludging
the fact that self.workdir is not set. Good enough to run the
test:

  Fetching asset from tests/avocado/load_bflt.py:LoadBFLT.test_stm32
  JOB ID     : 020d317281b042f46ad99013530d29df0f1d7eb7
  JOB LOG    : tests/results/job-2024-08-22T10.17-020d317/job.log
   (1/1) tests/avocado/load_bflt.py:LoadBFLT.test_stm32: PASS (0.09 s)
  RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
  JOB TIME   : 0.62 s

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/avocado/load_bflt.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/avocado/load_bflt.py b/tests/avocado/load_bflt.py
index bb50cec1ee..264489ee25 100644
--- a/tests/avocado/load_bflt.py
+++ b/tests/avocado/load_bflt.py
@@ -41,7 +41,7 @@ def test_stm32(self):
                       'Stm32_mini_rootfs.cpio.bz2')
         rootfs_hash = '9f065e6ba40cce7411ba757f924f30fcc57951e6'
         rootfs_path_bz2 = self.fetch_asset(rootfs_url, asset_hash=rootfs_hash)
-        busybox_path = os.path.join(self.workdir, "/bin/busybox")
+        busybox_path = os.path.join(self.workdir, "bin/busybox")
 
         self.extract_cpio(rootfs_path_bz2)
 
-- 
2.45.2

Re: [PATCH v2 4/4] tests/avocado: Run STM32 bFLT busybox binary in current directory

[Thomas Huth]

On 22/08/2024 11.50, Philippe Mathieu-Daudé wrote:
> When this test was added in commit 8011837a01, self.workdir was
> set to the test directory. As of this commit, it is not set
> anymore. Rather than using a full path to the busybox binary,
> we can run it in the current directory, effectively kludging
> the fact that self.workdir is not set. Good enough to run the
> test:
> 
>    Fetching asset from tests/avocado/load_bflt.py:LoadBFLT.test_stm32
>    JOB ID     : 020d317281b042f46ad99013530d29df0f1d7eb7
>    JOB LOG    : tests/results/job-2024-08-22T10.17-020d317/job.log
>     (1/1) tests/avocado/load_bflt.py:LoadBFLT.test_stm32: PASS (0.09 s)
>    RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
>    JOB TIME   : 0.62 s
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tests/avocado/load_bflt.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/avocado/load_bflt.py b/tests/avocado/load_bflt.py
> index bb50cec1ee..264489ee25 100644
> --- a/tests/avocado/load_bflt.py
> +++ b/tests/avocado/load_bflt.py
> @@ -41,7 +41,7 @@ def test_stm32(self):
>                         'Stm32_mini_rootfs.cpio.bz2')
>           rootfs_hash = '9f065e6ba40cce7411ba757f924f30fcc57951e6'
>           rootfs_path_bz2 = self.fetch_asset(rootfs_url, asset_hash=rootfs_hash)
> -        busybox_path = os.path.join(self.workdir, "/bin/busybox")
> +        busybox_path = os.path.join(self.workdir, "bin/busybox")

Good enough for now, indeed.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 0/4] linux-user/flatload: Take mmap_lock in load_flt_binary()

[Richard Henderson]

On 8/22/24 19:50, Philippe Mathieu-Daudé wrote:
> Philippe Mathieu-Daudé (4):
>    accel/tcg: Make page_set_flags() documentation public
>    linux-user/flatload: Take mmap_lock in load_flt_binary()
>    tests/avocado: Allow running user-mode tests
>    tests/avocado: Run STM32 bFLT busybox binary in current directory

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

r~

Re: [PATCH v2 0/4] linux-user/flatload: Take mmap_lock in load_flt_binary()

[Richard Henderson]

On 8/22/24 02:50, Philippe Mathieu-Daudé wrote:
> Fix for https://gitlab.com/qemu-project/qemu/-/issues/2525
> 
> Supersedes: <20240821153836.67987-1-philmd@linaro.org>
> 
> Philippe Mathieu-Daudé (4):
>    accel/tcg: Make page_set_flags() documentation public
>    linux-user/flatload: Take mmap_lock in load_flt_binary()

Queued these two patches.


r~

Re: [PATCH v2 2/4] linux-user/flatload: Take mmap_lock in load_flt_binary()

[Michael Tokarev]

22.08.2024 12:50, Philippe Mathieu-Daudé wrote:
> load_flt_binary() calls load_flat_file() -> page_set_flags().
> 
> page_set_flags() must be called with the mmap_lock held,
> otherwise it aborts:
> 
>    $ qemu-arm -L stm32/lib/ stm32/bin/busybox
>    qemu-arm: ../accel/tcg/user-exec.c:505: page_set_flags: Assertion `have_mmap_lock()' failed.
>    Aborted (core dumped)
> 
> Fix by taking the lock in load_flt_binary().
> 
> Fixes: fbd3c4cff6 ("linux-user/arm: Mark the commpage executable")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2525

This one seems like it should go to -stable, is it not?

Thanks,

/mjt

Re: [PATCH v2 2/4] linux-user/flatload: Take mmap_lock in load_flt_binary()

[Richard Henderson]

On 10/6/24 01:51, Michael Tokarev wrote:
> 22.08.2024 12:50, Philippe Mathieu-Daudé wrote:
>> load_flt_binary() calls load_flat_file() -> page_set_flags().
>>
>> page_set_flags() must be called with the mmap_lock held,
>> otherwise it aborts:
>>
>>    $ qemu-arm -L stm32/lib/ stm32/bin/busybox
>>    qemu-arm: ../accel/tcg/user-exec.c:505: page_set_flags: Assertion `have_mmap_lock()' 
>> failed.
>>    Aborted (core dumped)
>>
>> Fix by taking the lock in load_flt_binary().
>>
>> Fixes: fbd3c4cff6 ("linux-user/arm: Mark the commpage executable")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2525
> 
> This one seems like it should go to -stable, is it not?

Yes, I think so.

r~