Skip to contentSkip to conversation

: Improve MTU estimation

XFRM Patrick McHardy 2 participants 13 messages Aug 4, 2006

Subsystems (3)
Networking Network drivers Networking IPsec
lore
Share & tools
lore

Patrick McHardy

author
#0 lore raw

While trying to track down a PMTUD problem with IPsec, I misread the current MTU estimation code and initially thought it would overestimate the MTU. I then noticed that this was wrong, but by that time had already replaced it by an exact calculation. It fixes the common underestimation of the MTU by two bytes with ESP and should be a bit faster, so it still looks useful.

[XFRM]: Improve MTU estimation

Replace the probing based MTU estimation, which usually takes 2-3 iterations to find a fitting value and may underestimate the MTU, by an exact calculation.

Signed-off-by Patrick McHardy <[email protected]>

---
commit d5722ea7c8c7d3526788cd4fc3ab3e1237273fa8
tree 56ddec256902370864e93bb7bd095281162aea3b
parent a205729e2cd8e51257cd0ea738524c64da99b9e0
author Patrick McHardy <[email protected]> Fri, 04 Aug 2006 10:37:09 +0200 committer Patrick McHardy <[email protected]> Fri, 04 Aug 2006 10:37:09 +0200

 include/net/xfrm.h    |    3 +--
 net/ipv4/esp4.c       |   28 +++++++++++++++-------------
 net/ipv6/esp6.c       |   25 +++++++++++--------------
 net/xfrm/xfrm_state.c |   36 ++++++++----------------------------
 4 files changed, 35 insertions(+), 57 deletions(-)
include/net/xfrm.h +1 -2
index 9c5ee9f..ea1b028 100644
@@ -262,8 +262,7 @@ struct xfrm_type
 	void			(*destructor)(struct xfrm_state *);
 	int			(*input)(struct xfrm_state *, struct sk_buff *skb);
 	int			(*output)(struct xfrm_state *, struct sk_buff *pskb);
-	/* Estimate maximal size of result of transformation of a dgram */
-	u32			(*get_max_size)(struct xfrm_state *, int size);
+	u32			(*get_mtu)(struct xfrm_state *, int size);
 };
 
 extern int xfrm_register_type(struct xfrm_type *type, unsigned short family);
net/ipv4/esp4.c +15 -13
index fc2f8ce..5393dc2 100644
@@ -251,21 +251,19 @@ out:
 	return -EINVAL;
 }
 
-static u32 esp4_get_max_size(struct xfrm_state *x, int mtu)
+static u32 esp4_get_mtu(struct xfrm_state *x, int mtu)
 {
 	struct esp_data *esp = x->data;
-	u32 blksize = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
+	u32 align = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
 
-	if (x->props.mode) {
-		mtu = ALIGN(mtu + 2, blksize);
-	} else {
-		/* The worst case. */
-		mtu = ALIGN(mtu + 2, 4) + blksize - 4;
-	}
-	if (esp->conf.padlen)
-		mtu = ALIGN(mtu, esp->conf.padlen);
+	if (esp->conf.padlen > align)
+		align = esp->conf.padlen;
 
-	return mtu + x->props.header_len + esp->auth.icv_trunc_len;
+	mtu -= x->props.header_len + esp->auth.icv_trunc_len;
+	mtu &= ~(align - 1);
+	mtu -= 2;
+
+	return mtu;
 }
 
 static void esp4_err(struct sk_buff *skb, u32 info)

@@ -307,6 +305,7 @@ static void esp_destroy(struct xfrm_stat
 static int esp_init_state(struct xfrm_state *x)
 {
 	struct esp_data *esp = NULL;
+	u32 align;
 
 	/* null auth and encryption can have zero length keys */
 	if (x->aalg) {

@@ -385,7 +384,10 @@ static int esp_init_state(struct xfrm_st
 		}
 	}
 	x->data = esp;
-	x->props.trailer_len = esp4_get_max_size(x, 0) - x->props.header_len;
+	align = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
+	if (esp->conf.padlen)
+		align = ALIGN(align, esp->conf.padlen);
+	x->props.trailer_len = align - 1 + esp->auth.icv_trunc_len;
 	return 0;
 
 error:

@@ -402,7 +404,7 @@ static struct xfrm_type esp_type =
 	.proto	     	= IPPROTO_ESP,
 	.init_state	= esp_init_state,
 	.destructor	= esp_destroy,
-	.get_max_size	= esp4_get_max_size,
+	.get_mtu	= esp4_get_mtu,
 	.input		= esp_input,
 	.output		= esp_output
 };
net/ipv6/esp6.c +11 -14
index a278d5e..b8d0a05 100644
@@ -222,22 +222,19 @@ out:
 	return ret;
 }
 
-static u32 esp6_get_max_size(struct xfrm_state *x, int mtu)
+static u32 esp6_get_mtu(struct xfrm_state *x, int mtu)
 {
 	struct esp_data *esp = x->data;
-	u32 blksize = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
-
-	if (x->props.mode) {
-		mtu = ALIGN(mtu + 2, blksize);
-	} else {
-		/* The worst case. */
-		u32 padsize = ((blksize - 1) & 7) + 1;
-		mtu = ALIGN(mtu + 2, padsize) + blksize - padsize;
-	}
-	if (esp->conf.padlen)
-		mtu = ALIGN(mtu, esp->conf.padlen);
+	u32 align = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
+
+	if (esp->conf.padlen > align)
+		align = esp->conf.padlen;
+	
+	mtu -= x->props.header_len + esp->auth.icv_trunc_len;
+	mtu &= ~(align - 1);
+	mtu -= 2;
 
-	return mtu + x->props.header_len + esp->auth.icv_trunc_len;
+	return mtu;
 }
 
 static void esp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,

@@ -363,7 +360,7 @@ static struct xfrm_type esp6_type =
 	.proto	     	= IPPROTO_ESP,
 	.init_state	= esp6_init_state,
 	.destructor	= esp6_destroy,
-	.get_max_size	= esp6_get_max_size,
+	.get_mtu	= esp6_get_mtu,
 	.input		= esp6_input,
 	.output		= esp6_output
 };
net/xfrm/xfrm_state.c +8 -28
index 0021aad..39d9169 100644
@@ -1129,37 +1129,17 @@ void xfrm_state_delete_tunnel(struct xfr
 }
 EXPORT_SYMBOL(xfrm_state_delete_tunnel);
 
-/*
- * This function is NOT optimal.  For example, with ESP it will give an
- * MTU that's usually two bytes short of being optimal.  However, it will
- * usually give an answer that's a multiple of 4 provided the input is
- * also a multiple of 4.
- */
 int xfrm_state_mtu(struct xfrm_state *x, int mtu)
 {
-	int res = mtu;
-
-	res -= x->props.header_len;
-
-	for (;;) {
-		int m = res;
-
-		if (m < 68)
-			return 68;
-
-		spin_lock_bh(&x->lock);
-		if (x->km.state == XFRM_STATE_VALID &&
-		    x->type && x->type->get_max_size)
-			m = x->type->get_max_size(x, m);
-		else
-			m += x->props.header_len;
-		spin_unlock_bh(&x->lock);
-
-		if (m <= mtu)
-			break;
-		res -= (m - mtu);
-	}
+	int res;
 
+	spin_lock_bh(&x->lock);
+	if (x->km.state == XFRM_STATE_VALID &&
+	    x->type && x->type->get_mtu)
+		res = x->type->get_mtu(x, mtu);
+	else
+		res = mtu;
+	spin_unlock_bh(&x->lock);
 	return res;
 }
author
#1 lore raw
Quoted 25 lines · Patrick McHardy

