[PATCH ipsec 0/3] vti/vti6: minor tweaks + one fix

[PATCH ipsec 0/3] vti/vti6: minor tweaks + one fix

[Mathias Krause]

Hi Steffen,

this series addresses a few issues related to VTI. The first patch fixes
a bug in the vti6 module calling unregister_pernet_device() twice in the
error path. That's probably material for ipsec.git.
The second patch simplifies the error handling path in module init/fini
of vti6. The third patch does the same for vti. Those two are probably
material for ipsec-next.git as we're at -rc5 already. But I leave that
decision to you.

Please apply!

Mathias Krause (3):
  vti6: Don't unregister pernet ops twice on init errors
  vti6: Simplify error handling in module init and exit
  vti: Simplify error handling in module init and exit

 net/ipv4/ip_vti.c  |   54 +++++++++++++++++---------------------------
 net/ipv6/ip6_vti.c |   54 ++++++++++++++++----------------------------
 2 files changed, 41 insertions(+), 67 deletions(-)

-- 
1.7.10.4

[PATCH ipsec 1/3] vti6: Don't unregister pernet ops twice on init errors

[Mathias Krause]

If we fail to register one of the xfrm protocol handlers we will
unregister the pernet ops twice on the error exit path. This will
probably lead to a kernel panic as the double deregistration
leads to a double kfree().

Fix this by removing one of the calls to do it only once.

