* [PATCH] linux-user: Fix brk() to release pages
@ 2022-12-23 10:16 Helge Deller
2022-12-23 21:03 ` [PATCH v2] " Helge Deller
0 siblings, 1 reply; 4+ messages in thread
From: Helge Deller @ 2022-12-23 10:16 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel; +Cc: Helge Deller
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>
---
linux-user/syscall.c | 43 +++++++++++++++++++++++--------------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 4fee882cd7..d306b02e21 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -838,13 +838,12 @@ 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);
- brk_page = HOST_PAGE_ALIGN(target_brk);
+ target_brk = TARGET_PAGE_ALIGN(new_brk);
+ brk_page = HOST_PAGE_ALIGN(new_brk);
}
//#define DEBUGF_BRK(message, args...) do { fprintf(stderr, (message), ## args); } while (0)
@@ -855,29 +854,29 @@ abi_long do_brk(abi_ulong new_brk)
{
abi_long mapped_addr;
abi_ulong new_alloc_size;
+ abi_ulong new_host_brk_page;
/* brk pointers are always untagged */
+ new_brk = TARGET_PAGE_ALIGN(new_brk);
+ new_host_brk_page = HOST_PAGE_ALIGN(new_brk);
+
DEBUGF_BRK("do_brk(" TARGET_ABI_FMT_lx ") -> ", new_brk);
- if (!new_brk) {
+ if (!new_brk || new_brk == target_brk) {
DEBUGF_BRK(TARGET_ABI_FMT_lx " (!new_brk)\n", 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);
- 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);
- }
+ /* Release heap if necesary */
+ if (new_brk < target_brk) {
+ /* empty remaining bytes in (possibly larger) host page */
+ memset(g2h_untagged(new_brk), 0, new_host_brk_page - new_brk);
+
+ /* 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 = new_brk;
DEBUGF_BRK(TARGET_ABI_FMT_lx " (new_brk <= brk_page)\n", target_brk);
return target_brk;
@@ -889,10 +888,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
@@ -905,7 +908,7 @@ abi_long do_brk(abi_ulong new_brk)
memset(g2h_untagged(target_brk), 0, brk_page - target_brk);
target_brk = new_brk;
- brk_page = HOST_PAGE_ALIGN(target_brk);
+ brk_page = new_host_brk_page;
DEBUGF_BRK(TARGET_ABI_FMT_lx " (mapped_addr == brk_page)\n",
target_brk);
return target_brk;
--
2.38.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2] linux-user: Fix brk() to release pages
2022-12-23 10:16 [PATCH] linux-user: Fix brk() to release pages Helge Deller
@ 2022-12-23 21:03 ` Helge Deller
2022-12-25 8:23 ` [PATCH v3] " Helge Deller
0 siblings, 1 reply; 4+ messages in thread
From: Helge Deller @ 2022-12-23 21:03 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel
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>
---
v2:
- Fixed return value of brk(). The v1 version wrongly page-aligned the
provided address, while userspace expects unmodified the address
returned unmodified.
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 4fee882cd7..2fcb6dba06 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -838,49 +838,59 @@ 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);
- brk_page = HOST_PAGE_ALIGN(target_brk);
+ target_brk = new_brk;
+ brk_page = HOST_PAGE_ALIGN(new_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 */
+ /* return old brk value on zero brk_val */
+ if (!brk_val || brk_val == target_brk) {
+ return target_brk;
+ }
+
+ new_brk = TARGET_PAGE_ALIGN(brk_val);
+ new_host_brk_page = HOST_PAGE_ALIGN(brk_val);
+
DEBUGF_BRK("do_brk(" TARGET_ABI_FMT_lx ") -> ", new_brk);
- if (!new_brk) {
+ /* 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);
+ }
DEBUGF_BRK(TARGET_ABI_FMT_lx " (!new_brk)\n", 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);
- return target_brk;
+ target_brk = brk_val;
+ return brk_val;
}
- /* 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;
+ /* 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;
DEBUGF_BRK(TARGET_ABI_FMT_lx " (new_brk <= brk_page)\n", target_brk);
- return target_brk;
+ return brk_val;
}
/* We need to allocate more memory after the brk... Note that
@@ -889,10 +899,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
@@ -905,10 +919,10 @@ abi_long do_brk(abi_ulong new_brk)
memset(g2h_untagged(target_brk), 0, brk_page - target_brk);
target_brk = new_brk;
- brk_page = HOST_PAGE_ALIGN(target_brk);
+ brk_page = new_host_brk_page;
DEBUGF_BRK(TARGET_ABI_FMT_lx " (mapped_addr == brk_page)\n",
target_brk);
- return target_brk;
+ return brk_val;
} else if (mapped_addr != -1) {
/* Mapped but at wrong address, meaning there wasn't actually
* enough space for this brk.
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v3] linux-user: Fix brk() to release pages
2022-12-23 21:03 ` [PATCH v2] " Helge Deller
@ 2022-12-25 8:23 ` Helge Deller
2023-03-07 14:27 ` Laurent Vivier
0 siblings, 1 reply; 4+ messages in thread
From: Helge Deller @ 2022-12-25 8:23 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel
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)
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] linux-user: Fix brk() to release pages
2022-12-25 8:23 ` [PATCH v3] " Helge Deller
@ 2023-03-07 14:27 ` Laurent Vivier
0 siblings, 0 replies; 4+ messages in thread
From: Laurent Vivier @ 2023-03-07 14:27 UTC (permalink / raw)
To: Helge Deller, qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-03-07 14:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-23 10:16 [PATCH] linux-user: Fix brk() to release pages Helge Deller
2022-12-23 21:03 ` [PATCH v2] " Helge Deller
2022-12-25 8:23 ` [PATCH v3] " Helge Deller
2023-03-07 14:27 ` Laurent Vivier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.