From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753130AbcG3Nhz (ORCPT ); Sat, 30 Jul 2016 09:37:55 -0400 Received: from out03.mta.xmission.com ([166.70.13.233]:36878 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751064AbcG3Nhp (ORCPT ); Sat, 30 Jul 2016 09:37:45 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Michal Kubecek Cc: Pablo Neira Ayuso , Patrick McHardy , Jozsef Kadlecsik , netfilter-devel@vger.kernel.org, coreteam@netfilter.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org References: <20160729150033.E0250A0BD9@unicorn.suse.cz> <20160729161904.4F00BA0BD9@unicorn.suse.cz> Date: Sat, 30 Jul 2016 08:24:37 -0500 In-Reply-To: <20160729161904.4F00BA0BD9@unicorn.suse.cz> (Michal Kubecek's message of "Fri, 29 Jul 2016 18:19:04 +0200 (CEST)") Message-ID: <87shurb6ne.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1bTUSS-0005Nz-OR;;;mid=<87shurb6ne.fsf@x220.int.ebiederm.org>;;;hst=in01.mta.xmission.com;;;ip=67.3.204.119;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX18xKBd9H0n3ifLmsEQjS8J7h2G0+i58JVM= X-SA-Exim-Connect-IP: 67.3.204.119 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.7 XMSubLong Long Subject * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa07 1397; Body=1 Fuz1=1 Fuz2=1] * 0.0 T_TooManySym_01 4+ unique symbols in subject * 0.0 T_TooManySym_02 5+ unique symbols in subject X-Spam-DCC: XMission; sa07 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ;Michal Kubecek X-Spam-Relay-Country: X-Spam-Timing: total 549 ms - load_scoreonly_sql: 0.04 (0.0%), signal_user_changed: 4.6 (0.8%), b_tie_ro: 3.3 (0.6%), parse: 1.12 (0.2%), extract_message_metadata: 20 (3.6%), get_uri_detail_list: 3.5 (0.6%), tests_pri_-1000: 7 (1.3%), tests_pri_-950: 1.23 (0.2%), tests_pri_-900: 1.03 (0.2%), tests_pri_-400: 27 (4.9%), check_bayes: 26 (4.7%), b_tokenize: 8 (1.4%), b_tok_get_all: 9 (1.6%), b_comp_prob: 3.2 (0.6%), b_tok_touch_all: 3.6 (0.7%), b_finish: 0.92 (0.2%), tests_pri_0: 479 (87.3%), check_dkim_signature: 0.52 (0.1%), check_dkim_adsp: 22 (3.9%), tests_pri_500: 4.7 (0.9%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH RESEND nf] netfilter: avoid a race between nf_register_hook() and cleanup_net() X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Michal Kubecek writes: > There is a race condition between nf_{,un}register_hook() and > cleanup_net() which can either trigger WARN check or cause a memory > leak. The scenario is like this (2a and 2b are alternatives): > > 1. cleanup_net() removes one or more struct net from net_namespace_list > 2a. nf_register_hook() adds per-netns hooks to all netns (but not those > removed in step 1) and adds the hook to global nf_hook_list > 2b. nf_unregister_hook() deletes per-netns hooks from all netns (but not > those removed in step 1) and removes the hook from nf_hook_list > 3. cleanup_net() calls pernet subsystem exit functions for netns being > removed; one of them is netfilter_net_exit() which (among others) > calls nf_unregister_net_hook() to unregister per-netns hooks for all > hooks in nf_hook_list. > > In case (a), per-netns hooks are never added as the namespace was > already invisible to for_each_net() in step 2a but an attempt to remove > them in step 3 (the hook is already in nf_hook_list) triggers a WARN > check in nf_unregister_net_hook() (no real harm done, however). In case > (b), the per-netns hook is removed neither in step 2b (netns is already > invisible to for_each_net()) nor in step 3 (the hook is already removed > from nf_hook_list), causing a memory leak. > > Prevent the race by protecting the for_each_net() loop in > nf_{,un}register_hook() (also) by net_mutex. There is already a > precendens for this in rtnl_link_unregister() which addresses similar > race. So this analysis of a problem appears to be spot on. Reviewed-by: "Eric W. Biederman" I really really want there to be a better way to do this, but it is really not ok for a hook to continue it's life past nf_unregister_net_hook as after that point the code may be removed from the kernel (sigh). Although keeping with the precedent and minimizing net_mutex we could remove the WARN and keep nf_register_hook as it is. But that sounds entirely too clever for a fix that will probably be backported. But that sounds entirely too clever for a fix that likely needs to be backported. Eric > Fixes: 085db2c04557 ("netfilter: Per network namespace netfilter hooks.") > Signed-off-by: Michal Kubecek > --- > net/netfilter/core.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/net/netfilter/core.c b/net/netfilter/core.c > index f39276d1c2d7..860978c9f82e 100644 > --- a/net/netfilter/core.c > +++ b/net/netfilter/core.c > @@ -193,6 +193,8 @@ int nf_register_hook(struct nf_hook_ops *reg) > struct net *net, *last; > int ret; > > + /* prevent race with cleanup_net() */ > + mutex_lock(&net_mutex); > rtnl_lock(); > for_each_net(net) { > ret = nf_register_net_hook(net, reg); > @@ -201,6 +203,7 @@ int nf_register_hook(struct nf_hook_ops *reg) > } > list_add_tail(®->list, &nf_hook_list); > rtnl_unlock(); > + mutex_unlock(&net_mutex); > > return 0; > rollback: > @@ -211,6 +214,7 @@ rollback: > nf_unregister_net_hook(net, reg); > } > rtnl_unlock(); > + mutex_unlock(&net_mutex); > return ret; > } > EXPORT_SYMBOL(nf_register_hook); > @@ -219,11 +223,14 @@ void nf_unregister_hook(struct nf_hook_ops *reg) > { > struct net *net; > > + /* prevent race with cleanup_net() */ > + mutex_lock(&net_mutex); > rtnl_lock(); > list_del(®->list); > for_each_net(net) > nf_unregister_net_hook(net, reg); > rtnl_unlock(); > + mutex_unlock(&net_mutex); > } > EXPORT_SYMBOL(nf_unregister_hook);