Fri, Nov 24, 2017 at 12:27:58PM CET, c...@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 <c...@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 <j...@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
>
