Re: [PATCH bpf 1/2] bpf: Add override check to kprobe multi link attach

From: Jiri Olsa <olsajiri@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Alan Maguire <alan.maguire@oracle.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	stable@vger.kernel.org, bpf@vger.kernel.org,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@chromium.org>,
	Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>,
	Djalal Harouni <tixxdz@gmail.com>
Subject: Re: [PATCH bpf 1/2] bpf: Add override check to kprobe multi link attach
Date: Sat, 9 Sep 2023 20:51:22 +0200	[thread overview]
Message-ID: <ZPy+qpHt2JrVqCVZ@krava> (raw)
In-Reply-To: <CAEf4BzZ_=AQ2rt1z=FUE6QoHq44Y_fCmh+xjbn-39NhLw5-VNg@mail.gmail.com>

On Fri, Sep 08, 2023 at 04:50:54PM -0700, Andrii Nakryiko wrote:
> On Fri, Sep 8, 2023 at 6:49 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > On 07/09/2023 21:06, Jiri Olsa wrote:
> > > Currently the multi_kprobe link attach does not check error
> > > injection list for programs with bpf_override_return helper
> > > and allows them to attach anywhere. Adding the missing check.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: 0dcac2725406 ("bpf: Add multi kprobe link")
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> >
> > For the series,
> >
> > Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> >
> > ...with one small question below...
> >
> > > ---
> > >  kernel/trace/bpf_trace.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index a7264b2c17ad..c1c1af63ced2 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -2853,6 +2853,17 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3
> > >       return arr.mods_cnt;
> > >  }
> > >
> > > +static int addrs_check_error_injection_list(unsigned long *addrs, u32 cnt)
> > > +{
> > > +     u32 i;
> > > +
> > > +     for (i = 0; i < cnt; i++) {
> > > +             if (!within_error_injection_list(addrs[i]))
> > > +                     return -EINVAL;
> >
> > do we need a check like trace_kprobe_on_func_entry() to verify that
> > it's a combination of function entry+kprobe override, or is that
> > handled elsewhere/not needed? perf_event_attach_bpf_prog() does
> 
> multi-kprobe programs are always attached at function entry, so I
> believe it's not necessary?

yes, fprobe allows only function entry.. should have put it in comment

thanks,
jirka