[PATCH] linux-user/elfload: Fix handling of pure BSS segments

[PATCH] linux-user/elfload: Fix handling of pure BSS segments

[Stephen Long]

qemu-user fails to load ELFs with only BSS and no data section

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Stephen Long <steplong@quicinc.com>
---

Submitting this on behalf of Ben Hutchings. Feel free to edit the commit
msg.

 linux-user/elfload.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 0b02a92602..af16d94c61 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2783,7 +2783,7 @@ static void load_elf_image(const char *image_name, int image_fd,
              * segment, in that case just let zero_bss allocate an empty buffer
              * for it.
              */
-            if (eppnt->p_filesz != 0) {
+            if (vaddr_len != 0) {
                 error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
                                     MAP_PRIVATE | MAP_FIXED,
                                     image_fd, eppnt->p_offset - vaddr_po);
-- 
2.25.1

Re: [PATCH] linux-user/elfload: Fix handling of pure BSS segments

[Richard Henderson]

On 11/18/20 8:52 AM, Stephen Long wrote:
> qemu-user fails to load ELFs with only BSS and no data section
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Stephen Long <steplong@quicinc.com>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH] linux-user/elfload: Fix handling of pure BSS segments

[Peter Maydell]

On Wed, 18 Nov 2020 at 16:55, Stephen Long <steplong@quicinc.com> wrote:
>
> qemu-user fails to load ELFs with only BSS and no data section
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Stephen Long <steplong@quicinc.com>
> ---
>
> Submitting this on behalf of Ben Hutchings. Feel free to edit the commit
> msg.
>
>  linux-user/elfload.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 0b02a92602..af16d94c61 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2783,7 +2783,7 @@ static void load_elf_image(const char *image_name, int image_fd,
>               * segment, in that case just let zero_bss allocate an empty buffer
>               * for it.
>               */
> -            if (eppnt->p_filesz != 0) {
> +            if (vaddr_len != 0) {
>                  error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
>                                      MAP_PRIVATE | MAP_FIXED,
>                                      image_fd, eppnt->p_offset - vaddr_po);

So (having run into a different instance of this elsewhere), a
couple of questions:

(a) what does "fails to load" mean here? In the sample binary
I had, we got a SIGSEGV in zero_bss() when it tried to memset()
memory that hadn't been mmap()ed. Is that the only failure mode,
or can this manifest in other ways too?

(b) The comment immediately before this change says:
     * Some segments may be completely empty without any backing file
     * segment, in that case just let zero_bss allocate an empty buffer
     * for it.
which is justifying why it was looking at p_filesz and not vaddr_len.
With this change to the code, the comment becomes stale and needs
updating.

(c) After this change, are there still cases where zero_bss()
needs to do its mmap()/page_set_flags(), or does that become
unnecessary ?

thanks
-- PMM

Re: Re: [PATCH] linux-user/elfload: Fix handling of pure BSS segments

[Stephen Long]

Hi Peter, 

> (a) what does "fails to load" mean here? In the sample binary
> I had, we got a SIGSEGV in zero_bss() when it tried to memset()
> memory that hadn't been mmap()ed. Is that the only failure mode,
> or can this manifest in other ways too?

Apologies for the unclear commit msg. I was also seeing a SIGSEGV in
zero_bss() with the binaries I was generating. I was using LLD to generate
the binaries. The binaries all had LOAD segments with a file size of 0.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=919921 was the thread that
Philippe pointed me to with the requested change that solved my issue.

> (b) The comment immediately before this change says:
>    * Some segments may be completely empty without any backing file
>    * segment, in that case just let zero_bss allocate an empty buffer
>    * for it.
> which is justifying why it was looking at p_filesz and not vaddr_len.
> With this change to the code, the comment becomes stale and needs
> updating.

I'll update the comment and the commit msg if this change makes sense to
everybody.

> (c) After this change, are there still cases where zero_bss()
> needs to do its mmap()/page_set_flags(), or does that become
> unnecessary ?

Maybe someone else can speak to this. But, you might be right. I don't see
this being necessary anymore.

Thanks,
Stephen

Re: [PATCH] linux-user/elfload: Fix handling of pure BSS segments

[Alex Bennée]

Stephen Long <steplong@quicinc.com> writes:

> Hi Peter, 
>
>> (a) what does "fails to load" mean here? In the sample binary
>> I had, we got a SIGSEGV in zero_bss() when it tried to memset()
>> memory that hadn't been mmap()ed. Is that the only failure mode,
>> or can this manifest in other ways too?
>
> Apologies for the unclear commit msg. I was also seeing a SIGSEGV in
> zero_bss() with the binaries I was generating. I was using LLD to generate
> the binaries. The binaries all had LOAD segments with a file size of
> 0.

How hairy is the generation of these binaries? If it's all doable with
standard gcc/ldd command lines it would be useful to add them as a
tcg/multiarch test case.

>
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=919921 was the thread that
> Philippe pointed me to with the requested change that solved my issue.
>
>> (b) The comment immediately before this change says:
>>    * Some segments may be completely empty without any backing file
>>    * segment, in that case just let zero_bss allocate an empty buffer
>>    * for it.
>> which is justifying why it was looking at p_filesz and not vaddr_len.
>> With this change to the code, the comment becomes stale and needs
>> updating.
>
> I'll update the comment and the commit msg if this change makes sense to
> everybody.
>
>> (c) After this change, are there still cases where zero_bss()
>> needs to do its mmap()/page_set_flags(), or does that become
>> unnecessary ?
>
> Maybe someone else can speak to this. But, you might be right. I don't see
> this being necessary anymore.
>
> Thanks,
> Stephen


-- 
Alex Bennée

Re: [PATCH] linux-user/elfload: Fix handling of pure BSS segments

[Stephen Long]

Alex Bennee <alex.bennee@linaro.org> writes:

>> Apologies for the unclear commit msg. I was also seeing a SIGSEGV in
>> zero_bss() with the binaries I was generating. I was using LLD to generate
>> the binaries. The binaries all had LOAD segments with a file size of
>> 0.
>
> How hairy is the generation of these binaries? If it's all doable with
> standard gcc/ldd command lines it would be useful to add them as a
> tcg/multiarch test case.

We are linking with an old version of musl. I was able to produce an
ELF with a LOAD segment just for the BSS with the following:

volatile int num;

int main(void) {
    return num;
}

and compiling it with just aarch64-linux-gnu-gcc -fuse-ld=lld -static and
linking with cross compiled musl v1.1.9 on Ubuntu. I tried it with glibc and
it has a bunch of non-BSS variables, so the data section gets created.

Re: [PATCH] linux-user/elfload: Fix handling of pure BSS segments

[Alex Bennée]

Stephen Long <steplong@quicinc.com> writes:

> Alex Bennee <alex.bennee@linaro.org> writes:
>
>>> Apologies for the unclear commit msg. I was also seeing a SIGSEGV in
>>> zero_bss() with the binaries I was generating. I was using LLD to generate
>>> the binaries. The binaries all had LOAD segments with a file size of
>>> 0.
>>
>> How hairy is the generation of these binaries? If it's all doable with
>> standard gcc/ldd command lines it would be useful to add them as a
>> tcg/multiarch test case.
>
> We are linking with an old version of musl. I was able to produce an
> ELF with a LOAD segment just for the BSS with the following:
>
> volatile int num;
>
> int main(void) {
>     return num;
> }
>
> and compiling it with just aarch64-linux-gnu-gcc -fuse-ld=lld -static and
> linking with cross compiled musl v1.1.9 on Ubuntu. I tried it with glibc and
> it has a bunch of non-BSS variables, so the data section gets created.

Hmm I tried the following patch but evidently there is more to be done
to convince it:

  13:26:24 [alex@zen:~/l/q/b/arm.all] virtio/vhost-user-rpmb-v2|✚4…(+6/-3) 2 + make build-tcg-tests-aarch64-linux-user -j9 V=1
  make  -f /home/alex/lsrc/qemu.git/tests/tcg/Makefile.qemu SRC_PATH=/home/alex/lsrc/qemu.git V="1" TARGET="aarch64-linux-user" guest-tests
  make[1]: Entering directory '/home/alex/lsrc/qemu.git/builds/arm.all'
  (mkdir -p tests/tcg/aarch64-linux-user && cd tests/tcg/aarch64-linux-user && make -f ../Makefile.target TARGET="aarch64-linux-user" CC="aarch64-linux-gnu-gcc" SRC_PATH="/home/alex/lsrc/qemu.git" BUILD_STATIC=y EXTRA_CFLAGS="")
  make[2]: Entering directory '/home/alex/lsrc/qemu.git/builds/arm.all/tests/tcg/aarch64-linux-user'
  aarch64-linux-gnu-gcc  -Wall -O0 -g -fno-strict-aliasing  /home/alex/lsrc/qemu.git/tests/tcg/aarch64/zero-bss.c -o zero-bss  -static -fuse-ld=lld
  collect2: fatal error: cannot find 'ld'
  compilation terminated.
  make[2]: *** [../Makefile.target:103: zero-bss] Error 1
  make[2]: Leaving directory '/home/alex/lsrc/qemu.git/builds/arm.all/tests/tcg/aarch64-linux-user'
  make[1]: *** [/home/alex/lsrc/qemu.git/tests/tcg/Makefile.qemu:42: cross-build-guest-tests] Error 2
  make[1]: Leaving directory '/home/alex/lsrc/qemu.git/builds/arm.all'
  make: *** [/home/alex/lsrc/qemu.git/tests/Makefile.include:54: build-tcg-tests-aarch64-linux-user] Error 2


--8<---------------cut here---------------start------------->8---
tests/tcg: try and add a zero-bss test case (WIP)

3 files changed, 17 insertions(+), 1 deletion(-)
tests/tcg/aarch64/zero-bss.c                            | 13 +++++++++++++
tests/docker/dockerfiles/debian-arm64-test-cross.docker |  2 +-
tests/tcg/aarch64/Makefile.target                       |  3 +++

new file   tests/tcg/aarch64/zero-bss.c
@@ -0,0 +1,13 @@
+/*
+ * Zero BSS Test case
+ *
+ * Copyright (c) 2020 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+volatile int num;
+
+int main(void) {
+    return num;
+}
modified   tests/docker/dockerfiles/debian-arm64-test-cross.docker
@@ -10,4 +10,4 @@ RUN dpkg --add-architecture arm64
 RUN apt update && \
     DEBIAN_FRONTEND=noninteractive eatmydata \
         apt install -y --no-install-recommends \
-        crossbuild-essential-arm64 gcc-10-aarch64-linux-gnu
+        crossbuild-essential-arm64 gcc-10-aarch64-linux-gnu musl-dev:arm64
modified   tests/tcg/aarch64/Makefile.target
@@ -84,4 +84,7 @@ endif
 
 endif
 
+AARCH64_TESTS += zero-bss
+zero-bss: LDFLAGS+=-fuse-ld=lld
+
 TESTS += $(AARCH64_TESTS)
--8<---------------cut here---------------end--------------->8---

-- 
Alex Bennée

Re: [PATCH] linux-user/elfload: Fix handling of pure BSS segments

[Stephen Long]

Alex Bennee <alex.bennee@linaro.org> writes:

> Hmm I tried the following patch but evidently there is more to be done
> to convince it:

> collect2: fatal error: cannot find 'ld'

Oh, I'm using ld.lld-10. aarch64-linux-gnu-gcc -B/usr/lib/llvm-10/bin works for
me. You'll have to get the necessary packages for lld-10 and point gcc to
where it lives.

Re: [PATCH] linux-user/elfload: Fix handling of pure BSS segments

[Peter Maydell]

On Wed, 25 Nov 2020 at 09:39, Alex Bennée <alex.bennee@linaro.org> wrote:
> How hairy is the generation of these binaries? If it's all doable with
> standard gcc/ldd command lines it would be useful to add them as a
> tcg/multiarch test case.

Rather than using C it might be simpler just to create a failing
binary by-hand, as the StackOverflow example does:

https://stackoverflow.com/questions/64956322/alignment-requirements-for-arm64-elf-executables-run-in-qemu-assembled-by-gas

thanks
-- PMM

Re: [PATCH] linux-user/elfload: Fix handling of pure BSS segments

[Laurent Vivier]

Le 18/11/2020 à 17:52, Stephen Long a écrit :
> qemu-user fails to load ELFs with only BSS and no data section
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Stephen Long <steplong@quicinc.com>
> ---
> 
> Submitting this on behalf of Ben Hutchings. Feel free to edit the commit
> msg.
> 
>  linux-user/elfload.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 0b02a92602..af16d94c61 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2783,7 +2783,7 @@ static void load_elf_image(const char *image_name, int image_fd,
>               * segment, in that case just let zero_bss allocate an empty buffer
>               * for it.
>               */
> -            if (eppnt->p_filesz != 0) {
> +            if (vaddr_len != 0) {
>                  error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
>                                      MAP_PRIVATE | MAP_FIXED,
>                                      image_fd, eppnt->p_offset - vaddr_po);
> 

Applied to my linux-user-for-6.0 branch.

Thanks,
Laurent