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

[Daniel P . Berrangé]

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).
> 
> It seems to work, but it looks super weird.
> 
> Comments?
> 
> Warrner

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|