qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Warner Losh <imp@bsdimp.com>
To: qemu-devel@nongnu.org
Cc: Warner Losh <imp@bsdimp.com>, Kyle Evans <kevans@freebsd.org>,
	Jessica Clarke <jrtc27@jrtc27.com>
Subject: [PATCH 11/17] bsd-user: Replace set_brk and padzero with zerobss from linux-user
Date: Fri,  2 Aug 2024 17:56:11 -0600	[thread overview]
Message-ID: <20240802235617.7971-12-imp@bsdimp.com> (raw)
In-Reply-To: <20240802235617.7971-1-imp@bsdimp.com>

The zero_bss interface from linux-user is much better at doing this. Use
it in preference to set_brk (badly named) and padzero. These both have
issues with the new variable page size code, so it's best to just retire
them and reuse the code from linux-user. Also start to use the error
reporting code that linux-user uses to give better error messages on
failure.

Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/elfload.c | 110 +++++++++++++++++++++++----------------------
 1 file changed, 57 insertions(+), 53 deletions(-)

diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
index dba03f17465..0a2f2379c93 100644
--- a/bsd-user/elfload.c
+++ b/bsd-user/elfload.c
@@ -22,6 +22,7 @@
 #include "qemu.h"
 #include "disas/disas.h"
 #include "qemu/path.h"
+#include "qapi/error.h"
 
 static abi_ulong target_auxents;   /* Where the AUX entries are in target */
 static size_t target_auxents_sz;   /* Size of AUX entries including AT_NULL */
@@ -210,62 +211,63 @@ static void setup_arg_pages(struct bsd_binprm *bprm, struct image_info *info,
     }
 }
 
