[PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Simon Arlott]

The check for data only needs to apply where the packet length
could be increased by adding the MSS option. (The MSS option
itself applies to the sender's maximum receive size which is
not relevant to any data in its own packet.)

This moves the check for (header size != packet size) to after
attempting to modify an existing MSS option. Another check is
needed before looking through the header to ensure it doesn't
claim to be larger than the packet size.

The ERROR level printk() is also removed as it can be triggered
by remote hosts and is not useful:
[4941777.937417] xt_TCPMSS: bad length (40 bytes)
[4941782.409724] xt_TCPMSS: bad length (40 bytes)
[4941790.762332] xt_TCPMSS: bad length (40 bytes)

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 net/netfilter/xt_TCPMSS.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
index eda64c1..76f92bf 100644
--- a/net/netfilter/xt_TCPMSS.c
+++ b/net/netfilter/xt_TCPMSS.c
@@ -60,17 +60,9 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 	tcplen = skb->len - tcphoff;
 	tcph = (struct tcphdr *)(skb_network_header(skb) + tcphoff);
 
-	/* Since it passed flags test in tcp match, we know it is is
-	   not a fragment, and has data >= tcp header length.  SYN
-	   packets should not contain data: if they did, then we risk
-	   running over MTU, sending Frag Needed and breaking things
-	   badly. --RR */
-	if (tcplen != tcph->doff*4) {
-		if (net_ratelimit())
-			printk(KERN_ERR "xt_TCPMSS: bad length (%u bytes)\n",
-			       skb->len);
+	/* Header cannot be larger than the packet */
+	if (tcplen < tcph->doff*4)
 		return -1;
-	}
 
 	if (info->mss == XT_TCPMSS_CLAMP_PMTU) {
 		if (dst_mtu(skb_dst(skb)) <= minlen) {
@@ -115,6 +107,15 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 		}
 	}
 
+	/* Since it passed flags test in tcp match, we know it is
+	   not a fragment, and has data >= tcp header length.  SYN
+	   packets should not contain data: if they did, then we risk
+	   running over MTU, sending Frag Needed and breaking things
+	   badly. --RR
+	*/
+	if (tcplen > tcph->doff*4)
+		return -1;
+
 	/*
 	 * MSS Option not found ?! add it..
 	 */
-- 
1.6.3.3

-- 
Simon Arlott

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[William Allen Simpson]

Simon Arlott wrote:
> The check for data only needs to apply where the packet length
> could be increased by adding the MSS option. (The MSS option
> itself applies to the sender's maximum receive size which is
> not relevant to any data in its own packet.)
> 
> This moves the check for (header size != packet size) to after
> attempting to modify an existing MSS option. Another check is
> needed before looking through the header to ensure it doesn't
> claim to be larger than the packet size.
> 
What's the path from tcp_v[4,6]_rcv() to these tests?

1) Header larger than the packet is already tested in about 5 places,
and my patch "tcp: harmonize tcp_vx_rcv header length assumptions"
tries to get them all down to just *one* test.

2) There certainly *can* be data on SYN.  That code is already in
2.6.33....


