Re: [PATCH v4 4/6] elf: Add _bfd_elf_mmap_section and _bfd_elf_munmap_section_contents

["H.J. Lu" <hjl.tools@gmail.com>]

On Fri, Mar 8, 2024 at 7:28 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, Mar 7, 2024 at 4:41 PM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Wed, Mar 06, 2024 at 03:23:59PM -0800, H.J. Lu wrote:
> > > +void
> > > +_bfd_elf_munmap_section_contents (asection *sec ATTRIBUTE_UNUSED,
> > > +                               void *contents)
> > > +{
> > > +  /* NB: Since _bfd_elf_munmap_section_contents is called like free,
> > > +     CONTENTS may be NULL.  */
> > > +  if (contents == NULL)
> > > +    return;
> > > +
> > > +#ifdef USE_MMAP
> > > +  if (sec->mmapped_p)
> > > +    {
> > > +      /* NB: Don't free CONTENTS if it has been cached.  */
> > > +      if (elf_section_data (sec)->this_hdr.contents == contents)
> > > +     return;
> >
> > Why the above test?  This function must have such a test before it is
> > called if the contents might be cached.  Hmm, unless you have some
> > place where contents are cached only when mmapped?  (I didn't see such
> > a case.)

There is how it is cached:

(gdb) bt
#0  elf_x86_64_scan_relocs (abfd=0x684700, info=0x63bf80 <link_info>,
    sec=0x69f728, relocs=0x6e4540)
    at /export/gnu/import/git/gitlab/x86-binutils/bfd/elf64-x86-64.c:2600
#1  0x000000000048dda2 in _bfd_elf_link_iterate_on_relocs (abfd=0x684700,
    info=0x63bf80 <link_info>, action=0x44aac0 <elf_x86_64_scan_relocs>)
    at /export/gnu/import/git/gitlab/x86-binutils/bfd/elflink.c:4197
#2  0x000000000044c1b1 in elf_x86_64_always_size_sections (
    output_bfd=0x662980, info=0x63bf80 <link_info>)
    at /export/gnu/import/git/gitlab/x86-binutils/bfd/elf64-x86-64.c:2628
#3  0x0000000000493835 in bfd_elf_size_dynamic_sections (output_bfd=0x662980,
    soname=0x0, rpath=0x0, filter_shlib=0x0, audit=0x0, depaudit=0x0,
    auxiliary_filters=0x0, info=0x63bf80 <link_info>,
    sinterpptr=0x7fffffffd3a8)
    at /export/gnu/import/git/gitlab/x86-binutils/bfd/elflink.c:6673
#4  0x000000000042ce1c in ldelf_before_allocation (audit=0x0,
    depaudit=<optimized out>, default_interpreter_name=0x0)
    at /export/gnu/import/git/gitlab/x86-binutils/ld/ldelf.c:1839
#5  0x00000000004181b2 in lang_process ()
    at /export/gnu/import/git/gitlab/x86-binutils/ld/ldlang.c:8426
#6  0x0000000000404095 in main (argc=<optimized out>, argv=<optimized out>)
    at /export/gnu/import/git/gitlab/x86-binutils/ld/ldmain.c:504
(gdb)

and _bfd_elf_munmap_section_contents is called:

(gdb) bt
#0  _bfd_elf_munmap_section_contents (sec=0x69f728, contents=0x7ffff7c6dbe0)
    at /export/gnu/import/git/gitlab/x86-binutils/bfd/elf.c:14454
#1  0x00000000004ae92b in _bfd_elf_parse_eh_frame (abfd=0x684700,
    info=0x63bf80 <link_info>, sec=0x69f728, cookie=0x7fffffffd330)
    at /export/gnu/import/git/gitlab/x86-binutils/bfd/elf-eh-frame.c:1055
#2  0x00000000004a82b4 in bfd_elf_discard_info (output_bfd=0x662980,
    info=0x63bf80 <link_info>)
    at /export/gnu/import/git/gitlab/x86-binutils/bfd/elflink.c:14948
#3  0x0000000000425d39 in gldelf_x86_64_after_allocation ()
    at eelf_x86_64.c:145
#4  0x000000000041845d in lang_process ()
    at /export/gnu/import/git/gitlab/x86-binutils/ld/ldlang.c:8441
#5  0x0000000000404095 in main (argc=<optimized out>, argv=<optimized out>)
    at /export/gnu/import/git/gitlab/x86-binutils/ld/ldmain.c:504

> When _bfd_elf_munmap_section_contents is called from
> _bfd_elf_parse_eh_frame, the section contents may be cached.
> I updated the comments to clarify it.

Further clarification in comments:

      /* _bfd_elf_mmap_section_contents may return the previously
         mapped section contents.  Munmap the section contents only
         if they haven't been cached.  */
      if (elf_section_data (sec)->this_hdr.contents == contents)
        return;