-static void set_brk(abi_ulong start, abi_ulong end)
+/**
+ * zero_bss:
+ *
+ * Map and zero the bss.  We need to explicitly zero any fractional pages
+ * after the data section (i.e. bss).  Return false on mapping failure.
+ */
+static bool zero_bss(abi_ulong start_bss, abi_ulong end_bss,
+                     int prot, Error **errp)
 {
-    /* page-align the start and end addresses... */
-    start = HOST_PAGE_ALIGN(start);
-    end = HOST_PAGE_ALIGN(end);
-    if (end <= start) {
-        return;
-    }
-    if (target_mmap(start, end - start, PROT_READ | PROT_WRITE | PROT_EXEC,
-        MAP_FIXED | MAP_PRIVATE | MAP_ANON, -1, 0) == -1) {
-        perror("cannot mmap brk");
-        exit(-1);
+    abi_ulong align_bss;
+
+    /* We only expect writable bss; the code segment shouldn't need this. */
+    if (!(prot & PROT_WRITE)) {
+        error_setg(errp, "PT_LOAD with non-writable bss");
+        return false;
     }
-}
 
+    align_bss = TARGET_PAGE_ALIGN(start_bss);
+    end_bss = TARGET_PAGE_ALIGN(end_bss);
 
-/*
- * We need to explicitly zero any fractional pages after the data
- * section (i.e. bss).  This would contain the junk from the file that
- * should not be in memory.
- */
-static void padzero(abi_ulong elf_bss, abi_ulong last_bss)
-{
-    abi_ulong nbyte;
+    if (start_bss < align_bss) {
+        int flags = page_get_flags(start_bss);
 
-    if (elf_bss >= last_bss) {
-        return;
-    }
+        if (!(flags & PAGE_RWX)) {
+            /*
+             * The whole address space of the executable was reserved
+             * at the start, therefore all pages will be VALID.
+             * But assuming there are no PROT_NONE PT_LOAD segments,
+             * a PROT_NONE page means no data all bss, and we can
+             * simply extend the new anon mapping back to the start
+             * of the page of bss.
+             */
+            align_bss -= TARGET_PAGE_SIZE;
+        } else {
+            /*
+             * The start of the bss shares a page with something.
+             * The only thing that we expect is the data section,
+             * which would already be marked writable.
+             * Overlapping the RX code segment seems malformed.
+             */
+            if (!(flags & PAGE_WRITE)) {
+                error_setg(errp, "PT_LOAD with bss overlapping "
+                           "non-writable page");
+                return false;
+            }
 
-    /*
-     * XXX: this is really a hack : if the real host page size is
-     * smaller than the target page size, some pages after the end
-     * of the file may not be mapped. A better fix would be to
-     * patch target_mmap(), but it is more complicated as the file
-     * size must be known.
-     */
-    if (qemu_real_host_page_size() < qemu_host_page_size) {
-        abi_ulong end_addr, end_addr1;
-        end_addr1 = REAL_HOST_PAGE_ALIGN(elf_bss);
-        end_addr = HOST_PAGE_ALIGN(elf_bss);
-        if (end_addr1 < end_addr) {
-            mmap((void *)g2h_untagged(end_addr1), end_addr - end_addr1,
-                 PROT_READ | PROT_WRITE | PROT_EXEC,
-                 MAP_FIXED | MAP_PRIVATE | MAP_ANON, -1, 0);
+            /* The page is already mapped and writable. */
+            memset(g2h_untagged(start_bss), 0, align_bss - start_bss);
         }
     }
-
-    nbyte = elf_bss & (qemu_host_page_size - 1);
-    if (nbyte) {
-        nbyte = qemu_host_page_size - nbyte;
-        do {
-            /* FIXME - what to do if put_user() fails? */
-            put_user_u8(0, elf_bss);
-            elf_bss++;
-        } while (--nbyte);
+    if (align_bss < end_bss &&
+        target_mmap(align_bss, end_bss - align_bss, prot,
+                    MAP_FIXED | MAP_PRIVATE | MAP_ANON, -1, 0) == -1) {
+        error_setg_errno(errp, errno, "Error mapping bss");
+        return false;
     }
+    return true;
 }
 
 static abi_ulong load_elf_interp(const char *elf_interpreter,
@@ -535,6 +537,7 @@ load_elf_sections(const char *image_name, const struct elfhdr *hdr,
     abi_ulong baddr;
     int i;
     bool first;
+    Error *err = NULL;
 
     /*
      * Now we do a little grungy work by mmaping the ELF image into
@@ -579,12 +582,10 @@ load_elf_sections(const char *image_name, const struct elfhdr *hdr,
             start_bss = rbase + elf_ppnt->p_vaddr + elf_ppnt->p_filesz;
             end_bss = rbase + elf_ppnt->p_vaddr + elf_ppnt->p_memsz;
 
-            /*
-             * Calling set_brk effectively mmaps the pages that we need for the
-             * bss and break sections.
-             */
-            set_brk(start_bss, end_bss);
-            padzero(start_bss, end_bss);
+            if (start_bss < end_bss &&
+                !zero_bss(start_bss, end_bss, elf_prot, &err)) {
+                goto exit_errmsg;
+            }
         }
 
         if (first) {
@@ -597,6 +598,9 @@ load_elf_sections(const char *image_name, const struct elfhdr *hdr,
         *baddrp = baddr;
     }
     return 0;
+exit_errmsg:
+    error_reportf_err(err, "%s: ", image_name);
+    exit(-1);
 }
 
 int load_elf_binary(struct bsd_binprm *bprm, struct image_info *info)
-- 
2.45.1



  parent reply	other threads:[~2024-08-02 23:59 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-08-02 23:56 [PATCH 00/17] For 9.2: A bunch of cleanups and work towards variable pagesize support Warner Losh
2024-08-02 23:56 ` [PATCH 01/17] bsd-user: Delete TaskState next member Warner Losh
2024-08-04  7:07   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 02/17] bsd-user: Make init_task_state global Warner Losh
2024-08-04  7:08   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 03/17] bsd-user: Make cpu_model and cpu_type file scope Warner Losh
2024-08-04  7:22   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 04/17] bsd-user: Implement cpu_copy() Warner Losh
2024-08-04  7:24   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 05/17] bsd-user: Eliminate unused regs arg in load_elf_binary Warner Losh
2024-08-04  7:26   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 06/17] bsd-user: Remove load_flt_binary prototype Warner Losh
2024-08-04  7:26   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 07/17] bsd-user: Remove deprecated -p argument Warner Losh
2024-08-04  7:26   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 08/17] bsd-user: Eliminate unused qemu_uname_release Warner Losh
2024-08-04  7:27   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 09/17] bsd-user: target_msync unused, remove it Warner Losh
2024-08-04  7:28   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 10/17] bsd-user: Pass image name down the stack Warner Losh
2024-08-04  7:29   ` Richard Henderson
2024-08-02 23:56 ` Warner Losh [this message]
2024-08-04 11:38   ` [PATCH 11/17] bsd-user: Replace set_brk and padzero with zerobss from linux-user Richard Henderson
2024-08-02 23:56 ` [PATCH 12/17] bsd-user: Use guest_range_valid_untagged to validate range Warner Losh
2024-08-04 21:30   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 13/17] bsd-user: target_mprotect: rename prot to target_prot Warner Losh
2024-08-04 21:31   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 14/17] bsd-user: target_mmap*: change " Warner Losh
2024-08-04 21:32   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 15/17] bsd-user: target_mprotect: use helper host_page_size local Warner Losh
2024-08-04 21:33   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 16/17] bsd-user: Define validate_prot_to_pageflags and use in mprotect Warner Losh
2024-08-04 21:44   ` Richard Henderson
2024-08-02 23:56 ` [PATCH 17/17] bsd-user: copy linux-user target_mprotect impl Warner Losh
2024-08-04 21:47   ` Richard Henderson

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=20240802235617.7971-12-imp@bsdimp.com \
    --to=imp@bsdimp.com \
    --cc=jrtc27@jrtc27.com \
    --cc=kevans@freebsd.org \
    --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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).