Re: [PATCH bpf 1/2] bpf/test_run: fix unkillable BPF_PROG_TEST_RUN

[Stanislav Fomichev <sdf@fomichev.me>]

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=puNDLbu1dQOFQHxfU2onA@mail.gmail.com
> > 
> > I'll follow up with the same fix bpf_prog_test_run_flow_dissector in
> > bpf-next.
> > 
> > Reported-by: syzbot <syzkaller@googlegroups.com>
> > Signed-off-by: Stanislav Fomichev <sdf@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()).

> 
> >  		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,
> > 
>