From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [PATCH v2] net: sched: crash on blocks with goto chain action Date: Fri, 24 Nov 2017 13:20:52 +0100 Message-ID: <20171124122052.GD3384@nanopsycho> References: <20171124112758.5760-1-code@rkapl.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, xiyou.wangcong@gmail.com To: Roman Kapl Return-path: Received: from mail-wr0-f193.google.com ([209.85.128.193]:35132 "EHLO mail-wr0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753450AbdKXMUz (ORCPT ); Fri, 24 Nov 2017 07:20:55 -0500 Received: by mail-wr0-f193.google.com with SMTP id w95so20265148wrc.2 for ; Fri, 24 Nov 2017 04:20:54 -0800 (PST) Content-Disposition: inline In-Reply-To: <20171124112758.5760-1-code@rkapl.cz> Sender: netdev-owner@vger.kernel.org List-ID: 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 >--- >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 >+ */ >+ 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 >