> The ERROR level printk() is also removed as it can be triggered
> by remote hosts and is not useful:
> [4941777.937417] xt_TCPMSS: bad length (40 bytes)
> [4941782.409724] xt_TCPMSS: bad length (40 bytes)
> [4941790.762332] xt_TCPMSS: bad length (40 bytes)
> 
> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
> ---
>  net/netfilter/xt_TCPMSS.c |   21 +++++++++++----------
>  1 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
> index eda64c1..76f92bf 100644
> --- a/net/netfilter/xt_TCPMSS.c
> +++ b/net/netfilter/xt_TCPMSS.c
> @@ -60,17 +60,9 @@ tcpmss_mangle_packet(struct sk_buff *skb,
>  	tcplen = skb->len - tcphoff;
>  	tcph = (struct tcphdr *)(skb_network_header(skb) + tcphoff);
>  
> -	/* Since it passed flags test in tcp match, we know it is is
> -	   not a fragment, and has data >= tcp header length.  SYN
> -	   packets should not contain data: if they did, then we risk
> -	   running over MTU, sending Frag Needed and breaking things
> -	   badly. --RR */
> -	if (tcplen != tcph->doff*4) {
> -		if (net_ratelimit())
> -			printk(KERN_ERR "xt_TCPMSS: bad length (%u bytes)\n",
> -			       skb->len);
> +	/* Header cannot be larger than the packet */
> +	if (tcplen < tcph->doff*4)
>  		return -1;
> -	}
>  
>  	if (info->mss == XT_TCPMSS_CLAMP_PMTU) {
>  		if (dst_mtu(skb_dst(skb)) <= minlen) {
> @@ -115,6 +107,15 @@ tcpmss_mangle_packet(struct sk_buff *skb,
>  		}
>  	}
>  
> +	/* Since it passed flags test in tcp match, we know it is
> +	   not a fragment, and has data >= tcp header length.  SYN
> +	   packets should not contain data: if they did, then we risk
> +	   running over MTU, sending Frag Needed and breaking things
> +	   badly. --RR
> +	*/
> +	if (tcplen > tcph->doff*4)
> +		return -1;
> +
>  	/*
>  	 * MSS Option not found ?! add it..
>  	 */

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Patrick McHardy]

William Allen Simpson wrote:
> Simon Arlott wrote:
>> The check for data only needs to apply where the packet length
>> could be increased by adding the MSS option. (The MSS option
>> itself applies to the sender's maximum receive size which is
>> not relevant to any data in its own packet.)
>>
>> This moves the check for (header size != packet size) to after
>> attempting to modify an existing MSS option. Another check is
>> needed before looking through the header to ensure it doesn't
>> claim to be larger than the packet size.
>>
> What's the path from tcp_v[4,6]_rcv() to these tests?
> 
> 1) Header larger than the packet is already tested in about 5 places,
> and my patch "tcp: harmonize tcp_vx_rcv header length assumptions"
> tries to get them all down to just *one* test.

We're talking about a netfilter module here, which has to deal
with forwarded traffic and can only rely on the IP header checks
done in ip_rcv().

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Simon Arlott]

On Tue, January 19, 2010 09:30, Patrick McHardy wrote:
> William Allen Simpson wrote:
>> Simon Arlott wrote:
>>> This moves the check for (header size != packet size) to after
>>> attempting to modify an existing MSS option. Another check is
>>> needed before looking through the header to ensure it doesn't
>>> claim to be larger than the packet size.
>>>
>> What's the path from tcp_v[4,6]_rcv() to these tests?
>>
>> 1) Header larger than the packet is already tested in about 5 places,
>> and my patch "tcp: harmonize tcp_vx_rcv header length assumptions"
>> tries to get them all down to just *one* test.
>
> We're talking about a netfilter module here, which has to deal
> with forwarded traffic and can only rely on the IP header checks
> done in ip_rcv().

My gateway (where these error messages occur) is running 2.6.29,
and skb->len (from the prink) is 40 bytes.

If this is 20 (IPv4 Header) + 20 (TCP Header) = 40 bytes, then there
is no data and the header offset is wrong so it hasn't been checked.

-- 
Simon Arlott

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Patrick McHardy]

Simon Arlott wrote:
> On Tue, January 19, 2010 09:30, Patrick McHardy wrote:
>> William Allen Simpson wrote:
>>> Simon Arlott wrote:
>>>> This moves the check for (header size != packet size) to after
>>>> attempting to modify an existing MSS option. Another check is
>>>> needed before looking through the header to ensure it doesn't
>>>> claim to be larger than the packet size.
>>>>
>>> What's the path from tcp_v[4,6]_rcv() to these tests?
>>>
>>> 1) Header larger than the packet is already tested in about 5 places,
>>> and my patch "tcp: harmonize tcp_vx_rcv header length assumptions"
>>> tries to get them all down to just *one* test.
>> We're talking about a netfilter module here, which has to deal
>> with forwarded traffic and can only rely on the IP header checks
>> done in ip_rcv().
> 
> My gateway (where these error messages occur) is running 2.6.29,
> and skb->len (from the prink) is 40 bytes.
> 
> If this is 20 (IPv4 Header) + 20 (TCP Header) = 40 bytes, then there
> is no data and the header offset is wrong so it hasn't been checked.

