Re: [PATCH v2 1/2] elf: Add elf_backend_add_glibc_version_dependency

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

On Sat, Jan 6, 2024 at 2:10 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> When -z mark-plt is used to add DT_X86_64_PLT, DT_X86_64_PLTSZ and
> DT_X86_64_PLTENT, the r_addend field of the R_X86_64_JUMP_SLOT relocation
> stores the offset of the indirect branch instruction.  However, glibc
> versions which don't have this commit in glibc 2.36:
>
> commit f8587a61892cbafd98ce599131bf4f103466f084
> Author: H.J. Lu <hjl.tools@gmail.com>
> Date:   Fri May 20 19:21:48 2022 -0700
>
>     x86-64: Ignore r_addend for R_X86_64_GLOB_DAT/R_X86_64_JUMP_SLOT
>
>     According to x86-64 psABI, r_addend should be ignored for R_X86_64_GLOB_DAT
>     and R_X86_64_JUMP_SLOT.  Since linkers always set their r_addends to 0, we
>     can ignore their r_addends.
>
>     Reviewed-by: Fangrui Song <maskray@google.com>
>
> won't ignore the r_addend value in the R_X86_64_JUMP_SLOT relocation.
> Although this commit has been backported to glibc 2.33/2.34/2.35 release
> branches, it is safer to require glibc 2.36 for such binaries.
>
> Extend the glibc version dependency of GLIBC_ABI_DT_RELR for DT_RELR to
> also add GLIBC_2.36 version dependency for -z mark-plt on the the shared C
> library if it provides a GLIBC_2.XX symbol version.
>
>         * elflink.c (elf_find_verdep_info): Moved to ...
>         * elf-bfd.h (elf_find_verdep_info): Here.
>         (elf_backend_data): Add elf_backend_add_glibc_version_dependency.
>         (_bfd_elf_link_add_glibc_version_dependency): New function.
>         (_bfd_elf_link_add_dt_relr_dependency): Likewise.
>         * elf64-x86-64.c (elf_x86_64_add_glibc_version_dependency):
>         Likewise.
>         (elf_backend_add_glibc_version_dependency): New.
>         * elflink.c (elf_link_add_dt_relr_dependency): Renamed to ...
>         (elf_link_add_glibc_verneed): This.  Modified to support other
>         glibc dependencies.
>         (_bfd_elf_link_add_glibc_version_dependency): Likewise.
>         (_bfd_elf_link_add_dt_relr_dependency): Likewise.
>         (bfd_elf_size_dynamic_sections): Call
>         elf_backend_add_glibc_version_dependency instead of
>         elf_link_add_dt_relr_dependency.
>         * elfxx-target.h (elf_backend_add_glibc_version_dependency): New.
>         (elfNN_bed): Add elf_backend_add_glibc_version_dependency.
>
> ld/
>
>         * testsuite/ld-x86-64/mark-plt-1a.rd: New file.
>         * testsuite/ld-x86-64/mark-plt-1b.rd: Likewise.
>         * testsuite/ld-x86-64/x86-64.exp: Run -z mark-plt test for
>         GLIBC_2.36 dependency.
> ---
>  bfd/elf-bfd.h                         |  23 ++++
>  bfd/elf64-x86-64.c                    |  27 +++++
>  bfd/elflink.c                         | 146 +++++++++++++++-----------
>  bfd/elfxx-target.h                    |   5 +
>  ld/testsuite/ld-x86-64/mark-plt-1a.rd |   7 ++
>  ld/testsuite/ld-x86-64/mark-plt-1b.rd |   7 ++
>  ld/testsuite/ld-x86-64/x86-64.exp     |  14 +++
>  7 files changed, 167 insertions(+), 62 deletions(-)
>  create mode 100644 ld/testsuite/ld-x86-64/mark-plt-1a.rd
>  create mode 100644 ld/testsuite/ld-x86-64/mark-plt-1b.rd
>
> diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
> index a611349e3d9..3ed22fa6c52 100644
> --- a/bfd/elf-bfd.h
> +++ b/bfd/elf-bfd.h
> @@ -957,6 +957,19 @@ typedef struct elf_property_list
>    struct elf_property property;
>  } elf_property_list;
>
> +/* This structure is used to pass information to
> +   elf_backend_add_glibc_version_dependency.  */
> +
> +struct elf_find_verdep_info
> +{
> +  /* General link information.  */
> +  struct bfd_link_info *info;
> +  /* The number of dependencies.  */
> +  unsigned int vers;
> +  /* Whether we had a failure.  */
> +  bool failed;
> +};
> +
>  struct bfd_elf_section_reloc_data;
>
>  struct elf_backend_data
> @@ -1488,6 +1501,10 @@ struct elf_backend_data
>    bool (*elf_backend_write_section)
>      (bfd *, struct bfd_link_info *, asection *, bfd_byte *);
>
> +  /* This function adds glibc version dependency.  */
> +  void (*elf_backend_add_glibc_version_dependency)
> +    (struct elf_find_verdep_info *);
> +
>    /* This function, if defined, returns TRUE if it is section symbols
>       only that are considered local for the purpose of partitioning the
>       symbol table into local and global symbols.  This should be NULL
> @@ -2583,6 +2600,12 @@ extern bool _bfd_elf_link_output_relocs
>    (bfd *, asection *, Elf_Internal_Shdr *, Elf_Internal_Rela *,
>     struct elf_link_hash_entry **);
>
> +extern void _bfd_elf_link_add_glibc_version_dependency
> +  (struct elf_find_verdep_info *, const char *[]);
> +
> +extern void _bfd_elf_link_add_dt_relr_dependency
> +  (struct elf_find_verdep_info *);
> +
>  extern bool _bfd_elf_adjust_dynamic_copy
>    (struct bfd_link_info *, struct elf_link_hash_entry *, asection *);
>
> diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
> index ec001599cc1..1a2e64c031f 100644
> --- a/bfd/elf64-x86-64.c
> +++ b/bfd/elf64-x86-64.c
> @@ -5568,6 +5568,31 @@ elf_x86_64_link_setup_gnu_properties (struct bfd_link_info *info)
>    return _bfd_x86_elf_link_setup_gnu_properties (info, &init_table);
>  }
>
> +static void
> +elf_x86_64_add_glibc_version_dependency
> +  (struct elf_find_verdep_info *rinfo)
> +{
> +  unsigned int i = 0;
> +  const char *version[3] = { NULL, NULL, NULL };
> +  struct elf_x86_link_hash_table *htab;
> +
> +  if (rinfo->info->enable_dt_relr)
> +    {
> +      version[i] = "GLIBC_ABI_DT_RELR";
> +      i++;
> +    }
> +
> +  htab = elf_x86_hash_table (rinfo->info, X86_64_ELF_DATA);
> +  if (htab != NULL && htab->params->mark_plt)
> +    {
> +      version[i] = "GLIBC_2.36";
> +      i++;
> +    }
> +
> +  if (i != 0)
> +    _bfd_elf_link_add_glibc_version_dependency (rinfo, version);
> +}
> +
>  static const struct bfd_elf_special_section
>  elf_x86_64_special_sections[]=
>  {
> @@ -5652,6 +5677,8 @@ elf_x86_64_special_sections[]=
>    elf_x86_64_link_setup_gnu_properties
>  #define elf_backend_hide_symbol \
>    _bfd_x86_elf_hide_symbol
> +#define elf_backend_add_glibc_version_dependency \
> +  elf_x86_64_add_glibc_version_dependency
>
>  #undef elf64_bed
>  #define elf64_bed elf64_x86_64_bed
> diff --git a/bfd/elflink.c b/bfd/elflink.c
> index a577c957514..c2494b3e12e 100644
> --- a/bfd/elflink.c
> +++ b/bfd/elflink.c
> @@ -46,19 +46,6 @@ struct elf_info_failed
>    bool failed;
>  };
>
> -/* This structure is used to pass information to
> -   _bfd_elf_link_find_version_dependencies.  */
> -
> -struct elf_find_verdep_info
> -{
> -  /* General link information.  */
> -  struct bfd_link_info *info;
> -  /* The number of dependencies.  */
> -  unsigned int vers;
> -  /* Whether we had a failure.  */
> -  bool failed;
> -};
> -
>  static bool _bfd_elf_fix_symbol_flags
>    (struct elf_link_hash_entry *, struct elf_info_failed *);
>
> @@ -2217,64 +2204,64 @@ _bfd_elf_export_symbol (struct elf_link_hash_entry *h, void *data)
>    return true;
>  }
>
> -/* Return true if GLIBC_ABI_DT_RELR is added to the list of version
> -   dependencies successfully.  GLIBC_ABI_DT_RELR will be put into the
> -   .gnu.version_r section.  */
> +/* Return the glibc version reference if VERSION_DEP is added to the
> +   list of glibc version dependencies successfully.  VERSION_DEP will
> +   be put into the .gnu.version_r section.  */
>
> -static bool
> -elf_link_add_dt_relr_dependency (struct elf_find_verdep_info *rinfo)
> +static Elf_Internal_Verneed *
> +elf_link_add_glibc_verneed (struct elf_find_verdep_info *rinfo,
> +                           Elf_Internal_Verneed *glibc_verref,
> +                           const char *version_dep)
>  {
> -  bfd *glibc_bfd = NULL;
>    Elf_Internal_Verneed *t;
>    Elf_Internal_Vernaux *a;
>    size_t amt;
> -  const char *relr = "GLIBC_ABI_DT_RELR";
>
> -  /* See if we already know about GLIBC_PRIVATE_DT_RELR.  */
> -  for (t = elf_tdata (rinfo->info->output_bfd)->verref;
> -       t != NULL;
> -       t = t->vn_nextref)
> +  if (glibc_verref != NULL)
>      {
> -      const char *soname = bfd_elf_get_dt_soname (t->vn_bfd);
> -      /* Skip the shared library if it isn't libc.so.  */
> -      if (!soname || !startswith (soname, "libc.so."))
> -       continue;
> +      t = glibc_verref;
>
>        for (a = t->vn_auxptr; a != NULL; a = a->vna_nextptr)
>         {
> -         /* Return if GLIBC_PRIVATE_DT_RELR dependency has been
> -            added.  */
> -         if (a->vna_nodename == relr
> -             || strcmp (a->vna_nodename, relr) == 0)
> -           return true;
> -
> -         /* Check if libc.so provides GLIBC_2.XX version.  */
> -         if (!glibc_bfd && startswith (a->vna_nodename, "GLIBC_2."))
> -           glibc_bfd = t->vn_bfd;
> +         /* Return if VERSION_DEP dependency has been added.  */
> +         if (a->vna_nodename == version_dep
> +             || strcmp (a->vna_nodename, version_dep) == 0)
> +           return t;
>         }
> -
> -      break;
>      }
> +  else
> +    {
> +      bool is_glibc;
>
> -  /* Skip if it isn't linked against glibc.  */
> -  if (glibc_bfd == NULL)
> -    return true;
> +      for (t = elf_tdata (rinfo->info->output_bfd)->verref;
> +          t != NULL;
> +          t = t->vn_nextref)
> +       {
> +         const char *soname = bfd_elf_get_dt_soname (t->vn_bfd);
> +         if (soname != NULL && startswith (soname, "libc.so."))
> +           break;
> +       }
>
> -  /* This is a new version.  Add it to tree we are building.  */
> -  if (t == NULL)
> -    {
> -      amt = sizeof *t;
> -      t = (Elf_Internal_Verneed *) bfd_zalloc (rinfo->info->output_bfd,
> -                                              amt);
> +      /* Skip the shared library if it isn't libc.so.  */
>        if (t == NULL)
> +       return t;
> +
> +      is_glibc = false;
> +      for (a = t->vn_auxptr; a != NULL; a = a->vna_nextptr)
>         {
> -         rinfo->failed = true;
> -         return false;
> +         /* Return if VERSION_DEP dependency has been added.  */
> +         if (a->vna_nodename == version_dep
> +             || strcmp (a->vna_nodename, version_dep) == 0)
> +           return t;
> +
> +         /* Check if libc.so provides GLIBC_2.XX version.  */
> +         if (!is_glibc && startswith (a->vna_nodename, "GLIBC_2."))
> +           is_glibc = true;
>         }
>
> -      t->vn_bfd = glibc_bfd;
> -      t->vn_nextref = elf_tdata (rinfo->info->output_bfd)->verref;
> -      elf_tdata (rinfo->info->output_bfd)->verref = t;
> +      /* Skip if it isn't linked against glibc.  */
> +      if (!is_glibc)
> +       return NULL;
>      }
>
>    amt = sizeof *a;
> @@ -2282,10 +2269,10 @@ elf_link_add_dt_relr_dependency (struct elf_find_verdep_info *rinfo)
>    if (a == NULL)
>      {
>        rinfo->failed = true;
> -      return false;
> +      return NULL;
>      }
>
> -  a->vna_nodename = relr;
> +  a->vna_nodename = version_dep;
>    a->vna_flags = 0;
>    a->vna_nextptr = t->vn_auxptr;
>    a->vna_other = rinfo->vers + 1;
> @@ -2293,7 +2280,45 @@ elf_link_add_dt_relr_dependency (struct elf_find_verdep_info *rinfo)
>
>    t->vn_auxptr = a;
>
> -  return true;
> +  return t;
> +}
> +
> +/* Add VERSION_DEP to the list of version dependencies when linked
> +   against glibc.  */
> +
> +void
> +_bfd_elf_link_add_glibc_version_dependency
> +  (struct elf_find_verdep_info *rinfo,
> +   const char *version_dep[])
> +{
> +  Elf_Internal_Verneed *t = NULL;
> +
> +  do
> +    {
> +      t = elf_link_add_glibc_verneed (rinfo, t, *version_dep);
> +      /* Return if there is no glibc version reference.  */
> +      if (t == NULL)
> +       return;
> +      version_dep++;
> +    }
> +  while (*version_dep != NULL);
> +}
> +
> +/* Add GLIBC_ABI_DT_RELR to the list of version dependencies when
> +   linked against glibc.  */
> +
> +void
> +_bfd_elf_link_add_dt_relr_dependency (struct elf_find_verdep_info *rinfo)
> +{
> +  if (rinfo->info->enable_dt_relr)
> +    {
> +      const char *version[] =
> +       {
> +         "GLIBC_ABI_DT_RELR",
> +         NULL
> +       };
> +      _bfd_elf_link_add_glibc_version_dependency (rinfo, version);
> +    }
>  }
>
>  /* Look through the symbols which are defined in other shared
> @@ -7047,12 +7072,9 @@ bfd_elf_size_dynamic_sections (bfd *output_bfd,
>        if (sinfo.failed)
>         return false;
>
> -      if (info->enable_dt_relr)
> -       {
> -         elf_link_add_dt_relr_dependency (&sinfo);
> -         if (sinfo.failed)
> -           return false;
> -       }
> +      bed->elf_backend_add_glibc_version_dependency (&sinfo);
> +      if (sinfo.failed)
> +       return false;
>
>        if (elf_tdata (output_bfd)->verref == NULL)
>         s->flags |= SEC_EXCLUDE;
> diff --git a/bfd/elfxx-target.h b/bfd/elfxx-target.h
> index d4b14a022e3..a7f2fc6e320 100644
> --- a/bfd/elfxx-target.h
> +++ b/bfd/elfxx-target.h
> @@ -667,6 +667,10 @@
>  #ifndef elf_backend_write_section
>  #define elf_backend_write_section              NULL
>  #endif
> +#ifndef elf_backend_add_glibc_version_dependency
> +#define elf_backend_add_glibc_version_dependency \
> +  _bfd_elf_link_add_dt_relr_dependency
> +#endif
>  #ifndef elf_backend_elfsym_local_is_section
>  #define elf_backend_elfsym_local_is_section    NULL
>  #endif
> @@ -897,6 +901,7 @@ static const struct elf_backend_data elfNN_bed =
>    elf_backend_can_make_multiple_eh_frame,
>    elf_backend_encode_eh_address,
>    elf_backend_write_section,
> +  elf_backend_add_glibc_version_dependency,
>    elf_backend_elfsym_local_is_section,
>    elf_backend_mips_irix_compat,
>    elf_backend_mips_rtype_to_howto,
> diff --git a/ld/testsuite/ld-x86-64/mark-plt-1a.rd b/ld/testsuite/ld-x86-64/mark-plt-1a.rd
> new file mode 100644
> index 00000000000..1234fbe038c
> --- /dev/null
> +++ b/ld/testsuite/ld-x86-64/mark-plt-1a.rd
> @@ -0,0 +1,7 @@
> +#...
> +Version needs section '.gnu.version_r' contains 1 entry:
> + Addr: 0x[0-9a-f]+ +Offset: 0x[0-9a-f]+ +Link: +[0-9]+ +\(.dynstr\)
> + +0+: Version: 1 +File: libc\.so\.6(|\.1) +Cnt: +[0-9]+
> +#...
> +  0x[a-f0-9]+:   Name: GLIBC_2.36  Flags: none  Version: [0-9]+
> +#pass
> diff --git a/ld/testsuite/ld-x86-64/mark-plt-1b.rd b/ld/testsuite/ld-x86-64/mark-plt-1b.rd
> new file mode 100644
> index 00000000000..6556a6d939e
> --- /dev/null
> +++ b/ld/testsuite/ld-x86-64/mark-plt-1b.rd
> @@ -0,0 +1,7 @@
> +#...
> +Version needs section '.gnu.version_r' contains 1 entry:
> + Addr: 0x[0-9a-f]+ +Offset: 0x[0-9a-f]+ +Link: +[0-9]+ +\(.dynstr\)
> + +0+: Version: 1 +File: libc\.so\.6(|\.1) +Cnt: +[0-9]+
> +#...
> +  0x[a-f0-9]+:   Name: GLIBC_ABI_DT_RELR  Flags: none  Version: [0-9]+
> +#pass
> diff --git a/ld/testsuite/ld-x86-64/x86-64.exp b/ld/testsuite/ld-x86-64/x86-64.exp
> index 0af9f047600..d17bb9b7d7e 100644
> --- a/ld/testsuite/ld-x86-64/x86-64.exp
> +++ b/ld/testsuite/ld-x86-64/x86-64.exp
> @@ -2257,4 +2257,18 @@ run_dump_test "mark-plt-1b-x32"
>  run_dump_test "mark-plt-1c-x32"
>  run_dump_test "mark-plt-1d-x32"
>
> +if { [check_compiler_available] } {
> +    run_cc_link_tests [list \
> +       [list \
> +           "Build mark-plt-1.so" \
> +           "-shared -Wl,-z,mark-plt,-z,pack-relative-relocs" \
> +           "-fPIC" \
> +           { mark-plt-1.s } \
> +           {{readelf {-W --version-info} mark-plt-1a.rd} \
> +            {readelf {-W --version-info} mark-plt-1b.rd}} \
> +           "mark-plt-1.so" \
> +       ] \
> +    ]
> +}
> +
>  set ASFLAGS "$saved_ASFLAGS"
> --
> 2.43.0
>

Nick, Alan,

Any comments?

Thanks.

-- 
H.J.