| Message ID | CANCZdfrHEQ=dMfpKGvYBHbXMfbQb9fDQh+bkdZW0Z6zQWVVUCA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: bsd-user broken a while ago, is this the right fix? | expand |
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
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~
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~
On Fri, Jun 30, 2023 at 12:24 PM Richard Henderson < richard.henderson@linaro.org> wrote: > 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. > thanks richard. I'll try to test the patches you posted later today... .... but most of my plans for the day have been upended by a clogged sewer drain. Warner
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;
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: 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