That's odd. If the packet is really only 40 bytes large, then there
are no TCP options, so your patch shouldn't have any effect.

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Simon Arlott]

On Tue, January 19, 2010 09:17, William Allen Simpson wrote:
> Simon Arlott wrote:
>> The check for data only needs to apply where the packet length
>> could be increased by adding the MSS option. (The MSS option
>> itself applies to the sender's maximum receive size which is
>> not relevant to any data in its own packet.)
>>
>> This moves the check for (header size != packet size) to after
>> attempting to modify an existing MSS option. Another check is
>> needed before looking through the header to ensure it doesn't
>> claim to be larger than the packet size.
>>
>
> 2) There certainly *can* be data on SYN.  That code is already in
> 2.6.33....

I could change the comment too, but the same logic applies when
there is data and no MSS option - the packet can't be increased
in size if it would then exceed 576 bytes and/or the destination
MTU.

If it's possible to know that the packet can have an additional
option added without exceeding MTU then this could be changed.
The data part would need to be moved to make space at the end of
the header.

-- 
Simon Arlott

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[William Allen Simpson]

Simon Arlott wrote:
> On Tue, January 19, 2010 09:17, William Allen Simpson wrote:
>> 2) There certainly *can* be data on SYN.  That code is already in
>> 2.6.33....
> 
> I could change the comment too, but the same logic applies when
> there is data and no MSS option - the packet can't be increased
> in size if it would then exceed 576 bytes and/or the destination
> MTU.
> 
Please change the comment.

If there is no MSS option, it should *not* be added, under *ANY*
circumstances.  That violates the end-to-end arguments (some call
them principles).

MSS isn't about the _destination_ MTU, it's about the *source*.
If you cannot guarantee you know the source MTU, there's no basis
for deciding the MSS.

While I understand that sometimes it's useful to reduce (never,
NEVER, *NEVER* increase) the MSS as a packet goes into a tunnel
(because there are problems in some NAT'd networks with determining
Path MTU via ICMP), I'm not aware of any circumstance where the MSS
would need to be reduced below 536.

I'm having some difficulty figuring out how this code originated --
with a nice log entry explaining the exact manufacturer's device
and network topology that the contributor had in mind?


> If it's possible to know that the packet can have an additional
> option added without exceeding MTU then this could be changed.
> The data part would need to be moved to make space at the end of
> the header.
> 
No options should be added to TCP in a router -- ever!

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Simon Arlott]

On Tue, January 19, 2010 15:44, William Allen Simpson wrote:
> Simon Arlott wrote:
>> On Tue, January 19, 2010 09:17, William Allen Simpson wrote:
>> I could change the comment too, but the same logic applies when
>> there is data and no MSS option - the packet can't be increased
>> in size if it would then exceed 576 bytes and/or the destination
>> MTU.
>>
> Please change the comment.

I've made a new version of the patch which I'll be able to test
tonight.

> If there is no MSS option, it should *not* be added, under *ANY*
> circumstances.  That violates the end-to-end arguments (some call
> them principles).

Agreed. The added MSS is likely to be larger than 536 too... I've
removed this code.

> MSS isn't about the _destination_ MTU, it's about the *source*.
> If you cannot guarantee you know the source MTU, there's no basis
> for deciding the MSS.

I was referring to the SYN packet itself. It wouldn't always be
possible to add an option without exceeding the MTU of that packet's
destination if it had data.


On 19/01/10 12:53, Patrick McHardy wrote:
> Simon Arlott wrote:
>> If this is 20 (IPv4 Header) + 20 (TCP Header) = 40 bytes, then
>> there is no data and the header offset is wrong so it hasn't been
>> checked.
>
> That's odd. If the packet is really only 40 bytes large, then there
> are no TCP options, so your patch shouldn't have any effect.

Except to remove the printk which fills up my serial console (because
the header offset is wrong).

-- 
Simon Arlott

[PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Simon Arlott]

The TCPMSS target is dropping SYN packets where:
  1) There is data, or
  2) The data offset makes the TCP header larger than
  the packet.

Both of these result in an error level printk.

This change fixes the drop of SYN packets with data
(because the MSS option can safely be modified) and
passes packets with no MSS option instead of adding
one (which is not valid).

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
Tested mangle OUTPUT rule with IPv4 and IPv6.
SYN with data not tested.

 net/netfilter/xt_TCPMSS.c |   82 +++++++-------------------------------------
 1 files changed, 13 insertions(+), 69 deletions(-)

diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
index eda64c1..3648761 100644
--- a/net/netfilter/xt_TCPMSS.c
+++ b/net/netfilter/xt_TCPMSS.c
@@ -41,7 +41,7 @@ optlen(const u_int8_t *opt, unsigned int offset)
 		return opt[offset+1];
 }
 
-static int
+static unsigned int
 tcpmss_mangle_packet(struct sk_buff *skb,
 		     const struct xt_tcpmss_info *info,
 		     unsigned int in_mtu,
@@ -50,27 +50,18 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 {
 	struct tcphdr *tcph;
 	unsigned int tcplen, i;
-	__be16 oldval;
 	u16 newmss;
 	u8 *opt;
 
 	if (!skb_make_writable(skb, skb->len))
-		return -1;
+		return NF_DROP;
 
 	tcplen = skb->len - tcphoff;
 	tcph = (struct tcphdr *)(skb_network_header(skb) + tcphoff);
 
-	/* Since it passed flags test in tcp match, we know it is is
-	   not a fragment, and has data >= tcp header length.  SYN
-	   packets should not contain data: if they did, then we risk
-	   running over MTU, sending Frag Needed and breaking things
-	   badly. --RR */
-	if (tcplen != tcph->doff*4) {
-		if (net_ratelimit())
-			printk(KERN_ERR "xt_TCPMSS: bad length (%u bytes)\n",
-			       skb->len);
-		return -1;
-	}
+	/* Header cannot be larger than the packet */
+	if (tcplen < tcph->doff*4)
+		return NF_DROP;
 
 	if (info->mss == XT_TCPMSS_CLAMP_PMTU) {
 		if (dst_mtu(skb_dst(skb)) <= minlen) {
@@ -78,13 +69,13 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 				printk(KERN_ERR "xt_TCPMSS: "
 				       "unknown or invalid path-MTU (%u)\n",
 				       dst_mtu(skb_dst(skb)));
-			return -1;
+			return NF_DROP;
 		}
 		if (in_mtu <= minlen) {
 			if (net_ratelimit())
 				printk(KERN_ERR "xt_TCPMSS: unknown or "
 				       "invalid path-MTU (%u)\n", in_mtu);
-			return -1;
+			return NF_DROP;
 		}
 		newmss = min(dst_mtu(skb_dst(skb)), in_mtu) - minlen;
 	} else
@@ -103,7 +94,7 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 			 * on MSS being set correctly.
 			 */
 			if (oldmss <= newmss)
-				return 0;
+				return XT_CONTINUE;
 
 			opt[i+2] = (newmss & 0xff00) >> 8;
 			opt[i+3] = newmss & 0x00ff;
@@ -111,40 +102,12 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 			inet_proto_csum_replace2(&tcph->check, skb,
 						 htons(oldmss), htons(newmss),
 						 0);
-			return 0;
+			return XT_CONTINUE;
 		}
 	}
 
