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

[Richard Henderson <richard.henderson@linaro.org>]

On 6/26/23 11:52, Richard Henderson wrote:
> 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.

... and fyi there's now a follow-up that replaces this function entirely.
It is in fact much easier to do with the interval tree in hand.


r~