[Patch net 00/16] net_sched: fix races with RCU callbacks

[Patch net 00/16] net_sched: fix races with RCU callbacks

[Cong Wang]

Recently, the RCU callbacks used in TC filters and TC actions keep
drawing my attention, they introduce at least 4 race condition bugs:

1. A simple one fixed by Daniel:

commit c78e1746d3ad7d548bdf3fe491898cc453911a49
Author: Daniel Borkmann <daniel@iogearbox.net>
Date:   Wed May 20 17:13:33 2015 +0200

    net: sched: fix call_rcu() race on classifier module unloads

2. A very nasty one fixed by me:

commit 1697c4bb5245649a23f06a144cc38c06715e1b65
Author: Cong Wang <xiyou.wangcong@gmail.com>
Date:   Mon Sep 11 16:33:32 2017 -0700

    net_sched: carefully handle tcf_block_put()

3. Two more bugs found by Chris:
https://patchwork.ozlabs.org/patch/826696/
https://patchwork.ozlabs.org/patch/826695/

Usually RCU callbacks are simple, however for TC filters and actions,
they are complex because at least TC actions could be destroyed
together with the TC filter in one callback. And RCU callbacks are
invoked in BH context, without locking they are parallel too. All of
these contribute to the cause of these nasty bugs.

Alternatively, we could also:

a) Introduce a spinlock to serialize these RCU callbacks. But as I
said in commit 1697c4bb5245 ("net_sched: carefully handle
tcf_block_put()"), it is very hard to do because of tcf_chain_dump().
Potentially we need to do a lot of work to make it possible (if not
impossible).

b) Just get rid of these RCU callbacks, because they are not
necessary at all, callers of these call_rcu() are all on slow paths
and holding RTNL lock, so blocking is allowed in their contexts.
However, David and Eric dislike adding synchronize_rcu() here.

As suggested by Paul, we could defer the work to a workqueue and
gain the permission of holding RTNL again without any performance
impact, however, in tcf_block_put() we could have a deadlock when
flushing workqueue while hodling RTNL lock, the trick here is to
defer the work itself in workqueue and make it queued after all
other works so that we keep the same ordering to avoid any
use-after-free. Please see the first patch for details.

Patch 1 introduces the infrastructure, patch 2~12 move each
tc filter to the new tc filter workqueue, patch 13 adds
an assertion to catch potential bugs like this, patch 14
closes another rcu callback race, patch 15 and patch 16 add
new test cases.

Reported-by: Chris Mi <chrism@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Chris Mi (2):
  selftests: Introduce a new script to generate tc batch file
  selftests: Introduce a new test case to tc testsuite

Cong Wang (14):
  net_sched: introduce a workqueue for RCU callbacks of tc filter
  net_sched: use tcf_queue_work() in basic filter
  net_sched: use tcf_queue_work() in bpf filter
  net_sched: use tcf_queue_work() in cgroup filter
  net_sched: use tcf_queue_work() in flow filter
  net_sched: use tcf_queue_work() in flower filter
  net_sched: use tcf_queue_work() in fw filter
  net_sched: use tcf_queue_work() in matchall filter
  net_sched: use tcf_queue_work() in u32 filter
  net_sched: use tcf_queue_work() in route filter
  net_sched: use tcf_queue_work() in rsvp filter
  net_sched: use tcf_queue_work() in tcindex filter
  net_sched: add rtnl assertion to tcf_exts_destroy()
  net_sched: fix call_rcu() race on act_sample module removal

 include/net/pkt_cls.h                              |  3 +
 include/net/sch_generic.h                          |  2 +
 net/sched/act_sample.c                             |  1 +
 net/sched/cls_api.c                                | 69 ++++++++++++++++------
 net/sched/cls_basic.c                              | 20 ++++++-
 net/sched/cls_bpf.c                                | 19 +++++-
 net/sched/cls_cgroup.c                             | 22 +++++--
 net/sched/cls_flow.c                               | 19 +++++-
 net/sched/cls_flower.c                             | 19 +++++-
 net/sched/cls_fw.c                                 | 19 +++++-
 net/sched/cls_matchall.c                           | 19 +++++-
 net/sched/cls_route.c                              | 19 +++++-
 net/sched/cls_rsvp.h                               | 19 +++++-
 net/sched/cls_tcindex.c                            | 38 ++++++++++--
 net/sched/cls_u32.c                                | 29 ++++++++-
 .../tc-testing/tc-tests/filters/tests.json         | 23 +++++++-
 tools/testing/selftests/tc-testing/tdc.py          | 20 +++++--
 tools/testing/selftests/tc-testing/tdc_batch.py    | 62 +++++++++++++++++++
 tools/testing/selftests/tc-testing/tdc_config.py   |  2 +
 19 files changed, 367 insertions(+), 57 deletions(-)
 create mode 100755 tools/testing/selftests/tc-testing/tdc_batch.py

-- 
2.13.0

[Patch net 01/16] net_sched: introduce a workqueue for RCU callbacks of tc filter

[Cong Wang]

This patch introduces a dedicated workqueue for tc filters
so that each tc filter's RCU callback could defer their
action destroy work to this workqueue. The helper
tcf_queue_work() is introduced for them to use.

Because we hold RTNL lock when calling tcf_block_put(), we
can not simply flush works inside it, therefore we have to
defer it again to this workqueue and make sure all flying RCU
callbacks have already queued their work before this one, in
other words, to ensure this is the last one to execute to
prevent any use-after-free.

On the other hand, this makes tcf_block_put() ugly and
harder to understand. Since David and Eric strongly dislike
adding synchronize_rcu(), this is probably the only
solution that could make everyone happy.

Please also see the code comments below.

Reported-by: Chris Mi <chrism@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/pkt_cls.h     |  3 +++
 include/net/sch_generic.h |  2 ++
 net/sched/cls_api.c       | 68 +++++++++++++++++++++++++++++++++++------------
 3 files changed, 56 insertions(+), 17 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index e80edd8879ef..3009547f3c66 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -2,6 +2,7 @@
 #define __NET_PKT_CLS_H
 
 #include <linux/pkt_cls.h>
+#include <linux/workqueue.h>
 #include <net/sch_generic.h>
 #include <net/act_api.h>
 
@@ -17,6 +18,8 @@ struct tcf_walker {
 int register_tcf_proto_ops(struct tcf_proto_ops *ops);
 int unregister_tcf_proto_ops(struct tcf_proto_ops *ops);
 
+bool tcf_queue_work(struct work_struct *work);
+
 #ifdef CONFIG_NET_CLS
 struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
 				bool create);
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 135f5a2dd931..0dec8a23be57 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -10,6 +10,7 @@
 #include <linux/dynamic_queue_limits.h>
 #include <linux/list.h>
 #include <linux/refcount.h>
+#include <linux/workqueue.h>
 #include <net/gen_stats.h>
 #include <net/rtnetlink.h>
 
@@ -271,6 +272,7 @@ struct tcf_chain {
 
 struct tcf_block {
 	struct list_head chain_list;
+	struct work_struct work;
 };
 
 static inline void qdisc_cb_private_validate(const struct sk_buff *skb, int sz)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 0b2219adf520..045d13679ad6 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -77,6 +77,8 @@ int register_tcf_proto_ops(struct tcf_proto_ops *ops)
 }
 EXPORT_SYMBOL(register_tcf_proto_ops);
 
