[PATCH][IPSEC][4/6] changing "ipip tunnel" and xfrm4_tunnel to the API change

[PATCH][IPSEC][4/6] changing "ipip tunnel" and xfrm4_tunnel to the API change

[Kazunori MIYAZAWA]

Changing ipip tunnel and xfrm4_tunnel to the API

xfrm4_tunnel should regitser against both addresses so that
it should handle a tunnel packet whose internal address is
either IPv4 or IPv6.

signed-off-by Kazunori MIYAZAWA <miyazawa@linux-ipv6.org>

---
 net/ipv4/ipip.c         |    6 +++---
 net/ipv4/xfrm4_tunnel.c |   14 +++++++++-----
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index 9d719d6..775e950 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -870,7 +870,7 @@ static int __init ipip_init(void)
 
 	printk(banner);
 
-	if (xfrm4_tunnel_register(&ipip_handler)) {
+	if (xfrm4_tunnel_register(&ipip_handler, AF_INET)) {
 		printk(KERN_INFO "ipip init: can't register tunnel\n");
 		return -EAGAIN;
 	}
@@ -892,7 +892,7 @@ static int __init ipip_init(void)
  err2:
 	free_netdev(ipip_fb_tunnel_dev);
  err1:
-	xfrm4_tunnel_deregister(&ipip_handler);
+	xfrm4_tunnel_deregister(&ipip_handler, AF_INET);
 	goto out;
 }
 
