On Fri, Sep 8, 2023 at 6:49 AM Alan Maguire <alan.maguire@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > Fixes: 0dcac2725406 ("bpf: Add multi kprobe link")
> > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
>
> For the series,
>
> Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
>
> ...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?
>
> /*
> * Kprobe override only works if they are on the function entry,
> * and only if they are on the opt-in list.
> */
> if (prog->kprobe_override &&
> (!trace_kprobe_on_func_entry(event->tp_event) ||
> !trace_kprobe_error_injectable(event->tp_event)))
> return -EINVAL;
>
>
> if this is needed, it might be good to augment the selftest to
> cover the case of specifying non-entry+kprobe override. thanks!
>
> Alan
>
>
> > + return 0;
> > +}
> > +
> > int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > {
> > struct bpf_kprobe_multi_link *link = NULL;
> > @@ -2930,6 +2941,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > goto error;
> > }
> >
> > + if (prog->kprobe_override && addrs_check_error_injection_list(addrs, cnt)) {
> > + err = -EINVAL;
> > + goto error;
> > + }
> > +
> > link = kzalloc(sizeof(*link), GFP_KERNEL);
> > if (!link) {
> > err = -ENOMEM;
