On Tue, Nov 24, 2020 at 8:22 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> Add support to detect kernel module dtrace addresses and use
> it as filter for functions.
typo: dtrace -> ftrace?
>
> For kernel modules the ftrace addresses are stored in __mcount_loc
> section. Adding the code that detects this section and reads
> its data into array, which is then processed as filter by
> current code.
>
> There's one tricky point with kernel modules wrt Elf object,
> which we get from dwfl_module_getelf function. This function
> performs all possible relocations, including __mcount_loc
> section.
>
> So addrs array contains relocated values, which we need take
> into account when we compare them to functions values which
> are relative to their sections.
>
> With this change for example for xfs.ko module in my kernel
> config I'm getting slightly bigger number of functions:
>
> before: 2429, after: 2615
>
> Because of the malfunction DWARF's declaration tag, the
> 'before' functions contain also functions that are not
> part of the module. The 'after' functions contain only
> functions that are traceable and part of xfs.ko.
>
> Despite filtering out some declarations, this change
> also adds static functions, hence the total number
> of functions is bigger.
>
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> ---
> btf_encoder.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++--
> dutil.c | 16 ++++++++++
> dutil.h | 2 ++
> 3 files changed, 100 insertions(+), 3 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 467c4657b2c0..e6114c10ad01 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -36,6 +36,7 @@ struct funcs_layout {
> struct elf_function {
> const char *name;
> unsigned long addr;
> + unsigned long sh_addr;
> bool generated;
> };
>
> @@ -65,11 +66,11 @@ static void delete_functions(void)
> static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> {
> struct elf_function *new;
> + static GElf_Shdr sh;
> + static int last_idx;
>
> if (elf_sym__type(sym) != STT_FUNC)
> return 0;
> - if (!elf_sym__value(sym))
> - return 0;
>
> if (functions_cnt == functions_alloc) {
> functions_alloc = max(1000, functions_alloc * 3 / 2);
> @@ -84,8 +85,17 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> functions = new;
> }
>
> + if (elf_sym__section(sym) != last_idx) {
> + int idx = elf_sym__section(sym);
nit: elf_sym__section() called twice, maybe have sec_idx local variable instead?
> +
> + if (!elf_section_by_idx(btfe->elf, &sh, idx))
> + return 0;
isn't this an error and shouldn't return 0?..
> + last_idx = idx;
> + }
> +
> functions[functions_cnt].name = elf_sym__name(sym, btfe->symtab);
> functions[functions_cnt].addr = elf_sym__value(sym);
> + functions[functions_cnt].sh_addr = sh.sh_addr;
> functions[functions_cnt].generated = false;
> functions_cnt++;
> return 0;
> @@ -146,10 +156,60 @@ static int get_vmlinux_addrs(struct btf_elf *btfe, struct funcs_layout *fl,
> return 0;
> }
>
> +static int
> +get_kmod_addrs(struct btf_elf *btfe, unsigned long **paddrs, unsigned long *pcount)
> +{
> + unsigned long *addrs, count;
> + GElf_Shdr shdr_mcount;
> + Elf_Data *data;
> + Elf_Scn *sec;
> +
> + /* get __mcount_loc */
> + sec = elf_section_by_name(btfe->elf, &btfe->ehdr, &shdr_mcount,
> + "__mcount_loc", NULL);
> + if (!sec) {
> + if (btf_elf__verbose) {
> + printf("%s: '%s' doesn't have __mcount_loc section\n", __func__,
> + btfe->filename);
> + }
nit: unnecessary {} for single-statement if
> + return 0;
> + }
> +
> + data = elf_getdata(sec, NULL);
> + if (!data) {
> + fprintf(stderr, "Failed to data for __mcount_loc section.\n");
> + return -1;
> + }
> +
> + addrs = malloc(data->d_size);
> + if (!addrs) {
> + fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
> + return -1;
> + }
> +
> + count = data->d_size / sizeof(unsigned long);
not sure this is safe to do, e.g., if we are processing ELF of
different bitness (32 vs 64). Maybev we can get the size of an entry
from sh_entsize?
> + memcpy(addrs, data->d_buf, data->d_size);
> +
> + /*
> + * We get Elf object from dwfl_module_getelf function,
> + * which performs all possible relocations, including
> + * __mcount_loc section.
> + *
> + * So addrs array now contains relocated values, which
> + * we need take into account when we compare them to
> + * functions values, see comment in setup_functions
> + * function.
> + */
> + *paddrs = addrs;
> + *pcount = count;
> + return 0;
> +}
> +
> static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> {
> unsigned long *addrs = NULL, count, i;
> int functions_valid = 0;
> + bool kmod = false;
>
> /*
> * Check if we are processing vmlinux image and
> @@ -158,6 +218,16 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> if (get_vmlinux_addrs(btfe, fl, &addrs, &count))
> return -1;
>
> + /*
> + * Check if we are processing kernel module and
> + * get mcount data if it's detected.
> + */
> + if (!addrs) {
> + if (get_kmod_addrs(btfe, &addrs, &count))
> + return -1;
> + kmod = true;
> + }
> +
> if (!addrs) {
> if (btf_elf__verbose)
> printf("ftrace symbols not detected, falling back to DWARF data\n");
> @@ -174,9 +244,18 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> */
> for (i = 0; i < functions_cnt; i++) {
> struct elf_function *func = &functions[i];
> + /*
> + * For vmlinux image both addrs[x] and functions[x]::addr
> + * values are final address and are comparable.
> + *
> + * For kernel module addrs[x] is final address, but
> + * functions[x]::addr is relative address within section
> + * and needs to be relocated by adding sh_addr.
> + */
> + unsigned long addr = kmod ? func->addr + func->sh_addr : func->addr;
>
> /* Make sure function is within ftrace addresses. */
> - if (bsearch(&func->addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> + if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> /*
> * We iterate over sorted array, so we can easily skip
> * not valid item and move following valid field into
> diff --git a/dutil.c b/dutil.c
> index f7b853f0660d..5ebbd2f9c84c 100644
> --- a/dutil.c
> +++ b/dutil.c
> @@ -196,6 +196,22 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
> return sec;
> }
>
> +Elf_Scn *elf_section_by_idx(Elf *elf, GElf_Shdr *shp, int idx)
there is elf_getscn(), which does the same?
> +{
> + Elf_Scn *sec = NULL;
> + size_t cnt = 1;
> +
> + while ((sec = elf_nextscn(elf, sec)) != NULL) {
> + if (cnt == idx) {
> + gelf_getshdr(sec, shp);
> + return sec;
> + }
> + ++cnt;
> + }
> +
> + return NULL;
> +}
> +
> char *strlwr(char *s)
> {
> int len = strlen(s), i;
> diff --git a/dutil.h b/dutil.h
> index 676770d5d5c9..0838dff2d679 100644
> --- a/dutil.h
> +++ b/dutil.h
> @@ -324,6 +324,8 @@ void *zalloc(const size_t size);
> Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
> GElf_Shdr *shp, const char *name, size_t *index);
>
> +Elf_Scn *elf_section_by_idx(Elf *elf, GElf_Shdr *shp, int idx);
> +
> #ifndef SHT_GNU_ATTRIBUTES
> /* Just a way to check if we're using an old elfutils version */
> static inline int elf_getshdrstrndx(Elf *elf, size_t *dst)
> --
> 2.26.2
>
