xfrm: UFO + ESP = double fragmentation

xfrm: UFO + ESP = double fragmentation

[Jiri Bohac]

Hi,

I'm seeing wrong fragmentation on locally generated UDPv6 packets
going out over ESP (transport mode):

UFO is turned on on the outgoing interface and MTU is 1500.
When 8 kB is written to a UDP socket, udpv6_sendmsg() calls
ip_append_data() which generates a single 8 kB GSO skb.

Through ip6_send_skb() it reaches xfrm_output(). Since
skb_is_gso(skb) is nonzero, xfrm_output_gso() is called.
It immediatelly segments the skb via skb_gso_segment() and then
calls xfrm_output2() on each individual segment.

This is wrong. RFC4303 says:
	3.3.4.  Fragmentation
	   If necessary, fragmentation is performed after ESP
	   processing within an IPsec implementation.  Thus,
	   transport mode ESP is applied only to whole IP
	   datagrams (not to IP fragments).

Instead, xfrm_output_gso() applies the transform to each segment.
Since both the fragmentation header _and_ the ESP headers now
don't fit in the MTU and the ESP-encapsulated segments
are fragmented for a second time in ip6_finish_output().

The outcome is:
- the original 8k UDP packet is split into 6 ESP fragments
- the first 5 ESP fragments are 1508 bytes each, thus fragmented
  again into two fragments 

The destination host replies with ICMP parameter problem.

How is this supposed to work?
This hack fixes this specific case:

--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -198,7 +198,7 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
 	int err;
 
 	if (skb_is_gso(skb))
-		return xfrm_output_gso(net, sk, skb);
+		return xfrm_output2(net, sk, skb);
 
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		err = skb_checksum_help(skb);


Is there a situation when xfrm_output_gso() does the right thing?

Thanks,

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: xfrm: UFO + ESP = double fragmentation

[Herbert Xu]

On Sat, Jan 30, 2016 at 12:44:24AM +0100, Jiri Bohac wrote:
>
> Is there a situation when xfrm_output_gso() does the right thing?

Yes because you've just broken TSO over IPsec.

In fact you're remarkably close to the right solution which is
to avoid xfrm_output_gso for SKB_GSO_UDP packets.

You should also work through all the other types (e.g., tunnels)
one-by-one and determine which ones should be fragmented and
which ones shouldn't.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH] xfrm: don't segment UFO packets

[Jiri Bohac]

xfrm_output() will segment GSO packets, including UDP (UFO) packets.
this is wrong per RFC4303, section 3.3.4.  Fragmentation:

   If necessary, fragmentation is performed after ESP
   processing within an IPsec implementation.  Thus,
   transport mode ESP is applied only to whole IP
   datagrams (not to IP fragments).

Prevent xfrm_output() from segmenting UFO packets so that they will be
fragmented after the xfrm transforms.

Signed-off-by: Jiri Bohac <jbohac@suse.cz>


diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 4355129..6f3e814 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3501,6 +3501,12 @@ static inline bool skb_is_gso_v6(const struct sk_buff *skb)
 	return skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6;
 }
 
+/* Note: Should be called only if skb_is_gso(skb) is true */
+static inline bool skb_is_ufo(const struct sk_buff *skb)
+{
+	return skb_shinfo(skb)->gso_type & SKB_GSO_UDP;
+}
+
 void __skb_warn_lro_forwarding(const struct sk_buff *skb);
 
 static inline bool skb_warn_if_lro(const struct sk_buff *skb)
diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index cc3676e..c52cc8b 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -197,8 +197,12 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
 	struct net *net = dev_net(skb_dst(skb)->dev);
 	int err;
 
-	if (skb_is_gso(skb))
-		return xfrm_output_gso(net, sk, skb);
+	if (skb_is_gso(skb)) {
+		if (skb_is_ufo(skb))
+			return xfrm_output2(net, sk, skb);
+		else
+			return xfrm_output_gso(net, sk, skb);
+	}
 
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		err = skb_checksum_help(skb);
-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [PATCH] xfrm: don't segment UFO packets

