Re: [PATCH bpf-next 1/6] bpf: Support ->fill_link_info for kprobe_multi

[Yafang Shao <laoar.shao@gmail.com>]

On Sat, Jun 3, 2023 at 12:46 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> On 06/02, Yafang Shao wrote:
> > By adding support for ->fill_link_info to the kprobe_multi link, users will
> > be able to inspect it using `bpftool link show`. This enhancement will
> > expose both the count of probed functions and their respective addresses to
> > the user.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  include/uapi/linux/bpf.h       |  4 ++++
> >  kernel/trace/bpf_trace.c       | 26 ++++++++++++++++++++++++++
> >  tools/include/uapi/linux/bpf.h |  4 ++++
> >  3 files changed, 34 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index a7b5e91..22c8168 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -6438,6 +6438,10 @@ struct bpf_link_info {
> >                       __s32 priority;
> >                       __u32 flags;
> >               } netfilter;
> > +             struct {
> > +                     __u64 addrs;
> > +                     __u32 count;
> > +             } kprobe_multi;
> >       };
> >  } __attribute__((aligned(8)));
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 2bc41e6..ec53bc9 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2548,9 +2548,35 @@ static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
> >       kfree(kmulti_link);
> >  }
> >
> > +static int bpf_kprobe_multi_link_fill_link_info(const struct bpf_link *link,
> > +                                             struct bpf_link_info *info)
> > +{
> > +     u64 *uaddrs = (u64 *)u64_to_user_ptr(info->kprobe_multi.addrs);
>
> Maybe tag this as __user as well?
>
>         u64 __user *uaddrs = u64_to_user_ptr(info->kprobe_multi.addrs);
>
> copy_to_user expects __user tagged memory, so most likely sparse tool
> will complain on your current approach.

Thanks for pointing it out. Will correct it.

>
> > +     struct bpf_kprobe_multi_link *kmulti_link;
> > +     u32 ucount = info->kprobe_multi.count;
> > +
> > +     if (!uaddrs ^ !ucount)
> > +             return -EINVAL;
> > +
> > +     kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
> > +     if (!uaddrs) {
> > +             info->kprobe_multi.count = kmulti_link->cnt;
> > +             return 0;
> > +     }
> > +
> > +     if (!ucount)
> > +             return 0;
> > +     if (ucount != kmulti_link->cnt)
> > +             return -EINVAL;
>
> [..]
>
> > +     if (copy_to_user(uaddrs, kmulti_link->addrs, ucount * sizeof(u64)))
> > +             return -EFAULT;
>
> I'm not super familiar with this part, so maybe stupid question:
> do we need to hold any locks during the copy? IOW, can kmulti_link->addrs
> be updated concurrently?

No, we can't update kmulti_link->addrs concurrently.  Once a fprobe is
registered, we can't update it unless we unregister it. When we reach
the ->fill_link_info, it can't be released, so it is safe.


--
Regards
Yafang