| Message ID | Y6gId80ek49TK1xB@p100 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] linux-user: Fix brk() to release pages | expand |
Le 25/12/2022 à 09:23, Helge Deller a écrit : > The current brk() implementation does not de-allocate pages if a lower > address is given compared to earlier brk() calls. > But according to the manpage, brk() shall deallocate memory in this case > and currently it breaks a real-world application, specifically building > the debian gcl package in qemu-user. > > Fix this issue by reworking the qemu brk() implementation. > > Tested with the C-code testcase included in qemu commit 4d1de87c750, and > by building debian package of gcl in a hppa-linux guest on a x86-64 > host. > > Signed-off-by: Helge Deller <deller@gmx.de> > > --- > v3: > - Fixed one bug where page aligned address was returned instead > of requested address. > - Dropped debug info which is partly outdated now > - Reduced number of changed lines in diff compared to v2 to make > diff easier readable > - Fixed changelog of v2 > v2: > - Fixed return value of brk(). The v1 version wrongly page-aligned > the provided address, while userspace expects an unmodified > address returned. > > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 4fee882cd7..7d28802aa6 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -838,49 +838,52 @@ static inline int host_to_target_sock_type(int host_type) > } > > static abi_ulong target_brk; > -static abi_ulong target_original_brk; > static abi_ulong brk_page; > > void target_set_brk(abi_ulong new_brk) > { > - target_original_brk = target_brk = HOST_PAGE_ALIGN(new_brk); > + target_brk = new_brk; > brk_page = HOST_PAGE_ALIGN(target_brk); > } > > -//#define DEBUGF_BRK(message, args...) do { fprintf(stderr, (message), ## args); } while (0) > -#define DEBUGF_BRK(message, args...) > - > /* do_brk() must return target values and target errnos. */ > -abi_long do_brk(abi_ulong new_brk) > +abi_long do_brk(abi_ulong brk_val) > { > abi_long mapped_addr; > abi_ulong new_alloc_size; > + abi_ulong new_brk, new_host_brk_page; > > /* brk pointers are always untagged */ > > - DEBUGF_BRK("do_brk(" TARGET_ABI_FMT_lx ") -> ", new_brk); > - > - if (!new_brk) { > - DEBUGF_BRK(TARGET_ABI_FMT_lx " (!new_brk)\n", target_brk); > + /* return old brk value if brk_val unchanged or zero */ > + if (!brk_val || brk_val == target_brk) { > return target_brk; > } > - if (new_brk < target_original_brk) { > - DEBUGF_BRK(TARGET_ABI_FMT_lx " (new_brk < target_original_brk)\n", > - target_brk); > + > + new_brk = TARGET_PAGE_ALIGN(brk_val); > + new_host_brk_page = HOST_PAGE_ALIGN(brk_val); > + > + /* brk_val and old target_brk might be on the same page */ > + if (new_brk == TARGET_PAGE_ALIGN(target_brk)) { > + if (brk_val > target_brk) { > + /* empty remaining bytes in (possibly larger) host page */ > + memset(g2h_untagged(target_brk), 0, new_host_brk_page - target_brk); > + } > + target_brk = brk_val; > return target_brk; > } > > - /* If the new brk is less than the highest page reserved to the > - * target heap allocation, set it and we're almost done... */ > - if (new_brk <= brk_page) { > - /* Heap contents are initialized to zero, as for anonymous > - * mapped pages. */ > - if (new_brk > target_brk) { > - memset(g2h_untagged(target_brk), 0, new_brk - target_brk); > - } > - target_brk = new_brk; > - DEBUGF_BRK(TARGET_ABI_FMT_lx " (new_brk <= brk_page)\n", target_brk); > - return target_brk; > + /* Release heap if necesary */ > + if (new_brk < target_brk) { > + /* empty remaining bytes in (possibly larger) host page */ > + memset(g2h_untagged(brk_val), 0, new_host_brk_page - brk_val); > + > + /* free unused host pages and set new brk_page */ > + target_munmap(new_host_brk_page, brk_page - new_host_brk_page); > + brk_page = new_host_brk_page; > + > + target_brk = brk_val; > + return target_brk; > } > > /* We need to allocate more memory after the brk... Note that > @@ -889,10 +892,14 @@ abi_long do_brk(abi_ulong new_brk) > * itself); instead we treat "mapped but at wrong address" as > * a failure and unmap again. > */ > - new_alloc_size = HOST_PAGE_ALIGN(new_brk - brk_page); > - mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size, > + new_alloc_size = new_host_brk_page - brk_page; > + if (new_alloc_size) { > + mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size, > PROT_READ|PROT_WRITE, > MAP_ANON|MAP_PRIVATE, 0, 0)); > + } else { > + mapped_addr = brk_page; > + } > > if (mapped_addr == brk_page) { > /* Heap contents are initialized to zero, as for anonymous > @@ -904,10 +911,8 @@ abi_long do_brk(abi_ulong new_brk) > * then shrunken). */ > memset(g2h_untagged(target_brk), 0, brk_page - target_brk); > > - target_brk = new_brk; > - brk_page = HOST_PAGE_ALIGN(target_brk); > - DEBUGF_BRK(TARGET_ABI_FMT_lx " (mapped_addr == brk_page)\n", > - target_brk); > + target_brk = brk_val; > + brk_page = new_host_brk_page; > return target_brk; > } else if (mapped_addr != -1) { > /* Mapped but at wrong address, meaning there wasn't actually > @@ -915,10 +920,6 @@ abi_long do_brk(abi_ulong new_brk) > */ > target_munmap(mapped_addr, new_alloc_size); > mapped_addr = -1; > - DEBUGF_BRK(TARGET_ABI_FMT_lx " (mapped_addr != -1)\n", target_brk); > - } > - else { > - DEBUGF_BRK(TARGET_ABI_FMT_lx " (otherwise)\n", target_brk); > } > > #if defined(TARGET_ALPHA) > Applied to my linux-user-for-8.0 branch. Thanks, Laurent
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 4fee882cd7..7d28802aa6 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -838,49 +838,52 @@ static inline int host_to_target_sock_type(int host_type) } static abi_ulong target_brk; -static abi_ulong target_original_brk; static abi_ulong brk_page; void target_set_brk(abi_ulong new_brk) { - target_original_brk = target_brk = HOST_PAGE_ALIGN(new_brk); + target_brk = new_brk; brk_page = HOST_PAGE_ALIGN(target_brk); } -//#define DEBUGF_BRK(message, args...) do { fprintf(stderr, (message), ## args); } while (0) -#define DEBUGF_BRK(message, args...) - /* do_brk() must return target values and target errnos. */ -abi_long do_brk(abi_ulong new_brk) +abi_long do_brk(abi_ulong brk_val) { abi_long mapped_addr; abi_ulong new_alloc_size; + abi_ulong new_brk, new_host_brk_page; /* brk pointers are always untagged */ - DEBUGF_BRK("do_brk(" TARGET_ABI_FMT_lx ") -> ", new_brk); - - if (!new_brk) { - DEBUGF_BRK(TARGET_ABI_FMT_lx " (!new_brk)\n", target_brk); + /* return old brk value if brk_val unchanged or zero */ + if (!brk_val || brk_val == target_brk) { return target_brk; } - if (new_brk < target_original_brk) { - DEBUGF_BRK(TARGET_ABI_FMT_lx " (new_brk < target_original_brk)\n", - target_brk); + + new_brk = TARGET_PAGE_ALIGN(brk_val); + new_host_brk_page = HOST_PAGE_ALIGN(brk_val); + + /* brk_val and old target_brk might be on the same page */ + if (new_brk == TARGET_PAGE_ALIGN(target_brk)) { + if (brk_val > target_brk) { + /* empty remaining bytes in (possibly larger) host page */ + memset(g2h_untagged(target_brk), 0, new_host_brk_page - target_brk); + } + target_brk = brk_val; return target_brk; } - /* If the new brk is less than the highest page reserved to the - * target heap allocation, set it and we're almost done... */ - if (new_brk <= brk_page) { - /* Heap contents are initialized to zero, as for anonymous - * mapped pages. */ - if (new_brk > target_brk) { - memset(g2h_untagged(target_brk), 0, new_brk - target_brk); - } - target_brk = new_brk; - DEBUGF_BRK(TARGET_ABI_FMT_lx " (new_brk <= brk_page)\n", target_brk); - return target_brk; + /* Release heap if necesary */ + if (new_brk < target_brk) { + /* empty remaining bytes in (possibly larger) host page */ + memset(g2h_untagged(brk_val), 0, new_host_brk_page - brk_val); + + /* free unused host pages and set new brk_page */ + target_munmap(new_host_brk_page, brk_page - new_host_brk_page); + brk_page = new_host_brk_page; + + target_brk = brk_val; + return target_brk; } /* We need to allocate more memory after the brk... Note that @@ -889,10 +892,14 @@ abi_long do_brk(abi_ulong new_brk) * itself); instead we treat "mapped but at wrong address" as * a failure and unmap again. */ - new_alloc_size = HOST_PAGE_ALIGN(new_brk - brk_page); - mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size, + new_alloc_size = new_host_brk_page - brk_page; + if (new_alloc_size) { + mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size, PROT_READ|PROT_WRITE, MAP_ANON|MAP_PRIVATE, 0, 0)); + } else { + mapped_addr = brk_page; + } if (mapped_addr == brk_page) { /* Heap contents are initialized to zero, as for anonymous @@ -904,10 +911,8 @@ abi_long do_brk(abi_ulong new_brk) * then shrunken). */ memset(g2h_untagged(target_brk), 0, brk_page - target_brk); - target_brk = new_brk; - brk_page = HOST_PAGE_ALIGN(target_brk); - DEBUGF_BRK(TARGET_ABI_FMT_lx " (mapped_addr == brk_page)\n", - target_brk); + target_brk = brk_val; + brk_page = new_host_brk_page; return target_brk; } else if (mapped_addr != -1) { /* Mapped but at wrong address, meaning there wasn't actually @@ -915,10 +920,6 @@ abi_long do_brk(abi_ulong new_brk) */ target_munmap(mapped_addr, new_alloc_size); mapped_addr = -1; - DEBUGF_BRK(TARGET_ABI_FMT_lx " (mapped_addr != -1)\n", target_brk); - } - else { - DEBUGF_BRK(TARGET_ABI_FMT_lx " (otherwise)\n", target_brk); } #if defined(TARGET_ALPHA)
The current brk() implementation does not de-allocate pages if a lower address is given compared to earlier brk() calls. But according to the manpage, brk() shall deallocate memory in this case and currently it breaks a real-world application, specifically building the debian gcl package in qemu-user. Fix this issue by reworking the qemu brk() implementation. Tested with the C-code testcase included in qemu commit 4d1de87c750, and by building debian package of gcl in a hppa-linux guest on a x86-64 host. Signed-off-by: Helge Deller <deller@gmx.de> --- v3: - Fixed one bug where page aligned address was returned instead of requested address. - Dropped debug info which is partly outdated now - Reduced number of changed lines in diff compared to v2 to make diff easier readable - Fixed changelog of v2 v2: - Fixed return value of brk(). The v1 version wrongly page-aligned the provided address, while userspace expects an unmodified address returned.