Hello,

Kazunori MIYAZAWA wrote:
> This is the patch to support IPv6 over IPv4 IPsec
> 
> Signed-off-by: Miika Komu <miika@iki.fi>
> Signed-off-by: Diego Beltrami <Diego.Beltrami@hiit.fi>
> Signed-off-by: Kazunori Miyazawa <miyazawa@linux-ipv6.org>


This seems to break Mobile IPv6 route optimization (RO).
(This patch is commited as c82f963efe823d3cacaf1f1b7f1a35cc9628b188
 to David's tree.)

Please find my comment below.


> diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
> index 8dffd4d..a1ac537 100644
> --- a/net/ipv6/xfrm6_policy.c
> +++ b/net/ipv6/xfrm6_policy.c
> @@ -131,13 +131,11 @@ __xfrm6_bundle_create(struct xfrm_policy
>  	struct dst_entry *dst, *dst_prev;
>  	struct rt6_info *rt0 = (struct rt6_info*)(*dst_p);
>  	struct rt6_info *rt  = rt0;
> -	struct in6_addr *remote = &fl->fl6_dst;
> -	struct in6_addr *local  = &fl->fl6_src;
>  	struct flowi fl_tunnel = {
>  		.nl_u = {
>  			.ip6_u = {
> -				.saddr = *local,
> -				.daddr = *remote
> +				.saddr = fl->fl6_src,
> +				.daddr = fl->fl6_dst,
>  			}
>  		}
>  	};
> @@ -153,7 +151,6 @@ __xfrm6_bundle_create(struct xfrm_policy
>  	for (i = 0; i < nx; i++) {
>  		struct dst_entry *dst1 = dst_alloc(&xfrm6_dst_ops);
>  		struct xfrm_dst *xdst;
> -		int tunnel = 0;
>  
>  		if (unlikely(dst1 == NULL)) {
>  			err = -ENOBUFS;
> @@ -177,19 +174,27 @@ __xfrm6_bundle_create(struct xfrm_policy
>  
>  		dst1->next = dst_prev;
>  		dst_prev = dst1;
> -		if (xfrm[i]->props.mode != XFRM_MODE_TRANSPORT) {
> -			remote = __xfrm6_bundle_addr_remote(xfrm[i], remote);
> -			local  = __xfrm6_bundle_addr_local(xfrm[i], local);
> -			tunnel = 1;
> -		}
> +
>  		__xfrm6_bundle_len_inc(&header_len, &nfheader_len, xfrm[i]);
>  		trailer_len += xfrm[i]->props.trailer_len;
>  
> -		if (tunnel) {
> -			ipv6_addr_copy(&fl_tunnel.fl6_dst, remote);
> -			ipv6_addr_copy(&fl_tunnel.fl6_src, local);
> -			err = xfrm_dst_lookup((struct xfrm_dst **) &rt,
> -					      &fl_tunnel, AF_INET6);
> +		if (xfrm[i]->props.mode == XFRM_MODE_TUNNEL) {
> +			unsigned short encap_family = xfrm[i]->props.family;
> +			switch(encap_family) {
> +			case AF_INET:
> +				fl_tunnel.fl4_dst = xfrm[i]->id.daddr.a4;
> +				fl_tunnel.fl4_src = xfrm[i]->props.saddr.a4;
> +				break;
> +			case AF_INET6:
> +				ipv6_addr_copy(&fl_tunnel.fl6_dst, (struct in6_addr*)&xfrm[i]->id.daddr.a6);
> +				ipv6_addr_copy(&fl_tunnel.fl6_src, (struct in6_addr*)&xfrm[i]->props.saddr.a6);
> +				break;
> +			default:
> +				BUG_ON(1);
> +			}
> +
> + 			err = xfrm_dst_lookup((struct xfrm_dst **) &rt,
> +					      &fl_tunnel, encap_family);
>  			if (err)
>  				goto error;
>  		} else


You missed RO mode path when you changed semantics to check the mode
from "xfrm[i]->props.mode != XFRM_MODE_TRANSPORT"
to "xfrm[i]->props.mode == XFRM_MODE_TUNNEL" before
changing address. Your patch also makes two incline functions
__xfrm6_bundle_addr_{remote,local} are used by nobody.

I suggest a fix to add "|| xfrm[i]->props.mode == XFRM_MODE_ROUTEOPTIMIZATION" there
to make it clearer for other developers about RO-is-there than restoring the code.

# FYI, we don't have to fix another side of inter-family IPsec tunneling (xfrm4_policy.c)
# where you have similar patch (IPv4 over IPv6 IPsec tunnel) because the RO
# is used only for the case of "IPv6 flow and IPv6 extension headers".

Please give me comments for the attached patch.
I hope it will be applied (or replaced the original patch with including mine).


Regards,

-- 
Masahide NAKAMURA