[Patch 4.14 0/4] net_sched: backport tc filter fixes to 4.14

This patchset backports 4 important bug fixes for tc filter to
4.14 stable branch. Due to some big changes between 4.14 and 4.15,
the backports are not trivial, I have to adjust and fix the conflicts
manually.

Thanks to Roland for reporting the kernel warning and testing
the patches.

Reported-by: Roland Franke <redacted>
Tested-by: Roland Franke <redacted>
Cc: Jiri Pirko <redacted>
Cc: Roman Kapl <redacted>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <redacted>

---

Cong Wang (1):
  net_sched: get rid of rcu_barrier() in tcf_block_put_ext()

Jiri Pirko (1):
  net: sched: fix use-after-free in tcf_block_put_ext

Roman Kapl (2):
  net: sched: fix crash when deleting secondary chains
  net: sched: crash on blocks with goto chain action

 include/net/sch_generic.h |  1 -
 net/sched/cls_api.c       | 48 +++++++++++++++++++++++------------------------
 2 files changed, 23 insertions(+), 26 deletions(-)

-- 
2.13.0

From: Roman Kapl <redacted>

[ Upstream commit d7aa04a5e82b4f254d306926c81eae8df69e5200 ]

If you flush (delete) a filter chain other than chain 0 (such as when
deleting the device), the kernel may run into a use-after-free. The
chain refcount must not be decremented unless we are sure we are done
with the chain.

To reproduce the bug, run:
    ip link add dtest type dummy
    tc qdisc add dev dtest ingress
    tc filter add dev dtest chain 1  parent ffff: flower
    ip link del dtest

Introduced in: commit f93e1cdcf42c ("net/sched: fix filter flushing"),
but unless you have KAsan or luck, you won't notice it until
commit 0dadc117ac8b ("cls_flower: use tcf_exts_get_net() before call_rcu()")

Fixes: f93e1cdcf42c ("net/sched: fix filter flushing")
Acked-by: Jiri Pirko <redacted>
Signed-off-by: Roman Kapl <redacted>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/sched/cls_api.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index ecbb019efcbd..1451a56a8f93 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c

