On Fri, Oct 21, 2022 at 9:32 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Fri, Oct 21, 2022 at 04:02:03PM +0100, Alan Maguire wrote:
> > BTF deduplication was throwing some strange results, where core kernel
> > data types were failing to deduplicate due to the return values
> > of function type members being void (0) instead of the actual type
> > (unsigned int). An example of this can be seen below, where
> > "struct dst_ops" was failing to deduplicate between kernel and
> > module:
> >
> > struct dst_ops {
> > short unsigned int family;
> > unsigned int gc_thresh;
> > int (*gc)(struct dst_ops *);
> > struct dst_entry * (*check)(struct dst_entry *, __u32);
> > unsigned int (*default_advmss)(const struct dst_entry *);
> > unsigned int (*mtu)(const struct dst_entry *);
> > ...
> >
> > struct dst_ops___2 {
> > short unsigned int family;
> > unsigned int gc_thresh;
> > int (*gc)(struct dst_ops___2 *);
> > struct dst_entry___2 * (*check)(struct dst_entry___2 *, __u32);
> > void (*default_advmss)(const struct dst_entry___2 *);
> > void (*mtu)(const struct dst_entry___2 *);
> > ...
> >
> > This was seen with
> >
> > bcc648a10cbc ("btf_encoder: Encode DW_TAG_unspecified_type returning routines as void")
> >
> > ...which rewrites the return value as 0 (void) when it is marked
> > as matching DW_TAG_unspecified_type:
> >
> > static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t type_id_off, uint32_t tag_type)
> > {
> > if (tag_type == 0)
> > return 0;
> >
> > if (encoder->cu->unspecified_type.tag && tag_type == encoder->cu->unspecified_type.type) {
> > // No provision for encoding this, turn it into void.
> > return 0;
> > }
> >
> > return type_id_off + tag_type;
> > }
> >
> > However the odd thing was that on further examination, the unspecified type
> > was not being set, so why was this logic being tripped? Futher debugging
> > showed that the encoder->cu->unspecified_type.tag value was garbage, and
> > the type id happened to collide with "unsigned int"; as a result we
> > were replacing unsigned ints with void return values, and since this
> > was being done to function type members in structs, it triggered a
> > type mismatch which failed deduplication between kernel and module.
> >
> > The fix is simply to calloc() the cu in cu__new() instead.
> >
> > Fixes: bcc648a10cbc ("btf_encoder: Encode DW_TAG_unspecified_type returning routines as void")
> > Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
>
> awesome, this fixes the missing dedup I was seeing
> with current pahole:
>
> $ bpftool btf dump file ./vmlinux.test | grep "STRUCT 'task_struct'" | wc -l
> 69
>
> with this patch:
>
> $ bpftool btf dump file ./vmlinux.test | grep "STRUCT 'task_struct'" | wc -l
> 1
>
Nice and a great catch! I generally try to stick to calloc() in libbpf
exactly so I don't have to worry about stuff like this.
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
>
> thanks,
> jirka
>
>
> > ---
> > dwarves.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/dwarves.c b/dwarves.c
> > index fbebc1d..424381d 100644
> > --- a/dwarves.c
> > +++ b/dwarves.c
> > @@ -626,7 +626,7 @@ struct cu *cu__new(const char *name, uint8_t addr_size,
> > const unsigned char *build_id, int build_id_len,
> > const char *filename, bool use_obstack)
> > {
> > - struct cu *cu = malloc(sizeof(*cu) + build_id_len);
> > + struct cu *cu = calloc(1, sizeof(*cu) + build_id_len);
> >
> > if (cu != NULL) {
> > uint32_t void_id;
> > --
> > 2.31.1
> >