+static struct workqueue_struct *tc_filter_wq;
+
 int unregister_tcf_proto_ops(struct tcf_proto_ops *ops)
 {
 	struct tcf_proto_ops *t;
@@ -86,6 +88,7 @@ int unregister_tcf_proto_ops(struct tcf_proto_ops *ops)
 	 * tcf_proto_ops's destroy() handler.
 	 */
 	rcu_barrier();
+	flush_workqueue(tc_filter_wq);
 
 	write_lock(&cls_mod_lock);
 	list_for_each_entry(t, &tcf_proto_base, head) {
@@ -100,6 +103,12 @@ int unregister_tcf_proto_ops(struct tcf_proto_ops *ops)
 }
 EXPORT_SYMBOL(unregister_tcf_proto_ops);
 
+bool tcf_queue_work(struct work_struct *work)
+{
+	return queue_work(tc_filter_wq, work);
+}
+EXPORT_SYMBOL(tcf_queue_work);
+
 /* Select new prio value from the range, managed by kernel. */
 
 static inline u32 tcf_auto_prio(struct tcf_proto *tp)
@@ -266,23 +275,30 @@ int tcf_block_get(struct tcf_block **p_block,
 }
 EXPORT_SYMBOL(tcf_block_get);
 
-void tcf_block_put(struct tcf_block *block)
+static void tcf_block_put_final(struct work_struct *work)
 {
+	struct tcf_block *block = container_of(work, struct tcf_block, work);
 	struct tcf_chain *chain, *tmp;
 
-	if (!block)
-		return;
-
-	/* XXX: Standalone actions are not allowed to jump to any chain, and
-	 * bound actions should be all removed after flushing. However,
-	 * filters are destroyed in RCU callbacks, we have to hold the chains
-	 * first, otherwise we would always race with RCU callbacks on this list
-	 * without proper locking.
-	 */
+	/* At this point, all the chains should have refcnt == 1. */
+	rtnl_lock();
+	list_for_each_entry_safe(chain, tmp, &block->chain_list, list)
+		tcf_chain_put(chain);
+	rtnl_unlock();
+	kfree(block);
+}
 
-	/* Wait for existing RCU callbacks to cool down. */
-	rcu_barrier();
+/* XXX: Standalone actions are not allowed to jump to any chain, and bound
+ * actions should be all removed after flushing. However, filters are destroyed
+ * in RCU callbacks, we have to hold the chains first, otherwise we would
+ * always race with RCU callbacks on this list without proper locking.
+ */
+static void tcf_block_put_deferred(struct work_struct *work)
+{
+	struct tcf_block *block = container_of(work, struct tcf_block, work);
+	struct tcf_chain *chain;
 
+	rtnl_lock();
 	/* Hold a refcnt for all chains, except 0, in case they are gone. */
 	list_for_each_entry(chain, &block->chain_list, list)
 		if (chain->index)
@@ -292,13 +308,27 @@ void tcf_block_put(struct tcf_block *block)
 	list_for_each_entry(chain, &block->chain_list, list)
 		tcf_chain_flush(chain);
 
-	/* Wait for RCU callbacks to release the reference count. */
+	INIT_WORK(&block->work, tcf_block_put_final);
+	/* Wait for RCU callbacks to release the reference count and make
+	 * sure their works have been queued before this.
+	 */
 	rcu_barrier();
+	tcf_queue_work(&block->work);
+	rtnl_unlock();
+}
 
-	/* At this point, all the chains should have refcnt == 1. */
-	list_for_each_entry_safe(chain, tmp, &block->chain_list, list)
-		tcf_chain_put(chain);
-	kfree(block);
+void tcf_block_put(struct tcf_block *block)
+{
+	if (!block)
+		return;
+
+	INIT_WORK(&block->work, tcf_block_put_deferred);
+	/* Wait for existing RCU callbacks to cool down, make sure their works
+	 * have been queued before this. We can not flush pending works here
+	 * because we are holding the RTNL lock.
+	 */
+	rcu_barrier();
+	tcf_queue_work(&block->work);
 }
 EXPORT_SYMBOL(tcf_block_put);
 
@@ -1030,6 +1060,10 @@ EXPORT_SYMBOL(tcf_exts_get_dev);
 
 static int __init tc_filter_init(void)
 {
+	tc_filter_wq = alloc_ordered_workqueue("tc_filter_workqueue", 0);
+	if (!tc_filter_wq)
+		return -ENOMEM;
+
 	rtnl_register(PF_UNSPEC, RTM_NEWTFILTER, tc_ctl_tfilter, NULL, 0);
 	rtnl_register(PF_UNSPEC, RTM_DELTFILTER, tc_ctl_tfilter, NULL, 0);
 	rtnl_register(PF_UNSPEC, RTM_GETTFILTER, tc_ctl_tfilter,
-- 
2.13.0

Re: [Patch net 01/16] net_sched: introduce a workqueue for RCU callbacks of tc filter

[Eric Dumazet]

On Thu, 2017-10-26 at 18:24 -0700, Cong Wang wrote:
> ...

> On the other hand, this makes tcf_block_put() ugly and
> harder to understand. Since David and Eric strongly dislike
> adding synchronize_rcu(), this is probably the only
> solution that could make everyone happy.


...

> +static void tcf_block_put_deferred(struct work_struct *work)
> +{
> +	struct tcf_block *block = container_of(work, struct tcf_block, work);
> +	struct tcf_chain *chain;
>  
> +	rtnl_lock();
>  	/* Hold a refcnt for all chains, except 0, in case they are gone. */
>  	list_for_each_entry(chain, &block->chain_list, list)
>  		if (chain->index)
> @@ -292,13 +308,27 @@ void tcf_block_put(struct tcf_block *block)
>  	list_for_each_entry(chain, &block->chain_list, list)
>  		tcf_chain_flush(chain);
>  
> -	/* Wait for RCU callbacks to release the reference count. */
> +	INIT_WORK(&block->work, tcf_block_put_final);
> +	/* Wait for RCU callbacks to release the reference count and make
> +	 * sure their works have been queued before this.
> +	 */
>  	rcu_barrier();
> +	tcf_queue_work(&block->work);
> +	rtnl_unlock();
> +}


On a loaded server, rcu_barrier() typically takes 4 ms.

Way better than synchronize_rcu() (about 90 ms) but still an issue when
holding RTNL.

We have thousands of filters, and management daemon restarts and rebuild
TC hierarchy from scratch.

Simply getting rid of 1000 old filters might block RTNL for a while, or
maybe I misunderstood your patches.

Thanks.

Re: [Patch net 01/16] net_sched: introduce a workqueue for RCU callbacks of tc filter

[Cong Wang]

On Thu, Oct 26, 2017 at 9:05 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2017-10-26 at 18:24 -0700, Cong Wang wrote:
>> ...
>
>> On the other hand, this makes tcf_block_put() ugly and
>> harder to understand. Since David and Eric strongly dislike
>> adding synchronize_rcu(), this is probably the only
>> solution that could make everyone happy.
>
>
> ...
>
>> +static void tcf_block_put_deferred(struct work_struct *work)
>> +{
>> +     struct tcf_block *block = container_of(work, struct tcf_block, work);
>> +     struct tcf_chain *chain;
>>
>> +     rtnl_lock();
>>       /* Hold a refcnt for all chains, except 0, in case they are gone. */
>>       list_for_each_entry(chain, &block->chain_list, list)
>>               if (chain->index)
>> @@ -292,13 +308,27 @@ void tcf_block_put(struct tcf_block *block)
>>       list_for_each_entry(chain, &block->chain_list, list)
>>               tcf_chain_flush(chain);
>>
>> -     /* Wait for RCU callbacks to release the reference count. */
>> +     INIT_WORK(&block->work, tcf_block_put_final);
>> +     /* Wait for RCU callbacks to release the reference count and make
>> +      * sure their works have been queued before this.
>> +      */
>>       rcu_barrier();
>> +     tcf_queue_work(&block->work);
>> +     rtnl_unlock();
>> +}
>
>
> On a loaded server, rcu_barrier() typically takes 4 ms.
>
> Way better than synchronize_rcu() (about 90 ms) but still an issue when
> holding RTNL.
>
> We have thousands of filters, and management daemon restarts and rebuild
> TC hierarchy from scratch.
>
> Simply getting rid of 1000 old filters might block RTNL for a while, or
> maybe I misunderstood your patches.
>

Paul pointed out the same.

As I replied, this rcu_barrier() is NOT added by this patchset, it is already
there in current master branch.

Re: [Patch net 01/16] net_sched: introduce a workqueue for RCU callbacks of tc filter

[Eric Dumazet]

On Thu, 2017-10-26 at 21:28 -0700, Cong Wang wrote:
> On Thu, Oct 26, 2017 at 9:05 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Thu, 2017-10-26 at 18:24 -0700, Cong Wang wrote:
> >> ...
> >
> >> On the other hand, this makes tcf_block_put() ugly and
> >> harder to understand. Since David and Eric strongly dislike
> >> adding synchronize_rcu(), this is probably the only
> >> solution that could make everyone happy.
> >
> >
> > ...
> >
> >> +static void tcf_block_put_deferred(struct work_struct *work)
> >> +{
> >> +     struct tcf_block *block = container_of(work, struct tcf_block, work);
> >> +     struct tcf_chain *chain;
> >>
> >> +     rtnl_lock();
> >>       /* Hold a refcnt for all chains, except 0, in case they are gone. */
> >>       list_for_each_entry(chain, &block->chain_list, list)
> >>               if (chain->index)
> >> @@ -292,13 +308,27 @@ void tcf_block_put(struct tcf_block *block)
> >>       list_for_each_entry(chain, &block->chain_list, list)
> >>               tcf_chain_flush(chain);
> >>
> >> -     /* Wait for RCU callbacks to release the reference count. */
> >> +     INIT_WORK(&block->work, tcf_block_put_final);
> >> +     /* Wait for RCU callbacks to release the reference count and make
> >> +      * sure their works have been queued before this.
> >> +      */
> >>       rcu_barrier();
> >> +     tcf_queue_work(&block->work);
> >> +     rtnl_unlock();
> >> +}
> >
> >
> > On a loaded server, rcu_barrier() typically takes 4 ms.
> >
> > Way better than synchronize_rcu() (about 90 ms) but still an issue when
> > holding RTNL.
> >
> > We have thousands of filters, and management daemon restarts and rebuild
> > TC hierarchy from scratch.
> >
> > Simply getting rid of 1000 old filters might block RTNL for a while, or
> > maybe I misunderstood your patches.
> >
> 
> Paul pointed out the same.
> 
> As I replied, this rcu_barrier() is NOT added by this patchset, it is already
> there in current master branch.

You added the rtnl_lock()  rtnl_unlock()...

I really do not care if hundreds of tasks (not owning rtnl) call
rcu_barrier()...

Also we are still using a 4.3 based kernel, and no rcu_barrier() is used
in filters dismantle ( unregister_tcf_proto_ops() is not used in our
workloads )

Somehow something went very wrong in net/sched in recent kernels.

Re: [Patch net 01/16] net_sched: introduce a workqueue for RCU callbacks of tc filter

[Paul E. McKenney]

On Thu, Oct 26, 2017 at 09:39:26PM -0700, Eric Dumazet wrote:
> On Thu, 2017-10-26 at 21:28 -0700, Cong Wang wrote:
> > On Thu, Oct 26, 2017 at 9:05 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > On Thu, 2017-10-26 at 18:24 -0700, Cong Wang wrote:
> > >> ...
> > >
> > >> On the other hand, this makes tcf_block_put() ugly and
> > >> harder to understand. Since David and Eric strongly dislike
> > >> adding synchronize_rcu(), this is probably the only
> > >> solution that could make everyone happy.
> > >
> > >
> > > ...
> > >
> > >> +static void tcf_block_put_deferred(struct work_struct *work)
> > >> +{
> > >> +     struct tcf_block *block = container_of(work, struct tcf_block, work);
> > >> +     struct tcf_chain *chain;
> > >>
> > >> +     rtnl_lock();
> > >>       /* Hold a refcnt for all chains, except 0, in case they are gone. */
> > >>       list_for_each_entry(chain, &block->chain_list, list)
> > >>               if (chain->index)
> > >> @@ -292,13 +308,27 @@ void tcf_block_put(struct tcf_block *block)
> > >>       list_for_each_entry(chain, &block->chain_list, list)
> > >>               tcf_chain_flush(chain);
> > >>
> > >> -     /* Wait for RCU callbacks to release the reference count. */
> > >> +     INIT_WORK(&block->work, tcf_block_put_final);
> > >> +     /* Wait for RCU callbacks to release the reference count and make
> > >> +      * sure their works have been queued before this.
> > >> +      */
> > >>       rcu_barrier();
> > >> +     tcf_queue_work(&block->work);
> > >> +     rtnl_unlock();
> > >> +}
> > >
> > >
> > > On a loaded server, rcu_barrier() typically takes 4 ms.
> > >
> > > Way better than synchronize_rcu() (about 90 ms) but still an issue when
> > > holding RTNL.
> > >
> > > We have thousands of filters, and management daemon restarts and rebuild
> > > TC hierarchy from scratch.
> > >
> > > Simply getting rid of 1000 old filters might block RTNL for a while, or
> > > maybe I misunderstood your patches.
> > >
> > 
> > Paul pointed out the same.
> > 
> > As I replied, this rcu_barrier() is NOT added by this patchset, it is already
> > there in current master branch.
> 
> You added the rtnl_lock()  rtnl_unlock()...
> 
> I really do not care if hundreds of tasks (not owning rtnl) call
> rcu_barrier()...
> 
> Also we are still using a 4.3 based kernel, and no rcu_barrier() is used
> in filters dismantle ( unregister_tcf_proto_ops() is not used in our
> workloads )
> 
> Somehow something went very wrong in net/sched in recent kernels.

Would this be a good time for me to repeat my suggestion that timers
be used to aggregate the work done in the workqueue handlers, thus
decreasing the number of rcu_barrier() calls done under RTNL?

							Thanx, Paul

Re: [Patch net 01/16] net_sched: introduce a workqueue for RCU callbacks of tc filter

[Cong Wang]

On Fri, Oct 27, 2017 at 4:55 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
>
> Would this be a good time for me to repeat my suggestion that timers
> be used to aggregate the work done in the workqueue handlers, thus
> decreasing the number of rcu_barrier() calls done under RTNL?
>

Yes, but not in this patchset. The current code is already so, therefore
needs a different patchset.

Quite contrarily, my patchset actually improves it _a little bit_ by moving
the second rcu_barrier() in current code base to a workqueue... The
contention is same, just that we no longer block tcf_block_put() path
with my patchset.

Re: [Patch net 01/16] net_sched: introduce a workqueue for RCU callbacks of tc filter

[Cong Wang]

On Thu, Oct 26, 2017 at 9:39 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2017-10-26 at 21:28 -0700, Cong Wang wrote:
>> On Thu, Oct 26, 2017 at 9:05 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> Paul pointed out the same.
>>
>> As I replied, this rcu_barrier() is NOT added by this patchset, it is already
>> there in current master branch.
>
> You added the rtnl_lock()  rtnl_unlock()...


Unfortunately you are wrong again, tcf_block_put() holds RTNL
in current code as well...


>
> I really do not care if hundreds of tasks (not owning rtnl) call
> rcu_barrier()...
>

You are so welcome to improve current code base, but why keep
blaming my patchset which neither introduces any rcu_barrier()
nor any RTNL? Did you even take a look at current master branch?


> Also we are still using a 4.3 based kernel, and no rcu_barrier() is used
> in filters dismantle ( unregister_tcf_proto_ops() is not used in our
> workloads )


My patchset is unfortunately not based on 4.3, this argument is totally
nonsense.

Also, filter block was introduced during v4.13.


>
> Somehow something went very wrong in net/sched in recent kernels.
>

What stops you to optimize current code?

Re: [Patch net 01/16] net_sched: introduce a workqueue for RCU callbacks of tc filter

[Eric Dumazet]

On Fri, 2017-10-27 at 08:37 -0700, Cong Wang wrote:
> On Thu, Oct 26, 2017 at 9:39 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Thu, 2017-10-26 at 21:28 -0700, Cong Wang wrote:
> >> On Thu, Oct 26, 2017 at 9:05 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> Paul pointed out the same.
> >>
> >> As I replied, this rcu_barrier() is NOT added by this patchset, it is already
> >> there in current master branch.
> >
> > You added the rtnl_lock()  rtnl_unlock()...
> 
> 
> Unfortunately you are wrong again, tcf_block_put() holds RTNL
> in current code as well...

Then current code must be fixed, without adding more crap.

> 

> What stops you to optimize current code?

I might be forced to do so, eventually.

[Patch net 02/16] net_sched: use tcf_queue_work() in basic filter

[Cong Wang]

Defer the tcf_exts_destroy() in RCU callback to
tc filter workqueue and get RTNL lock.

Reported-by: Chris Mi <chrism@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_basic.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index d89ebafd2239..f177649a2419 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -34,7 +34,10 @@ struct basic_filter {
 	struct tcf_result	res;
 	struct tcf_proto	*tp;
 	struct list_head	link;
-	struct rcu_head		rcu;
+	union {
+		struct work_struct	work;
+		struct rcu_head		rcu;
+	};
 };
 
 static int basic_classify(struct sk_buff *skb, const struct tcf_proto *tp,
@@ -82,15 +85,26 @@ static int basic_init(struct tcf_proto *tp)
 	return 0;
 }
 
-static void basic_delete_filter(struct rcu_head *head)
+static void basic_delete_filter_work(struct work_struct *work)
 {
-	struct basic_filter *f = container_of(head, struct basic_filter, rcu);
+	struct basic_filter *f = container_of(work, struct basic_filter, work);
 
+	rtnl_lock();
 	tcf_exts_destroy(&f->exts);
 	tcf_em_tree_destroy(&f->ematches);
+	rtnl_unlock();
+
 	kfree(f);
 }
 
+static void basic_delete_filter(struct rcu_head *head)
+{
+	struct basic_filter *f = container_of(head, struct basic_filter, rcu);
+
+	INIT_WORK(&f->work, basic_delete_filter_work);
+	tcf_queue_work(&f->work);
+}
+
 static void basic_destroy(struct tcf_proto *tp)
 {
 	struct basic_head *head = rtnl_dereference(tp->root);
-- 
2.13.0

[Patch net 03/16] net_sched: use tcf_queue_work() in bpf filter

[Cong Wang]

Defer the tcf_exts_destroy() in RCU callback to
tc filter workqueue and get RTNL lock.

Reported-by: Chris Mi <chrism@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_bpf.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 520c5027646a..037a3ae86829 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -49,7 +49,10 @@ struct cls_bpf_prog {
 	struct sock_filter *bpf_ops;
 	const char *bpf_name;
 	struct tcf_proto *tp;
-	struct rcu_head rcu;
+	union {
+		struct work_struct work;
+		struct rcu_head rcu;
+	};
 };
 
 static const struct nla_policy bpf_policy[TCA_BPF_MAX + 1] = {
@@ -257,9 +260,21 @@ static void __cls_bpf_delete_prog(struct cls_bpf_prog *prog)
 	kfree(prog);
 }
 
+static void cls_bpf_delete_prog_work(struct work_struct *work)
+{
+	struct cls_bpf_prog *prog = container_of(work, struct cls_bpf_prog, work);
+
+	rtnl_lock();
+	__cls_bpf_delete_prog(prog);
+	rtnl_unlock();
+}
+
 static void cls_bpf_delete_prog_rcu(struct rcu_head *rcu)
 {
-	__cls_bpf_delete_prog(container_of(rcu, struct cls_bpf_prog, rcu));
+	struct cls_bpf_prog *prog = container_of(rcu, struct cls_bpf_prog, rcu);
+
+	INIT_WORK(&prog->work, cls_bpf_delete_prog_work);
+	tcf_queue_work(&prog->work);
 }
 
 static void __cls_bpf_delete(struct tcf_proto *tp, struct cls_bpf_prog *prog)
-- 
2.13.0

[Patch net 04/16] net_sched: use tcf_queue_work() in cgroup filter

[Cong Wang]

Defer the tcf_exts_destroy() in RCU callback to
tc filter workqueue and get RTNL lock.

Reported-by: Chris Mi <chrism@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_cgroup.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index d48452f87975..a97e069bee89 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -23,7 +23,10 @@ struct cls_cgroup_head {
 	struct tcf_exts		exts;
 	struct tcf_ematch_tree	ematches;
 	struct tcf_proto	*tp;
-	struct rcu_head		rcu;
+	union {
+		struct work_struct	work;
+		struct rcu_head		rcu;
+	};
 };
 
 static int cls_cgroup_classify(struct sk_buff *skb, const struct tcf_proto *tp,
@@ -57,15 +60,26 @@ static const struct nla_policy cgroup_policy[TCA_CGROUP_MAX + 1] = {
 	[TCA_CGROUP_EMATCHES]	= { .type = NLA_NESTED },
 };
 
+static void cls_cgroup_destroy_work(struct work_struct *work)
+{
+	struct cls_cgroup_head *head = container_of(work,
+						    struct cls_cgroup_head,
+						    work);
+	rtnl_lock();
+	tcf_exts_destroy(&head->exts);
+	tcf_em_tree_destroy(&head->ematches);
+	kfree(head);
+	rtnl_unlock();
+}
+
 static void cls_cgroup_destroy_rcu(struct rcu_head *root)
 {
 	struct cls_cgroup_head *head = container_of(root,
 						    struct cls_cgroup_head,
 						    rcu);
 
-	tcf_exts_destroy(&head->exts);
-	tcf_em_tree_destroy(&head->ematches);
-	kfree(head);
+	INIT_WORK(&head->work, cls_cgroup_destroy_work);
+	tcf_queue_work(&head->work);
 }
 
 static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
-- 
2.13.0

[Patch net 05/16] net_sched: use tcf_queue_work() in flow filter

[Cong Wang]

Defer the tcf_exts_destroy() in RCU callback to
tc filter workqueue and get RTNL lock.

Reported-by: Chris Mi <chrism@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_flow.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 2a3a60ec5b86..67f3a2af6aab 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -57,7 +57,10 @@ struct flow_filter {
 	u32			divisor;
 	u32			baseclass;
 	u32			hashrnd;
-	struct rcu_head		rcu;
+	union {
+		struct work_struct	work;
+		struct rcu_head		rcu;
+	};
 };
 
 static inline u32 addr_fold(void *addr)
@@ -369,14 +372,24 @@ static const struct nla_policy flow_policy[TCA_FLOW_MAX + 1] = {
 	[TCA_FLOW_PERTURB]	= { .type = NLA_U32 },
 };
 
-static void flow_destroy_filter(struct rcu_head *head)
+static void flow_destroy_filter_work(struct work_struct *work)
 {
-	struct flow_filter *f = container_of(head, struct flow_filter, rcu);
+	struct flow_filter *f = container_of(work, struct flow_filter, work);
 
+	rtnl_lock();
 	del_timer_sync(&f->perturb_timer);
 	tcf_exts_destroy(&f->exts);
 	tcf_em_tree_destroy(&f->ematches);
 	kfree(f);
+	rtnl_unlock();
+}
+
+static void flow_destroy_filter(struct rcu_head *head)
+{
+	struct flow_filter *f = container_of(head, struct flow_filter, rcu);
+
+	INIT_WORK(&f->work, flow_destroy_filter_work);
+	tcf_queue_work(&f->work);
 }
 
 static int flow_change(struct net *net, struct sk_buff *in_skb,
-- 
2.13.0

[Patch net 06/16] net_sched: use tcf_queue_work() in flower filter

[Cong Wang]

Defer the tcf_exts_destroy() in RCU callback to
tc filter workqueue and get RTNL lock.

Reported-by: Chris Mi <chrism@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_flower.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index b480d7c792ba..5b5722c8b32c 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -87,7 +87,10 @@ struct cls_fl_filter {
 	struct list_head list;
 	u32 handle;
 	u32 flags;
-	struct rcu_head	rcu;
+	union {
+		struct work_struct work;
+		struct rcu_head	rcu;
+	};
 	struct net_device *hw_dev;
 };
 
@@ -215,12 +218,22 @@ static int fl_init(struct tcf_proto *tp)
 	return 0;
 }
 
-static void fl_destroy_filter(struct rcu_head *head)
+static void fl_destroy_filter_work(struct work_struct *work)
 {
-	struct cls_fl_filter *f = container_of(head, struct cls_fl_filter, rcu);
+	struct cls_fl_filter *f = container_of(work, struct cls_fl_filter, work);
 
+	rtnl_lock();
 	tcf_exts_destroy(&f->exts);
 	kfree(f);
+	rtnl_unlock();
+}
+
+static void fl_destroy_filter(struct rcu_head *head)
+{
+	struct cls_fl_filter *f = container_of(head, struct cls_fl_filter, rcu);
+
+	INIT_WORK(&f->work, fl_destroy_filter_work);
+	tcf_queue_work(&f->work);
 }
 
 static void fl_hw_destroy_filter(struct tcf_proto *tp, struct cls_fl_filter *f)
-- 
2.13.0

[Patch net 07/16] net_sched: use tcf_queue_work() in fw filter

[Cong Wang]

Defer the tcf_exts_destroy() in RCU callback to
tc filter workqueue and get RTNL lock.

Reported-by: Chris Mi <chrism@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_fw.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 941245ad07fd..99183b8621ec 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -46,7 +46,10 @@ struct fw_filter {
 #endif /* CONFIG_NET_CLS_IND */
 	struct tcf_exts		exts;
 	struct tcf_proto	*tp;
-	struct rcu_head		rcu;
+	union {
+		struct work_struct	work;
+		struct rcu_head		rcu;
+	};
 };
 
 static u32 fw_hash(u32 handle)
@@ -119,12 +122,22 @@ static int fw_init(struct tcf_proto *tp)
 	return 0;
 }
 
-static void fw_delete_filter(struct rcu_head *head)
+static void fw_delete_filter_work(struct work_struct *work)
 {
-	struct fw_filter *f = container_of(head, struct fw_filter, rcu);
+	struct fw_filter *f = container_of(work, struct fw_filter, work);
 
+	rtnl_lock();
 	tcf_exts_destroy(&f->exts);
 	kfree(f);
+	rtnl_unlock();
+}
+
+static void fw_delete_filter(struct rcu_head *head)
+{
+	struct fw_filter *f = container_of(head, struct fw_filter, rcu);
+
+	INIT_WORK(&f->work, fw_delete_filter_work);
+	tcf_queue_work(&f->work);
 }
 
 static void fw_destroy(struct tcf_proto *tp)
-- 
2.13.0

[Patch net 08/16] net_sched: use tcf_queue_work() in matchall filter

[Cong Wang]

Defer the tcf_exts_destroy() in RCU callback to
tc filter workqueue and get RTNL lock.

Reported-by: Chris Mi <chrism@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_matchall.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index eeac606c95ab..c33f711b9019 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -21,7 +21,10 @@ struct cls_mall_head {
 	struct tcf_result res;
 	u32 handle;
 	u32 flags;
-	struct rcu_head	rcu;
+	union {
+		struct work_struct work;
+		struct rcu_head	rcu;
+	};
 };
 
 static int mall_classify(struct sk_buff *skb, const struct tcf_proto *tp,
@@ -41,13 +44,23 @@ static int mall_init(struct tcf_proto *tp)
 	return 0;
 }
 
+static void mall_destroy_work(struct work_struct *work)
+{
+	struct cls_mall_head *head = container_of(work, struct cls_mall_head,
+						  work);
+	rtnl_lock();
+	tcf_exts_destroy(&head->exts);
+	kfree(head);
+	rtnl_unlock();
+}
+
 static void mall_destroy_rcu(struct rcu_head *rcu)
 {
 	struct cls_mall_head *head = container_of(rcu, struct cls_mall_head,
 						  rcu);
 
-	tcf_exts_destroy(&head->exts);
-	kfree(head);
+	INIT_WORK(&head->work, mall_destroy_work);
+	tcf_queue_work(&head->work);
 }
 
 static int mall_replace_hw_filter(struct tcf_proto *tp,
-- 
2.13.0

[Patch net 09/16] net_sched: use tcf_queue_work() in u32 filter

[Cong Wang]

Defer the tcf_exts_destroy() in RCU callback to
tc filter workqueue and get RTNL lock.

Reported-by: Chris Mi <chrism@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_u32.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 10b8d851fc6b..dadd1b344497 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -68,7 +68,10 @@ struct tc_u_knode {
 	u32 __percpu		*pcpu_success;
 #endif
 	struct tcf_proto	*tp;
-	struct rcu_head		rcu;
+	union {
+		struct work_struct	work;
+		struct rcu_head		rcu;
+	};
 	/* The 'sel' field MUST be the last field in structure to allow for
 	 * tc_u32_keys allocated at end of structure.
 	 */
@@ -418,11 +421,21 @@ static int u32_destroy_key(struct tcf_proto *tp, struct tc_u_knode *n,
  * this the u32_delete_key_rcu variant does not free the percpu
  * statistics.
  */
+static void u32_delete_key_work(struct work_struct *work)
+{
+	struct tc_u_knode *key = container_of(work, struct tc_u_knode, work);
+
+	rtnl_lock();
+	u32_destroy_key(key->tp, key, false);
+	rtnl_unlock();
+}
+
 static void u32_delete_key_rcu(struct rcu_head *rcu)
 {
 	struct tc_u_knode *key = container_of(rcu, struct tc_u_knode, rcu);
 
-	u32_destroy_key(key->tp, key, false);
+	INIT_WORK(&key->work, u32_delete_key_work);
+	tcf_queue_work(&key->work);
 }
 
 /* u32_delete_key_freepf_rcu is the rcu callback variant
@@ -432,11 +445,21 @@ static void u32_delete_key_rcu(struct rcu_head *rcu)
  * for the variant that should be used with keys return from
  * u32_init_knode()
  */
+static void u32_delete_key_freepf_work(struct work_struct *work)
+{
+	struct tc_u_knode *key = container_of(work, struct tc_u_knode, work);
+
+	rtnl_lock();
+	u32_destroy_key(key->tp, key, true);
+	rtnl_unlock();
+}
+
 static void u32_delete_key_freepf_rcu(struct rcu_head *rcu)
 {
 	struct tc_u_knode *key = container_of(rcu, struct tc_u_knode, rcu);
 
-	u32_destroy_key(key->tp, key, true);
+	INIT_WORK(&key->work, u32_delete_key_freepf_work);
+	tcf_queue_work(&key->work);
 }
 
 static int u32_delete_key(struct tcf_proto *tp, struct tc_u_knode *key)
-- 
2.13.0

[Patch net 10/16] net_sched: use tcf_queue_work() in route filter

[Cong Wang]

Defer the tcf_exts_destroy() in RCU callback to
tc filter workqueue and get RTNL lock.

Reported-by: Chris Mi <chrism@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_route.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 9ddde65915d2..4b14ccd8b8f2 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -57,7 +57,10 @@ struct route4_filter {
 	u32			handle;
 	struct route4_bucket	*bkt;
 	struct tcf_proto	*tp;
-	struct rcu_head		rcu;
+	union {
+		struct work_struct	work;
+		struct rcu_head		rcu;
+	};
 };
 
 #define ROUTE4_FAILURE ((struct route4_filter *)(-1L))
@@ -254,12 +257,22 @@ static int route4_init(struct tcf_proto *tp)
 	return 0;
 }
 
-static void route4_delete_filter(struct rcu_head *head)
+static void route4_delete_filter_work(struct work_struct *work)
 {
-	struct route4_filter *f = container_of(head, struct route4_filter, rcu);
+	struct route4_filter *f = container_of(work, struct route4_filter, work);
 
+	rtnl_lock();
 	tcf_exts_destroy(&f->exts);
 	kfree(f);
+	rtnl_unlock();
+}
+
+static void route4_delete_filter(struct rcu_head *head)
+{
+	struct route4_filter *f = container_of(head, struct route4_filter, rcu);
+
+	INIT_WORK(&f->work, route4_delete_filter_work);
+	tcf_queue_work(&f->work);
 }
 
 static void route4_destroy(struct tcf_proto *tp)
-- 
2.13.0

[Patch net 11/16] net_sched: use tcf_queue_work() in rsvp filter

[Cong Wang]

Defer the tcf_exts_destroy() in RCU callback to
tc filter workqueue and get RTNL lock.

Reported-by: Chris Mi <chrism@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_rsvp.h | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index b1f6ed48bc72..bdbc541787f8 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -97,7 +97,10 @@ struct rsvp_filter {
 
 	u32				handle;
 	struct rsvp_session		*sess;
-	struct rcu_head			rcu;
+	union {
+		struct work_struct		work;
+		struct rcu_head			rcu;
+	};
 };
 
 static inline unsigned int hash_dst(__be32 *dst, u8 protocol, u8 tunnelid)
@@ -282,12 +285,22 @@ static int rsvp_init(struct tcf_proto *tp)
 	return -ENOBUFS;
 }
 
-static void rsvp_delete_filter_rcu(struct rcu_head *head)
+static void rsvp_delete_filter_work(struct work_struct *work)
 {
-	struct rsvp_filter *f = container_of(head, struct rsvp_filter, rcu);
+	struct rsvp_filter *f = container_of(work, struct rsvp_filter, work);
 
+	rtnl_lock();
 	tcf_exts_destroy(&f->exts);
 	kfree(f);
+	rtnl_unlock();
+}
+
+static void rsvp_delete_filter_rcu(struct rcu_head *head)
+{
+	struct rsvp_filter *f = container_of(head, struct rsvp_filter, rcu);
+
+	INIT_WORK(&f->work, rsvp_delete_filter_work);
+	tcf_queue_work(&f->work);
 }
 
 static void rsvp_delete_filter(struct tcf_proto *tp, struct rsvp_filter *f)
-- 
2.13.0

[Patch net 12/16] net_sched: use tcf_queue_work() in tcindex filter

[Cong Wang]

Defer the tcf_exts_destroy() in RCU callback to
tc filter workqueue and get RTNL lock.

Reported-by: Chris Mi <chrism@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_tcindex.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 14a7e08b2fa9..beaa95e09c25 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -27,14 +27,20 @@
 struct tcindex_filter_result {
 	struct tcf_exts		exts;
 	struct tcf_result	res;
-	struct rcu_head		rcu;
+	union {
+		struct work_struct	work;
+		struct rcu_head		rcu;
+	};
 };
 
 struct tcindex_filter {
 	u16 key;
 	struct tcindex_filter_result result;
 	struct tcindex_filter __rcu *next;
-	struct rcu_head rcu;
+	union {
+		struct work_struct work;
+		struct rcu_head rcu;
+	};
 };
 
 
@@ -133,12 +139,34 @@ static int tcindex_init(struct tcf_proto *tp)
 	return 0;
 }
 
+static void tcindex_destroy_rexts_work(struct work_struct *work)
+{
+	struct tcindex_filter_result *r;
+
+	r = container_of(work, struct tcindex_filter_result, work);
+	rtnl_lock();
+	tcf_exts_destroy(&r->exts);
+	rtnl_unlock();
+}
+
 static void tcindex_destroy_rexts(struct rcu_head *head)
 {
 	struct tcindex_filter_result *r;
 
 	r = container_of(head, struct tcindex_filter_result, rcu);
-	tcf_exts_destroy(&r->exts);
+	INIT_WORK(&r->work, tcindex_destroy_rexts_work);
+	tcf_queue_work(&r->work);
+}
+
+static void tcindex_destroy_fexts_work(struct work_struct *work)
+{
+	struct tcindex_filter *f = container_of(work, struct tcindex_filter,
+						work);
+
+	rtnl_lock();
+	tcf_exts_destroy(&f->result.exts);
+	kfree(f);
+	rtnl_unlock();
 }
 
 static void tcindex_destroy_fexts(struct rcu_head *head)
@@ -146,8 +174,8 @@ static void tcindex_destroy_fexts(struct rcu_head *head)
 	struct tcindex_filter *f = container_of(head, struct tcindex_filter,
 						rcu);
 
-	tcf_exts_destroy(&f->result.exts);
-	kfree(f);
+	INIT_WORK(&f->work, tcindex_destroy_fexts_work);
+	tcf_queue_work(&f->work);
 }
 
 static int tcindex_delete(struct tcf_proto *tp, void *arg, bool *last)
-- 
2.13.0

[Patch net 13/16] net_sched: add rtnl assertion to tcf_exts_destroy()

[Cong Wang]

After previous patches, it is now safe to claim that
tcf_exts_destroy() is always called with RTNL lock.

Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_api.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 045d13679ad6..231181c602ed 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -909,6 +909,7 @@ void tcf_exts_destroy(struct tcf_exts *exts)
 #ifdef CONFIG_NET_CLS_ACT
 	LIST_HEAD(actions);
 
+	ASSERT_RTNL();
 	tcf_exts_to_list(exts, &actions);
 	tcf_action_destroy(&actions, TCA_ACT_UNBIND);
 	kfree(exts->actions);
-- 
2.13.0

[Patch net 14/16] net_sched: fix call_rcu() race on act_sample module removal

[Cong Wang]

Similar to commit c78e1746d3ad
("net: sched: fix call_rcu() race on classifier module unloads"),
we need to wait for flying RCU callback tcf_sample_cleanup_rcu().

Cc: Yotam Gigi <yotamg@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/act_sample.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index ec986ae52808..a9f9a2ccc664 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -264,6 +264,7 @@ static int __init sample_init_module(void)
 
 static void __exit sample_cleanup_module(void)
 {
+	rcu_barrier();
 	tcf_unregister_action(&act_sample_ops, &sample_net_ops);
 }
 
-- 
2.13.0

[Patch net 15/16] selftests: Introduce a new script to generate tc batch file

[Cong Wang]

From: Chris Mi <chrism@mellanox.com>

  # ./tdc_batch.py -h
  usage: tdc_batch.py [-h] [-n NUMBER] [-o] [-s] [-p] device file

  TC batch file generator

  positional arguments:
    device                device name
    file                  batch file name

  optional arguments:
    -h, --help            show this help message and exit
    -n NUMBER, --number NUMBER
                          how many lines in batch file
    -o, --skip_sw         skip_sw (offload), by default skip_hw
    -s, --share_action    all filters share the same action
    -p, --prio            all filters have different prio

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Lucas Bates <lucasb@mojatatu.com>
Signed-off-by: Chris Mi <chrism@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 tools/testing/selftests/tc-testing/tdc_batch.py | 62 +++++++++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100755 tools/testing/selftests/tc-testing/tdc_batch.py

diff --git a/tools/testing/selftests/tc-testing/tdc_batch.py b/tools/testing/selftests/tc-testing/tdc_batch.py
new file mode 100755
index 000000000000..707c6bfef689
--- /dev/null
+++ b/tools/testing/selftests/tc-testing/tdc_batch.py
@@ -0,0 +1,62 @@
+#!/usr/bin/python3
+
+"""
+tdc_batch.py - a script to generate TC batch file
+
+Copyright (C) 2017 Chris Mi <chrism@mellanox.com>
+"""
+
+import argparse
+
+parser = argparse.ArgumentParser(description='TC batch file generator')
+parser.add_argument("device", help="device name")
+parser.add_argument("file", help="batch file name")
+parser.add_argument("-n", "--number", type=int,
+                    help="how many lines in batch file")
+parser.add_argument("-o", "--skip_sw",
+                    help="skip_sw (offload), by default skip_hw",
+                    action="store_true")
+parser.add_argument("-s", "--share_action",
+                    help="all filters share the same action",
+                    action="store_true")
+parser.add_argument("-p", "--prio",
+                    help="all filters have different prio",
+                    action="store_true")
+args = parser.parse_args()
+
+device = args.device
+file = open(args.file, 'w')
+
+number = 1
+if args.number:
+    number = args.number
+
+skip = "skip_hw"
+if args.skip_sw:
+    skip = "skip_sw"
+
+share_action = ""
+if args.share_action:
+    share_action = "index 1"
+
+prio = "prio 1"
+if args.prio:
+    prio = ""
+    if number > 0x4000:
+        number = 0x4000
+
+index = 0
+for i in range(0x100):
+    for j in range(0x100):
+        for k in range(0x100):
+            mac = ("%02x:%02x:%02x" % (i, j, k))
+            src_mac = "e4:11:00:" + mac
+            dst_mac = "e4:12:00:" + mac
+            cmd = ("filter add dev %s %s protocol ip parent ffff: flower %s "
+                   "src_mac %s dst_mac %s action drop %s" %
+                   (device, prio, skip, src_mac, dst_mac, share_action))
+            file.write("%s\n" % cmd)
+            index += 1
+            if index >= number:
+                file.close()
+                exit(0)
-- 
2.13.0

[Patch net 16/16] selftests: Introduce a new test case to tc testsuite

[Cong Wang]

From: Chris Mi <chrism@mellanox.com>

In this patchset, we fixed a tc bug. This patch adds the test case
that reproduces the bug. To run this test case, user should specify
an existing NIC device:
  # sudo ./tdc.py -d enp4s0f0

This test case belongs to category "flower". If user doesn't specify
a NIC device, the test cases belong to "flower" will not be run.

In this test case, we create 1M filters and all filters share the same
action. When destroying all filters, kernel should not panic. It takes
about 18s to run it.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Lucas Bates <lucasb@mojatatu.com>
Signed-off-by: Chris Mi <chrism@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 .../tc-testing/tc-tests/filters/tests.json         | 23 +++++++++++++++++++++-
 tools/testing/selftests/tc-testing/tdc.py          | 20 +++++++++++++++----
 tools/testing/selftests/tc-testing/tdc_config.py   |  2 ++
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
index c727b96a59b0..5fa02d86b35f 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
@@ -17,5 +17,26 @@
         "teardown": [
             "$TC qdisc del dev $DEV1 ingress"
         ]
+    },
+    {
+        "id": "d052",
+        "name": "Add 1M filters with the same action",
+        "category": [
+            "filter",
+            "flower"
+        ],
+        "setup": [
+            "$TC qdisc add dev $DEV2 ingress",
+            "./tdc_batch.py $DEV2 $BATCH_FILE --share_action -n 1000000"
+        ],
+        "cmdUnderTest": "$TC -b $BATCH_FILE",
+        "expExitCode": "0",
+        "verifyCmd": "$TC actions list action gact",
+        "matchPattern": "action order 0: gact action drop.*index 1 ref 1000000 bind 1000000",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DEV2 ingress",
+            "/bin/rm $BATCH_FILE"
+        ]
     }
-]
\ No newline at end of file
+]
diff --git a/tools/testing/selftests/tc-testing/tdc.py b/tools/testing/selftests/tc-testing/tdc.py
index cd61b7844c0d..5f11f5d7456e 100755
--- a/tools/testing/selftests/tc-testing/tdc.py
+++ b/tools/testing/selftests/tc-testing/tdc.py
@@ -88,7 +88,7 @@ USE_NS = True
             exit(1)
 
 