> > > +      /* When bfd_mmap_local returns (void *) -1 on an unknown input,
> > > +      CONTENTS is malloced and CONTENTS_ADDR is set to NULL.  */
> >
> > The above comment adds confusion.  As far as I can see there isn't a
> > case where bfd_mmap_local returns -1 causing contents to be malloc'd.
> > Delete the comment, or mention _bfd_elf_mmap_section behaviour here.
>
> Changed to
>
>      /* When _bfd_elf_mmap_section_contents returns CONTENTS as
>          malloced, CONTENTS_ADDR is set to NULL.  */
>
> > BTW, why does this function have _contents in the name while
> > _bfd_elf_mmap_section does not?
>
> Changed to _bfd_elf_mmap_section_contents.
>
> > > +      if (elf_section_data (sec)->contents_addr != NULL)
> > > +     {
> > > +       /* NB: CONTENTS_ADDR and CONTENTS_SIZE must be valid.  */
> > > +       if (munmap (elf_section_data (sec)->contents_addr,
> > > +                   elf_section_data (sec)->contents_size) != 0)
> > > +         abort ();
> > > +       sec->mmapped_p = 0;
> > > +       sec->contents = NULL;
> > > +       elf_section_data (sec)->contents_addr = NULL;
> > > +       elf_section_data (sec)->contents_size = 0;
> > > +       return;
> > > +     }
> > > +    }
> > > +#endif
> > > +
> > > +  free (contents);
> > > +}
> >
> > > diff --git a/bfd/libbfd.c b/bfd/libbfd.c
> > > index 237f91e5c97..c847c4f0180 100644
> > > --- a/bfd/libbfd.c
> > > +++ b/bfd/libbfd.c
> > > @@ -21,6 +21,7 @@
> > >
> > >  #include "sysdep.h"
> > >  #include "bfd.h"
> > > +#include "elf-bfd.h"
> > >  #include "libbfd.h"
> > >  #include "objalloc.h"
> > >
> > > @@ -1202,6 +1203,19 @@ _bfd_generic_get_section_contents (bfd *abfd,
> > >        return false;
> > >      }
> > >
> > > +#ifdef USE_MMAP
> > > +  if (section->mmapped_p
> > > +      && (section->contents != NULL || location != NULL))
> > > +    {
> > > +      _bfd_error_handler
> > > +     /* xgettext:c-format */
> > > +     (_("%pB: mapped section %pA has non-NULLL buffer"),
> >
> > typo
>
> Fixed.
>
> >
> > > +      abfd, section);
> > > +      bfd_set_error (bfd_error_invalid_operation);
> > > +      return false;
> > > +    }
> > > +#endif
> > > +
> > >    sz = bfd_get_section_limit_octets (abfd, section);
> > >    if (offset + count < count
> > >        || offset + count > sz
> > > @@ -1214,8 +1228,49 @@ _bfd_generic_get_section_contents (bfd *abfd,
> > >        return false;
> > >      }
> > >
> > > -  if (bfd_seek (abfd, section->filepos + offset, SEEK_SET) != 0
> > > -      || bfd_read (location, count, abfd) != count)
> > > +  if (bfd_seek (abfd, section->filepos + offset, SEEK_SET) != 0)
> > > +    return false;
> > > +
> > > +#ifdef USE_MMAP
> > > +  if (section->mmapped_p)
> > > +    {
> > > +      if (location != 0
> > > +       || bfd_get_flavour (abfd) != bfd_target_elf_flavour)
> > > +     abort ();
> > > +
> > > +      int prot = ((section->reloc_count == 0)
> > > +               ? PROT_READ : PROT_READ | PROT_WRITE);
> > > +
> > > +      location = bfd_mmap_local
> > > +     (abfd, count, prot, &elf_section_data (section)->contents_addr,
> > > +      &elf_section_data (section)->contents_size);
> > > +
> > > +      if (location == NULL)
> > > +     return false;
> > > +
> > > +      /* Check for the unknown input file.  */
> >
> > Not "the unknown input file" but rather "iovec not supporting mmap".
>
> Fixed.
>
> > > +      if (location != (void *) -1)
> > > +     {
> > > +       section->contents = location;
> > > +       return true;
> > > +     }
> > > +
> > > +      /* Malloc the buffer and call bfd_read.  */
> > > +      location = (bfd_byte *) bfd_malloc (count);
> > > +      if (location == NULL)
> > > +     {
> > > +       if (bfd_get_error () == bfd_error_no_memory)
> > > +         _bfd_error_handler
> > > +           /* xgettext:c-format */
> > > +           (_("error: %pB(%pA) is too large (%#" PRIx64 " bytes)"),
> > > +            abfd, section, (uint64_t) count);
> > > +       return false;
> > > +     }
> > > +      section->contents = location;
> > > +    }
> > > +#endif
> > > +
> > > +  if (bfd_read (location, count, abfd) != count)
> > >      return false;
> > >
> > >    return true;
> >
> > OK with those things fixed.
> >
> > --
> > Alan Modra
> > Australia Development Lab, IBM
>
> Thanks.
>
> --
> H.J.



-- 
H.J.