int xfrm_state_mtu(struct xfrm_state *x, int mtu) {

(quoted diff) +1 -22
-	int res = mtu;
-
-	res -= x->props.header_len;
-
-	for (;;) {
-		int m = res;
-
-		if (m < 68)
-			return 68;
-
-		spin_lock_bh(&x->lock);
-		if (x->km.state == XFRM_STATE_VALID &&
-		    x->type && x->type->get_max_size)
-			m = x->type->get_max_size(x, m);
-		else
-			m += x->props.header_len;
-		spin_unlock_bh(&x->lock);
-
-		if (m <= mtu)
-			break;
-		res -= (m - mtu);
-	}
+	int res;

Review

Quoted 10 lines
(quoted diff) +7 -0
 
+	spin_lock_bh(&x->lock);
+	if (x->km.state == XFRM_STATE_VALID &&
+	    x->type && x->type->get_mtu)
+		res = x->type->get_mtu(x, mtu);
+	else
+		res = mtu;
+	spin_unlock_bh(&x->lock);
 	return res;
 }

That broke estimation for AH. This one should be fine.

[XFRM]: Improve MTU estimation

Replace the probing based MTU estimation, which usually takes 2-3 iterations to find a fitting value and may underestimate the MTU, by an exact calculation.

Signed-off-by Patrick McHardy <[email protected]>

---
commit 503d86d9420b2c21121a5c2cbda9e42cc559031f
tree a65939379cd3f200070a784fb079854270d26940
parent a205729e2cd8e51257cd0ea738524c64da99b9e0
author Patrick McHardy <[email protected]> Fri, 04 Aug 2006 11:27:20 +0200 committer Patrick McHardy <[email protected]> Fri, 04 Aug 2006 11:27:20 +0200

 include/net/xfrm.h    |    3 +--
 net/ipv4/esp4.c       |   28 +++++++++++++++-------------
 net/ipv6/esp6.c       |   25 +++++++++++--------------
 net/xfrm/xfrm_state.c |   37 ++++++++++---------------------------
 4 files changed, 37 insertions(+), 56 deletions(-)
include/net/xfrm.h +1 -2
index 9c5ee9f..ea1b028 100644
@@ -262,8 +262,7 @@ struct xfrm_type
 	void			(*destructor)(struct xfrm_state *);
 	int			(*input)(struct xfrm_state *, struct sk_buff *skb);
 	int			(*output)(struct xfrm_state *, struct sk_buff *pskb);
-	/* Estimate maximal size of result of transformation of a dgram */
-	u32			(*get_max_size)(struct xfrm_state *, int size);
+	u32			(*get_mtu)(struct xfrm_state *, int size);
 };
 
 extern int xfrm_register_type(struct xfrm_type *type, unsigned short family);
net/ipv4/esp4.c +15 -13
index fc2f8ce..5393dc2 100644
@@ -251,21 +251,19 @@ out:
 	return -EINVAL;
 }
 
-static u32 esp4_get_max_size(struct xfrm_state *x, int mtu)
+static u32 esp4_get_mtu(struct xfrm_state *x, int mtu)
 {
 	struct esp_data *esp = x->data;
-	u32 blksize = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
+	u32 align = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
 
-	if (x->props.mode) {
-		mtu = ALIGN(mtu + 2, blksize);
-	} else {
-		/* The worst case. */
-		mtu = ALIGN(mtu + 2, 4) + blksize - 4;
-	}
-	if (esp->conf.padlen)
-		mtu = ALIGN(mtu, esp->conf.padlen);
+	if (esp->conf.padlen > align)
+		align = esp->conf.padlen;
 
-	return mtu + x->props.header_len + esp->auth.icv_trunc_len;
+	mtu -= x->props.header_len + esp->auth.icv_trunc_len;
+	mtu &= ~(align - 1);
+	mtu -= 2;
+
+	return mtu;
 }
 
 static void esp4_err(struct sk_buff *skb, u32 info)