[Herbert Xu]

On Wed, Mar 16, 2016 at 05:00:26PM +0100, Jiri Bohac wrote:
> xfrm_output() will segment GSO packets, including UDP (UFO) packets.
> this is wrong per RFC4303, section 3.3.4.  Fragmentation:
> 
>    If necessary, fragmentation is performed after ESP
>    processing within an IPsec implementation.  Thus,
>    transport mode ESP is applied only to whole IP
>    datagrams (not to IP fragments).
> 
> Prevent xfrm_output() from segmenting UFO packets so that they will be
> fragmented after the xfrm transforms.
> 
> Signed-off-by: Jiri Bohac <jbohac@suse.cz>

Fair enough.  But I wonder if this is enough.  Wouldn't UDP notice
that we're doing IPsec and prefragment the packet anyway? So I think
this check may also be needed in the UDP output path.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] xfrm: don't segment UFO packets

[Jiri Bohac]

On Thu, Mar 17, 2016 at 01:03:59PM +0800, Herbert Xu wrote:
> On Wed, Mar 16, 2016 at 05:00:26PM +0100, Jiri Bohac wrote:
> > Prevent xfrm_output() from segmenting UFO packets so that they will be
> > fragmented after the xfrm transforms.
> 
> Fair enough.  But I wonder if this is enough.  Wouldn't UDP notice
> that we're doing IPsec and prefragment the packet anyway? So I think
> this check may also be needed in the UDP output path.

Fixes my broken case. Ftracing a sendmsg call that sends ~8k of
data over the UDP socket, I see a single 8k skb travel through the
stack all the way to xfrm_output(). MTU is 1500.
That's the whole poing of fragmentation offloading, to pass all
the data at once as far as we can, isn't it? What UDP code did
you think would "notice and prefragment"?

Thanks,

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [PATCH] xfrm: don't segment UFO packets

[Steffen Klassert]

On Thu, Mar 17, 2016 at 10:41:15AM +0100, Jiri Bohac wrote:
> On Thu, Mar 17, 2016 at 01:03:59PM +0800, Herbert Xu wrote:
> > On Wed, Mar 16, 2016 at 05:00:26PM +0100, Jiri Bohac wrote:
> > > Prevent xfrm_output() from segmenting UFO packets so that they will be
> > > fragmented after the xfrm transforms.
> > 
> > Fair enough.  But I wonder if this is enough.  Wouldn't UDP notice
> > that we're doing IPsec and prefragment the packet anyway? So I think
> > this check may also be needed in the UDP output path.
> 
> Fixes my broken case. 

Is this IPv4 or IPv6? IPv4 should not create a GSO skb
if IPsec is done. It checks for rt->dst.header_len
in __ip_append_data() and does a fallback to the
standard case if rt->dst.header_len is non zero.

In IPv6 this check is missing, so this could be the
problem if this is IPv6.

Re: [PATCH] xfrm: don't segment UFO packets

[Jiri Bohac]

On Thu, Mar 17, 2016 at 11:24:59AM +0100, Steffen Klassert wrote:
> > > On Wed, Mar 16, 2016 at 05:00:26PM +0100, Jiri Bohac wrote:
> > Fixes my broken case. 
> 
> Is this IPv4 or IPv6? IPv4 should not create a GSO skb
> if IPsec is done. It checks for rt->dst.header_len
> in __ip_append_data() and does a fallback to the
> standard case if rt->dst.header_len is non zero.

It's IPv6.

> In IPv6 this check is missing, so this could be the
> problem if this is IPv6.

Doesn't the check do exactly the opposite of what the RFC says?
The RFC wants ESP to be performed first and fragmentation after
that. UDPv4 currently seems to be doing the opposite. Well at
least it works, unlike in the IPv6 case, where the packet is
fragmented, but not enough space is reserved, so after adding the
ESP headers, it is fragmented once more.

(Details can be found in my first e-mail in this thread, I now
replied into the old thread after >1 month, sorry for that:
http://thread.gmane.org/gmane.linux.network/396952
)
-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [PATCH] xfrm: don't segment UFO packets

