Re: [PATCH v5 bpf-next 01/11] bpf: Support ->fill_link_info for kprobe_multi

From: Yafang Shao <laoar.shao@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: ast@kernel.org, daniel@iogearbox.net, john.fastabend@gmail.com,
	 andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
	yhs@fb.com,  kpsingh@kernel.org, sdf@google.com,
	haoluo@google.com, jolsa@kernel.org,  quentin@isovalent.com,
	rostedt@goodmis.org, mhiramat@kernel.org,  bpf@vger.kernel.org,
	linux-trace-kernel@vger.kernel.org
Subject: Re: [PATCH v5 bpf-next 01/11] bpf: Support ->fill_link_info for kprobe_multi
Date: Sun, 25 Jun 2023 22:34:43 +0800	[thread overview]
Message-ID: <CALOAHbBrmRJfXTqv6W5G=S5A-k=es91KLym3drec2xkxpFMv8w@mail.gmail.com> (raw)
In-Reply-To: <CAEf4BzaYmAmkm9HL1BPoddPtq=A2caqPm0QR_yQn44GA7TZVVQ@mail.gmail.com>

On Sat, Jun 24, 2023 at 5:45 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jun 23, 2023 at 7:16 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > With the addition of support for fill_link_info to the kprobe_multi link,
> > users will gain the ability to inspect it conveniently using the
> > `bpftool link show`. This enhancement provides valuable information to the
> > user, including the count of probed functions and their respective
> > addresses. It's important to note that if the kptr_restrict setting is not
> > permitted, the probed address will not be exposed, ensuring security.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  include/uapi/linux/bpf.h       |  5 +++++
> >  kernel/trace/bpf_trace.c       | 28 ++++++++++++++++++++++++++++
> >  tools/include/uapi/linux/bpf.h |  5 +++++
> >  3 files changed, 38 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index a7b5e91..23691ea 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -6438,6 +6438,11 @@ struct bpf_link_info {
> >                         __s32 priority;
> >                         __u32 flags;
> >                 } netfilter;
> > +               struct {
> > +                       __aligned_u64 addrs; /* in/out: addresses buffer ptr */
> > +                       __u32 count;
> > +                       __u32 flags;
> > +               } kprobe_multi;
> >         };
> >  } __attribute__((aligned(8)));
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 2bc41e6..2123197b 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2459,6 +2459,7 @@ struct bpf_kprobe_multi_link {
> >         u32 cnt;
> >         u32 mods_cnt;
> >         struct module **mods;
> > +       u32 flags;
> >  };
> >
> >  struct bpf_kprobe_multi_run_ctx {
> > @@ -2548,9 +2549,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 __user *uaddrs = u64_to_user_ptr(info->kprobe_multi.addrs);
> > +       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);
> > +       info->kprobe_multi.count = kmulti_link->cnt;
> > +       info->kprobe_multi.flags = kmulti_link->flags;
> > +
> > +       if (!uaddrs)
> > +               return 0;
> > +       if (ucount < kmulti_link->cnt)
> > +               return -EINVAL;
>
> it would be probably sane behavior to copy ucount items and return -E2BIG

Agree.

>
> > +       if (!kallsyms_show_value(current_cred()))
> > +               return 0;
>
> at least we should zero out kmulti_link->cnt elements. Otherwise it's
> hard for user-space know whether returned data is garbage or not?

Agree. Should clear it.

>
>
> > +       if (copy_to_user(uaddrs, kmulti_link->addrs, ucount * sizeof(u64)))
>
> s/ucount/kmulti_link->cnt/ ?

Yes. Thanks for pointing it out.

-- 
Regards
Yafang