On Fri, Nov 03, 2017 at 05:52:22PM +0100, Daniel Borkmann wrote:
> On 11/03/2017 03:31 PM, Josef Bacik wrote:
> > On Fri, Nov 03, 2017 at 12:12:13AM +0100, Daniel Borkmann wrote:
> > > Hi Josef,
> > >
> > > one more issue I just noticed, see comment below:
> > >
> > > On 11/02/2017 03:37 PM, Josef Bacik wrote:
> > > [...]
> > > > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > > > index cdd78a7beaae..dfa44fd74bae 100644
> > > > --- a/include/linux/filter.h
> > > > +++ b/include/linux/filter.h
> > > > @@ -458,7 +458,8 @@ struct bpf_prog {
> > > > locked:1, /* Program image
> > > > locked? */
> > > > gpl_compatible:1, /* Is filter GPL
> > > > compatible? */
> > > > cb_access:1, /* Is control block
> > > > accessed? */
> > > > - dst_needed:1; /* Do we need dst
> > > > entry? */
> > > > + dst_needed:1, /* Do we need dst
> > > > entry? */
> > > > + kprobe_override:1; /* Do we override a
> > > > kprobe? */
> > > > kmemcheck_bitfield_end(meta);
> > > > enum bpf_prog_type type; /* Type of BPF program
> > > > */
> > > > u32 len; /* Number of filter
> > > > blocks */
> > > [...]
> > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > index d906775e12c1..f8f7927a9152 100644
> > > > --- a/kernel/bpf/verifier.c
> > > > +++ b/kernel/bpf/verifier.c
> > > > @@ -4189,6 +4189,8 @@ static int fixup_bpf_calls(struct
> > > > bpf_verifier_env *env)
> > > > prog->dst_needed = 1;
> > > > if (insn->imm == BPF_FUNC_get_prandom_u32)
> > > > bpf_user_rnd_init_once();
> > > > + if (insn->imm == BPF_FUNC_override_return)
> > > > + prog->kprobe_override = 1;
> > > > if (insn->imm == BPF_FUNC_tail_call) {
> > > > /* If we tail call into other programs, we
> > > > * cannot make any assumptions since they can
> > > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > > index 9660ee65fbef..0d7fce52391d 100644
> > > > --- a/kernel/events/core.c
> > > > +++ b/kernel/events/core.c
> > > > @@ -8169,6 +8169,13 @@ static int perf_event_set_bpf_prog(struct
> > > > perf_event *event, u32 prog_fd)
> > > > return -EINVAL;
> > > > }
> > > >
> > > > + /* Kprobe override only works for kprobes, not uprobes. */
> > > > + if (prog->kprobe_override &&
> > > > + !(event->tp_event->flags & TRACE_EVENT_FL_KPROBE)) {
> > > > + bpf_prog_put(prog);
> > > > + return -EINVAL;
> > > > + }
> > >
> > > Can we somehow avoid the prog->kprobe_override flag here completely
> > > and also same in the perf_event_attach_bpf_prog() handler?
> > >
> > > Reason is that it's not reliable for bailing out this way: Think of
> > > the main program you're attaching doesn't use bpf_override_return()
> > > helper, but it tail-calls into other BPF progs that make use of it
> > > instead. So above check would be useless and will fail and we continue
> > > to attach the prog for probes where it's not intended to be used.
> > >
> > > We've had similar issues in the past e.g. c2002f983767 ("bpf: fix
> > > checking xdp_adjust_head on tail calls") is just one of those. Thus,
> > > can we avoid the flag altogether and handle such error case differently?
> >
> > So if I'm reading this right there's no way to know what we'll tail call at
> > any
> > given point, so I need to go back to my previous iteration of this patch and
> > always save the state of the kprobe in the per-cpu variable to make sure we
> > don't use bpf_override_return in the wrong case?
>
> Yeah.
>
> > The tail call functions won't be in the BPF_PROG_ARRAY right? It'll be just
> > some other arbitrary function? If that's the case then we really need
> > something
> > like this
>
> With BPF_PROG_ARRAY you mean BPF_MAP_TYPE_PROG_ARRAY or the prog array
> for the tracing/multiprog attach point? The program you're calling into
> is inside the BPF_MAP_TYPE_PROG_ARRAY map, but can change at any time
> and can have nesting as well.
>
> > https://patchwork.kernel.org/patch/10034815/
> >
> > and I need to bring that back right? Thanks,
>
> I'm afraid so. The thing with skb cb_access which was brought up there is
> that once you have a tail call in the prog you cannot make any assumptions
> anymore, therefore the cb_access flag is set to 1 so we save/restore for
> those cases precautionary since it could be accessed or not later on. In
> your case I think this wouldn't work since legitimate bpf kprobes progs could
> use tail calls today, so setting prog->kprobe_override there would prevent
> attaching for non-kprobes due to subsequent flags & TRACE_EVENT_FL_KPROBE
> check.
how about preventing programs that use bpf_override_return to be
store into prog_array? imo that would be cleaner solution.
doing tail_call into them is kinda useless anyway.
Then we can keep the bit and fast path.
