qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: RFC: bsd-user broken a while ago, is this the right fix?


From: Richard Henderson
Subject: Re: RFC: bsd-user broken a while ago, is this the right fix?
Date: Mon, 26 Jun 2023 11:52:17 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0

On 6/26/23 10:28, Daniel P. Berrangé wrote:
Just CC'ing Richard to make sure it catches his attention.

On Sat, Jun 24, 2023 at 12:40:33AM -0600, Warner Losh wrote:
This change:

commit f00506aeca2f6d92318967693f8da8c713c163f3
Merge: d37158bb242 87e303de70f
Author: Peter Maydell <peter.maydell@linaro.org>
Date:   Wed Mar 29 11:19:19 2023 +0100

     Merge tag 'pull-tcg-20230328' of https://gitlab.com/rth7680/qemu into
staging

     Use a local version of GTree [#285]
     Fix page_set_flags vs the last page of the address space [#1528]
     Re-enable gdbstub breakpoints under KVM

     # -----BEGIN PGP SIGNATURE-----
     #
     # iQFRBAABCgA7FiEEekgeeIaLTbaoWgXAZN846K9+IV8FAmQjcLIdHHJpY2hhcmQu
     # aGVuZGVyc29uQGxpbmFyby5vcmcACgkQZN846K9+IV8rkgf/ZazodovRKxfaO622
     # mGW7ywIm+hIZYmKC7ObiMKFrBoCyeXH9yOLSx42T70QstWvBMukjovLMz1+Ttbo1
     # VOvpGH2B5W76l3i+muAlKxFRbBH2kMLTaL+BXtkmkL4FJ9bS8WiPApsL3lEX/q2E
     # 3kqaT3N3C09sWO5oVAPGTUHL0EutKhOar2VZL0+PVPFzL3BNPhnQH9QcbNvDBV3n
     # cx3GSXZyL7Plyi+qwsKf/3Jo+F2wr2NVf3Dqscu9T1N1kI5hSjRpwqUEJzJZ5rei
     # ly/gBXC/J7+WN+x+w2JlN0kWXWqC0QbDfZnj96Pd3owWZ7j4sT9zR5fcNenecxlR
     # 38Bo0w==
     # =ysF7
     # -----END PGP SIGNATURE-----
     # gpg: Signature made Tue 28 Mar 2023 23:56:50 BST
     # gpg:                using RSA key
7A481E78868B4DB6A85A05C064DF38E8AF7E215F
     # gpg:                issuer "richard.henderson@linaro.org"
     # gpg: Good signature from "Richard Henderson <
richard.henderson@linaro.org>" [full]
     # Primary key fingerprint: 7A48 1E78 868B 4DB6 A85A  05C0 64DF 38E8
AF7E 215F

     * tag 'pull-tcg-20230328' of https://gitlab.com/rth7680/qemu:
       softmmu: Restore use of CPU watchpoint for all accelerators
       softmmu/watchpoint: Add missing 'qemu/error-report.h' include
       softmmu: Restrict cpu_check_watchpoint / address_matches to TCG accel
       linux-user/arm: Take more care allocating commpage
       include/exec: Change reserved_va semantics to last byte
       linux-user: Pass last not end to probe_guest_base
       accel/tcg: Pass last not end to tb_invalidate_phys_range
       accel/tcg: Pass last not end to tb_invalidate_phys_page_range__locked
       accel/tcg: Pass last not end to page_collection_lock
       accel/tcg: Pass last not end to PAGE_FOR_EACH_TB
       accel/tcg: Pass last not end to page_reset_target_data
       accel/tcg: Pass last not end to page_set_flags
       linux-user: Diagnose misaligned -R size
       tcg: use QTree instead of GTree
       util: import GTree as QTree

     Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

breaks bsd-user. when I merge it to the bsd-user upstream blitz branch I
get memory allocation errors on startup. At least for armv7.

specifically, if I back out the bsd-user part of both
commit 95059f9c313a7fbd7f22e4cdc1977c0393addc7b
Author: Richard Henderson <richard.henderson@linaro.org>
Date:   Mon Mar 6 01:26:29 2023 +0300

     include/exec: Change reserved_va semantics to last byte

     Change the semantics to be the last byte of the guest va, rather
     than the following byte.  This avoids some overflow conditions.

     Reviewed-by: Philippe Mathieu-Daud<C3><A9> <philmd@linaro.org>
     Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

and

commit 49840a4a098149067789255bca6894645f411036
Author: Richard Henderson <richard.henderson@linaro.org>
Date:   Mon Mar 6 01:51:09 2023 +0300

     accel/tcg: Pass last not end to page_set_flags

     Pass the address of the last byte to be changed, rather than
     the first address past the last byte.  This avoids overflow
     when the last page of the address space is involved.

     Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1528
     Reviewed-by: Philippe Mathieu-Daud<C3><A9> <philmd@linaro.org>
     Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

things work again. If I backout parts, it fails still. If I back out only
one of
the two, but not both, then it fails.

What's happening is that we're picking a reserved_va that's overflowing when
we add 1 to it. this overflow goes away if I make the overflows not
possible:
diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index a88251f8705..bd86c0a8689 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -237,8 +237,8 @@ unsigned long last_brk;
  static abi_ulong mmap_find_vma_reserved(abi_ulong start, abi_ulong size,
                                          abi_ulong alignment)
  {
-    abi_ulong addr;
-    abi_ulong end_addr;
+    uint64_t addr;
+    uint64_t end_addr;
      int prot;
      int looped = 0;

My question is, is this the right fix? The old code avoided the overflow in
two ways. 1 it set reserve_va to a page short (which if I fix that, it
works better, but not quite right). and it never computes an address that
may overflow (which the new code does without the above patch).

Not really correct, though it will work for the 32-bit guests.

You want to change end_addr to last_addr, which would be end_addr - 1, and do that basically everywhere. That's the only way to avoid overflow properly, and is what I'm settling on with page_set_flags et al.


r~



reply via email to

[Prev in Thread] Current Thread [Next in Thread]