| Message ID | 20190418153631.rl3lgadzrqqof72p@debian (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-user: elf: Map empty PT_LOAD sections | expand |
Friendly inactivity ping. On Thu, 18 Apr 2019 at 17:42, Giuseppe Musacchio <thatlemon@gmail.com> wrote: > > Some PT_LOAD sections may be completely zeroed out and their p_filesize > is zero, in that case the loader should just allocate a page that's at > least p_memsz bytes large (plus eventual alignment padding). > > Calling zero_bss does this job for us, all we have to do is make sure we > don't try to mmap a zero-length page. > > Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com> > --- > linux-user/elfload.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/linux-user/elfload.c b/linux-user/elfload.c > index c1a2602..e9a0951 100644 > --- a/linux-user/elfload.c > +++ b/linux-user/elfload.c > @@ -2366,11 +2366,17 @@ static void load_elf_image(const char *image_name, int image_fd, > vaddr_ps = TARGET_ELF_PAGESTART(vaddr); > vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po); > > - error = target_mmap(vaddr_ps, vaddr_len, > - elf_prot, MAP_PRIVATE | MAP_FIXED, > - image_fd, eppnt->p_offset - vaddr_po); > - if (error == -1) { > - goto exit_perror; > + /* Some sections may be completely empty without any backing file > + * segment, in that case just let zero_bss allocate an empty buffer > + * for it. */ > + if (eppnt->p_filesz != 0) { > + error = target_mmap(vaddr_ps, vaddr_len, elf_prot, > + MAP_PRIVATE | MAP_FIXED, > + image_fd, eppnt->p_offset - vaddr_po); > + > + if (error == -1) { > + goto exit_perror; > + } > } > > vaddr_ef = vaddr + eppnt->p_filesz; > -- > 2.20.1 >
On Thu, 18 Apr 2019 at 17:46, Giuseppe Musacchio <thatlemon@gmail.com> wrote: > > Some PT_LOAD sections may be completely zeroed out and their p_filesize > is zero, in that case the loader should just allocate a page that's at > least p_memsz bytes large (plus eventual alignment padding). Thanks for this patch -- codewise it looks good, so I just have a couple of minor tweaks I think we should make to the comment and commit message: The things we're iterating through here are segments, not sections (an ELF section is a different sort of data structure): could you fix the text in your commit message and comment, please? > Calling zero_bss does this job for us, all we have to do is make sure we > don't try to mmap a zero-length page. > > Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com> > --- > linux-user/elfload.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/linux-user/elfload.c b/linux-user/elfload.c > index c1a2602..e9a0951 100644 > --- a/linux-user/elfload.c > +++ b/linux-user/elfload.c > @@ -2366,11 +2366,17 @@ static void load_elf_image(const char *image_name, int image_fd, > vaddr_ps = TARGET_ELF_PAGESTART(vaddr); > vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po); > > - error = target_mmap(vaddr_ps, vaddr_len, > - elf_prot, MAP_PRIVATE | MAP_FIXED, > - image_fd, eppnt->p_offset - vaddr_po); > - if (error == -1) { > - goto exit_perror; > + /* Some sections may be completely empty without any backing file > + * segment, in that case just let zero_bss allocate an empty buffer > + * for it. */ Our coding style says that multi-line comments should be /* * like this, with the start and * end markers on lines of their own */ (if you run your patch through scripts/checkpatch.pl it ought to detect this sort of minor coding style nit). > + if (eppnt->p_filesz != 0) { > + error = target_mmap(vaddr_ps, vaddr_len, elf_prot, > + MAP_PRIVATE | MAP_FIXED, > + image_fd, eppnt->p_offset - vaddr_po); > + > + if (error == -1) { > + goto exit_perror; > + } > } > > vaddr_ef = vaddr + eppnt->p_filesz; Other than those minor things: Reviewed-by: Peter Maydell <peter.maydell@linaro.org> so if you fix them and send out a v2 patch you can include my Reviewed-by: tag in the commit message (under your signed-off-by) in that patch email. thanks -- PMM
diff --git a/linux-user/elfload.c b/linux-user/elfload.c index c1a2602..e9a0951 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -2366,11 +2366,17 @@ static void load_elf_image(const char *image_name, int image_fd, vaddr_ps = TARGET_ELF_PAGESTART(vaddr); vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po); - error = target_mmap(vaddr_ps, vaddr_len, - elf_prot, MAP_PRIVATE | MAP_FIXED, - image_fd, eppnt->p_offset - vaddr_po); - if (error == -1) { - goto exit_perror; + /* Some sections may be completely empty without any backing file + * segment, in that case just let zero_bss allocate an empty buffer + * for it. */ + if (eppnt->p_filesz != 0) { + error = target_mmap(vaddr_ps, vaddr_len, elf_prot, + MAP_PRIVATE | MAP_FIXED, + image_fd, eppnt->p_offset - vaddr_po); + + if (error == -1) { + goto exit_perror; + } } vaddr_ef = vaddr + eppnt->p_filesz;
Some PT_LOAD sections may be completely zeroed out and their p_filesize is zero, in that case the loader should just allocate a page that's at least p_memsz bytes large (plus eventual alignment padding). Calling zero_bss does this job for us, all we have to do is make sure we don't try to mmap a zero-length page. Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com> --- linux-user/elfload.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)