From patchwork Wed Apr 18 12:37:43 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans Schillstrom X-Patchwork-Id: 153486 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id F1DE9B6EEB for ; Wed, 18 Apr 2012 22:37:54 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752992Ab2DRMhx (ORCPT ); Wed, 18 Apr 2012 08:37:53 -0400 Received: from smtp-gw21.han.skanova.net ([81.236.55.21]:35812 "EHLO smtp-gw21.han.skanova.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752021Ab2DRMhv convert rfc822-to-8bit (ORCPT ); Wed, 18 Apr 2012 08:37:51 -0400 Received: from mailer (213.65.94.224) by smtp-gw21.han.skanova.net (8.5.133) id 4EDE1D1703905989; Wed, 18 Apr 2012 14:37:49 +0200 Received: from schillstrom.com (wiki2.mlab.se [172.24.1.4]) by mailer (8.14.4/8.14.4) with ESMTP id q3ICbg4t011817; Wed, 18 Apr 2012 14:37:42 +0200 MIME-Version: 1.0 X-Mailer: AfterLogic WebMail PHP X-Originating-IP: 172.24.1.70 X-Priority: 3 (Normal) Message-ID: <3r6npeu.69e743752dad7d8eb6a19cbbbdfd1ed9@obelix.schillstrom.com> From: "Hans Schillstrom" To: "Julian Anastasov" , "horms@verge.net.au" Cc: "Hans Schillstrom" , "wensong@linux-vs.org" , "lvs-devel@vger.kernel.org" , "netdev@vger.kernel.org" , "netfilter-devel@vger.kernel.org" Subject: Re[2]: [PATCH 2/2] IPVS: make failure of netns init more stable Date: Wed, 18 Apr 2012 14:37:43 +0200 (CEST) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Hello > >Hello, > >On Tue, 17 Apr 2012, Hans Schillstrom wrote: > >> I wonder if we are chasing ghosts... >> >> With proper fault handling I can't even see a case when it (net->ipvs) can be used. >> Can you see a case when it could happen? >> Still we can set it to NULL on error exit and cleanup as you suggested, that doesn't harm I think. >> >> A. If you add a netns and it fails the entire ns will be rolled back, >> and no access to that ns can occur. >> That ns does not exist > > Agreed > >> B. If you insert ip_vs.ko when having one or more name spaces and >> __ip_vs_init() returns an error the module will be unloaded. >> All ready loaded ns will not be affected. > > Yes, ip_vs_init fails. > >> C. insmod of ex. ip_vs_ftp only affects loaded name spaces >> and if the load of ip_vs_ftp fails it will be unloaded without affecting ip_vs(.ko) >> (If ip_vs.ko is not loaded then it has to be loaded first case B...) >> >> With a "compiled in" ip_vs case B doesn't exist. > > It is this case that can happen, we can only guess how >difficult is to get ENOMEM here. IIRC, we can generate only >ENOMEM error on IPVS core load. > > I assume Simon has such setup and changes code to >trigger load error. When I generate ENOMEM on IPVS core init >for such case I get ENOENT from register_ip_vs_app when >patch 1 and 2 for apps are applied, i.e. net->ipvs is NULL. >You can check it with NF_CONNTRACK=y, IP_VS=y and >IP_VS_FTP=m. You only need to trigger ENOMEM in __ip_vs_init. I did test this with 4 netns loaded and modprobe ip_vs_ftp In the 4:th netns (ipvs->gen >= 4) fire a -ENOMEM The result was as expected, ip_vs_ftp was not loaded. All patches below was loaded. (included the ipvs NULL check) Just for "fun" I also added a printk in the ipvs NULL check but I can't trigger it. Simon: do you have any possibility to test it or give me a hint how to do ? (Just to make sure that the patches below will be sufficient) > >> With proper fault handling i.e. all ways returning fault codes to the netns init, >> there is no need for checking for "if (!net->ipvs)" or any other action. > > Probably but one check on load does not hurt much. I think I have tested all of above now and my conclusion is that we need the following patches which also was applied when the tests was run. (with a small reservation that I might have missed some..) [PATCH v3 1/2] netfilter: ipvs: Verify that IP_VS protocol has been registered, Sasha Levin [PATCH v3 2/2] netfilter: ipvs: use GFP_KERNEL allocation where possible, Sasha Levin [PATCH 0/6] Convert some GFP_ATOMIC allocations, Julian Anastasov [PATCH 1/6] ipvs: timeout tables do not need GFP_ATOMIC allocation, Julian Anastasov [PATCH 2/6] ipvs: SH scheduler does not need GFP_ATOMIC allocation, Julian Anastasov [PATCH 5/6] ipvs: LBLCR scheduler does not need GFP_ATOMIC allocation on init, Julian Anastasov [PATCH 6/6] ipvs: WRR scheduler does not need GFP_ATOMIC allocation, Julian Anastasov [PATCH 3/6] ipvs: DH scheduler does not need GFP_ATOMIC allocation, Julian Anastasov [PATCH 4/6] ipvs: LBLC scheduler does not need GFP_ATOMIC allocation on init, Julian Anastasov [PATCH] ipvs: fix crash in ip_vs_control_net_cleanup on unload, Julian Anastasov [PATCH 1/2] ipvs: reset ipvs pointer in netns, Julian Anastasov [PATCH 1/2] IPVS: take care of return value from protocol init_netns, Hans Schillstrom To be safe, add this to [PATCH 1/2] ipvs: reset ipvs pointer in netns or make a new patch > >Regards > >-- >Julian Anastasov Regards Hans Schillstrom --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c index 538d74e..c757359 100644 --- a/net/netfilter/ipvs/ip_vs_ftp.c +++ b/net/netfilter/ipvs/ip_vs_ftp.c @@ -439,6 +439,9 @@ static int __net_init __ip_vs_ftp_init(struct net *net) struct ip_vs_app *app; struct netns_ipvs *ipvs = net_ipvs(net); + if (!ipvs) + return ERR_PTR(-ENOENT); + app = kmemdup(&ip_vs_ftp, sizeof(struct ip_vs_app), GFP_KERNEL); if (!app) return -ENOMEM; diff --git a/net/netfilter/ipvs/ip_vs_lblc.c b/net/netfilter/ipvs/ip_vs_lblc.c index 74c7278..1d74996 100644 --- a/net/netfilter/ipvs/ip_vs_lblc.c +++ b/net/netfilter/ipvs/ip_vs_lblc.c @@ -549,6 +549,9 @@ static int __net_init __ip_vs_lblc_init(struct net *net) { struct netns_ipvs *ipvs = net_ipvs(net); + if (!ipvs) + return ERR_PTR(-ENOENT); + if (!net_eq(net, &init_net)) { ipvs->lblc_ctl_table = kmemdup(vs_vars_table, sizeof(vs_vars_table), diff --git a/net/netfilter/ipvs/ip_vs_lblcr.c b/net/netfilter/ipvs/ip_vs_lblcr.c index 8620c68..c328ee0 100644 --- a/net/netfilter/ipvs/ip_vs_lblcr.c +++ b/net/netfilter/ipvs/ip_vs_lblcr.c @@ -743,6 +743,9 @@ static int __net_init __ip_vs_lblcr_init(struct net *net) { struct netns_ipvs *ipvs = net_ipvs(net); + if (!ipvs) + return ERR_PTR(-ENOENT); + if (!net_eq(net, &init_net)) { ipvs->lblcr_ctl_table = kmemdup(vs_vars_table, sizeof(vs_vars_table),