-	/*
-	 * MSS Option not found ?! add it..
-	 */
-	if (skb_tailroom(skb) < TCPOLEN_MSS) {
-		if (pskb_expand_head(skb, 0,
-				     TCPOLEN_MSS - skb_tailroom(skb),
-				     GFP_ATOMIC))
-			return -1;
-		tcph = (struct tcphdr *)(skb_network_header(skb) + tcphoff);
-	}
-
-	skb_put(skb, TCPOLEN_MSS);
-
-	opt = (u_int8_t *)tcph + sizeof(struct tcphdr);
-	memmove(opt + TCPOLEN_MSS, opt, tcplen - sizeof(struct tcphdr));
-
-	inet_proto_csum_replace2(&tcph->check, skb,
-				 htons(tcplen), htons(tcplen + TCPOLEN_MSS), 1);
-	opt[0] = TCPOPT_MSS;
-	opt[1] = TCPOLEN_MSS;
-	opt[2] = (newmss & 0xff00) >> 8;
-	opt[3] = newmss & 0x00ff;
-
-	inet_proto_csum_replace4(&tcph->check, skb, 0, *((__be32 *)opt), 0);
-
-	oldval = ((__be16 *)tcph)[6];
-	tcph->doff += TCPOLEN_MSS/4;
-	inet_proto_csum_replace2(&tcph->check, skb,
-				 oldval, ((__be16 *)tcph)[6], 0);
-	return TCPOLEN_MSS;
+	/* MSS Option not found */
+	return XT_CONTINUE;
 }
 
 static u_int32_t tcpmss_reverse_mtu(const struct sk_buff *skb,
@@ -177,22 +140,11 @@ static unsigned int
 tcpmss_tg4(struct sk_buff *skb, const struct xt_target_param *par)
 {
 	struct iphdr *iph = ip_hdr(skb);
-	__be16 newlen;
-	int ret;
 
-	ret = tcpmss_mangle_packet(skb, par->targinfo,
+	return tcpmss_mangle_packet(skb, par->targinfo,
 				   tcpmss_reverse_mtu(skb, PF_INET),
 				   iph->ihl * 4,
 				   sizeof(*iph) + sizeof(struct tcphdr));
-	if (ret < 0)
-		return NF_DROP;
-	if (ret > 0) {
-		iph = ip_hdr(skb);
-		newlen = htons(ntohs(iph->tot_len) + ret);
-		csum_replace2(&iph->check, iph->tot_len, newlen);
-		iph->tot_len = newlen;
-	}
-	return XT_CONTINUE;
 }
 
 #if defined(CONFIG_IP6_NF_IPTABLES) || defined(CONFIG_IP6_NF_IPTABLES_MODULE)
@@ -202,23 +154,15 @@ tcpmss_tg6(struct sk_buff *skb, const struct xt_target_param *par)
 	struct ipv6hdr *ipv6h = ipv6_hdr(skb);
 	u8 nexthdr;
 	int tcphoff;
-	int ret;
 
 	nexthdr = ipv6h->nexthdr;
 	tcphoff = ipv6_skip_exthdr(skb, sizeof(*ipv6h), &nexthdr);
 	if (tcphoff < 0)
 		return NF_DROP;
-	ret = tcpmss_mangle_packet(skb, par->targinfo,
+	return tcpmss_mangle_packet(skb, par->targinfo,
 				   tcpmss_reverse_mtu(skb, PF_INET6),
 				   tcphoff,
 				   sizeof(*ipv6h) + sizeof(struct tcphdr));
-	if (ret < 0)
-		return NF_DROP;
-	if (ret > 0) {
-		ipv6h = ipv6_hdr(skb);
-		ipv6h->payload_len = htons(ntohs(ipv6h->payload_len) + ret);
-	}
-	return XT_CONTINUE;
 }
 #endif
 
-- 
1.6.3.3

-- 
Simon Arlott

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Jan Engelhardt]

On Wednesday 2010-01-20 22:21, Simon Arlott wrote:

>The TCPMSS target is dropping SYN packets where:
>  1) There is data, or
>  2) The data offset makes the TCP header larger than
>  the packet.
>
>Both of these result in an error level printk.
>
>This change fixes the drop of SYN packets with data
>(because the MSS option can safely be modified) and
>passes packets with no MSS option instead of adding
>one (which is not valid).

Can you explain why the automatic addition of a MSS option is removed?

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Jan Engelhardt]

On Wednesday 2010-01-20 22:39, Jan Engelhardt wrote:

>On Wednesday 2010-01-20 22:21, Simon Arlott wrote:
>
>>The TCPMSS target is dropping SYN packets where:
>>  1) There is data, or
>>  2) The data offset makes the TCP header larger than
>>  the packet.
>>
>>Both of these result in an error level printk.
>>
>>This change fixes the drop of SYN packets with data
>>(because the MSS option can safely be modified) and
>>passes packets with no MSS option instead of adding
>>one (which is not valid).
>
>Can you explain why the automatic addition of a MSS option is removed?

That is, of course, for the git log. If I followed the thread right, it 
was that adding the option could exceed the MTU. Well, can't we check 
for the outgoing MTU?

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Simon Arlott]

