Re: [PATCH bpf-next] bpf, seccomp: fix false positive preemption splat for cbpf->ebpf progs

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Wed, Feb 20, 2019 at 10:27 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 02/20/2019 06:07 PM, Alexei Starovoitov wrote:
> > On Wed, Feb 20, 2019 at 12:06:29PM +0100, Daniel Borkmann wrote:
> >> In 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> >> a check was added for BPF_PROG_RUN() that for every invocation preemption has
> >> to be disabled to not break eBPF assumptions (e.g. per-cpu map). Of course this
> >> does not count for seccomp because only cBPF -> eBPF is loaded here and it does
> >> not make use of any functionality that would require this assertion. Fix this
> >> false positive by adding and using __BPF_PROG_RUN() variant that does not have
> >> the cant_sleep(); check.
> >>
> >> Fixes: 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> >> Reported-by: syzbot+8bf19ee2aa580de7a2a7@syzkaller.appspotmail.com
> >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> >> ---
> >>  include/linux/filter.h | 9 ++++++++-
> >>  kernel/seccomp.c       | 2 +-
> >>  2 files changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/filter.h b/include/linux/filter.h
> >> index f32b3ec..2648fd7 100644
> >> --- a/include/linux/filter.h
> >> +++ b/include/linux/filter.h
> >> @@ -533,7 +533,14 @@ struct sk_filter {
> >>      struct bpf_prog *prog;
> >>  };
> >>
> >> -#define BPF_PROG_RUN(filter, ctx)  ({ cant_sleep(); (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
> >> +#define bpf_prog_run__non_preempt(prog, ctx)        \
> >> +    ({ cant_sleep(); __BPF_PROG_RUN(prog, ctx); })
> >> +/* Native eBPF or cBPF -> eBPF transitions. Preemption must be disabled. */
> >> +#define BPF_PROG_RUN(prog, ctx)                     \
> >> +    bpf_prog_run__non_preempt(prog, ctx)
> >> +/* Direct use for cBPF -> eBPF only, but not for native eBPF. */
> >
> > I think the comment is too abstract.
> > May be it should say that this is seccomp cBPF only ?
> > And macro name should be explicit as well ?
>
> I think macro naming is probably okay imho as used internally as
> well from BPF_PROG_RUN(), but I'll improve the comment to state
> seccomp specifically as an example there and providing some more
> background.

I'm worried about misuse of the macro.
If there was a word seccomp in it it would made people
think much harder before calling it.