[IPV6]: Fix IPsec datagram fragmentation

[IPV6]: Fix IPsec datagram fragmentation

[Herbert Xu]

Hi Dave:

[IPV6]: Fix IPsec datagram fragmentation

This is a long-standing bug in the IPsec IPv6 code that breaks
when we emit a IPsec tunnel-mode datagram packet.  The problem
is that the code the emits the packet assumes the IPv6 stack
will fragment it later, but the IPv6 stack assumes that whoever
is emitting the packet is going to pre-fragment the packet.

In the long term we need to fix both sides, e.g., to get the
datagram code to pre-fragment as well as to get the IPv6 stack
to fragment locally generated tunnel-mode packet.

For now this patch does the second part which should make it
work for the IPsec host case.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 9ac6ca2..4e9a2fe 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -621,7 +621,7 @@ static int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 	 * or if the skb it not generated by a local socket.  (This last
 	 * check should be redundant, but it's free.)
 	 */
-	if (!np || np->pmtudisc >= IPV6_PMTUDISC_DO) {
+	if (skb->local_df) {
 		skb->dev = skb->dst->dev;
 		icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu, skb->dev);
 		IP6_INC_STATS(ip6_dst_idev(skb->dst), IPSTATS_MIB_FRAGFAILS);
@@ -1420,6 +1420,10 @@ int ip6_push_pending_frames(struct sock *sk)
 		tmp_skb->sk = NULL;
 	}
 
+	/* Allow local fragmentation. */
+	if (np->pmtudisc >= IPV6_PMTUDISC_DO)
+		skb->local_df = 1;
+
 	ipv6_addr_copy(final_dst, &fl->fl6_dst);
 	__skb_pull(skb, skb_network_header_len(skb));
 	if (opt && opt->opt_flen)
diff --git a/net/ipv6/xfrm6_output.c b/net/ipv6/xfrm6_output.c
index b34c58c..79ccfb0 100644
--- a/net/ipv6/xfrm6_output.c
+++ b/net/ipv6/xfrm6_output.c
@@ -36,7 +36,7 @@ static int xfrm6_tunnel_check_size(struct sk_buff *skb)
 	if (mtu < IPV6_MIN_MTU)
 		mtu = IPV6_MIN_MTU;
 
-	if (skb->len > mtu) {
+	if (!skb->local_df && skb->len > mtu) {
 		skb->dev = dst->dev;
 		icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu, skb->dev);
 		ret = -EMSGSIZE;

Re: [IPV6]: Fix IPsec datagram fragmentation

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 13 Feb 2008 11:04:37 +1100

> [IPV6]: Fix IPsec datagram fragmentation
> 
> This is a long-standing bug in the IPsec IPv6 code that breaks
> when we emit a IPsec tunnel-mode datagram packet.  The problem
> is that the code the emits the packet assumes the IPv6 stack
> will fragment it later, but the IPv6 stack assumes that whoever
> is emitting the packet is going to pre-fragment the packet.
> 
> In the long term we need to fix both sides, e.g., to get the
> datagram code to pre-fragment as well as to get the IPv6 stack
> to fragment locally generated tunnel-mode packet.
> 
> For now this patch does the second part which should make it
> work for the IPsec host case.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, and I'll queue this up to -stable as well.

Thanks!

Re: [IPV6]: Fix IPsec datagram fragmentation

[Herbert Xu]

On Tue, Feb 12, 2008 at 06:08:28PM -0800, David Miller wrote:
> 
> > [IPV6]: Fix IPsec datagram fragmentation
>
> Applied, and I'll queue this up to -stable as well.

Sorry, David Stevens just told me that it doesn't work as intended.

[IPV6]: Fix reversed local_df test in ip6_fragment

I managed to reverse the local_df test when forward-porting this
patch so it actually makes things worse by never fragmenting at
all.

Thanks to David Stevens for testing and reporting this bug.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 4e9a2fe..35ba693 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -621,7 +621,7 @@ static int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 	 * or if the skb it not generated by a local socket.  (This last
 	 * check should be redundant, but it's free.)
 	 */
-	if (skb->local_df) {
+	if (!skb->local_df) {
 		skb->dev = skb->dst->dev;
 		icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu, skb->dev);
 		IP6_INC_STATS(ip6_dst_idev(skb->dst), IPSTATS_MIB_FRAGFAILS);

Re: [IPV6]: Fix IPsec datagram fragmentation

[Bill Fink]

Hi Herbert,

On Fri, 15 Feb 2008, Herbert Xu wrote:

> On Tue, Feb 12, 2008 at 06:08:28PM -0800, David Miller wrote:
> > 
> > > [IPV6]: Fix IPsec datagram fragmentation
> >
> > Applied, and I'll queue this up to -stable as well.
> 
> Sorry, David Stevens just told me that it doesn't work as intended.
> 
> [IPV6]: Fix reversed local_df test in ip6_fragment
> 
> I managed to reverse the local_df test when forward-porting this
> patch so it actually makes things worse by never fragmenting at
> all.
> 
> Thanks to David Stevens for testing and reporting this bug.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> --
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 4e9a2fe..35ba693 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -621,7 +621,7 @@ static int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
>  	 * or if the skb it not generated by a local socket.  (This last
>  	 * check should be redundant, but it's free.)
>  	 */
> -	if (skb->local_df) {
> +	if (!skb->local_df) {
>  		skb->dev = skb->dst->dev;
>  		icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu, skb->dev);
>  		IP6_INC_STATS(ip6_dst_idev(skb->dst), IPSTATS_MIB_FRAGFAILS);

I think the setting of skb->local_def is still backwards in your
original patch:

> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 9ac6ca2..4e9a2fe 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c

...

> @@ -1420,6 +1420,10 @@ int ip6_push_pending_frames(struct sock *sk)
> 		tmp_skb->sk = NULL;
> 	}
> 
> +	/* Allow local fragmentation. */
> +	if (np->pmtudisc >= IPV6_PMTUDISC_DO)
> +		skb->local_df = 1;
> +
> 	ipv6_addr_copy(final_dst, &fl->fl6_dst);
> 	__skb_pull(skb, skb_network_header_len(skb));
> 	if (opt && opt->opt_flen)

I think the test should be:

	if (np->pmtudisc < IPV6_PMTUDISC_DO)

as it is in net/ipv4/ip_output.c:

/* Unless user demanded real pmtu discovery (IP_PMTUDISC_DO), we allow
 * to fragment the frame generated here. No matter, what transforms
 * how transforms change size of the packet, it will come out.
 */
	if (inet->pmtudisc < IP_PMTUDISC_DO)
		skb->local_df = 1;

Or perhaps I'm just missing something obvious.

						-Bill

Re: [IPV6]: Fix IPsec datagram fragmentation

[Herbert Xu]

On Fri, Feb 15, 2008 at 01:13:13AM -0500, Bill Fink wrote:
>
> I think the setting of skb->local_def is still backwards in your
> original patch:

You're quite right.  Thanks for pointing this out!

[IPV6]: Fix reversed local_df test in ip6_fragment

I managed to reverse the local_df test when forward-porting this
patch so it actually makes things worse by never fragmenting at
all.

Thanks to David Stevens for testing and reporting this bug.

Bill Fink pointed out that the local_df setting is also the wrong
way around.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 4e9a2fe..8b67ca0 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -621,7 +621,7 @@ static int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 	 * or if the skb it not generated by a local socket.  (This last
 	 * check should be redundant, but it's free.)
 	 */
-	if (skb->local_df) {
+	if (!skb->local_df) {
 		skb->dev = skb->dst->dev;
 		icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu, skb->dev);
 		IP6_INC_STATS(ip6_dst_idev(skb->dst), IPSTATS_MIB_FRAGFAILS);
@@ -1421,7 +1421,7 @@ int ip6_push_pending_frames(struct sock *sk)
 	}
 
 	/* Allow local fragmentation. */
-	if (np->pmtudisc >= IPV6_PMTUDISC_DO)
+	if (np->pmtudisc < IPV6_PMTUDISC_DO)
 		skb->local_df = 1;
 
 	ipv6_addr_copy(final_dst, &fl->fl6_dst);

Re: [IPV6]: Fix IPsec datagram fragmentation

[David Stevens]

I think the field name is kind of confusing here. It appears
that "local_df == 1" means "allow fragmentation" and
"local_df == 0" means "don't fragment". But "DF" to me
means "don't fragment", as in "IP_DF", so the field value
is backwards from what I'd expect. And maybe that's
what happened to in your patch, too, Herbert.

For the future, maybe we should rename that, or reverse
the sense of it (in v4 as well). :-)

                                        +-DLS

Re: [IPV6]: Fix IPsec datagram fragmentation

[Herbert Xu]

On Fri, Feb 15, 2008 at 12:05:32AM -0800, David Stevens wrote:
>
> For the future, maybe we should rename that, or reverse
> the sense of it (in v4 as well). :-)

Yeah it really should be called local_mayfrag.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt