All of lore.kernel.org
 help / color / mirror / Atom feed
From: Helge Deller <deller@gmx.de>
To: Laurent Vivier <laurent@vivier.eu>, qemu-devel@nongnu.org
Cc: Helge Deller <deller@gmx.de>
Subject: [PATCH] linux-user: Fix brk() to release pages
Date: Fri, 23 Dec 2022 11:16:28 +0100	[thread overview]
Message-ID: <20221223101626.19215-1-deller@gmx.de> (raw)

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



             reply	other threads:[~2022-12-23 10:18 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-23 10:16 Helge Deller [this message]
2022-12-23 21:03 ` [PATCH v2] linux-user: Fix brk() to release pages Helge Deller
2022-12-25  8:23   ` [PATCH v3] " Helge Deller
2023-03-07 14:27     ` Laurent Vivier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221223101626.19215-1-deller@gmx.de \
    --to=deller@gmx.de \
    --cc=laurent@vivier.eu \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.