[PATCH v2] net: sched: crash on blocks with goto chain action

[PATCH v2] net: sched: crash on blocks with goto chain action

[Roman Kapl]

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 <code@rkapl.cz>
---
v1 -> v2: Hold all chains instead of just the currently iterated one,
          the code should be more clear this way.
---
 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 7d97f612c9b9..ddcf04b4ab43 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -336,7 +336,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();
@@ -344,15 +345,21 @@ 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_ext(struct tcf_block *block, struct Qdisc *q,
 		       struct tcf_block_ext_info *ei)
 {
-	struct tcf_chain *chain, *tmp;
+	struct tcf_chain *chain;
 
-	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);
 
 	tcf_block_offload_unbind(block, q, ei);
-- 
2.15.0

Re: [PATCH v2] net: sched: crash on blocks with goto chain action

[Jiri Pirko]

Fri, Nov 24, 2017 at 12:27:58PM CET, code@rkapl.cz wrote:
>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 <code@rkapl.cz>
>---
>v1 -> v2: Hold all chains instead of just the currently iterated one,
>          the code should be more clear this way.
>---
> 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 7d97f612c9b9..ddcf04b4ab43 100644
>--- a/net/sched/cls_api.c
>+++ b/net/sched/cls_api.c
>@@ -336,7 +336,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();
>@@ -344,15 +345,21 @@ 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_ext(struct tcf_block *block, struct Qdisc *q,
> 		       struct tcf_block_ext_info *ei)
> {
>-	struct tcf_chain *chain, *tmp;
>+	struct tcf_chain *chain;
> 
>-	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.

Would be perhaps nice to mention that the appropriate tcf_chain_put
is done in tcf_block_put_final()

Regardless of this:

Acked-by: Jiri Pirko <jiri@mellanox.com>



>+	 */
>+	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);
> 
> 	tcf_block_offload_unbind(block, q, ei);
>-- 
>2.15.0
>

Re: [PATCH v2] net: sched: crash on blocks with goto chain action

[Cong Wang]

On Fri, Nov 24, 2017 at 3:27 AM, Roman Kapl <code@rkapl.cz> wrote:
>
> Fixes: 822e86d997 ("net_sched: remove tcf_block_put_deferred()")

You blame a wrong commit here.

Commit 822e86d997 was correct at that time, it is the patchset
which includes commit e4b95c41df36befcfd11721 makes it buggy
again.