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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Dec 2017 15:40:49 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Jakub Kicinski <kubakici@...pl>
Cc:     xiyou.wangcong@...il.com, jiri@...nulli.us, davem@...emloft.net,
        netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: 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:
>> 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@...il.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);
>>> 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