@@ -307,6 +305,7 @@ static void esp_destroy(struct xfrm_stat
 static int esp_init_state(struct xfrm_state *x)
 {
 	struct esp_data *esp = NULL;
+	u32 align;
 
 	/* null auth and encryption can have zero length keys */
 	if (x->aalg) {

@@ -385,7 +384,10 @@ static int esp_init_state(struct xfrm_st
 		}
 	}
 	x->data = esp;
-	x->props.trailer_len = esp4_get_max_size(x, 0) - x->props.header_len;
+	align = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
+	if (esp->conf.padlen)
+		align = ALIGN(align, esp->conf.padlen);
+	x->props.trailer_len = align - 1 + esp->auth.icv_trunc_len;
 	return 0;
 
 error:

@@ -402,7 +404,7 @@ static struct xfrm_type esp_type =
 	.proto	     	= IPPROTO_ESP,
 	.init_state	= esp_init_state,
 	.destructor	= esp_destroy,
-	.get_max_size	= esp4_get_max_size,
+	.get_mtu	= esp4_get_mtu,
 	.input		= esp_input,
 	.output		= esp_output
 };
net/ipv6/esp6.c +11 -14
index a278d5e..b8d0a05 100644
@@ -222,22 +222,19 @@ out:
 	return ret;
 }
 
-static u32 esp6_get_max_size(struct xfrm_state *x, int mtu)
+static u32 esp6_get_mtu(struct xfrm_state *x, int mtu)
 {
 	struct esp_data *esp = x->data;
-	u32 blksize = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
-
-	if (x->props.mode) {
-		mtu = ALIGN(mtu + 2, blksize);
-	} else {
-		/* The worst case. */
-		u32 padsize = ((blksize - 1) & 7) + 1;
-		mtu = ALIGN(mtu + 2, padsize) + blksize - padsize;
-	}
-	if (esp->conf.padlen)
-		mtu = ALIGN(mtu, esp->conf.padlen);
+	u32 align = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
+
+	if (esp->conf.padlen > align)
+		align = esp->conf.padlen;
+	
+	mtu -= x->props.header_len + esp->auth.icv_trunc_len;
+	mtu &= ~(align - 1);
+	mtu -= 2;
 
-	return mtu + x->props.header_len + esp->auth.icv_trunc_len;
+	return mtu;
 }
 
 static void esp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,

@@ -363,7 +360,7 @@ static struct xfrm_type esp6_type =
 	.proto	     	= IPPROTO_ESP,
 	.init_state	= esp6_init_state,
 	.destructor	= esp6_destroy,
-	.get_max_size	= esp6_get_max_size,
+	.get_mtu	= esp6_get_mtu,
 	.input		= esp6_input,
 	.output		= esp6_output
 };
net/xfrm/xfrm_state.c +10 -27
index 0021aad..7b0b100 100644
@@ -1129,37 +1129,20 @@ void xfrm_state_delete_tunnel(struct xfr
 }
 EXPORT_SYMBOL(xfrm_state_delete_tunnel);
 
-/*
- * This function is NOT optimal.  For example, with ESP it will give an
- * MTU that's usually two bytes short of being optimal.  However, it will
- * usually give an answer that's a multiple of 4 provided the input is
- * also a multiple of 4.
- */
 int xfrm_state_mtu(struct xfrm_state *x, int mtu)
 {
-	int res = mtu;
-
-	res -= x->props.header_len;
-
-	for (;;) {
-		int m = res;
-
-		if (m < 68)
-			return 68;
-
-		spin_lock_bh(&x->lock);
-		if (x->km.state == XFRM_STATE_VALID &&
-		    x->type && x->type->get_max_size)
-			m = x->type->get_max_size(x, m);
-		else
-			m += x->props.header_len;
-		spin_unlock_bh(&x->lock);
+	int res;
 
-		if (m <= mtu)
-			break;
-		res -= (m - mtu);
-	}
+	spin_lock_bh(&x->lock);
+	if (x->km.state == XFRM_STATE_VALID &&
+	    x->type && x->type->get_mtu)
+		res = x->type->get_mtu(x, mtu);
+	else
+		res = mtu - x->props.header_len;
+	spin_unlock_bh(&x->lock);
 
+	if (res < 68)
+		res = 68;
 	return res;
 }
