diff mbox series

[for-8.1,v10,08/14] linux-user: Do not adjust zero_bss for host page size

Message ID 20230807163705.9848-9-richard.henderson@linaro.org
State New
Headers show
Series linux-user: image mapping fixes | expand

Commit Message

Richard Henderson Aug. 7, 2023, 4:36 p.m. UTC 
Rely on target_mmap to handle guest vs host page size mismatch.

Tested-by: Helge Deller <deller@gmx.de>
Reviewed-by: Helge Deller <deller@gmx.de>
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/elfload.c | 54 +++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 31 deletions(-)

Comments

Alex Bennée Aug. 8, 2023, 11:38 a.m. UTC | #1 
Richard Henderson <richard.henderson@linaro.org> writes:

> Rely on target_mmap to handle guest vs host page size mismatch.
>
> Tested-by: Helge Deller <deller@gmx.de>
> Reviewed-by: Helge Deller <deller@gmx.de>
> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/elfload.c | 54 +++++++++++++++++++-------------------------
>  1 file changed, 23 insertions(+), 31 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 964b21f997..6c28cb70ef 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2213,44 +2213,36 @@ static abi_ulong setup_arg_pages(struct linux_binprm *bprm,
>  
>  /* Map and zero the bss.  We need to explicitly zero any fractional pages
>     after the data section (i.e. bss).  */
> -static void zero_bss(abi_ulong elf_bss, abi_ulong last_bss, int prot)
> +static void zero_bss(abi_ulong start_bss, abi_ulong end_bss, int prot)
>  {
> -    uintptr_t host_start, host_map_start, host_end;
> +    abi_ulong align_bss;
>  
> -    last_bss = TARGET_PAGE_ALIGN(last_bss);
> +    align_bss = TARGET_PAGE_ALIGN(start_bss);
> +    end_bss = TARGET_PAGE_ALIGN(end_bss);
>  
> -    /* ??? There is confusion between qemu_real_host_page_size and
> -       qemu_host_page_size here and elsewhere in target_mmap, which
> -       may lead to the end of the data section mapping from the file
> -       not being mapped.  At least there was an explicit test and
> -       comment for that here, suggesting that "the file size must
> -       be known".  The comment probably pre-dates the introduction
> -       of the fstat system call in target_mmap which does in fact
> -       find out the size.  What isn't clear is if the workaround
> -       here is still actually needed.  For now, continue with it,
> -       but merge it with the "normal" mmap that would allocate the bss.  */
> +    if (start_bss < align_bss) {
> +        int flags = page_get_flags(start_bss);
>  
> -    host_start = (uintptr_t) g2h_untagged(elf_bss);
> -    host_end = (uintptr_t) g2h_untagged(last_bss);
> -    host_map_start = REAL_HOST_PAGE_ALIGN(host_start);
> -
> -    if (host_map_start < host_end) {
> -        void *p = mmap((void *)host_map_start, host_end - host_map_start,
> -                       prot, MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> -        if (p == MAP_FAILED) {
> -            perror("cannot mmap brk");
> -            exit(-1);
> +        if (!(flags & PAGE_VALID)) {
> +            /* Map the start of the bss. */
> +            align_bss -= TARGET_PAGE_SIZE;
> +        } else if (flags & PAGE_WRITE) {
> +            /* The page is already mapped writable. */
> +            memset(g2h_untagged(start_bss), 0, align_bss - start_bss);
> +        } else {
> +            /* Read-only zeros? */
> +            g_assert_not_reached();
>          }
>      }
>  
> -    /* Ensure that the bss page(s) are valid */
> -    if ((page_get_flags(last_bss-1) & prot) != prot) {
> -        page_set_flags(elf_bss & TARGET_PAGE_MASK, last_bss - 1,
> -                       prot | PAGE_VALID);
> -    }
> -
> -    if (host_start < host_map_start) {
> -        memset((void *)host_start, 0, host_map_start - host_start);
> +    if (align_bss < end_bss) {
> +        abi_long err = target_mmap(align_bss, end_bss - align_bss, prot,
> +                                   MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS,
> +                                   -1, 0);
> +        if (err == -1) {
> +            perror("cannot mmap brk");
> +            exit(-1);

brk != bss even if brk generally comes after the bss section.

> +        }
>      }
>  }
Richard Henderson Aug. 8, 2023, 3:56 p.m. UTC | #2 
On 8/8/23 04:38, Alex Bennée wrote:
>> -    if (host_start < host_map_start) {
>> -        memset((void *)host_start, 0, host_map_start - host_start);
>> +    if (align_bss < end_bss) {
>> +        abi_long err = target_mmap(align_bss, end_bss - align_bss, prot,
>> +                                   MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS,
>> +                                   -1, 0);
>> +        if (err == -1) {
>> +            perror("cannot mmap brk");
>> +            exit(-1);
> 
> brk != bss even if brk generally comes after the bss section.

I've removed this error report entirely, returning failure to the caller, which will then 
handle it like any other mmap failure of the image.


r~
diff mbox series

Patch

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 964b21f997..6c28cb70ef 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2213,44 +2213,36 @@  static abi_ulong setup_arg_pages(struct linux_binprm *bprm,
 
 /* Map and zero the bss.  We need to explicitly zero any fractional pages
    after the data section (i.e. bss).  */
-static void zero_bss(abi_ulong elf_bss, abi_ulong last_bss, int prot)
+static void zero_bss(abi_ulong start_bss, abi_ulong end_bss, int prot)
 {
-    uintptr_t host_start, host_map_start, host_end;
+    abi_ulong align_bss;
 
-    last_bss = TARGET_PAGE_ALIGN(last_bss);
+    align_bss = TARGET_PAGE_ALIGN(start_bss);
+    end_bss = TARGET_PAGE_ALIGN(end_bss);
 
-    /* ??? There is confusion between qemu_real_host_page_size and
-       qemu_host_page_size here and elsewhere in target_mmap, which
-       may lead to the end of the data section mapping from the file
-       not being mapped.  At least there was an explicit test and
-       comment for that here, suggesting that "the file size must
-       be known".  The comment probably pre-dates the introduction
-       of the fstat system call in target_mmap which does in fact
-       find out the size.  What isn't clear is if the workaround
-       here is still actually needed.  For now, continue with it,
-       but merge it with the "normal" mmap that would allocate the bss.  */
+    if (start_bss < align_bss) {
+        int flags = page_get_flags(start_bss);
 
-    host_start = (uintptr_t) g2h_untagged(elf_bss);
-    host_end = (uintptr_t) g2h_untagged(last_bss);
-    host_map_start = REAL_HOST_PAGE_ALIGN(host_start);
-
-    if (host_map_start < host_end) {
-        void *p = mmap((void *)host_map_start, host_end - host_map_start,
-                       prot, MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
-        if (p == MAP_FAILED) {
-            perror("cannot mmap brk");
-            exit(-1);
+        if (!(flags & PAGE_VALID)) {
+            /* Map the start of the bss. */
+            align_bss -= TARGET_PAGE_SIZE;
+        } else if (flags & PAGE_WRITE) {
+            /* The page is already mapped writable. */
+            memset(g2h_untagged(start_bss), 0, align_bss - start_bss);
+        } else {
+            /* Read-only zeros? */
+            g_assert_not_reached();
         }
     }
 
-    /* Ensure that the bss page(s) are valid */
-    if ((page_get_flags(last_bss-1) & prot) != prot) {
-        page_set_flags(elf_bss & TARGET_PAGE_MASK, last_bss - 1,
-                       prot | PAGE_VALID);
-    }
-
-    if (host_start < host_map_start) {
-        memset((void *)host_start, 0, host_map_start - host_start);
+    if (align_bss < end_bss) {
+        abi_long err = target_mmap(align_bss, end_bss - align_bss, prot,
+                                   MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS,
+                                   -1, 0);
+        if (err == -1) {
+            perror("cannot mmap brk");
+            exit(-1);
+        }
     }
 }