Fixes: fa9ad96d49 ("vti6: Update the ipv6 side to use its own...")
Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 net/ipv6/ip6_vti.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
index b7c0f82714..a51100379f 100644
--- a/net/ipv6/ip6_vti.c
+++ b/net/ipv6/ip6_vti.c
@@ -1097,7 +1097,6 @@ static int __init vti6_tunnel_init(void)
 
 	err = xfrm6_protocol_register(&vti_esp6_protocol, IPPROTO_ESP);
 	if (err < 0) {
-		unregister_pernet_device(&vti6_net_ops);
 		pr_err("%s: can't register vti6 protocol\n", __func__);
 
 		goto out;
@@ -1106,7 +1105,6 @@ static int __init vti6_tunnel_init(void)
 	err = xfrm6_protocol_register(&vti_ah6_protocol, IPPROTO_AH);
 	if (err < 0) {
 		xfrm6_protocol_deregister(&vti_esp6_protocol, IPPROTO_ESP);
-		unregister_pernet_device(&vti6_net_ops);
 		pr_err("%s: can't register vti6 protocol\n", __func__);
 
 		goto out;
@@ -1116,7 +1114,6 @@ static int __init vti6_tunnel_init(void)
 	if (err < 0) {
 		xfrm6_protocol_deregister(&vti_ah6_protocol, IPPROTO_AH);
 		xfrm6_protocol_deregister(&vti_esp6_protocol, IPPROTO_ESP);
-		unregister_pernet_device(&vti6_net_ops);
 		pr_err("%s: can't register vti6 protocol\n", __func__);
 
 		goto out;
-- 
1.7.10.4

[PATCH ipsec 2/3] vti6: Simplify error handling in module init and exit

[Mathias Krause]

The error handling in the module init and exit functions can be
shortened to safe us some code.

1/ Remove the code duplications in the init function, jump straight to
the existing cleanup code by adding some labels. Also give the error
message some more value by telling the reason why loading the module has
failed.

2/ Remove the error handling in the exit function as the only legitimate
reason xfrm6_protocol_deregister() might fail is inet6_del_protocol()
returning -1. That, in turn, means some other protocol handler had been
registered for this very protocol in the meantime. But that essentially
means we haven't been handling that protocol any more, anyway. What it
definitely means not is that we "can't deregister protocol". Therefore
just get rid of that bogus warning. It's plain wrong.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 net/ipv6/ip6_vti.c |   51 +++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 31 deletions(-)

diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
index a51100379f..59201ffc17 100644
--- a/net/ipv6/ip6_vti.c
+++ b/net/ipv6/ip6_vti.c
@@ -1089,36 +1089,26 @@ static struct xfrm6_protocol vti_ipcomp6_protocol __read_mostly = {
  **/
 static int __init vti6_tunnel_init(void)
 {
-	int  err;
+	const char *msg;
+	int err;
 
+	msg = "tunnel device";
 	err = register_pernet_device(&vti6_net_ops);
 	if (err < 0)
-		goto out_pernet;
+		goto pernet_dev_failed;
 
+	msg = "tunnel protocols";
 	err = xfrm6_protocol_register(&vti_esp6_protocol, IPPROTO_ESP);
-	if (err < 0) {
-		pr_err("%s: can't register vti6 protocol\n", __func__);
-
-		goto out;
-	}
-
+	if (err < 0)
+		goto xfrm_proto_esp_failed;
 	err = xfrm6_protocol_register(&vti_ah6_protocol, IPPROTO_AH);
-	if (err < 0) {
-		xfrm6_protocol_deregister(&vti_esp6_protocol, IPPROTO_ESP);
-		pr_err("%s: can't register vti6 protocol\n", __func__);
-
-		goto out;
-	}
-
+	if (err < 0)
+		goto xfrm_proto_ah_failed;
 	err = xfrm6_protocol_register(&vti_ipcomp6_protocol, IPPROTO_COMP);
-	if (err < 0) {
-		xfrm6_protocol_deregister(&vti_ah6_protocol, IPPROTO_AH);
-		xfrm6_protocol_deregister(&vti_esp6_protocol, IPPROTO_ESP);
-		pr_err("%s: can't register vti6 protocol\n", __func__);
-
-		goto out;
-	}
+	if (err < 0)
+		goto xfrm_proto_comp_failed;
 
+	msg = "netlink interface";
 	err = rtnl_link_register(&vti6_link_ops);
 	if (err < 0)
 		goto rtnl_link_failed;
@@ -1127,11 +1117,14 @@ static int __init vti6_tunnel_init(void)
 
 rtnl_link_failed:
 	xfrm6_protocol_deregister(&vti_ipcomp6_protocol, IPPROTO_COMP);
+xfrm_proto_comp_failed:
 	xfrm6_protocol_deregister(&vti_ah6_protocol, IPPROTO_AH);
+xfrm_proto_ah_failed:
 	xfrm6_protocol_deregister(&vti_esp6_protocol, IPPROTO_ESP);
-out:
+xfrm_proto_esp_failed:
 	unregister_pernet_device(&vti6_net_ops);
-out_pernet:
+pernet_dev_failed:
+	pr_err("vti6 init: failed to register %s\n", msg);
 	return err;
 }
 
@@ -1141,13 +1134,9 @@ out_pernet:
 static void __exit vti6_tunnel_cleanup(void)
 {
 	rtnl_link_unregister(&vti6_link_ops);
-	if (xfrm6_protocol_deregister(&vti_ipcomp6_protocol, IPPROTO_COMP))
-		pr_info("%s: can't deregister protocol\n", __func__);
-	if (xfrm6_protocol_deregister(&vti_ah6_protocol, IPPROTO_AH))
-		pr_info("%s: can't deregister protocol\n", __func__);
-	if (xfrm6_protocol_deregister(&vti_esp6_protocol, IPPROTO_ESP))
-		pr_info("%s: can't deregister protocol\n", __func__);
-
+	xfrm6_protocol_deregister(&vti_ipcomp6_protocol, IPPROTO_COMP);
+	xfrm6_protocol_deregister(&vti_ah6_protocol, IPPROTO_AH);
+	xfrm6_protocol_deregister(&vti_esp6_protocol, IPPROTO_ESP);
 	unregister_pernet_device(&vti6_net_ops);
 }
 
-- 
1.7.10.4

[PATCH ipsec 3/3] vti: Simplify error handling in module init and exit

[Mathias Krause]

The error handling in the module init and exit functions can be
shortened to safe us some code.

1/ Remove the code duplications in the init function, jump straight to
the existing cleanup code by adding some labels. Also give the error
message some more value by telling the reason why loading the module has
failed. Furthermore fix the "IPSec" typo -- it should be "IPsec" instead.

2/ Remove the error handling in the exit function as the only legitimate
reason xfrm4_protocol_deregister() might fail is inet_del_protocol()
returning -1. That, in turn, means some other protocol handler had been
registered for this very protocol in the meantime. But that essentially
means we haven't been handling that protocol any more, anyway. What it
definitely means not is that we "can't deregister tunnel". Therefore
just get rid of that bogus warning. It's plain wrong.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 net/ipv4/ip_vti.c |   54 ++++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 33 deletions(-)

diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
index afcee51b90..d8d2cbe6d4 100644
--- a/net/ipv4/ip_vti.c
+++ b/net/ipv4/ip_vti.c
@@ -525,40 +525,28 @@ static struct rtnl_link_ops vti_link_ops __read_mostly = {
 
 static int __init vti_init(void)
 {
+	const char *msg;
 	int err;
 
-	pr_info("IPv4 over IPSec tunneling driver\n");
+	pr_info("IPv4 over IPsec tunneling driver\n");
 
+	msg = "tunnel device";
 	err = register_pernet_device(&vti_net_ops);
 	if (err < 0)
-		return err;
-	err = xfrm4_protocol_register(&vti_esp4_protocol, IPPROTO_ESP);
-	if (err < 0) {
-		unregister_pernet_device(&vti_net_ops);
-		pr_info("vti init: can't register tunnel\n");
-
-		return err;
-	}
+		goto pernet_dev_failed;
 
+	msg = "tunnel protocols";
+	err = xfrm4_protocol_register(&vti_esp4_protocol, IPPROTO_ESP);
+	if (err < 0)
+		goto xfrm_proto_esp_failed;
 	err = xfrm4_protocol_register(&vti_ah4_protocol, IPPROTO_AH);
-	if (err < 0) {
-		xfrm4_protocol_deregister(&vti_esp4_protocol, IPPROTO_ESP);
-		unregister_pernet_device(&vti_net_ops);
-		pr_info("vti init: can't register tunnel\n");
-
-		return err;
-	}
-
+	if (err < 0)
+		goto xfrm_proto_ah_failed;
 	err = xfrm4_protocol_register(&vti_ipcomp4_protocol, IPPROTO_COMP);
-	if (err < 0) {
-		xfrm4_protocol_deregister(&vti_ah4_protocol, IPPROTO_AH);
-		xfrm4_protocol_deregister(&vti_esp4_protocol, IPPROTO_ESP);
-		unregister_pernet_device(&vti_net_ops);
-		pr_info("vti init: can't register tunnel\n");
-
-		return err;
-	}
+	if (err < 0)
+		goto xfrm_proto_comp_failed;
 
+	msg = "netlink interface";
 	err = rtnl_link_register(&vti_link_ops);
 	if (err < 0)
 		goto rtnl_link_failed;
@@ -567,23 +555,23 @@ static int __init vti_init(void)
 
 rtnl_link_failed:
 	xfrm4_protocol_deregister(&vti_ipcomp4_protocol, IPPROTO_COMP);
+xfrm_proto_comp_failed:
 	xfrm4_protocol_deregister(&vti_ah4_protocol, IPPROTO_AH);
+xfrm_proto_ah_failed:
 	xfrm4_protocol_deregister(&vti_esp4_protocol, IPPROTO_ESP);
+xfrm_proto_esp_failed:
 	unregister_pernet_device(&vti_net_ops);
+pernet_dev_failed:
+	pr_err("vti init: failed to register %s\n", msg);
 	return err;
 }
 
 static void __exit vti_fini(void)
 {
 	rtnl_link_unregister(&vti_link_ops);
-	if (xfrm4_protocol_deregister(&vti_ipcomp4_protocol, IPPROTO_COMP))
-		pr_info("vti close: can't deregister tunnel\n");
-	if (xfrm4_protocol_deregister(&vti_ah4_protocol, IPPROTO_AH))
-		pr_info("vti close: can't deregister tunnel\n");
-	if (xfrm4_protocol_deregister(&vti_esp4_protocol, IPPROTO_ESP))
-		pr_info("vti close: can't deregister tunnel\n");
-
-
+	xfrm4_protocol_deregister(&vti_ipcomp4_protocol, IPPROTO_COMP);
+	xfrm4_protocol_deregister(&vti_ah4_protocol, IPPROTO_AH);
+	xfrm4_protocol_deregister(&vti_esp4_protocol, IPPROTO_ESP);
 	unregister_pernet_device(&vti_net_ops);
 }
 
-- 
1.7.10.4

Re: [PATCH ipsec 1/3] vti6: Don't unregister pernet ops twice on init errors

[Steffen Klassert]

On Fri, May 09, 2014 at 11:43:40PM +0200, Mathias Krause wrote:
> If we fail to register one of the xfrm protocol handlers we will
> unregister the pernet ops twice on the error exit path. This will
> probably lead to a kernel panic as the double deregistration
> leads to a double kfree().
> 
> Fix this by removing one of the calls to do it only once.
> 
> Fixes: fa9ad96d49 ("vti6: Update the ipv6 side to use its own...")
> Signed-off-by: Mathias Krause <minipli@googlemail.com>

Applied to the ipsec tree, thanks!

Re: [PATCH ipsec 0/3] vti/vti6: minor tweaks + one fix

[Steffen Klassert]

On Fri, May 09, 2014 at 11:43:39PM +0200, Mathias Krause wrote:
> Hi Steffen,
> 
> this series addresses a few issues related to VTI. The first patch fixes
> a bug in the vti6 module calling unregister_pernet_device() twice in the
> error path. That's probably material for ipsec.git.
> The second patch simplifies the error handling path in module init/fini
> of vti6. The third patch does the same for vti. Those two are probably
> material for ipsec-next.git as we're at -rc5 already. But I leave that
> decision to you.

Right, patches two and three should go to ipsec-next. But the second
patch does not apply without the first patch. Please send separate
patchsets for ipsec and ipsec-next in future.

Thanks!

Re: [PATCH ipsec 0/3] vti/vti6: minor tweaks + one fix

[Mathias Krause]

On 13 May 2014 10:41, Steffen Klassert <steffen.klassert@secunet.com> wrote:
> On Fri, May 09, 2014 at 11:43:39PM +0200, Mathias Krause wrote:
>> Hi Steffen,
>>
>> this series addresses a few issues related to VTI. The first patch fixes
>> a bug in the vti6 module calling unregister_pernet_device() twice in the
>> error path. That's probably material for ipsec.git.
>> The second patch simplifies the error handling path in module init/fini
>> of vti6. The third patch does the same for vti. Those two are probably
>> material for ipsec-next.git as we're at -rc5 already. But I leave that
>> decision to you.
>
> Right, patches two and three should go to ipsec-next. But the second
> patch does not apply without the first patch. Please send separate
> patchsets for ipsec and ipsec-next in future.

Patch 2 depends on patch 1 because it's a series ;) I explicitly
didn't want to create different patches for ipsec-next because I
wanted to avoid the merge conflicts on your side when ipsec-next would
rebase to/merge a tree which would contain patch 1.
One way to solve it would be to merge ipsec/master into
ipsec-next/master prior to applying patches 2 and 3 -- just as Dave
does with net-next from time to time, i.e. merging net/master. Another
way would be to wait until Dave has merged ipsec/master into
net/master and after that, has merged net/master into net-next. This
way you can merge net-next into ipsec-next and after that apply
patches 2 and 3 without conflicts. Your choice. You know the
interdependencies between these trees better than me. But the first
solution sounds simpler to me. ;)

Mathias

Re: [PATCH ipsec 0/3] vti/vti6: minor tweaks + one fix

[Steffen Klassert]

On Tue, May 13, 2014 at 10:38:32PM +0200, Mathias Krause wrote:
> On 13 May 2014 10:41, Steffen Klassert <steffen.klassert@secunet.com> wrote:
> > On Fri, May 09, 2014 at 11:43:39PM +0200, Mathias Krause wrote:
> >> Hi Steffen,
> >>
> >> this series addresses a few issues related to VTI. The first patch fixes
> >> a bug in the vti6 module calling unregister_pernet_device() twice in the
> >> error path. That's probably material for ipsec.git.
> >> The second patch simplifies the error handling path in module init/fini
> >> of vti6. The third patch does the same for vti. Those two are probably
> >> material for ipsec-next.git as we're at -rc5 already. But I leave that
> >> decision to you.
> >
> > Right, patches two and three should go to ipsec-next. But the second
> > patch does not apply without the first patch. Please send separate
> > patchsets for ipsec and ipsec-next in future.
> 
> Patch 2 depends on patch 1 because it's a series ;) I explicitly
> didn't want to create different patches for ipsec-next because I
> wanted to avoid the merge conflicts on your side when ipsec-next would
> rebase to/merge a tree which would contain patch 1.
> One way to solve it would be to merge ipsec/master into
> ipsec-next/master prior to applying patches 2 and 3 -- just as Dave
> does with net-next from time to time, i.e. merging net/master. Another
> way would be to wait until Dave has merged ipsec/master into
> net/master and after that, has merged net/master into net-next. This
> way you can merge net-next into ipsec-next and after that apply
> patches 2 and 3 without conflicts. Your choice. You know the
> interdependencies between these trees better than me. But the first
> solution sounds simpler to me. ;)
> 

I've applied the remaining two patches to ipsec-next now, thanks!