On 20/01/10 21:41, Jan Engelhardt wrote:
> On Wednesday 2010-01-20 22:39, Jan Engelhardt wrote:
> 
>>On Wednesday 2010-01-20 22:21, Simon Arlott wrote:
>>
>>>The TCPMSS target is dropping SYN packets where:
>>>  1) There is data, or
>>>  2) The data offset makes the TCP header larger than
>>>  the packet.
>>>
>>>Both of these result in an error level printk.
>>>
>>>This change fixes the drop of SYN packets with data
>>>(because the MSS option can safely be modified) and
>>>passes packets with no MSS option instead of adding
>>>one (which is not valid).
>>
>>Can you explain why the automatic addition of a MSS option is removed?
> 
> That is, of course, for the git log. If I followed the thread right, it 
> was that adding the option could exceed the MTU. Well, can't we check 
> for the outgoing MTU?

The MSS option is for the MRU of whoever sent the SYN packet. There's no
way of knowing this information so it's not possible to avoid using an
MSS that is too large. With no option, "any" segment size could be used,
which implies 536 to match the MRU of 576.

The other reason for not being able to add it is that it may increase the
packet size beyond an MRU/MTU limit if there is data. There's no guarantee
we'll see an ICMP error message if this occurs, because the limit doesn't
have to be local and the return path does not need to be the same. The
original host won't know that the packet is going to be increased in size.

-- 
Simon Arlott

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Amos Jeffries]

On Wed, 20 Jan 2010 21:51:33 +0000, Simon Arlott <simon@fire.lp0.eu>
wrote:
> On 20/01/10 21:41, Jan Engelhardt wrote:
>> On Wednesday 2010-01-20 22:39, Jan Engelhardt wrote:
>> 
>>>On Wednesday 2010-01-20 22:21, Simon Arlott wrote:
>>>
>>>>The TCPMSS target is dropping SYN packets where:
>>>>  1) There is data, or
>>>>  2) The data offset makes the TCP header larger than
>>>>  the packet.
>>>>
>>>>Both of these result in an error level printk.
>>>>
>>>>This change fixes the drop of SYN packets with data
>>>>(because the MSS option can safely be modified) and
>>>>passes packets with no MSS option instead of adding
>>>>one (which is not valid).
>>>
>>>Can you explain why the automatic addition of a MSS option is removed?
>> 
>> That is, of course, for the git log. If I followed the thread right, it

>> was that adding the option could exceed the MTU. Well, can't we check 
>> for the outgoing MTU?
> 
> The MSS option is for the MRU of whoever sent the SYN packet. There's no
> way of knowing this information so it's not possible to avoid using an
> MSS that is too large. With no option, "any" segment size could be used,
> which implies 536 to match the MRU of 576.
> 
> The other reason for not being able to add it is that it may increase
the
> packet size beyond an MRU/MTU limit if there is data. There's no
guarantee
> we'll see an ICMP error message if this occurs, because the limit
doesn't
> have to be local and the return path does not need to be the same. The
> original host won't know that the packet is going to be increased in
size.

(I know little, so just my 2c)

So... packets are 'tunneled' down a link where MSS is required/added.
However packets which will not fit into the MTU of that 'tunnel' are send
down it without MSS and without fragmentation? I wonder what would happen
if all TCP MTUs worked that way...

Maybe I've misunderstood how path MTU discovery works. But is it and TCP
not built on the premise that the origin source host always receives the
ACKs regardless of reverse route? With PMTU discovery built on that
guarantee, to return the ICMP error to the same source the ACK would go?

If ICMP is administrively crippled to break TCP its not iptables fault,
nor the admin who is using TCP/ICMP correctly to signal available MTU.

AYJ

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Patrick McHardy]

Jan Engelhardt wrote:
> On Wednesday 2010-01-20 22:39, Jan Engelhardt wrote:
> 
>> On Wednesday 2010-01-20 22:21, Simon Arlott wrote:
>>
>>> The TCPMSS target is dropping SYN packets where:
>>>  1) There is data, or
>>>  2) The data offset makes the TCP header larger than
>>>  the packet.
>>>
>>> Both of these result in an error level printk.
>>>
>>> This change fixes the drop of SYN packets with data
>>> (because the MSS option can safely be modified) and
>>> passes packets with no MSS option instead of adding
>>> one (which is not valid).
>> Can you explain why the automatic addition of a MSS option is removed?
> 
> That is, of course, for the git log. If I followed the thread right, it 
> was that adding the option could exceed the MTU. Well, can't we check 
> for the outgoing MTU?