@@ -912,7 +912,7 @@ static void __exit ipip_destroy_tunnels(
 
 static void __exit ipip_fini(void)
 {
-	if (xfrm4_tunnel_deregister(&ipip_handler))
+	if (xfrm4_tunnel_deregister(&ipip_handler, AF_INET))
 		printk(KERN_INFO "ipip close: can't deregister tunnel\n");
 
 	rtnl_lock();
diff --git a/net/ipv4/xfrm4_tunnel.c b/net/ipv4/xfrm4_tunnel.c
index f110af5..00fc09f 100644
--- a/net/ipv4/xfrm4_tunnel.c
+++ b/net/ipv4/xfrm4_tunnel.c
@@ -66,12 +66,15 @@ static struct xfrm_tunnel xfrm_tunnel_ha
 
 static int __init ipip_init(void)
 {
-	if (xfrm_register_type(&ipip_type, AF_INET) < 0) {
-		printk(KERN_INFO "ipip init: can't add xfrm type\n");
+	if (xfrm_register_type(&ipip_type, AF_INET) < 0)
+		return -EAGAIN;
+
+	if (xfrm4_tunnel_register(&xfrm_tunnel_handler, AF_INET)) {
+		printk(KERN_INFO "ipip init: can't add xfrm handler for AF_INET\n");
 		return -EAGAIN;
 	}
-	if (xfrm4_tunnel_register(&xfrm_tunnel_handler)) {
-		printk(KERN_INFO "ipip init: can't add xfrm handler\n");
+	if (xfrm4_tunnel_register(&xfrm_tunnel_handler, AF_INET6)) {
+		printk(KERN_INFO "ipip init: can't add xfrm handler for AF_INET6\n");
 		xfrm_unregister_type(&ipip_type, AF_INET);
 		return -EAGAIN;
 	}
@@ -80,7 +83,8 @@ static int __init ipip_init(void)
 
 static void __exit ipip_fini(void)
 {
-	if (xfrm4_tunnel_deregister(&xfrm_tunnel_handler))
+	if (xfrm4_tunnel_deregister(&xfrm_tunnel_handler, AF_INET) ||
+	    xfrm4_tunnel_deregister(&xfrm_tunnel_handler, AF_INET6))
 		printk(KERN_INFO "ipip close: can't remove xfrm handler\n");
 	if (xfrm_unregister_type(&ipip_type, AF_INET) < 0)
 		printk(KERN_INFO "ipip close: can't remove xfrm type\n");
-- 
1.4.1

Re: [PATCH][IPSEC][4/6] changing "ipip tunnel" and xfrm4_tunnel to the API change

[Patrick McHardy]

Kazunori MIYAZAWA wrote:
> Changing ipip tunnel and xfrm4_tunnel to the API

Fixing up users after API changes should be done in the same
patch as the API change itself to avoid breaking compilation
for people doing a bisection.

> index f110af5..00fc09f 100644
> --- a/net/ipv4/xfrm4_tunnel.c
> +++ b/net/ipv4/xfrm4_tunnel.c
> @@ -66,12 +66,15 @@ static struct xfrm_tunnel xfrm_tunnel_ha
>  
>  static int __init ipip_init(void)
>  {
> -	if (xfrm_register_type(&ipip_type, AF_INET) < 0) {
> -		printk(KERN_INFO "ipip init: can't add xfrm type\n");
> +	if (xfrm_register_type(&ipip_type, AF_INET) < 0)
> +		return -EAGAIN;

Why are you removing the printk?

> +
> +	if (xfrm4_tunnel_register(&xfrm_tunnel_handler, AF_INET)) {
> +		printk(KERN_INFO "ipip init: can't add xfrm handler for AF_INET\n");
>  		return -EAGAIN;

Does not unregister ipip_type on error.

>  	}
> -	if (xfrm4_tunnel_register(&xfrm_tunnel_handler)) {
> -		printk(KERN_INFO "ipip init: can't add xfrm handler\n");
> +	if (xfrm4_tunnel_register(&xfrm_tunnel_handler, AF_INET6)) {

You register xfrm_tunnel_handler twice, which will corrupt the list.

> +		printk(KERN_INFO "ipip init: can't add xfrm handler for AF_INET6\n");
>  		xfrm_unregister_type(&ipip_type, AF_INET);

Does not unregister AF_INET tunnel handler on error.

>  		return -EAGAIN;
>  	}

The next patch has similar problems.

Re: [PATCH][IPSEC][4/6] changing "ipip tunnel" and xfrm4_tunnel to the API change

[Kazunori MIYAZAWA]

Thank you Patrick for your review.
I'll fix and send again.

BTW, I'm going to use separete xfrm_tunnel_handler to solve
the issue which you pointed at the moment. But why does
"register twice" corrupt the list even if I use separate lists
for the address family.

Thank you,

Patrick McHardy wrote:
> Kazunori MIYAZAWA wrote:
>> Changing ipip tunnel and xfrm4_tunnel to the API
> 
> Fixing up users after API changes should be done in the same
> patch as the API change itself to avoid breaking compilation
> for people doing a bisection.
> 
>> index f110af5..00fc09f 100644
>> --- a/net/ipv4/xfrm4_tunnel.c
>> +++ b/net/ipv4/xfrm4_tunnel.c
>> @@ -66,12 +66,15 @@ static struct xfrm_tunnel xfrm_tunnel_ha
>>  
>>  static int __init ipip_init(void)
>>  {
>> -	if (xfrm_register_type(&ipip_type, AF_INET) < 0) {
>> -		printk(KERN_INFO "ipip init: can't add xfrm type\n");
>> +	if (xfrm_register_type(&ipip_type, AF_INET) < 0)
>> +		return -EAGAIN;
> 
> Why are you removing the printk?
> 
>> +
>> +	if (xfrm4_tunnel_register(&xfrm_tunnel_handler, AF_INET)) {
>> +		printk(KERN_INFO "ipip init: can't add xfrm handler for AF_INET\n");
>>  		return -EAGAIN;
> 
> Does not unregister ipip_type on error.
> 
>>  	}
>> -	if (xfrm4_tunnel_register(&xfrm_tunnel_handler)) {
>> -		printk(KERN_INFO "ipip init: can't add xfrm handler\n");
>> +	if (xfrm4_tunnel_register(&xfrm_tunnel_handler, AF_INET6)) {
> 
> You register xfrm_tunnel_handler twice, which will corrupt the list.
> 
>> +		printk(KERN_INFO "ipip init: can't add xfrm handler for AF_INET6\n");
>>  		xfrm_unregister_type(&ipip_type, AF_INET);
> 
> Does not unregister AF_INET tunnel handler on error.
> 
>>  		return -EAGAIN;
>>  	}
> 
> The next patch has similar problems.
> -
> 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

--
Kazunori Miyazawa

Re: [PATCH][IPSEC][4/6] changing "ipip tunnel" and xfrm4_tunnel to the API change

[Patrick McHardy]

Kazunori MIYAZAWA wrote:
> Thank you Patrick for your review.
> I'll fix and send again.

Thanks.

> BTW, I'm going to use separete xfrm_tunnel_handler to solve
> the issue which you pointed at the moment. But why does
> "register twice" corrupt the list even if I use separate lists
> for the address family.

The next pointer in xfrm_tunnel_handler will get overwritten on
the second registration.