Re: [PATCH] net: Revert "net_sched: no need to free qdisc in RCU callback"

On 12/20/2017 01:59 PM, Jakub Kicinski wrote:

On Wed, 20 Dec 2017 12:09:19 -0800, John Fastabend wrote:

quoted

RCU grace period is needed for lockless qdiscs added in the commit
c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array").

It is needed now that qdiscs may be lockless otherwise we risk
free'ing a qdisc that is still in use from datapath. Additionally,
push list cleanup into RCU callback. Otherwise we risk the datapath
adding skbs during removal.

Fixes: c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>

Seems like this revert may be too heavy handed:

# ./tools/testing/selftests/bpf/test_offload.py --log /tmp/log
Test destruction of generic XDP...
Test TC non-offloaded...
Test TC non-offloaded isn't getting bound...
Test TC offloads are off by default...
Test TC offload by default...
Test TC cBPF bytcode tries offload by default...
Test TC cBPF unbound bytecode doesn't offload...
Test TC offloads work...
FAIL: TC filter did not load with TC offloads enabled

And it's triggering:

WARNING: CPU: 15 PID: 1853 at ../drivers/net/netdevsim/bpf.c:372 nsim_bpf_uninit+0x2e/0x41 [netdevsim]

Which is:

   368	void nsim_bpf_uninit(struct netdevsim *ns)
   369	{
   370		WARN_ON(!list_empty(&ns->bpf_bound_progs));
   371		WARN_ON(ns->xdp_prog);

quoted

quoted

372		WARN_ON(ns->bpf_offloaded);

   373	}

(Meaning the offload was not stopped by the stack before ndo_uninit.)

Dang. So offload code depends on destroy being called on a qdisc
to in turn destroy the filters and unbind any offloads.

I was hoping I could get away with tearing down live qdiscs without
too much work. Looks like not.

Note the fixes tag was bogus nothing is actually broken in current
code until a lockless qdisc with classes shows up.

.John