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

[Jiri Pirko <jiri@resnulli.us>]

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
>