From: William Allen Simpson on
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!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Simon Arlott on
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jan Engelhardt on
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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jan Engelhardt on
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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Simon Arlott on
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/