We certainly can, and in fact the packet would get fragmented
by the IP layer in case we would exceed the PMTU. Additionally
we currently check that the packet contains no data, even with
the first version of this patch, so there's no way the packet
could exceed the MTU.

This feature has been there from day one since the TCPMSS target
has been merged and people are using this with knowledge of their
MTUs to work around broken ISPs. I'm not apply this.

The first version seemed fine to me though :)

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Simon Arlott]

On Wed, January 20, 2010 23:14, Patrick McHardy wrote:
> Jan Engelhardt wrote:
>> On Wednesday 2010-01-20 22:39, Jan Engelhardt wrote:
>>> On Wednesday 2010-01-20 22:21, Simon Arlott wrote:
>>>> The TCPMSS target is dropping SYN packets where:
>>>>  1) There is data, or
>>>>  2) The data offset makes the TCP header larger than
>>>>  the packet.
>>>>
>>>> Both of these result in an error level printk.
>>>>
>>>> This change fixes the drop of SYN packets with data
>>>> (because the MSS option can safely be modified) and
>>>> passes packets with no MSS option instead of adding
>>>> one (which is not valid).
>>> Can you explain why the automatic addition of a MSS option is removed?
>>
>> That is, of course, for the git log. If I followed the thread right, it
>> was that adding the option could exceed the MTU. Well, can't we check
>> for the outgoing MTU?
>
> We certainly can, and in fact the packet would get fragmented
> by the IP layer in case we would exceed the PMTU. Additionally
> we currently check that the packet contains no data, even with
> the first version of this patch, so there's no way the packet
> could exceed the MTU.

If DF is set and the MTU is exceeded (for the SYN packet) at a
hop further away, the original host will not understand that it
needs to allow for the MSS option being added.

(Header + Data + New MSS Option) can't exceed 576 bytes and
there's no way to know that more than 576 bytes is allowed
because the ICMP error message may not go via the same host that
is mangling the packet.

Of course, it could just allow fragmentation for this one SYN
packet but that doesn't work for IPv6.

> This feature has been there from day one since the TCPMSS target
> has been merged and people are using this with knowledge of their
> MTUs to work around broken ISPs. I'm not apply this.

The TCPMSS target can be applied to more than just one direction
of traffic. I'm modifying incoming traffic too, so adding the MSS
option and setting it to over 536 is wrong (although the first ICMP
error will fix it).

Existing users use this target precisely because their hosts are
sending an unwanted MSS value, so it will never need to be added.

> The first version seemed fine to me though :)

The first version is ok with me. Only SYN packets with data and
no MSS option will be dropped. William objects to ever adding the
MSS option.

Although ideally SYN packets with data and no MSS option should
be accepted without adding an option. Dropping arbitrary traffic
(especially when new kernels allow data to be sent with SYN
packets) is not a good idea. If that is ok with you then I'll
make another patch to do it and update the comments.

-- 
Simon Arlott

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Jan Engelhardt]

On Thursday 2010-01-21 13:47, Simon Arlott wrote:
>
>The TCPMSS target can be applied to more than just one direction
>of traffic. I'm modifying incoming traffic too, so adding the MSS
>option and setting it to over 536 is wrong (although the first ICMP
>error will fix it).
>
>Existing users use this target precisely because their hosts are
>sending an unwanted MSS value, so it will never need to be added.

Ah, so they should be using TCPOPTSTRIP ;-)

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Patrick McHardy]

Simon Arlott wrote:
> On Wed, January 20, 2010 23:14, Patrick McHardy wrote:
>> Jan Engelhardt wrote:
>>>> Can you explain why the automatic addition of a MSS option is removed?
>>> That is, of course, for the git log. If I followed the thread right, it
>>> was that adding the option could exceed the MTU. Well, can't we check
>>> for the outgoing MTU?
>> We certainly can, and in fact the packet would get fragmented
>> by the IP layer in case we would exceed the PMTU. Additionally
>> we currently check that the packet contains no data, even with
>> the first version of this patch, so there's no way the packet
>> could exceed the MTU.
> 
> If DF is set and the MTU is exceeded (for the SYN packet) at a
> hop further away, the original host will not understand that it
> needs to allow for the MSS option being added.