-def test_runner(filtered_tests):
+def test_runner(filtered_tests, args):
     """
     Driver function for the unit tests.
 
@@ -105,6 +105,8 @@ USE_NS = True
     for tidx in testlist:
         result = True
         tresult = ""
+        if "flower" in tidx["category"] and args.device == None:
+            continue
         print("Test " + tidx["id"] + ": " + tidx["name"])
         prepare_env(tidx["setup"])
         (p, procout) = exec_cmd(tidx["cmdUnderTest"])
@@ -152,6 +154,10 @@ USE_NS = True
         exec_cmd(cmd, False)
         cmd = 'ip -s $NS link set $DEV1 up'
         exec_cmd(cmd, False)
+        cmd = 'ip link set $DEV2 netns $NS'
+        exec_cmd(cmd, False)
+        cmd = 'ip -s $NS link set $DEV2 up'
+        exec_cmd(cmd, False)
 
 
 def ns_destroy():
@@ -211,7 +217,8 @@ USE_NS = True
                         help='Execute the single test case with specified ID')
     parser.add_argument('-i', '--id', action='store_true', dest='gen_id',
                         help='Generate ID numbers for new test cases')
-    return parser
+    parser.add_argument('-d', '--device',
+                        help='Execute the test case in flower category')
     return parser
 
 
@@ -225,6 +232,8 @@ USE_NS = True
 
     if args.path != None:
          NAMES['TC'] = args.path
+    if args.device != None:
+         NAMES['DEV2'] = args.device
     if not os.path.isfile(NAMES['TC']):
         print("The specified tc path " + NAMES['TC'] + " does not exist.")
         exit(1)
@@ -381,14 +390,17 @@ USE_NS = True
             if (len(alltests) == 0):
                 print("Cannot find a test case with ID matching " + target_id)
                 exit(1)
-        catresults = test_runner(alltests)
+        catresults = test_runner(alltests, args)
         print("All test results: " + "\n\n" + catresults)
     elif (len(target_category) > 0):
+        if (target_category == "flower") and args.device == None:
+            print("Please specify a NIC device (-d) to run category flower")
+            exit(1)
         if (target_category not in ucat):
             print("Specified category is not present in this file.")
             exit(1)
         else:
-            catresults = test_runner(testcases[target_category])
+            catresults = test_runner(testcases[target_category], args)
             print("Category " + target_category + "\n\n" + catresults)
 
     ns_destroy()
diff --git a/tools/testing/selftests/tc-testing/tdc_config.py b/tools/testing/selftests/tc-testing/tdc_config.py
index 01087375a7c3..b6352515c1b5 100644
--- a/tools/testing/selftests/tc-testing/tdc_config.py
+++ b/tools/testing/selftests/tc-testing/tdc_config.py
@@ -12,6 +12,8 @@ NAMES = {
           # Name of veth devices to be created for the namespace
           'DEV0': 'v0p0',
           'DEV1': 'v0p1',
+          'DEV2': '',
+          'BATCH_FILE': './batch.txt',
           # Name of the namespace to use
           'NS': 'tcut'
         }
-- 
2.13.0

Re: [Patch net 00/16] net_sched: fix races with RCU callbacks

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 26 Oct 2017 18:24:27 -0700

> Recently, the RCU callbacks used in TC filters and TC actions keep
> drawing my attention, they introduce at least 4 race condition bugs:
 ...
> As suggested by Paul, we could defer the work to a workqueue and
> gain the permission of holding RTNL again without any performance
> impact, however, in tcf_block_put() we could have a deadlock when
> flushing workqueue while hodling RTNL lock, the trick here is to
> defer the work itself in workqueue and make it queued after all
> other works so that we keep the same ordering to avoid any
> use-after-free. Please see the first patch for details.
> 
> Patch 1 introduces the infrastructure, patch 2~12 move each
> tc filter to the new tc filter workqueue, patch 13 adds
> an assertion to catch potential bugs like this, patch 14
> closes another rcu callback race, patch 15 and patch 16 add
> new test cases.

I know Eric has some reservations about how things have become in
this layer, but we have to fix this for 'net' somehow.

So I've applied this series, thanks.

Re: [Patch net 00/16] net_sched: fix races with RCU callbacks

[Lucas Bates]

e.On Thu, Oct 26, 2017 at 9:24 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> Recently, the RCU callbacks used in TC filters and TC actions keep
> drawing my attention, they introduce at least 4 race condition bugs:
<snip>
> As suggested by Paul, we could defer the work to a workqueue and
> gain the permission of holding RTNL again without any performance
> impact, however, in tcf_block_put() we could have a deadlock when
> flushing workqueue while hodling RTNL lock, the trick here is to
> defer the work itself in workqueue and make it queued after all
> other works so that we keep the same ordering to avoid any
> use-after-free. Please see the first patch for details.

Cong, I don't believe the problem's been resolved just yet....  I have
a new kernel, compiled just today and I'm still tripping over a kernel
bug in this scenario when I run Chris' new test case.

I'm doing this on a machine where I don't have a spare device to use
on the run. Instead I created a veth pair that will have one end
migrated into the container.

The bug isn't consistent. I'm running into it anywhere between one and
four runs of the d052 test case.

Steps to reproduce:

sudo ip li add foo type veth
sudo ./tdc.py -d foo -c flower
[repeat until kernel bug encountered]

Re: [Patch net 00/16] net_sched: fix races with RCU callbacks

[Cong Wang]

On Mon, Oct 30, 2017 at 3:39 PM, Lucas Bates <lucasb@mojatatu.com> wrote:
> e.On Thu, Oct 26, 2017 at 9:24 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> Recently, the RCU callbacks used in TC filters and TC actions keep
>> drawing my attention, they introduce at least 4 race condition bugs:
> <snip>
>> As suggested by Paul, we could defer the work to a workqueue and
>> gain the permission of holding RTNL again without any performance
>> impact, however, in tcf_block_put() we could have a deadlock when
>> flushing workqueue while hodling RTNL lock, the trick here is to
>> defer the work itself in workqueue and make it queued after all
>> other works so that we keep the same ordering to avoid any
>> use-after-free. Please see the first patch for details.
>
> Cong, I don't believe the problem's been resolved just yet....  I have
> a new kernel, compiled just today and I'm still tripping over a kernel
> bug in this scenario when I run Chris' new test case.
>

Without a stack trace, I can't do anything. "a kernel bug" could
be anything, why do you believe it is caused by this patchset?

Note, there is a use-after-free bug caused by this patchset, the
fix is already on netdev in case you hit the same bug.

> I'm doing this on a machine where I don't have a spare device to use
> on the run. Instead I created a veth pair that will have one end
> migrated into the container.
>
> The bug isn't consistent. I'm running into it anywhere between one and
> four runs of the d052 test case.
>
> Steps to reproduce:
>
> sudo ip li add foo type veth
> sudo ./tdc.py -d foo -c flower
> [repeat until kernel bug encountered]

I tested with basic and u32 filter, since it is not specific to any type
of filters, all filters had the same problem. I will try flower filter too
before you provide a stack trace or a detailed report.

Re: [Patch net 00/16] net_sched: fix races with RCU callbacks

[Lucas Bates]

On Mon, Oct 30, 2017 at 7:12 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Oct 30, 2017 at 3:39 PM, Lucas Bates <lucasb@mojatatu.com> wrote:
>> e.On Thu, Oct 26, 2017 at 9:24 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> Recently, the RCU callbacks used in TC filters and TC actions keep
>>> drawing my attention, they introduce at least 4 race condition bugs:
>> <snip>
>>> As suggested by Paul, we could defer the work to a workqueue and
>>> gain the permission of holding RTNL again without any performance
>>> impact, however, in tcf_block_put() we could have a deadlock when
>>> flushing workqueue while hodling RTNL lock, the trick here is to
>>> defer the work itself in workqueue and make it queued after all
>>> other works so that we keep the same ordering to avoid any
>>> use-after-free. Please see the first patch for details.
>>
>> Cong, I don't believe the problem's been resolved just yet....  I have
>> a new kernel, compiled just today and I'm still tripping over a kernel
>> bug in this scenario when I run Chris' new test case.
>>
>
> Without a stack trace, I can't do anything. "a kernel bug" could
> be anything, why do you believe it is caused by this patchset?
>
(Sending again due to email format issues)

The stack trace I saw touched on the same code that was affected by
the patchset. I've sent a photo of the trace to you directly - I'm
sorry, I should have sent it earlier.  I also apologize for having to
send the photo but I was doing the testing on a small device lacking
any kind of serial console access.


>> I'm doing this on a machine where I don't have a spare device to use
>> on the run. Instead I created a veth pair that will have one end
>> migrated into the container.
>>
>> The bug isn't consistent. I'm running into it anywhere between one and
>> four runs of the d052 test case.
>>
>> Steps to reproduce:
>>
>> sudo ip li add foo type veth
>> sudo ./tdc.py -d foo -c flower
>> [repeat until kernel bug encountered]
>
> I tested with basic and u32 filter, since it is not specific to any type
> of filters, all filters had the same problem. I will try flower filter too
> before you provide a stack trace or a detailed report.
If you need more information or need me to try something else, I'll be
able to tomorrow.

Re: [Patch net 00/16] net_sched: fix races with RCU callbacks

[Cong Wang]

On Mon, Oct 30, 2017 at 6:03 PM, Lucas Bates <lucasb@mojatatu.com> wrote:
> On Oct 30, 2017 19:13, "Cong Wang" <xiyou.wangcong@gmail.com> wrote:
>>
>> On Mon, Oct 30, 2017 at 3:39 PM, Lucas Bates <lucasb@mojatatu.com> wrote:
>> > e.On Thu, Oct 26, 2017 at 9:24 PM, Cong Wang <xiyou.wangcong@gmail.com>
>> > wrote:
>> >> Recently, the RCU callbacks used in TC filters and TC actions keep
>> >> drawing my attention, they introduce at least 4 race condition bugs:
>> > <snip>
>> >> As suggested by Paul, we could defer the work to a workqueue and
>> >> gain the permission of holding RTNL again without any performance
>> >> impact, however, in tcf_block_put() we could have a deadlock when
>> >> flushing workqueue while hodling RTNL lock, the trick here is to
>> >> defer the work itself in workqueue and make it queued after all
>> >> other works so that we keep the same ordering to avoid any
>> >> use-after-free. Please see the first patch for details.
>> >
>> > Cong, I don't believe the problem's been resolved just yet....  I have
>> > a new kernel, compiled just today and I'm still tripping over a kernel
>> > bug in this scenario when I run Chris' new test case.
>> >
>>
>> Without a stack trace, I can't do anything. "a kernel bug" could
>> be anything, why do you believe it is caused by this patchset?
>>
> The stack trace I saw touched on the same code that was affected by the
> patchset.  I've attached a photo of the trace - sorry, I should have sent it
> earlier.  I also apologize for having to send a photo but I was doing the
> testing on a small device lacking any kind of serial console access.


Can you try this patch? From your stack trace it is not clear where
the cause is, but we know that the crash is in __tcf_idr_release(),
this is how I came up with the following patch:


diff --git a/include/net/act_api.h b/include/net/act_api.h
index b944e0eb93be..5072446d5f06 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -122,7 +122,9 @@ void tcf_idrinfo_destroy(const struct tc_action_ops *ops,

 static inline void tc_action_net_exit(struct tc_action_net *tn)
 {
+       rtnl_lock();
        tcf_idrinfo_destroy(tn->ops, tn->idrinfo);
+       rtnl_unlock();
        kfree(tn->idrinfo);
 }



>
> If you need more information or need me to try something else, I'll be able
> to tomorrow.
>

I will look deeper tomorrow. It doesn't look like caused by this patchset
so far, probably yet another missing rtnl like what the above patch shows.


Thanks!

Re: [Patch net 00/16] net_sched: fix races with RCU callbacks

[Jamal Hadi Salim]

On 17-10-31 01:44 AM, Cong Wang wrote:
> On Mon, Oct 30, 2017 at 6:03 PM, Lucas Bates <lucasb@mojatatu.com> wrote:
>> On Oct 30, 2017 19:13, "Cong Wang" <xiyou.wangcong@gmail.com> wrote:
>>>

> Can you try this patch? From your stack trace it is not clear where
> the cause is, but we know that the crash is in __tcf_idr_release(),
> this is how I came up with the following patch:
> 
> 
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index b944e0eb93be..5072446d5f06 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -122,7 +122,9 @@ void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
> 
>   static inline void tc_action_net_exit(struct tc_action_net *tn)
>   {
> +       rtnl_lock();
>          tcf_idrinfo_destroy(tn->ops, tn->idrinfo);
> +       rtnl_unlock();
>          kfree(tn->idrinfo);
>   }
> 
>

Looks like an excellent bet;-> tdc kills the container at the end of the
test.

cheers,
jamal

Re: [Patch net 00/16] net_sched: fix races with RCU callbacks

[Lucas Bates]

On Tue, Oct 31, 2017 at 7:00 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 17-10-31 01:44 AM, Cong Wang wrote:
>>
>> Can you try this patch? From your stack trace it is not clear where
>> the cause is, but we know that the crash is in __tcf_idr_release(),
>> this is how I came up with the following patch:
>>
>>
>> diff --git a/include/net/act_api.h b/include/net/act_api.h
>> index b944e0eb93be..5072446d5f06 100644
>> --- a/include/net/act_api.h
>> +++ b/include/net/act_api.h
>> @@ -122,7 +122,9 @@ void tcf_idrinfo_destroy(const struct tc_action_ops
>> *ops,
>>
>>   static inline void tc_action_net_exit(struct tc_action_net *tn)
>>   {
>> +       rtnl_lock();
>>          tcf_idrinfo_destroy(tn->ops, tn->idrinfo);
>> +       rtnl_unlock();
>>          kfree(tn->idrinfo);
>>   }
>>
>>
>
> Looks like an excellent bet;-> tdc kills the container at the end of the
> test.

Unfortunately it doesn't seem to have had any effect, I'm still seeing
the same bug as yesterday. At Jamal's suggestion I put in a delay
after tdc completed running the tests but before it deleted the
container - and I didn't run into the bug after many runs. I had no
luck getting serial console access on any of our other systems so I
have another stack trace photo which I'll send to you directly.

Re: [Patch net 00/16] net_sched: fix races with RCU callbacks

[Lucas Bates]

On Tue, Oct 31, 2017 at 2:55 PM, Lucas Bates <lucasb@mojatatu.com> wrote:
> On Tue, Oct 31, 2017 at 7:00 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 17-10-31 01:44 AM, Cong Wang wrote:
>>>
>>> Can you try this patch? From your stack trace it is not clear where
>>> the cause is, but we know that the crash is in __tcf_idr_release(),
>>> this is how I came up with the following patch:
>>>
>>>
>>> diff --git a/include/net/act_api.h b/include/net/act_api.h
>>> index b944e0eb93be..5072446d5f06 100644
>>> --- a/include/net/act_api.h
>>> +++ b/include/net/act_api.h
>>> @@ -122,7 +122,9 @@ void tcf_idrinfo_destroy(const struct tc_action_ops
>>> *ops,
>>>
>>>   static inline void tc_action_net_exit(struct tc_action_net *tn)
>>>   {
>>> +       rtnl_lock();
>>>          tcf_idrinfo_destroy(tn->ops, tn->idrinfo);
>>> +       rtnl_unlock();
>>>          kfree(tn->idrinfo);
>>>   }
>>>
>>>
>>
>> Looks like an excellent bet;-> tdc kills the container at the end of the
>> test.
>
> Unfortunately it doesn't seem to have had any effect, I'm still seeing
> the same bug as yesterday. At Jamal's suggestion I put in a delay
> after tdc completed running the tests but before it deleted the
> container - and I didn't run into the bug after many runs. I had no
> luck getting serial console access on any of our other systems so I
> have another stack trace photo which I'll send to you directly.

I forgot to mention: it appears the bug appears to be related to
deleting the container. Test d052 is the last to run before tdc does
its teardown and deletes the container; if I add the sleep the bug
never gets triggered.

Re: [Patch net 00/16] net_sched: fix races with RCU callbacks

[Cong Wang]

On Tue, Oct 31, 2017 at 12:13 PM, Lucas Bates <lucasb@mojatatu.com> wrote:
> On Tue, Oct 31, 2017 at 2:55 PM, Lucas Bates <lucasb@mojatatu.com> wrote:
>> Unfortunately it doesn't seem to have had any effect, I'm still seeing
>> the same bug as yesterday. At Jamal's suggestion I put in a delay

As I replied to you privately, it is probably because idrinfo is freed
before action itself. So just RTNL is not enough.


>> after tdc completed running the tests but before it deleted the
>> container - and I didn't run into the bug after many runs. I had no
>> luck getting serial console access on any of our other systems so I
>> have another stack trace photo which I'll send to you directly.
>
> I forgot to mention: it appears the bug appears to be related to
> deleting the container. Test d052 is the last to run before tdc does
> its teardown and deletes the container; if I add the sleep the bug
> never gets triggered.

This almost rules out the guilty of this patchset.

I will provide a patch for you to test, since I can't reproduce it here.

Thanks!

Re: [Patch net 00/16] net_sched: fix races with RCU callbacks

[Cong Wang]

[-- Attachment #1: Type: text/plain, Size: 495 bytes --]

On Tue, Oct 31, 2017 at 3:09 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> This almost rules out the guilty of this patchset.
>
> I will provide a patch for you to test, since I can't reproduce it here.
>

Lucas, please test the attached patch, it applies to latest -net.

Note, it is a combination of 3 patches which together close the
use-after-free you reported here, I hope.

Please let me know if this works. My basic tests run well without
any stack traces or memory leaks.

Thanks!

[-- Attachment #2: tc-action-fixes.diff --]
[-- Type: text/plain, Size: 9945 bytes --]

diff --git a/include/net/act_api.h b/include/net/act_api.h
index b944e0eb93be..c6c8129c3647 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -13,6 +13,7 @@
 struct tcf_idrinfo {
 	spinlock_t	lock;
 	struct idr	action_idr;
+	struct net	*net;
 };
 
 struct tc_action_ops;
@@ -100,11 +101,12 @@ struct tc_action_ops {
 struct tc_action_net {
 	struct tcf_idrinfo *idrinfo;
 	const struct tc_action_ops *ops;
+	struct net *net;
 };
 
 static inline
 int tc_action_net_init(struct tc_action_net *tn,
-		       const struct tc_action_ops *ops)
+		       const struct tc_action_ops *ops, struct net *net)
 {
 	int err = 0;
 
@@ -112,6 +114,7 @@ int tc_action_net_init(struct tc_action_net *tn,
 	if (!tn->idrinfo)
 		return -ENOMEM;
 	tn->ops = ops;
+	tn->idrinfo->net = net;
 	spin_lock_init(&tn->idrinfo->lock);
 	idr_init(&tn->idrinfo->action_idr);
 	return err;
@@ -122,7 +125,9 @@ void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
 
 static inline void tc_action_net_exit(struct tc_action_net *tn)
 {
+	rtnl_lock();
 	tcf_idrinfo_destroy(tn->ops, tn->idrinfo);
+	rtnl_unlock();
 	kfree(tn->idrinfo);
 }
 
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index da6fa82c98a8..241bf3f925de 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -78,6 +78,7 @@ static void tcf_idr_remove(struct tcf_idrinfo *idrinfo, struct tc_action *p)
 	spin_lock_bh(&idrinfo->lock);
 	idr_remove_ext(&idrinfo->action_idr, p->tcfa_index);
 	spin_unlock_bh(&idrinfo->lock);
+	put_net(idrinfo->net);
 	gen_kill_estimator(&p->tcfa_rate_est);
 	free_tcf(p);
 }
@@ -86,6 +87,8 @@ int __tcf_idr_release(struct tc_action *p, bool bind, bool strict)
 {
 	int ret = 0;
 
+	ASSERT_RTNL();
+
 	if (p) {
 		if (bind)
 			p->tcfa_bindcnt--;
@@ -93,7 +96,7 @@ int __tcf_idr_release(struct tc_action *p, bool bind, bool strict)
 			return -EPERM;
 
 		p->tcfa_refcnt--;
-		if (p->tcfa_bindcnt <= 0 && p->tcfa_refcnt <= 0) {
+		if (p->tcfa_bindcnt == 0 && p->tcfa_refcnt == 0) {
 			if (p->ops->cleanup)
 				p->ops->cleanup(p, bind);
 			tcf_idr_remove(p->idrinfo, p);
@@ -334,6 +337,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
 	p->idrinfo = idrinfo;
 	p->ops = ops;
 	INIT_LIST_HEAD(&p->list);
+	get_net(idrinfo->net);
 	*a = p;
 	return 0;
 }
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index c0c707eb2c96..9bce8cc84cbb 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -398,7 +398,7 @@ static __net_init int bpf_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, bpf_net_id);
 
-	return tc_action_net_init(tn, &act_bpf_ops);
+	return tc_action_net_init(tn, &act_bpf_ops, net);
 }
 
 static void __net_exit bpf_exit_net(struct net *net)
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 10b7a8855a6c..34e52d01a5dd 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -206,7 +206,7 @@ static __net_init int connmark_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, connmark_net_id);
 
-	return tc_action_net_init(tn, &act_connmark_ops);
+	return tc_action_net_init(tn, &act_connmark_ops, net);
 }
 
 static void __net_exit connmark_exit_net(struct net *net)
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 1c40caadcff9..35171df2ebef 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -626,7 +626,7 @@ static __net_init int csum_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, csum_net_id);
 
-	return tc_action_net_init(tn, &act_csum_ops);
+	return tc_action_net_init(tn, &act_csum_ops, net);
 }
 
 static void __net_exit csum_exit_net(struct net *net)
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index e29a48ef7fc3..ef7f7f39d26d 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -232,7 +232,7 @@ static __net_init int gact_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, gact_net_id);
 
-	return tc_action_net_init(tn, &act_gact_ops);
+	return tc_action_net_init(tn, &act_gact_ops, net);
 }
 
 static void __net_exit gact_exit_net(struct net *net)
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 8ccd35825b6b..f65e4b5058e0 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -818,7 +818,7 @@ static __net_init int ife_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, ife_net_id);
 
-	return tc_action_net_init(tn, &act_ife_ops);
+	return tc_action_net_init(tn, &act_ife_ops, net);
 }
 
 static void __net_exit ife_exit_net(struct net *net)
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index d9e399a7e3d5..dbdf3b2470d5 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -334,7 +334,7 @@ static __net_init int ipt_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, ipt_net_id);
 
-	return tc_action_net_init(tn, &act_ipt_ops);
+	return tc_action_net_init(tn, &act_ipt_ops, net);
 }
 
 static void __net_exit ipt_exit_net(struct net *net)
@@ -384,7 +384,7 @@ static __net_init int xt_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, xt_net_id);
 
-	return tc_action_net_init(tn, &act_xt_ops);
+	return tc_action_net_init(tn, &act_xt_ops, net);
 }
 
 static void __net_exit xt_exit_net(struct net *net)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 416627c66f08..84759cfd5a33 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -343,7 +343,7 @@ static __net_init int mirred_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, mirred_net_id);
 
-	return tc_action_net_init(tn, &act_mirred_ops);
+	return tc_action_net_init(tn, &act_mirred_ops, net);
 }
 
 static void __net_exit mirred_exit_net(struct net *net)
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index c365d01b99c8..7eeaaf9217b6 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -307,7 +307,7 @@ static __net_init int nat_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, nat_net_id);
 
-	return tc_action_net_init(tn, &act_nat_ops);
+	return tc_action_net_init(tn, &act_nat_ops, net);
 }
 
 static void __net_exit nat_exit_net(struct net *net)
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 491fe5deb09e..b3d82c334a5f 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -450,7 +450,7 @@ static __net_init int pedit_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, pedit_net_id);
 
-	return tc_action_net_init(tn, &act_pedit_ops);
+	return tc_action_net_init(tn, &act_pedit_ops, net);
 }
 
 static void __net_exit pedit_exit_net(struct net *net)
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 3bb2ebf9e9ae..9ec42b26e4b9 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -331,7 +331,7 @@ static __net_init int police_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, police_net_id);
 
-	return tc_action_net_init(tn, &act_police_ops);
+	return tc_action_net_init(tn, &act_police_ops, net);
 }
 
 static void __net_exit police_exit_net(struct net *net)
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index a9f9a2ccc664..71b8405e9718 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -240,7 +240,7 @@ static __net_init int sample_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, sample_net_id);
 
-	return tc_action_net_init(tn, &act_sample_ops);
+	return tc_action_net_init(tn, &act_sample_ops, net);
 }
 
 static void __net_exit sample_exit_net(struct net *net)
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index e7b57e5071a3..a8d0ea95f894 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -201,7 +201,7 @@ static __net_init int simp_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, simp_net_id);
 
-	return tc_action_net_init(tn, &act_simp_ops);
+	return tc_action_net_init(tn, &act_simp_ops, net);
 }
 
 static void __net_exit simp_exit_net(struct net *net)
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 59949d61f20d..fbac62472e09 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -238,7 +238,7 @@ static __net_init int skbedit_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, skbedit_net_id);
 
-	return tc_action_net_init(tn, &act_skbedit_ops);
+	return tc_action_net_init(tn, &act_skbedit_ops, net);
 }
 
 static void __net_exit skbedit_exit_net(struct net *net)
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index b642ad3d39dd..8e12d8897d2f 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -263,7 +263,7 @@ static __net_init int skbmod_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, skbmod_net_id);
 
-	return tc_action_net_init(tn, &act_skbmod_ops);
+	return tc_action_net_init(tn, &act_skbmod_ops, net);
 }
 
 static void __net_exit skbmod_exit_net(struct net *net)
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 30c96274c638..c33faa373cf2 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -322,7 +322,7 @@ static __net_init int tunnel_key_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, tunnel_key_net_id);
 
-	return tc_action_net_init(tn, &act_tunnel_key_ops);
+	return tc_action_net_init(tn, &act_tunnel_key_ops, net);
 }
 
 static void __net_exit tunnel_key_exit_net(struct net *net)
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 16eb067a8d8f..115fc33cc6d8 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -269,7 +269,7 @@ static __net_init int vlan_init_net(struct net *net)
 {
 	struct tc_action_net *tn = net_generic(net, vlan_net_id);
 
-	return tc_action_net_init(tn, &act_vlan_ops);
+	return tc_action_net_init(tn, &act_vlan_ops, net);
 }
 
 static void __net_exit vlan_exit_net(struct net *net)

Re: [Patch net 00/16] net_sched: fix races with RCU callbacks

[Lucas Bates]

On Tue, Oct 31, 2017 at 7:02 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Oct 31, 2017 at 3:09 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>
>> This almost rules out the guilty of this patchset.
>>
>> I will provide a patch for you to test, since I can't reproduce it here.
>>
>
> Lucas, please test the attached patch, it applies to latest -net.
>
> Note, it is a combination of 3 patches which together close the
> use-after-free you reported here, I hope.
>
> Please let me know if this works. My basic tests run well without
> any stack traces or memory leaks.

The results look good to me.

I compiled with the patch and ran the test again in a loop. I set it
to run 50 times in a row and didn't trigger the bug once (previous
record was 4 times).

Re: [Patch net 00/16] net_sched: fix races with RCU callbacks

[Cong Wang]

On Wed, Nov 1, 2017 at 9:55 AM, Lucas Bates <lucasb@mojatatu.com> wrote:
> On Tue, Oct 31, 2017 at 7:02 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Tue, Oct 31, 2017 at 3:09 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>
>>> This almost rules out the guilty of this patchset.
>>>
>>> I will provide a patch for you to test, since I can't reproduce it here.
>>>
>>
>> Lucas, please test the attached patch, it applies to latest -net.
>>
>> Note, it is a combination of 3 patches which together close the
>> use-after-free you reported here, I hope.
>>
>> Please let me know if this works. My basic tests run well without
>> any stack traces or memory leaks.
>
> The results look good to me.
>
> I compiled with the patch and ran the test again in a loop. I set it
> to run 50 times in a row and didn't trigger the bug once (previous
> record was 4 times).

Thanks a lot! I will add your Reported-by and Tested-by and send
out the patches.