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: [email protected]
>> Signed-off-by: Daniel Borkmann <[email protected]>
>> ---
>> 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.
>> +#define __BPF_PROG_RUN(prog, ctx) \
>> + (*(prog)->bpf_func)(ctx, (prog)->insnsi)
>>
>> #define BPF_SKB_CB_LEN QDISC_CB_PRIV_LEN
>>
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index e815781..826d4e4 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -268,7 +268,7 @@ static u32 seccomp_run_filters(const struct seccomp_data
>> *sd,
>> * value always takes priority (ignoring the DATA).
>> */
>> for (; f; f = f->prev) {
>> - u32 cur_ret = BPF_PROG_RUN(f->prog, sd);
>> + u32 cur_ret = __BPF_PROG_RUN(f->prog, sd);
>>
>> if (ACTION_ONLY(cur_ret) < ACTION_ONLY(ret)) {
>> ret = cur_ret;
>> --
>> 2.9.5
>>
