On Wed, Feb 20, 2019 at 10:27 AM Daniel Borkmann <dan...@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+8bf19ee2aa580de7a...@syzkaller.appspotmail.com
> >> Signed-off-by: Daniel Borkmann <dan...@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.