maintainer
#2 lore raw
Quoted 6 lines · Patrick McHardy

[XFRM]: Improve MTU estimation

Replace the probing based MTU estimation, which usually takes 2-3 iterations to find a fitting value and may underestimate the MTU, by an exact calculation.

Thanks Patrick, this is definitely the better way to do it.

Quoted 14 lines
net/ipv4/esp4.c +2 -9
-static u32 esp4_get_max_size(struct xfrm_state *x, int mtu)
+static u32 esp4_get_mtu(struct xfrm_state *x, int mtu)
 {
 	struct esp_data *esp = x->data;
-	u32 blksize = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
+	u32 align = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
 
-	if (x->props.mode) {
-		mtu = ALIGN(mtu + 2, blksize);
-	} else {
-		/* The worst case. */
-		mtu = ALIGN(mtu + 2, 4) + blksize - 4;
-	}
-	if (esp->conf.padlen)

net/ipv4/esp4.c out: · line 266

Quoted 10 lines
net/ipv4/esp4.c +7 -2
-		mtu = ALIGN(mtu, esp->conf.padlen);
+	if (esp->conf.padlen > align)
+		align = esp->conf.padlen;
 
-	return mtu + x->props.header_len + esp->auth.icv_trunc_len;
+	mtu -= x->props.header_len + esp->auth.icv_trunc_len;
+	mtu &= ~(align - 1);
+	mtu -= 2;
+
+	return mtu;

I haven't actually done the math, but I don't think this can be right from a quick look. The reason is that transport mode is fundamentally different from tunnel mode in that the IP options are not encrypted and therefore do not contribute to the encryption block padding.

So as the code doesn't distinguish between transport mode and tunnel mode, you might be producing an overestimate for transport mode.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#3 lore raw
Quoted 33 lines · Herbert Xu
Quoted 14 lines · Patrick McHardy
net/ipv4/esp4.c +1 -1
-static u32 esp4_get_max_size(struct xfrm_state *x, int mtu)
+static u32 esp4_get_mtu(struct xfrm_state *x, int mtu)

{
	struct esp_data *esp = x->data;
net/ipv4/esp4.c +1 -8
-	u32 blksize = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
+	u32 align = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
 
-	if (x->props.mode) {
-		mtu = ALIGN(mtu + 2, blksize);
-	} else {
-		/* The worst case. */
-		mtu = ALIGN(mtu + 2, 4) + blksize - 4;
-	}
-	if (esp->conf.padlen)

net/ipv4/esp4.c out: · line 266

Quoted 10 lines
net/ipv4/esp4.c +7 -2
-		mtu = ALIGN(mtu, esp->conf.padlen);
+	if (esp->conf.padlen > align)
+		align = esp->conf.padlen;
 
-	return mtu + x->props.header_len + esp->auth.icv_trunc_len;
+	mtu -= x->props.header_len + esp->auth.icv_trunc_len;
+	mtu &= ~(align - 1);
+	mtu -= 2;
+
+	return mtu;

I haven't actually done the math, but I don't think this can be right from a quick look. The reason is that transport mode is fundamentally different from tunnel mode in that the IP options are not encrypted and therefore do not contribute to the encryption block padding.

Quoted 2 lines

So as the code doesn't distinguish between transport mode and tunnel mode, you might be producing an overestimate for transport mode.

I was wondering why the old code distinguished between transport mode and tunnel mode, I couldn't spot anything that would be affected. I'll look into the transport mode case again.

maintainer
#4 lore raw
Quoted 4 lines · Patrick McHardy

I was wondering why the old code distinguished between transport mode and tunnel mode, I couldn't spot anything that would be affected. I'll look into the transport mode case again.

The problem is basically you don't know a priori the size of the IP options. So you assume the worst case where the IP options causes the largest amount of padding for encryption (IP option length itself must be a multiple of 4, so for a block size of 8 the worst is 4, and the worst is 12 for a block size of 16).

Of course it gets hairier if you have ESP padding. I'm not even sure if the current code gets that right.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#5 lore raw
Quoted 7 lines · Herbert Xu
Quoted 3 lines · Patrick McHardy

I was wondering why the old code distinguished between transport mode and tunnel mode, I couldn't spot anything that would be affected. I'll look into the transport mode case again.

Quoted 8 lines

The problem is basically you don't know a priori the size of the IP options. So you assume the worst case where the IP options causes the largest amount of padding for encryption (IP option length itself must be a multiple of 4, so for a block size of 8 the worst is 4, and the worst is 12 for a block size of 16).

Of course it gets hairier if you have ESP padding. I'm not even sure if the current code gets that right.

Unless I'm missing something, the padding caused by IP options is always less than the worst case that can happen anyway (max(block size, padlen)-1), so it can simply be ignored.

maintainer
#6 lore raw
Quoted 7 lines · Patrick McHardy
Quoted 3 lines

Of course it gets hairier if you have ESP padding. I'm not even sure if the current code gets that right.

Unless I'm missing something, the padding caused by IP options is always less than the worst case that can happen anyway (max(block size, padlen)-1), so it can simply be ignored.

Are you talking about the ESP padding case, or transport mode in general?

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#7 lore raw
Quoted 5 lines · Herbert Xu
Quoted 3 lines · Patrick McHardy
Quoted 2 lines

Of course it gets hairier if you have ESP padding. I'm not even sure if the current code gets that right.

Quoted 7 lines
Quoted 3 lines

Unless I'm missing something, the padding caused by IP options is always less than the worst case that can happen anyway (max(block size, padlen)-1), so it can simply be ignored.

Are you talking about the ESP padding case, or transport mode in general?

Transport mode in general.

maintainer
#8 lore raw
In reply to Herbert Xu
Quoted 3 lines · herbert

Are you talking about the ESP padding case, or transport mode in general?

I've reread your patches and your handling of ESP padding is spot on. It's anyone's guess whether the current code gets it right or not :)

However, I believe that the transport mode handling does run into problems with IP options. Basically, your calculation returns a length that is a precise multiple of block size minus 2.

Now imagine that we have 4 bytes of IP options, given a block size of 8 taking away 4 bytes from inside the encrypted area simply causes them to be padded out so the encrypted length does not change. However, we have to put those 4 bytes outside the encrypted area. The problem is that we may not have those 4 bytes given the MTU.

For a standard 1500 MTU and the block size of 8 it just happens that we do have 4 bytes (because 1500 % 8 == 4). However, this breaks down if you start with say 1480 (standard MTU for 1500 with IPIP on the outside).

You run into problems even with 1500 if your block size happens to be 16 (AES).

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#9 lore raw
Quoted 13 lines · Herbert Xu

I've reread your patches and your handling of ESP padding is spot on. It's anyone's guess whether the current code gets it right or not :)

However, I believe that the transport mode handling does run into problems with IP options. Basically, your calculation returns a length that is a precise multiple of block size minus 2.

Now imagine that we have 4 bytes of IP options, given a block size of 8 taking away 4 bytes from inside the encrypted area simply causes them to be padded out so the encrypted length does not change. However, we have to put those 4 bytes outside the encrypted area. The problem is that we may not have those 4 bytes given the MTU.

Quoted 7 lines

For a standard 1500 MTU and the block size of 8 it just happens that we do have 4 bytes (because 1500 % 8 == 4). However, this breaks down if you start with say 1480 (standard MTU for 1500 with IPIP on the outside).

You run into problems even with 1500 if your block size happens to be 16 (AES).

Now I get it, thanks :) I missed that the IP header isn't part of the length when it is aligned. So the worst-case increases by block-size
- 4 (- 8 for IPv6). How does this look?

author
#10 lore raw
Quoted 3 lines · Patrick McHardy

Now I get it, thanks :) I missed that the IP header isn't part of the length when it is aligned. So the worst-case increases by block-size
- 4 (- 8 for IPv6). How does this look?

