Re: [PATCH 2/2] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, Jan 22, 2021 at 05:39:20PM +0100, Jiri Olsa escreveu:
> For very large ELF objects (with many sections), we could
> get special value SHN_XINDEX (65535) for symbol's st_shndx.
> 
> This patch is adding code to detect the optional extended
> section index table and use it to resolve symbol's section
> index.
> 
> Adding elf_symtab__for_each_symbol_index macro that returns
> symbol's section index and usign it in collect functions.

From a quick look it seems you addressed Andrii's review comments,
right?

I've merged it locally, but would like to have some detailed set of
steps on how to test this, so that I can add it to a "Committer testing"
section in the cset commit log and probably add it to my local set of
regression tests.

Who originally reported this? Joe? Also can someone provide a Tested-by:
in addition to mine when I get this detailed set of steps to test?

Thanks,

- Arnaldo
 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++--------------
>  elf_symtab.c  | 39 +++++++++++++++++++++++++++++++++-
>  elf_symtab.h  |  2 ++
>  3 files changed, 83 insertions(+), 17 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 5557c9efd365..56ee55965093 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -63,13 +63,13 @@ static void delete_functions(void)
>  #define max(x, y) ((x) < (y) ? (y) : (x))
>  #endif
>  
> -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
> +			    Elf32_Word sym_sec_idx)
>  {
>  	struct elf_function *new;
>  	static GElf_Shdr sh;
> -	static int last_idx;
> +	static Elf32_Word last_idx;
>  	const char *name;
> -	int idx;
>  
>  	if (elf_sym__type(sym) != STT_FUNC)
>  		return 0;
> @@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
>  		functions = new;
>  	}
>  
> -	idx = elf_sym__section(sym);
> -
> -	if (idx != last_idx) {
> -		if (!elf_section_by_idx(btfe->elf, &sh, idx))
> +	if (sym_sec_idx != last_idx) {
> +		if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx))
>  			return 0;
> -		last_idx = idx;
> +		last_idx = sym_sec_idx;
>  	}
>  
>  	functions[functions_cnt].name = name;
> @@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name)
>  	return true;
>  }
>  
> -static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
> +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym,
> +			      Elf32_Word sym_sec_idx)
>  {
>  	const char *sym_name;
>  	uint64_t addr;
>  	uint32_t size;
>  
>  	/* compare a symbol's shndx to determine if it's a percpu variable */
> -	if (elf_sym__section(sym) != btfe->percpu_shndx)
> +	if (sym_sec_idx != btfe->percpu_shndx)
>  		return 0;
>  	if (elf_sym__type(sym) != STT_OBJECT)
>  		return 0;
> @@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
>  	return 0;
>  }
>  
> -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
> +static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl,
> +			   Elf32_Word sym_sec_idx)
>  {
>  	if (!fl->mcount_start &&
>  	    !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) {
>  		fl->mcount_start = sym->st_value;
> -		fl->mcount_sec_idx = sym->st_shndx;
> +		fl->mcount_sec_idx = sym_sec_idx;
>  	}
>  
>  	if (!fl->mcount_stop &&
> @@ -598,9 +598,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
>  		fl->mcount_stop = sym->st_value;
>  }
>  
> +static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
> +			 int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx)
> +{
> +	if (!gelf_getsym(syms, id, sym))
> +		return false;
> +
> +	*sym_sec_idx = sym->st_shndx;
> +
> +	if (sym->st_shndx == SHN_XINDEX) {
> +		if (!syms_sec_idx_table)
> +			return false;
> +		if (!gelf_getsymshndx(syms, syms_sec_idx_table,
> +				      id, sym, sym_sec_idx))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)		\
> +	for (id = 0;								\
> +	     id < symtab->nr_syms &&						\
> +	     elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,		\
> +			  id, &sym, &sym_sec_idx);				\
> +	     id++)
> +
>  static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>  {
>  	struct funcs_layout fl = { };
> +	Elf32_Word sym_sec_idx;
>  	uint32_t core_id;
>  	GElf_Sym sym;
>  
> @@ -608,12 +635,12 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>  	percpu_var_cnt = 0;
>  
>  	/* search within symtab for percpu variables */
> -	elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
> -		if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
> +	elf_symtab__for_each_symbol_index(btfe->symtab, core_id, sym, sym_sec_idx) {
> +		if (collect_percpu_vars && collect_percpu_var(btfe, &sym, sym_sec_idx))
>  			return -1;
> -		if (collect_function(btfe, &sym))
> +		if (collect_function(btfe, &sym, sym_sec_idx))
>  			return -1;
> -		collect_symbol(&sym, &fl);
> +		collect_symbol(&sym, &fl, sym_sec_idx);
>  	}
>  
>  	if (collect_percpu_vars) {
> diff --git a/elf_symtab.c b/elf_symtab.c
> index 741990ea3ed9..fad5e0c0ba3c 100644
> --- a/elf_symtab.c
> +++ b/elf_symtab.c
> @@ -17,11 +17,13 @@
>  
>  struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
>  {
> +	size_t symtab_index;
> +
>  	if (name == NULL)
>  		name = ".symtab";
>  
>  	GElf_Shdr shdr;
> -	Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, NULL);
> +	Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, &symtab_index);
>  
>  	if (sec == NULL)
>  		return NULL;
> @@ -41,6 +43,12 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
>  	if (symtab->syms == NULL)
>  		goto out_free_name;
>  
> +	/*
> +	 * This returns extended section index table's
> +	 * section index, if it exists.
> +	 */
> +	int symtab_xindex = elf_scnshndx(sec);
> +
>  	sec = elf_getscn(elf, shdr.sh_link);
>  	if (sec == NULL)
>  		goto out_free_name;
> @@ -49,6 +57,35 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
>  	if (symtab->symstrs == NULL)
>  		goto out_free_name;
>  
> +	/*
> +	 * The .symtab section has optional extended section index
> +	 * table, load its data so it can be used to resolve symbol's
> +	 * section index.
> +	 **/
> +	if (symtab_xindex > 0) {
> +		GElf_Shdr shdr_xindex;
> +		Elf_Scn *sec_xindex;
> +
> +		sec_xindex = elf_getscn(elf, symtab_xindex);
> +		if (sec_xindex == NULL)
> +			goto out_free_name;
> +
> +		if (gelf_getshdr(sec_xindex, &shdr_xindex) == NULL)
> +			goto out_free_name;
> +
> +		/* Extra check to verify it's correct type */
> +		if (shdr_xindex.sh_type != SHT_SYMTAB_SHNDX)
> +			goto out_free_name;
> +
> +		/* Extra check to verify it belongs to the .symtab */
> +		if (symtab_index != shdr_xindex.sh_link)
> +			goto out_free_name;
> +
> +		symtab->syms_sec_idx_table = elf_getdata(elf_getscn(elf, symtab_xindex), NULL);
> +		if (symtab->syms_sec_idx_table == NULL)
> +			goto out_free_name;
> +	}
> +
>  	symtab->nr_syms = shdr.sh_size / shdr.sh_entsize;
>  
>  	return symtab;
> diff --git a/elf_symtab.h b/elf_symtab.h
> index 359add69c8ab..2e05ca98158b 100644
> --- a/elf_symtab.h
> +++ b/elf_symtab.h
> @@ -16,6 +16,8 @@ struct elf_symtab {
>  	uint32_t  nr_syms;
>  	Elf_Data  *syms;
>  	Elf_Data  *symstrs;
> +	/* Data of SHT_SYMTAB_SHNDX section. */
> +	Elf_Data  *syms_sec_idx_table;
>  	char	  *name;
>  };
>  
> -- 
> 2.26.2
> 

-- 

- Arnaldo