@@ -197,14 +197,15 @@ static struct tcf_chain *tcf_chain_create(struct tcf_block *block,
 
 static void tcf_chain_flush(struct tcf_chain *chain)
 {
-	struct tcf_proto *tp;
+	struct tcf_proto *tp = rtnl_dereference(chain->filter_chain);
 
 	if (chain->p_filter_chain)
 		RCU_INIT_POINTER(*chain->p_filter_chain, NULL);
-	while ((tp = rtnl_dereference(chain->filter_chain)) != NULL) {
+	while (tp) {
 		RCU_INIT_POINTER(chain->filter_chain, tp->next);
-		tcf_chain_put(chain);
 		tcf_proto_destroy(tp);
+		tp = rtnl_dereference(chain->filter_chain);
+		tcf_chain_put(chain);
 	}
 }
 

-- 
2.13.0

From: Roman Kapl <redacted>

[ Upstream commit a60b3f515d30d0fe8537c64671926879a3548103 ]

tcf_block_put_ext has assumed that all filters (and thus their goto
actions) are destroyed in RCU callback and thus can not race with our
list iteration. However, that is not true during netns cleanup (see
tcf_exts_get_net comment).

Prevent the user after free by holding all chains (except 0, that one is
already held). foreach_safe is not enough in this case.

To reproduce, run the following in a netns and then delete the ns:
    ip link add dtest type dummy
    tc qdisc add dev dtest ingress
    tc filter add dev dtest chain 1 parent ffff: handle 1 prio 1 flower action goto chain 2

Fixes: 822e86d997 ("net_sched: remove tcf_block_put_deferred()")
Signed-off-by: Roman Kapl <redacted>
Acked-by: Jiri Pirko <redacted>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/sched/cls_api.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 1451a56a8f93..ddae7053b745 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c

@@ -282,7 +282,8 @@ static void tcf_block_put_final(struct work_struct *work)
 	struct tcf_chain *chain, *tmp;
 
 	rtnl_lock();
-	/* Only chain 0 should be still here. */
+
+	/* 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);
 	rtnl_unlock();

@@ -290,17 +291,23 @@ static void tcf_block_put_final(struct work_struct *work)
 }
 
 /* XXX: Standalone actions are not allowed to jump to any chain, and bound
- * actions should be all removed after flushing. However, filters are now
- * destroyed in tc filter workqueue with RTNL lock, they can not race here.
+ * actions should be all removed after flushing.
  */
 void tcf_block_put(struct tcf_block *block)
 {
-	struct tcf_chain *chain, *tmp;
+	struct tcf_chain *chain;
 
 	if (!block)
 		return;
 
-	list_for_each_entry_safe(chain, tmp, &block->chain_list, list)
+	/* Hold a refcnt for all chains, except 0, so that they don't disappear
+	 * while we are iterating.
+	 */
+	list_for_each_entry(chain, &block->chain_list, list)
+		if (chain->index)
+			tcf_chain_hold(chain);
+
+	list_for_each_entry(chain, &block->chain_list, list)
 		tcf_chain_flush(chain);
 
 	INIT_WORK(&block->work, tcf_block_put_final);

-- 
2.13.0

[ Upstream commit efbf78973978b0d25af59bc26c8013 ]

Both Eric and Paolo noticed the rcu_barrier() we use in
tcf_block_put_ext() could be a performance bottleneck when
we have a lot of tc classes.

Paolo provided the following to demonstrate the issue:

tc qdisc add dev lo root htb
for I in `seq 1 1000`; do
        tc class add dev lo parent 1: classid 1:$I htb rate 100kbit
        tc qdisc add dev lo parent 1:$I handle $((I + 1)): htb
        for J in `seq 1 10`; do
                tc filter add dev lo parent $((I + 1)): u32 match ip src 1.1.1.$J
        done
done
time tc qdisc del dev root

real    0m54.764s
user    0m0.023s
sys     0m0.000s

The rcu_barrier() there is to ensure we free the block after all chains
are gone, that is, to queue tcf_block_put_final() at the tail of workqueue.
We can achieve this ordering requirement by refcnt'ing tcf block instead,
that is, the tcf block is freed only when the last chain in this block is
gone. This also simplifies the code.

Paolo reported after this patch we get:

real    0m0.017s
user    0m0.000s
sys     0m0.017s

Tested-by: Paolo Abeni <pabeni@redhat.com>
Cc: Eric Dumazet <redacted>
Cc: Jiri Pirko <redacted>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <redacted>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 include/net/sch_generic.h |  1 -
 net/sched/cls_api.c       | 29 +++++++++--------------------
 2 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 236bfe5b2ffe..6073e8bae025 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h

@@ -273,7 +273,6 @@ 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 ddae7053b745..135147c1deed 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c

@@ -211,8 +211,12 @@ static void tcf_chain_flush(struct tcf_chain *chain)
 
 static void tcf_chain_destroy(struct tcf_chain *chain)
 {
+	struct tcf_block *block = chain->block;
+
 	list_del(&chain->list);
 	kfree(chain);
+	if (list_empty(&block->chain_list))
+		kfree(block);
 }
 
 static void tcf_chain_hold(struct tcf_chain *chain)

@@ -276,26 +280,12 @@ int tcf_block_get(struct tcf_block **p_block,
 }
 EXPORT_SYMBOL(tcf_block_get);
 
-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;
-
-	rtnl_lock();
-
-	/* 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);
-	rtnl_unlock();
-	kfree(block);
-}
-
 /* XXX: Standalone actions are not allowed to jump to any chain, and bound
  * actions should be all removed after flushing.
  */
 void tcf_block_put(struct tcf_block *block)
 {
-	struct tcf_chain *chain;
+	struct tcf_chain *chain, *tmp;
 
 	if (!block)
 		return;

@@ -310,12 +300,11 @@ void tcf_block_put(struct tcf_block *block)
 	list_for_each_entry(chain, &block->chain_list, list)
 		tcf_chain_flush(chain);
 
-	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.
+	/* At this point, all the chains should have refcnt >= 1. Block will be
+	 * freed after all chains are gone.
 	 */
-	rcu_barrier();
-	tcf_queue_work(&block->work);
+	list_for_each_entry_safe(chain, tmp, &block->chain_list, list)
+		tcf_chain_put(chain);
 }
 EXPORT_SYMBOL(tcf_block_put);
 

-- 
2.13.0

From: Jiri Pirko <redacted>

[ Upstream commit df45bf84e4f5a48f23d4b1a07d21d5 ]

Since the block is freed with last chain being put, once we reach the
end of iteration of list_for_each_entry_safe, the block may be
already freed. I'm hitting this only by creating and deleting clsact:

[  202.171952] ==================================================================
[  202.180182] BUG: KASAN: use-after-free in tcf_block_put_ext+0x240/0x390
[  202.187590] Read of size 8 at addr ffff880225539a80 by task tc/796
[  202.194508]
[  202.196185] CPU: 0 PID: 796 Comm: tc Not tainted 4.15.0-rc2jiri+ #5
[  202.203200] Hardware name: Mellanox Technologies Ltd. "MSN2100-CB2F"/"SA001017", BIOS 5.6.5 06/07/2016
[  202.213613] Call Trace:
[  202.216369]  dump_stack+0xda/0x169
[  202.220192]  ? dma_virt_map_sg+0x147/0x147
[  202.224790]  ? show_regs_print_info+0x54/0x54
[  202.229691]  ? tcf_chain_destroy+0x1dc/0x250
[  202.234494]  print_address_description+0x83/0x3d0
[  202.239781]  ? tcf_block_put_ext+0x240/0x390
[  202.244575]  kasan_report+0x1ba/0x460
[  202.248707]  ? tcf_block_put_ext+0x240/0x390
[  202.253518]  tcf_block_put_ext+0x240/0x390
[  202.258117]  ? tcf_chain_flush+0x290/0x290
[  202.262708]  ? qdisc_hash_del+0x82/0x1a0
[  202.267111]  ? qdisc_hash_add+0x50/0x50
[  202.271411]  ? __lock_is_held+0x5f/0x1a0
[  202.275843]  clsact_destroy+0x3d/0x80 [sch_ingress]
[  202.281323]  qdisc_destroy+0xcb/0x240
[  202.285445]  qdisc_graft+0x216/0x7b0
[  202.289497]  tc_get_qdisc+0x260/0x560

Fix this by holding the block also by chain 0 and put chain 0
explicitly, out of the list_for_each_entry_safe loop at the very
end of tcf_block_put_ext.

Fixes: efbf78973978 ("net_sched: get rid of rcu_barrier() in tcf_block_put_ext()")
Signed-off-by: Jiri Pirko <redacted>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/sched/cls_api.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 135147c1deed..934c239cf98d 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c

@@ -290,21 +290,22 @@ void tcf_block_put(struct tcf_block *block)
 	if (!block)
 		return;
 
-	/* Hold a refcnt for all chains, except 0, so that they don't disappear
+	/* Hold a refcnt for all chains, so that they don't disappear
 	 * while we are iterating.
 	 */
 	list_for_each_entry(chain, &block->chain_list, list)
-		if (chain->index)
-			tcf_chain_hold(chain);
+		tcf_chain_hold(chain);
 
 	list_for_each_entry(chain, &block->chain_list, list)
 		tcf_chain_flush(chain);
 
-	/* At this point, all the chains should have refcnt >= 1. Block will be
-	 * freed after all chains are gone.
-	 */
+	/* 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);
+
+	/* Finally, put chain 0 and allow block to be freed. */
+	chain = list_first_entry(&block->chain_list, struct tcf_chain, list);
+	tcf_chain_put(chain);
 }
 EXPORT_SYMBOL(tcf_block_put);
 

-- 
2.13.0

From: Cong Wang <redacted>
Date: Thu,  1 Mar 2018 13:46:35 -0800

This patchset backports 4 important bug fixes for tc filter to
4.14 stable branch. Due to some big changes between 4.14 and 4.15,
the backports are not trivial, I have to adjust and fix the conflicts
manually.

Thanks to Roland for reporting the kernel warning and testing
the patches.

Reported-by: Roland Franke <redacted>
Tested-by: Roland Franke <redacted>
Cc: Jiri Pirko <redacted>
Cc: Roman Kapl <redacted>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <redacted>

Greg, please queue up this series for -stable.

Thank you!

On Thu, Mar 01, 2018 at 08:56:17PM -0500, David Miller wrote:

From: Cong Wang <redacted>
Date: Thu,  1 Mar 2018 13:46:35 -0800

quoted

This patchset backports 4 important bug fixes for tc filter to
4.14 stable branch. Due to some big changes between 4.14 and 4.15,
the backports are not trivial, I have to adjust and fix the conflicts
manually.

Thanks to Roland for reporting the kernel warning and testing
the patches.

Reported-by: Roland Franke <redacted>
Tested-by: Roland Franke <redacted>
Cc: Jiri Pirko <redacted>
Cc: Roman Kapl <redacted>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <redacted>

Greg, please queue up this series for -stable.

All now applied, thanks.

greg k-h