Forgot the patch ..

include/net/xfrm.h +1 -2
index 9c5ee9f..ea1b028 100644
@@ -262,8 +262,7 @@ struct xfrm_type
 	void			(*destructor)(struct xfrm_state *);
 	int			(*input)(struct xfrm_state *, struct sk_buff *skb);
 	int			(*output)(struct xfrm_state *, struct sk_buff *pskb);
-	/* Estimate maximal size of result of transformation of a dgram */
-	u32			(*get_max_size)(struct xfrm_state *, int size);
+	u32			(*get_mtu)(struct xfrm_state *, int size);
 };
 
 extern int xfrm_register_type(struct xfrm_type *type, unsigned short family);
net/ipv4/esp4.c +18 -12
index fc2f8ce..1ce06cc 100644
@@ -251,21 +251,23 @@ out:
 	return -EINVAL;
 }
 
-static u32 esp4_get_max_size(struct xfrm_state *x, int mtu)
+static u32 esp4_get_mtu(struct xfrm_state *x, int mtu)
 {
 	struct esp_data *esp = x->data;
 	u32 blksize = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
+	u32 align;
 
-	if (x->props.mode) {
-		mtu = ALIGN(mtu + 2, blksize);
-	} else {
-		/* The worst case. */
-		mtu = ALIGN(mtu + 2, 4) + blksize - 4;
-	}
-	if (esp->conf.padlen)
-		mtu = ALIGN(mtu, esp->conf.padlen);
+	align = blksize;
+	if (esp->conf.padlen > align)
+		align = esp->conf.padlen;
 
-	return mtu + x->props.header_len + esp->auth.icv_trunc_len;
+	mtu -= x->props.header_len + esp->auth.icv_trunc_len;
+	mtu &= ~(align - 1);
+	mtu -= 2;
+	if (!x->props.mode)
+		mtu -= blksize - 4;
+
+	return mtu;
 }
 
 static void esp4_err(struct sk_buff *skb, u32 info)

