On 02/18/2019 06:29 PM, Stanislav Fomichev wrote:
> On 02/16, Daniel Borkmann wrote:
>> On 02/13/2019 12:42 AM, Stanislav Fomichev wrote:
>>> Syzbot found out that running BPF_PROG_TEST_RUN with repeat=0xffffffff
>>> makes process unkillable. The problem is that when CONFIG_PREEMPT is
>>> enabled, we never see need_resched() return true. This is due to the
>>> fact that preempt_enable() (which we do in bpf_test_run_one on each
>>> iteration) now handles resched if it's needed.
>>>
>>> Let's disable preemption for the whole run, not per test. In this case
>>> we can properly see whether resched is needed.
>>> Let's also properly return -EINTR to the userspace in case of a signal
>>> interrupt.
>>>
>>> See recent discussion:
>>> http://lore.kernel.org/netdev/CAH3MdRWHr4N8jei8jxDppXjmw-Nw=pundlbu1dqofqhxfu2...@mail.gmail.com
>>>
>>> I'll follow up with the same fix bpf_prog_test_run_flow_dissector in
>>> bpf-next.
>>>
>>> Reported-by: syzbot <syzkal...@googlegroups.com>
>>> Signed-off-by: Stanislav Fomichev <s...@google.com>
>>> ---
>>> net/bpf/test_run.c | 45 ++++++++++++++++++++++++---------------------
>>> 1 file changed, 24 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
>>> index fa2644d276ef..e31e1b20f7f4 100644
>>> --- a/net/bpf/test_run.c
>>> +++ b/net/bpf/test_run.c
>>> @@ -13,27 +13,13 @@
>>> #include <net/sock.h>
>>> #include <net/tcp.h>
>>>
>>> -static __always_inline u32 bpf_test_run_one(struct bpf_prog *prog, void
>>> *ctx,
>>> - struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE])
>>> -{
>>> - u32 ret;
>>> -
>>> - preempt_disable();
>>> - rcu_read_lock();
>>> - bpf_cgroup_storage_set(storage);
>>> - ret = BPF_PROG_RUN(prog, ctx);
>>> - rcu_read_unlock();
>>> - preempt_enable();
>>> -
>>> - return ret;
>>> -}
>>> -
>>> -static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat, u32
>>> *ret,
>>> - u32 *time)
>>> +static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
>>> + u32 *retval, u32 *time)
>>> {
>>> struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { 0 };
>>> enum bpf_cgroup_storage_type stype;
>>> u64 time_start, time_spent = 0;
>>> + int ret = 0;
>>> u32 i;
>>>
>>> for_each_cgroup_storage_type(stype) {
>>> @@ -48,25 +34,42 @@ static int bpf_test_run(struct bpf_prog *prog, void
>>> *ctx, u32 repeat, u32 *ret,
>>>
>>> if (!repeat)
>>> repeat = 1;
>>> +
>>> + rcu_read_lock();
>>> + preempt_disable();
>>> time_start = ktime_get_ns();
>>> for (i = 0; i < repeat; i++) {
>>> - *ret = bpf_test_run_one(prog, ctx, storage);
>>> + bpf_cgroup_storage_set(storage);
>>> + *retval = BPF_PROG_RUN(prog, ctx);
>>> +
>>> + if (signal_pending(current)) {
>>> + ret = -EINTR;
>>> + break;
>>> + }
>>
>> Wouldn't it be enough to just move the signal_pending() test to
>> the above as you did to actually fix the unkillable issue? For
>> CONFIG_PREEMPT the below need_resched() is never triggered as you
>> mention as preempt_enable() handles rescheduling internally in
>> this situation, so moving it only out should suffice.
>>
>> The rationale for disabling preemption for the whole run is imho
>> a bit different, namely that you would not screw up the ktime
>> measurements due to rescheduling happening in between otherwise.
> That's exactly the reason why we need to preempt_disable() the whole
> run; we can't preempt on preempt_enable(), it would screw up our
> ktime estimation.
>
>> But then, once preemption is disabled for the whole run, is there
>> a need to move out the extra signal_pending() test (presumably as
>> need_resched() does not handle TIF_SIGPENDING but only TIF_NEED_RESCHED
>> but we still wouldn't get into a unkillable situation here, no)?
> I'm not sure, they look like two separate flags, it feels safer to handle
> them separately (and we have a precedent in do_check in verifier.c). While
> we do set them both when sending signal, it looks like need_resched is
> for the cases where we wake up a task with a higher priority. So, in
> theory, we can have a signal_pending without need_resched. (Also, with
> CONFIG_PREEMT=y kernel, there is another complication with
> preempt_count()).
Yeah, given there is no separation, it's better to move it out, agree.
Applied both, thanks!
>>> if (need_resched()) {
>>> - if (signal_pending(current))
>>> - break;
>>> time_spent += ktime_get_ns() - time_start;
>>> + preempt_enable();
>>> + rcu_read_unlock();
>>> +
>>> cond_resched();
>>> +
>>> + rcu_read_lock();
>>> + preempt_disable();
>>> time_start = ktime_get_ns();
>>> }
>>> }
>>> time_spent += ktime_get_ns() - time_start;
>>> + preempt_enable();
>>> + rcu_read_unlock();
>>> +
>>> do_div(time_spent, repeat);
>>> *time = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
>>>
>>> for_each_cgroup_storage_type(stype)
>>> bpf_cgroup_storage_free(storage[stype]);
>>>
>>> - return 0;
>>> + return ret;
>>> }
>>>
>>> static int bpf_test_finish(const union bpf_attr *kattr,
>>>
>>
