[PATCH 0/2] Changes for app registrations

[PATCH 0/2] Changes for app registrations

[Julian Anastasov]

	This changeset fixes problems with failed netns
initializations.

Julian Anastasov (2):
  ipvs: reset ipvs pointer in netns
  ipvs: fix app registration in netns

 include/net/ip_vs.h             |    5 +--
 net/netfilter/ipvs/ip_vs_app.c  |   57 ++++++++++++++++++++++++++++++---------
 net/netfilter/ipvs/ip_vs_core.c |    2 +
 net/netfilter/ipvs/ip_vs_ftp.c  |   22 ++++-----------
 4 files changed, 54 insertions(+), 32 deletions(-)

-- 
1.7.3.4

[PATCH 1/2] ipvs: reset ipvs pointer in netns

[Julian Anastasov]

	Make sure net->ipvs is reset on netns cleanup or failed
initialization. It is needed for IPVS applications to know that
IPVS core is not loaded in netns.

Signed-off-by: Julian Anastasov <ja@ssi.bg>
---
 net/netfilter/ipvs/ip_vs_core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index 2555816..260b9ef 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -1924,6 +1924,7 @@ protocol_fail:
 control_fail:
 	ip_vs_estimator_net_cleanup(net);
 estimator_fail:
+	net->ipvs = NULL;
 	return -ENOMEM;
 }
 
@@ -1936,6 +1937,7 @@ static void __net_exit __ip_vs_cleanup(struct net *net)
 	ip_vs_control_net_cleanup(net);
 	ip_vs_estimator_net_cleanup(net);
 	IP_VS_DBG(2, "ipvs netns %d released\n", net_ipvs(net)->gen);
+	net->ipvs = NULL;
 }
 
 static void __net_exit __ip_vs_dev_cleanup(struct net *net)
-- 
1.7.3.4

[PATCH 2/2] ipvs: fix app registration in netns

[Julian Anastasov]

	Avoid crash when registering applications (ftp) when
the IPVS core initialization for netns fails. Do this by
checking for present core (net->ipvs) when registering apps.

	As result this gets rid of the ftp_app pointer and allows
applications to be registered without adding fields in the
netns_ipvs structure.

	Also, make sure applications are unregistered from the
core automatically, should not be needed in theory.

Signed-off-by: Julian Anastasov <ja@ssi.bg>
---
 include/net/ip_vs.h            |    5 +--
 net/netfilter/ipvs/ip_vs_app.c |   57 ++++++++++++++++++++++++++++++---------
 net/netfilter/ipvs/ip_vs_ftp.c |   22 ++++-----------
 3 files changed, 52 insertions(+), 32 deletions(-)

diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index 8ed41eb..578806b 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -798,8 +798,6 @@ struct netns_ipvs {
 	struct list_head	rs_table[IP_VS_RTAB_SIZE];
 	/* ip_vs_app */
 	struct list_head	app_list;
-	/* ip_vs_ftp */
-	struct ip_vs_app	*ftp_app;
 	/* ip_vs_proto */
 	#define IP_VS_PROTO_TAB_SIZE	32	/* must be power of 2 */
 	struct ip_vs_proto_data *proto_data_table[IP_VS_PROTO_TAB_SIZE];
@@ -1136,7 +1134,8 @@ extern void ip_vs_service_net_cleanup(struct net *net);
  *      (from ip_vs_app.c)
  */
 #define IP_VS_APP_MAX_PORTS  8
-extern int register_ip_vs_app(struct net *net, struct ip_vs_app *app);
+extern struct ip_vs_app *register_ip_vs_app(struct net *net,
+					    struct ip_vs_app *app);
 extern void unregister_ip_vs_app(struct net *net, struct ip_vs_app *app);
 extern int ip_vs_bind_app(struct ip_vs_conn *cp, struct ip_vs_protocol *pp);
 extern void ip_vs_unbind_app(struct ip_vs_conn *cp);
diff --git a/net/netfilter/ipvs/ip_vs_app.c b/net/netfilter/ipvs/ip_vs_app.c
index fe6cb43..c33139b 100644
--- a/net/netfilter/ipvs/ip_vs_app.c
+++ b/net/netfilter/ipvs/ip_vs_app.c
@@ -184,19 +184,40 @@ register_ip_vs_app_inc(struct net *net, struct ip_vs_app *app, __u16 proto,
 /*
  *	ip_vs_app registration routine
  */
-int register_ip_vs_app(struct net *net, struct ip_vs_app *app)
+struct ip_vs_app *register_ip_vs_app(struct net *net, struct ip_vs_app *app)
 {
 	struct netns_ipvs *ipvs = net_ipvs(net);
-	/* increase the module use count */
-	ip_vs_use_count_inc();
+	struct ip_vs_app *a;
+	int err = 0;
+
+	if (!ipvs)
+		return ERR_PTR(-ENOENT);
 
 	mutex_lock(&__ip_vs_app_mutex);
 
-	list_add(&app->a_list, &ipvs->app_list);
+	list_for_each_entry(a, &ipvs->app_list, a_list) {
+		if (!strcmp(app->name, a->name)) {
+			err = -EEXIST;
+			break;
+		}
+	}
+	if (!err) {
+		a = kmemdup(app, sizeof(*app), GFP_KERNEL);
+		if (!a)
+			err = -ENOMEM;
+	}
+	if (!err) {
+		INIT_LIST_HEAD(&a->incs_list);
+		list_add(&a->a_list, &ipvs->app_list);
+		/* increase the module use count */
+		ip_vs_use_count_inc();
+	}
 
 	mutex_unlock(&__ip_vs_app_mutex);
 
-	return 0;
+	if (err)
+		return ERR_PTR(err);
+	return a;
 }
 
 
@@ -206,20 +227,29 @@ int register_ip_vs_app(struct net *net, struct ip_vs_app *app)
  */
 void unregister_ip_vs_app(struct net *net, struct ip_vs_app *app)
 {
-	struct ip_vs_app *inc, *nxt;
+	struct netns_ipvs *ipvs = net_ipvs(net);
+	struct ip_vs_app *a, *anxt, *inc, *nxt;
+
+	if (!ipvs)
+		return;
 
 	mutex_lock(&__ip_vs_app_mutex);
 
-	list_for_each_entry_safe(inc, nxt, &app->incs_list, a_list) {
-		ip_vs_app_inc_release(net, inc);
-	}
+	list_for_each_entry_safe(a, anxt, &ipvs->app_list, a_list) {
+		if (app && strcmp(app->name, a->name))
+			continue;
+		list_for_each_entry_safe(inc, nxt, &a->incs_list, a_list) {
+			ip_vs_app_inc_release(net, inc);
+		}
 
-	list_del(&app->a_list);
+		list_del(&a->a_list);
+		kfree(a);
 
-	mutex_unlock(&__ip_vs_app_mutex);
+		/* decrease the module use count */
+		ip_vs_use_count_dec();
+	}
 
-	/* decrease the module use count */
-	ip_vs_use_count_dec();
+	mutex_unlock(&__ip_vs_app_mutex);
 }
 
 
@@ -587,5 +617,6 @@ int __net_init ip_vs_app_net_init(struct net *net)
 
 void __net_exit ip_vs_app_net_cleanup(struct net *net)
 {
+	unregister_ip_vs_app(net, NULL /* all */);
 	proc_net_remove(net, "ip_vs_app");
 }
diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
index 538d74e..f1c8ef9 100644
--- a/net/netfilter/ipvs/ip_vs_ftp.c
+++ b/net/netfilter/ipvs/ip_vs_ftp.c
@@ -437,18 +437,10 @@ static int __net_init __ip_vs_ftp_init(struct net *net)
 {
 	int i, ret;
 	struct ip_vs_app *app;
-	struct netns_ipvs *ipvs = net_ipvs(net);
-
-	app = kmemdup(&ip_vs_ftp, sizeof(struct ip_vs_app), GFP_KERNEL);
-	if (!app)
-		return -ENOMEM;
-	INIT_LIST_HEAD(&app->a_list);
-	INIT_LIST_HEAD(&app->incs_list);
-	ipvs->ftp_app = app;
 
-	ret = register_ip_vs_app(net, app);
-	if (ret)
-		goto err_exit;
+	app = register_ip_vs_app(net, &ip_vs_ftp);
+	if (IS_ERR(app))
+		return PTR_ERR(app);
 
 	for (i = 0; i < ports_count; i++) {
 		if (!ports[i])
@@ -462,9 +454,7 @@ static int __net_init __ip_vs_ftp_init(struct net *net)
 	return 0;
 
 err_unreg:
-	unregister_ip_vs_app(net, app);
-err_exit:
-	kfree(ipvs->ftp_app);
+	unregister_ip_vs_app(net, &ip_vs_ftp);
 	return ret;
 }
 /*
@@ -474,8 +464,8 @@ static void __ip_vs_ftp_exit(struct net *net)
 {
 	struct netns_ipvs *ipvs = net_ipvs(net);
 
-	unregister_ip_vs_app(net, ipvs->ftp_app);
-	kfree(ipvs->ftp_app);
+	if (ipvs)
+		unregister_ip_vs_app(net, &ip_vs_ftp);
 }
 
 static struct pernet_operations ip_vs_ftp_ops = {
-- 
1.7.3.4

Re: [PATCH 1/2] ipvs: reset ipvs pointer in netns

[Hans Schillstrom]

Hello Julian
On Friday 13 April 2012 10:44:01 Julian Anastasov wrote:
> 	Make sure net->ipvs is reset on netns cleanup or failed
> initialization. It is needed for IPVS applications to know that
> IPVS core is not loaded in netns.
> 
> Signed-off-by: Julian Anastasov <ja@ssi.bg>
> ---
>  net/netfilter/ipvs/ip_vs_core.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 2555816..260b9ef 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1924,6 +1924,7 @@ protocol_fail:
>  control_fail:
>  	ip_vs_estimator_net_cleanup(net);
>  estimator_fail:
> +	net->ipvs = NULL;

You can't do this since name space cleanup will call __ip_vs_cleanup later
and that will cause  NULL pointer problems


Note that any netns init faiulure will be rolled back later by net_namespace.c
And you will not receive any trafic into a namepace until all init is OK


>  	return -ENOMEM;
>  }
>  
> @@ -1936,6 +1937,7 @@ static void __net_exit __ip_vs_cleanup(struct net *net)
>  	ip_vs_control_net_cleanup(net);
>  	ip_vs_estimator_net_cleanup(net);
>  	IP_VS_DBG(2, "ipvs netns %d released\n", net_ipvs(net)->gen);
> +	net->ipvs = NULL;
>  }
>  
>  static void __net_exit __ip_vs_dev_cleanup(struct net *net)

-- 
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [PATCH 2/2] ipvs: fix app registration in netns

[Hans Schillstrom]

Hello Julian
On Friday 13 April 2012 10:44:02 Julian Anastasov wrote:
> 	Avoid crash when registering applications (ftp) when
> the IPVS core initialization for netns fails. Do this by
> checking for present core (net->ipvs) when registering apps.
> 
> 	As result this gets rid of the ftp_app pointer and allows
> applications to be registered without adding fields in the
> netns_ipvs structure.
> 
> 	Also, make sure applications are unregistered from the
> core automatically, should not be needed in theory.
> 

I think we can do this much simpler,
remove the auto init of ftp (ip_vs_ftp_init and ip_vs_ftp_exit) 
and let ip_vs_app call __ip_vs_ftp_init() as a we do with the protocols.

If the init fails before ip_vs_app_net_init() ftp init will not be called
and if it fails after a proper cleanup will be performed.

i.e. same design every where.

ex remove the
 ip_vs_ftp_init(void)
 ip_vs_ftp_exit(void)

-- 
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [PATCH 1/2] ipvs: reset ipvs pointer in netns

[Julian Anastasov]

	Hello,

On Fri, 13 Apr 2012, Hans Schillstrom wrote:

> > --- a/net/netfilter/ipvs/ip_vs_core.c
> > +++ b/net/netfilter/ipvs/ip_vs_core.c
> > @@ -1924,6 +1924,7 @@ protocol_fail:
> >  control_fail:
> >  	ip_vs_estimator_net_cleanup(net);
> >  estimator_fail:
> > +	net->ipvs = NULL;
> 
> You can't do this since name space cleanup will call __ip_vs_cleanup later
> and that will cause  NULL pointer problems

	This is not true. __ip_vs_init must free everything
on error as a netns init handler. What will happen if
net_generic fails and netns tries cleanup on such failed
subsys? __ip_vs_cleanup is called only after successful
__ip_vs_init.

	Looking at setup_net() it correctly uses
_continue_ list walking variant to call exit handler only
after successful init. Also, there is comment about this:

        /* Walk through the list backwards calling the exit functions
         * for the pernet modules whose init functions did not fail.
         */

	IPVS actually reaches __register_pernet_operations
where every net is added in net_exit_list for cleanup only
on successful init.

	It is true that some places such as ip_vs_in
and ip_vs_out touch net_ipvs(net) without any checks
for net->ipvs. This is not a problem because during
netns cleanup the devices are unregistered before
subsys (eg. IPVS), so we can not see packets during
cleanup. And on netns creation net is registered only
when all init handlers succeed. So, net->ipvs is
always safe to access in hooks.

	So, this patch should be not needed if only
one subsys was registered (IPVS core) but now we register
more:

- applications
- schedulers

	LBLC and LBLCR suffer from same problem. They
attach to net->ipvs without any checks for net->ipvs.

> Note that any netns init faiulure will be rolled back later by net_namespace.c
> And you will not receive any trafic into a namepace until all init is OK

	What I know about netns is:

- all registrations are in one list, all subsys before all devices

- exit handlers are called only on successful init

- exit handlers are called in reverse registration order: last
device, ... first device, last subsys ... first subsys

	So, if IPVS modules register any pernet handlers
they must check if net->ipvs (IPVS core) is successfully
initialized for netns. If Simon confirms that the patches
work I have to provide similar fix for the LBLC* schedulers.

	Simon, do you have IPVS core in kernel? I assume
this is the case, right? Not as a module?

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH 2/2] ipvs: fix app registration in netns

[Julian Anastasov]

	Hello,

On Fri, 13 Apr 2012, Hans Schillstrom wrote:

> Hello Julian
> On Friday 13 April 2012 10:44:02 Julian Anastasov wrote:
> > 	Avoid crash when registering applications (ftp) when
> > the IPVS core initialization for netns fails. Do this by
> > checking for present core (net->ipvs) when registering apps.
> > 
> > 	As result this gets rid of the ftp_app pointer and allows
> > applications to be registered without adding fields in the
> > netns_ipvs structure.
> > 
> > 	Also, make sure applications are unregistered from the
> > core automatically, should not be needed in theory.
> > 
> 
> I think we can do this much simpler,
> remove the auto init of ftp (ip_vs_ftp_init and ip_vs_ftp_exit) 
> and let ip_vs_app call __ip_vs_ftp_init() as a we do with the protocols.
> 
> If the init fails before ip_vs_app_net_init() ftp init will not be called
> and if it fails after a proper cleanup will be performed.
> 
> i.e. same design every where.
> 
> ex remove the
>  ip_vs_ftp_init(void)
>  ip_vs_ftp_exit(void)

	OK, go ahead. But also change init_netns for
proto (and now for apps) to return int error. Now
__ip_vs_tcp_init does not return error if the
pd->timeout_table allocation fails. This is fatal.

	May be you can do this change on top of these
two patches because init_netns for ftp should be NULL,
we do not need to allocate anything for netns. Only
&ip_vs_ftp should go in some new global list to be cloned
for every netns as my 2nd patch:

- new: global list for registered apps
- present: app per netns: to hold incs_list
- present: inc (app instance for port) per netns: to hold the
instance in the per-netns context of proto

Regards

--
Julian Anastasov <ja@ssi.bg>