Yes, but we don't add it for SYNs containing data.

> (Header + Data + New MSS Option) can't exceed 576 bytes and
> there's no way to know that more than 576 bytes is allowed
> because the ICMP error message may not go via the same host that
> is mangling the packet.
> 
> Of course, it could just allow fragmentation for this one SYN
> packet but that doesn't work for IPv6.
> 
>> This feature has been there from day one since the TCPMSS target
>> has been merged and people are using this with knowledge of their
>> MTUs to work around broken ISPs. I'm not apply this.
> 
> The TCPMSS target can be applied to more than just one direction
> of traffic. I'm modifying incoming traffic too, so adding the MSS
> option and setting it to over 536 is wrong (although the first ICMP
> error will fix it).

It might be wrong, but so is dropping ICMP fragmentation required
packets. This is a workaround for broken behaviour and you should
of course only use MSS values that you know are valid.

> Existing users use this target precisely because their hosts are
> sending an unwanted MSS value, so it will never need to be added.

Its mainly used for ISPs surpressing ICMP fragmentation required
messages. That affects hosts not adding an MSS option as well.

>> The first version seemed fine to me though :)
> 
> The first version is ok with me. Only SYN packets with data and
> no MSS option will be dropped. William objects to ever adding the
> MSS option.

Well, he's about 10 years late.

> Although ideally SYN packets with data and no MSS option should
> be accepted without adding an option. Dropping arbitrary traffic
> (especially when new kernels allow data to be sent with SYN
> packets) is not a good idea. If that is ok with you then I'll
> make another patch to do it and update the comments.

I agree, it shouldn't drop packets unless it really has to.
Please go ahead with a new patch.

[PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Simon Arlott]

The TCPMSS target is dropping SYN packets where:
  1) There is data, or
  2) The data offset makes the TCP header larger than the packet.

Both of these result in an error level printk. This printk has been
removed.

This change avoids dropping SYN packets containing data. If there
is also no MSS option (as well as data), one will not be added
because of possible complications due to the increased packet size.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 net/netfilter/xt_TCPMSS.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
index eda64c1..6f21b43 100644
--- a/net/netfilter/xt_TCPMSS.c
+++ b/net/netfilter/xt_TCPMSS.c
@@ -60,17 +60,9 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 	tcplen = skb->len - tcphoff;
 	tcph = (struct tcphdr *)(skb_network_header(skb) + tcphoff);
 
-	/* Since it passed flags test in tcp match, we know it is is
-	   not a fragment, and has data >= tcp header length.  SYN
-	   packets should not contain data: if they did, then we risk
-	   running over MTU, sending Frag Needed and breaking things
-	   badly. --RR */
-	if (tcplen != tcph->doff*4) {
-		if (net_ratelimit())
-			printk(KERN_ERR "xt_TCPMSS: bad length (%u bytes)\n",
-			       skb->len);
+	/* Header cannot be larger than the packet */
+	if (tcplen < tcph->doff*4)
 		return -1;
-	}
 
 	if (info->mss == XT_TCPMSS_CLAMP_PMTU) {
 		if (dst_mtu(skb_dst(skb)) <= minlen) {
@@ -115,6 +107,12 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 		}
 	}
 
+	/* There is data after the header so the option can't be added
+	   without moving it, and doing so may make the SYN packet
+	   itself too large. Accept the packet unmodified instead. */
+	if (tcplen > tcph->doff*4)
+		return 0;
+
 	/*
 	 * MSS Option not found ?! add it..
 	 */
-- 
1.6.3.3

-- 
Simon Arlott

Re: [PATCH] xt_TCPMSS: SYN packets are allowed to contain data

[Patrick McHardy]

Simon Arlott wrote:
> The TCPMSS target is dropping SYN packets where:
>   1) There is data, or
>   2) The data offset makes the TCP header larger than the packet.
> 
> Both of these result in an error level printk. This printk has been
> removed.
> 
> This change avoids dropping SYN packets containing data. If there
> is also no MSS option (as well as data), one will not be added
> because of possible complications due to the increased packet size.

Applied, thanks Simon.