Re: [PATCH bpf-next 2/3] bpf: Add size arg to build_id_parse function

[Yonghong Song]

On 1/14/21 12:01 PM, Jiri Olsa wrote:
> On Thu, Jan 14, 2021 at 10:56:33AM -0800, Yonghong Song wrote:
> > On 1/14/21 5:40 AM, Jiri Olsa wrote:
> > > It's possible to have other build id types (other than default SHA1).
> > > Currently there's also ld support for MD5 build id.
> >
> > Currently, bpf build_id based stackmap does not returns the size of
> > the build_id. Did you see an issue here? I guess user space can check
> > the length of non-zero bits of the build id to decide what kind of
> > type it is, right?
>
> you can have zero bytes in the build id hash, so you need to get the size
>
> I never saw MD5 being used in practise just SHA1, but we added the
> size to be complete and make sure we'll fit with build id, because
> there's only limited space in mmap2 event

I am asking to check whether we should extend uapi struct
bpf_stack_build_id to include build_id_size as well. I guess
we can delay this until a real use case.


> jirka
>
> > > Adding size argument to build_id_parse function, that returns (if defined)
> > > size of the parsed build id, so we can recognize the build id type.
> > >
> > > Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> > > Cc: Song Liu <songliubraving@xxxxxx>
> > > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > ---
> > >    include/linux/buildid.h |  3 ++-
> > >    kernel/bpf/stackmap.c   |  2 +-
> > >    lib/buildid.c           | 29 +++++++++++++++++++++--------
> > >    3 files changed, 24 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/linux/buildid.h b/include/linux/buildid.h
> > > index 08028a212589..40232f90db6e 100644
> > > --- a/include/linux/buildid.h
> > > +++ b/include/linux/buildid.h
> > > @@ -6,6 +6,7 @@
> > >    #define BUILD_ID_SIZE_MAX 20
> > > -int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id);
> > > +int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
> > > +		   __u32 *size);
> > >    #endif
> > > diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> > > index 55d254a59f07..cabaf7db8efc 100644
> > > --- a/kernel/bpf/stackmap.c
> > > +++ b/kernel/bpf/stackmap.c
> > > @@ -189,7 +189,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
> > >    	for (i = 0; i < trace_nr; i++) {
> > >    		vma = find_vma(current->mm, ips[i]);
> > > -		if (!vma || build_id_parse(vma, id_offs[i].build_id)) {
> > > +		if (!vma || build_id_parse(vma, id_offs[i].build_id, NULL)) {
> > >    			/* per entry fall back to ips */
> > >    			id_offs[i].status = BPF_STACK_BUILD_ID_IP;
> > >    			id_offs[i].ip = ips[i];
> > > diff --git a/lib/buildid.c b/lib/buildid.c
> > > index 4a4f520c0e29..6156997c3895 100644
> > > --- a/lib/buildid.c
> > > +++ b/lib/buildid.c
> > > @@ -12,6 +12,7 @@
> > >     */
> > >    static inline int parse_build_id(void *page_addr,
> > >    				 unsigned char *build_id,
> > > +				 __u32 *size,
> > >    				 void *note_start,
> > >    				 Elf32_Word note_size)
> > >    {
> > > @@ -38,6 +39,8 @@ static inline int parse_build_id(void *page_addr,
> > >    			       nhdr->n_descsz);
> > >    			memset(build_id + nhdr->n_descsz, 0,
> > >    			       BUILD_ID_SIZE_MAX - nhdr->n_descsz);
> > > +			if (size)
> > > +				*size = nhdr->n_descsz;
> > >    			return 0;
> > >    		}
> > >    		new_offs = note_offs + sizeof(Elf32_Nhdr) +
> > > @@ -50,7 +53,8 @@ static inline int parse_build_id(void *page_addr,
> > >    }
> > [...]