Date:   Wed, 19 Apr 2023 17:43:25 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Espen Grindhaug <espen.grindhaug@...il.com>,
        Alan Maguire <alan.maguire@...cle.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] libbpf: Improve version handling when attaching uprobe

cc Alan, can you please take a look, as you've worked on this code previously

On Mon, Apr 17, 2023 at 12:34 PM Espen Grindhaug
<espen.grindhaug@...il.com> wrote:
>
> This change fixes the handling of versions in elf_find_func_offset.
> In the previous implementation, we incorrectly assumed that the
> version information would be present in the string found in the
> string table.
>
> We now look up the correct version string in the version symbol
> table before constructing the full name and then comparing.
>
> This patch adds support for both name@...sion and name@@version to
> match output of the various elf parsers.
>
> Signed-off-by: Espen Grindhaug <espen.grindhaug@...il.com>
> ---

Espen, can you please also add a test to exercise this logic? We can
probably reuse urandom_read_lib and make sure we have versioned
symbols in it, and then try to attach to specific version or
something.

>  tools/lib/bpf/libbpf.c | 148 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 129 insertions(+), 19 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 49cd304ae3bc..ef5e11ce6241 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -10620,31 +10620,94 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
>  }
>
>  /* Return next ELF section of sh_type after scn, or first of that type if scn is NULL. */
> -static Elf_Scn *elf_find_next_scn_by_type(Elf *elf, int sh_type, Elf_Scn *scn)
> +static Elf_Scn *elf_find_next_scn_by_type(Elf *elf, int sh_type, Elf_Scn *scn, size_t *idx)
>  {
>         while ((scn = elf_nextscn(elf, scn)) != NULL) {
>                 GElf_Shdr sh;
>
>                 if (!gelf_getshdr(scn, &sh))
>                         continue;
> -               if (sh.sh_type == sh_type)
> +               if (sh.sh_type == sh_type) {
> +                       if (idx)
> +                               *idx = sh.sh_link;
>                         return scn;
> +               }
> +       }
> +       return NULL;
> +}
> +
> +static Elf_Data *elf_find_data_by_type(Elf *elf, int sh_type, size_t *idx)
> +{
> +       Elf_Scn *scn = elf_find_next_scn_by_type(elf, sh_type, NULL, idx);
> +
> +       if (scn)
> +               return elf_getdata(scn, NULL);
> +
> +       return NULL;
> +}
> +
> +static Elf64_Verdef *elf_verdef_by_offset(Elf_Data *data, size_t offset)
> +{
> +       if (offset + sizeof(Elf64_Verdef) > data->d_size)
> +               return NULL;
> +
> +       return (Elf64_Verdef *)((char *) data->d_buf + offset);
> +}
> +
> +static Elf64_Versym *elf_versym_by_idx(Elf_Data *data, size_t idx)
> +{
> +       if (idx >= data->d_size / sizeof(Elf64_Versym))
> +               return NULL;
> +
> +       return (Elf64_Versym *)(data->d_buf + idx * sizeof(Elf64_Versym));
> +}
> +
> +static Elf64_Verdaux *elf_verdaux_by_offset(Elf_Data *data, size_t offset)
> +{
> +       if (offset + sizeof(Elf64_Verdaux) > data->d_size)
> +               return NULL;
> +
> +       return (Elf64_Verdaux *)((char *) data->d_buf + offset);
> +}
> +
> +#define ELF_VERSYM_HIDDEN 0x8000
> +#define ELF_VERSYM_IDX_MASK 0x7fff
> +
> +static Elf64_Verdaux *elf_get_verdaux_by_versym(Elf_Data *verdef_data, Elf64_Versym *versym)
> +{
> +       size_t offset = 0;
> +
> +       while (offset + sizeof(Elf64_Verdef) <= verdef_data->d_size) {
> +               Elf64_Verdef *verdef = elf_verdef_by_offset(verdef_data, offset);
> +
> +               if (!verdef)
> +                       break;
> +
> +               if (verdef->vd_ndx == (*versym & ELF_VERSYM_IDX_MASK))
> +                       return elf_verdaux_by_offset(verdef_data, offset + verdef->vd_aux);
> +
> +               if (verdef->vd_next == 0)
> +                       break;
> +
> +               offset += verdef->vd_next;
>         }
>         return NULL;
>  }
>
>  /* Find offset of function name in the provided ELF object. "binary_path" is
>   * the path to the ELF binary represented by "elf", and only used for error
> - * reporting matters. "name" matches symbol name or name@@LIB for library
> - * functions.
> + * reporting matters. "name" matches symbol name, name@LIB or name@@LIB for
> + * library functions.
>   */
>  static long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name)
>  {
>         int i, sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB };
>         bool is_shared_lib, is_name_qualified;
>         long ret = -ENOENT;
> -       size_t name_len;
>         GElf_Ehdr ehdr;
> +       Elf_Data *versym_data = NULL;
> +       Elf_Data *verdef_data = NULL;
> +       size_t verdef_stridx = 0;
>
>         if (!gelf_getehdr(elf, &ehdr)) {
>                 pr_warn("elf: failed to get ehdr from %s: %s\n", binary_path, elf_errmsg(-1));
> @@ -10654,9 +10717,12 @@ static long elf_find_func_offset(Elf *elf, const char *binary_path, const char *
>         /* for shared lib case, we do not need to calculate relative offset */
>         is_shared_lib = ehdr.e_type == ET_DYN;
>
> -       name_len = strlen(name);
> -       /* Does name specify "@@LIB"? */
> -       is_name_qualified = strstr(name, "@@") != NULL;
> +       /* Does name specify "@@LIB" or "@LIB"? */
> +       is_name_qualified = strstr(name, "@") != NULL;
> +
> +       /* Extract version definition and version symbol table */
> +       versym_data = elf_find_data_by_type(elf, SHT_GNU_versym, NULL);
> +       verdef_data = elf_find_data_by_type(elf, SHT_GNU_verdef, &verdef_stridx);
>
>         /* Search SHT_DYNSYM, SHT_SYMTAB for symbol. This search order is used because if
>          * a binary is stripped, it may only have SHT_DYNSYM, and a fully-statically
> @@ -10671,10 +10737,10 @@ static long elf_find_func_offset(Elf *elf, const char *binary_path, const char *
>                 const char *sname;
>                 GElf_Shdr sh;
>
> -               scn = elf_find_next_scn_by_type(elf, sh_types[i], NULL);
> +               scn = elf_find_next_scn_by_type(elf, sh_types[i], NULL, NULL);
>                 if (!scn) {
>                         pr_debug("elf: failed to find symbol table ELF sections in '%s'\n",
> -                                binary_path);
> +                               binary_path);
>                         continue;
>                 }
>                 if (!gelf_getshdr(scn, &sh))
> @@ -10705,16 +10771,60 @@ static long elf_find_func_offset(Elf *elf, const char *binary_path, const char *
>                         if (!sname)
>                                 continue;
>
> -                       curr_bind = GELF_ST_BIND(sym.st_info);
> +                       if (is_name_qualified) {
> +                               Elf64_Versym *versym;
> +                               Elf64_Verdaux *verdaux;
> +                               int res;
> +                               char full_name[256];
>
> -                       /* User can specify func, func@@LIB or func@@LIB_VERSION. */
> -                       if (strncmp(sname, name, name_len) != 0)
> -                               continue;
> -                       /* ...but we don't want a search for "foo" to match 'foo2" also, so any
> -                        * additional characters in sname should be of the form "@@LIB".
> -                        */
> -                       if (!is_name_qualified && sname[name_len] != '\0' && sname[name_len] != '@')
> -                               continue;
> +                               /* check that name at least starts with sname before building
> +                                * the full name
> +                                */
> +                               if (strncmp(name, sname, strlen(sname)) != 0)
> +                                       continue;
> +
> +                               if (!versym_data || !verdef_data) {
> +                                       pr_warn("elf: failed to find version definition or version symbol table in '%s'\n",
> +                                               binary_path);
> +                                       break;
> +                               }
> +
> +                               versym = elf_versym_by_idx(versym_data, idx);
> +                               if (!versym) {
> +                                       pr_warn("elf: failed to lookup versym for '%s' in '%s'\n",
> +                                               sname, binary_path);
> +                                       continue;
> +                               }
> +
> +                               verdaux = elf_get_verdaux_by_versym(verdef_data, versym);
> +                               if (!verdaux) {
> +                                       pr_warn("elf: failed to lookup verdaux for '%s' in '%s'\n",
> +                                               sname, binary_path);
> +                                       continue;
> +                               }
> +
> +                               res = snprintf(full_name, sizeof(full_name),
> +                                              (*versym & ELF_VERSYM_HIDDEN) ? "%s@%s" :
> +                                                                   "%s@@%s",
> +                                              sname,
> +                                              elf_strptr(elf, verdef_stridx,
> +                                                         verdaux->vda_name));
> +
> +                               if (res < 0 || res >= sizeof(full_name)) {
> +                                       pr_warn("elf: failed to build full name for '%s' in '%s'\n",
> +                                               sname, binary_path);
> +                                       continue;
> +                               }
> +
> +                               if (strcmp(full_name, name) != 0)
> +                                       continue;
> +                       } else {
> +                               /* If name is not qualified, we want to match the symbol name */
> +                               if (strcmp(sname, name) != 0)
> +                                       continue;
> +                       }
> +
> +                       curr_bind = GELF_ST_BIND(sym.st_info);
>
>                         if (ret >= 0) {
>                                 /* handle multiple matches */
> --
> 2.34.1
>

Powered by blists - more mailing lists