Re: [PATCH v5 3/6] fs/procfs: add build ID fetching to PROCMAP_QUERY API

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Wed, Jun 19, 2024 at 3:14 AM Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> On Tue, Jun 18, 2024 at 03:45:22PM -0700, Andrii Nakryiko wrote:
> > The need to get ELF build ID reliably is an important aspect when
> > dealing with profiling and stack trace symbolization, and
> > /proc/<pid>/maps textual representation doesn't help with this.
>
> > @@ -539,6 +543,21 @@ static int do_procmap_query(struct proc_maps_private *priv, void __user *uarg)
> >               }
> >       }
> >
> > +     if (karg.build_id_size) {
> > +             __u32 build_id_sz;
> > +
> > +             err = build_id_parse(vma, build_id_buf, &build_id_sz);
>
> This is not your bug but build_id_parse() assumes program headers
> immediately follow ELF header which is not guaranteed.

Yes, I'm aware, and I think I stated somewhere that I want to
fix/improve that. The thing is, current build_id_parse() was built for
BPF under NMI context assumption, which is why it can't page in memory
and so on (and this "build ID has to be in the first page" was a
surprise to me, but probably just a technical shortcut to make it a
bit easier to implement). Regardless, my plan, once this API is
merged, is to follow up with make build_id_parse() variant that would
work reliably under sleepable context assumptions. Hopefully that's ok
not to bundle all that with these patches?

>
> > +      * If this field is set to non-zero value, build_id_addr should point
> > +      * to valid user space memory buffer of at least build_id_size bytes.
> > +      * If set to zero, build_id_addr should be set to zero as well
> > +      */
> > +     __u32 build_id_size;            /* in/out */
> >       /*
> >        * User-supplied address of a buffer of at least vma_name_size bytes
> >        * for kernel to fill with matched VMA's name (see vma_name_size field
> > @@ -519,6 +539,14 @@ struct procmap_query {
> >        * Should be set to zero if VMA name should not be returned.
> >        */
> >       __u64 vma_name_addr;            /* in */
> > +     /*
> > +      * User-supplied address of a buffer of at least build_id_size bytes
> > +      * for kernel to fill with matched VMA's ELF build ID, if available
> > +      * (see build_id_size field description above for details).
> > +      *
> > +      * Should be set to zero if build ID should not be returned.
> > +      */
> > +     __u64 build_id_addr;            /* in */
>
> Can this be simplified to 512-bit buffer in ioctl structure?
> BUILD_ID_SIZE_MAX is 20 which is sha1.

I'd prefer not to because vma_name can't use the same trick, so we'll
have to have this size+buffer address approach anyway. And because of
that I'd like to have all these optional variable-length/string output
arguments handled in a uniform way. In practice, it's really simple to
use this from user-space, the only mildly annoying part is casting
pointer to __u64. But as I said, for vma_name users will do this
anyways, so not much benefit simplifying the build_id part only.