[Steffen Klassert]

On Thu, Mar 17, 2016 at 11:49:53AM +0100, Jiri Bohac wrote:
> On Thu, Mar 17, 2016 at 11:24:59AM +0100, Steffen Klassert wrote:
> > > > On Wed, Mar 16, 2016 at 05:00:26PM +0100, Jiri Bohac wrote:
> > > Fixes my broken case. 
> > 
> > Is this IPv4 or IPv6? IPv4 should not create a GSO skb
> > if IPsec is done. It checks for rt->dst.header_len
> > in __ip_append_data() and does a fallback to the
> > standard case if rt->dst.header_len is non zero.
> 
> It's IPv6.
> 
> > In IPv6 this check is missing, so this could be the
> > problem if this is IPv6.
> 
> Doesn't the check do exactly the opposite of what the RFC says?
> The RFC wants ESP to be performed first and fragmentation after
> that. UDPv4 currently seems to be doing the opposite. 

No, __ip_append_data() only prepares the packets for fragmentation
and enqueues them. Then __ip_make_skb() dequeues and builds
one skb with a fraglist. Then the xfrm layer is called, so
esp linearizes (unfortunately) the skb and applies the
transformation. Fragmentation happens after that.

Re: [PATCH] xfrm: don't segment UFO packets

[Jiri Bohac]

On Thu, Mar 17, 2016 at 11:24:59AM +0100, Steffen Klassert wrote:
> In IPv6 this check is missing, so this could be the
> problem if this is IPv6.

indeed, this patch also fixes my problem:

--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1353,6 +1353,7 @@ emsgsize:
 	     (skb && skb_is_gso(skb))) &&
 	    (sk->sk_protocol == IPPROTO_UDP) &&
 	    (rt->dst.dev->features & NETIF_F_UFO) &&
+	    !rt->dst.header_len &&
 	    (sk->sk_type == SOCK_DGRAM)) {
 		err = ip6_ufo_append_data(sk, queue, getfrag, from, length,
 					  hh_len, fragheaderlen,

I can't say which is better. Herbert originally seemed to like
the fix inside xfrm_output().

The IPv4 part is fixed by commit
c146066ab80267c3305de5dda6a4083f06df9265 (ipv4: Don't use ufo
handling on later transformed packets)

Thanks,

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [PATCH] xfrm: don't segment UFO packets

[Herbert Xu]

On Thu, Mar 17, 2016 at 06:08:55PM +0100, Jiri Bohac wrote:
> On Thu, Mar 17, 2016 at 11:24:59AM +0100, Steffen Klassert wrote:
> > In IPv6 this check is missing, so this could be the
> > problem if this is IPv6.
> 
> indeed, this patch also fixes my problem:

Hmm, is this what you really want? If I understood you correctly,
you want the fragmentation to occur after IPsec.  So while this
might generate the same output, it is still going to prefragment
the data, only to merge them back for IPsec and then refragment
again.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] xfrm: don't segment UFO packets

[Steffen Klassert]

On Fri, Mar 18, 2016 at 10:36:53AM +0800, Herbert Xu wrote:
> On Thu, Mar 17, 2016 at 06:08:55PM +0100, Jiri Bohac wrote:
> > On Thu, Mar 17, 2016 at 11:24:59AM +0100, Steffen Klassert wrote:
> > > In IPv6 this check is missing, so this could be the
> > > problem if this is IPv6.
> > 
> > indeed, this patch also fixes my problem:
> 
> Hmm, is this what you really want? If I understood you correctly,
> you want the fragmentation to occur after IPsec.

The main problem was probably that UFO handling does not work at
all on IPv6 IPsec. 

> So while this
> might generate the same output, it is still going to prefragment
> the data, only to merge them back for IPsec and then refragment
> again.

This is far away from being optimal, but this is what usually
happens if a local application sends data that we need to
fragment.

We currently work on avoiding the linearization on IPsec,
but having a skb with a fraglist is really the worst case.