@@ -307,6 +309,7 @@ static void esp_destroy(struct xfrm_stat
 static int esp_init_state(struct xfrm_state *x)
 {
 	struct esp_data *esp = NULL;
+	u32 align;
 
 	/* null auth and encryption can have zero length keys */
 	if (x->aalg) {

@@ -385,7 +388,10 @@ static int esp_init_state(struct xfrm_st
 		}
 	}
 	x->data = esp;
-	x->props.trailer_len = esp4_get_max_size(x, 0) - x->props.header_len;
+	align = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
+	if (esp->conf.padlen)
+		align = ALIGN(align, esp->conf.padlen);
+	x->props.trailer_len = align - 1 + esp->auth.icv_trunc_len;
 	return 0;
 
 error:

@@ -402,7 +408,7 @@ static struct xfrm_type esp_type =
 	.proto	     	= IPPROTO_ESP,
 	.init_state	= esp_init_state,
 	.destructor	= esp_destroy,
-	.get_max_size	= esp4_get_max_size,
+	.get_mtu	= esp4_get_mtu,
 	.input		= esp_input,
 	.output		= esp_output
 };
net/ipv6/esp6.c +15 -14
index a278d5e..0c9c159 100644
@@ -222,22 +222,23 @@ out:
 	return ret;
 }
 
-static u32 esp6_get_max_size(struct xfrm_state *x, int mtu)
+static u32 esp6_get_mtu(struct xfrm_state *x, int mtu)
 {
 	struct esp_data *esp = x->data;
 	u32 blksize = ALIGN(crypto_tfm_alg_blocksize(esp->conf.tfm), 4);
-
-	if (x->props.mode) {
-		mtu = ALIGN(mtu + 2, blksize);
-	} else {
-		/* The worst case. */
-		u32 padsize = ((blksize - 1) & 7) + 1;
-		mtu = ALIGN(mtu + 2, padsize) + blksize - padsize;
-	}
-	if (esp->conf.padlen)
-		mtu = ALIGN(mtu, esp->conf.padlen);
-
-	return mtu + x->props.header_len + esp->auth.icv_trunc_len;
+	u32 align;
+
+	align = blksize;
+	if (esp->conf.padlen > align)
+		align = esp->conf.padlen;
+	
+	mtu -= x->props.header_len + esp->auth.icv_trunc_len;
+	mtu &= ~(align - 1);
+	mtu -= 2;
+	if (!x->props.mode && blksize >= 8)
+		mtu -= blksize - 8;
+
+	return mtu;
 }
 
 static void esp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,

@@ -363,7 +364,7 @@ static struct xfrm_type esp6_type =
 	.proto	     	= IPPROTO_ESP,
 	.init_state	= esp6_init_state,
 	.destructor	= esp6_destroy,
-	.get_max_size	= esp6_get_max_size,
+	.get_mtu	= esp6_get_mtu,
 	.input		= esp6_input,
 	.output		= esp6_output
 };
net/xfrm/xfrm_state.c +10 -27
index 0021aad..7b0b100 100644
@@ -1129,37 +1129,20 @@ void xfrm_state_delete_tunnel(struct xfr
 }
 EXPORT_SYMBOL(xfrm_state_delete_tunnel);
 
-/*
- * This function is NOT optimal.  For example, with ESP it will give an
- * MTU that's usually two bytes short of being optimal.  However, it will
- * usually give an answer that's a multiple of 4 provided the input is
- * also a multiple of 4.
- */
 int xfrm_state_mtu(struct xfrm_state *x, int mtu)
 {
-	int res = mtu;
-
-	res -= x->props.header_len;
-
-	for (;;) {
-		int m = res;
-
-		if (m < 68)
-			return 68;
-
-		spin_lock_bh(&x->lock);
-		if (x->km.state == XFRM_STATE_VALID &&
-		    x->type && x->type->get_max_size)
-			m = x->type->get_max_size(x, m);
-		else
-			m += x->props.header_len;
-		spin_unlock_bh(&x->lock);
+	int res;
 
-		if (m <= mtu)
-			break;
-		res -= (m - mtu);
-	}
+	spin_lock_bh(&x->lock);
+	if (x->km.state == XFRM_STATE_VALID &&
+	    x->type && x->type->get_mtu)
+		res = x->type->get_mtu(x, mtu);
+	else
+		res = mtu - x->props.header_len;
+	spin_unlock_bh(&x->lock);
 
+	if (res < 68)
+		res = 68;
 	return res;
 }
maintainer
#12 lore raw
In reply to Patrick McHardy
Quoted 4 lines · Patrick McHardy

Now I get it, thanks :) I missed that the IP header isn't part of the length when it is aligned. So the worst-case increases by block-size
- 4 (- 8 for IPv6). How does this look?

That should work.

If you want to be fancy you can also take any bits you shaved off outside the encrypted block into account. For example, in the 1500 case we have to shave off 4 bytes to make it a multiple of 8, therefore there is no extra overhead for transport mode at all.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
maintainer
#11 lore raw
In reply to Patrick McHardy

net/ipv4/esp4.c out: · line 268

Quoted 3 lines · Patrick McHardy
net/ipv4/esp4.c +2 -0
+	if (!x->props.mode)
+		mtu -= blksize - 4;

You probably want to check for overflows here since mtu is unsigned.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt