Re: [PATCHv4 0/7] Per route TCP options support kill switches

[Gilad Ben-Yossef <gilad@codefidence.com>]

Eric Dumazet wrote:

> Gilad Ben-Yossef a écrit :
>   
>> Allow selectively turning off support for specific TCP options
>> on a per route basis.
>>
>> One normally want to disable SACK, DSACK, time stamp or window
>> scale if one got a piece of broken networking equipment somewhere
>> as a stop gap until you can bring a big enough hammer to deal with
>> the broken network equipment. It doesn't make sense to "punish" the
>> entire connections going through the machine to destinations not
>> related to the broken equipment.
>>
>> This is doubly true when one is dealing with network containers
>> used to isolate several virtual domains.
>>
>> Per route options implemented in free bits in the features route
>> entry property, which in some cases were reserved by name for these
>> options, so this does not inflate any structure.
>>
>> Global sysctls for these options are still preserved and retain 
>> the exact original meaning (e.g. you have to have both the global 
>> sysctl turned on and not turn off the TCP option parsing in the
>> specific route to have it proccessed).
>>
>> It is not possible to turn off globally an option but turn it on
>> per route, so as to not subtly change the meaning of current
>> establish sysctls (and this is a rare need anyway).
>>
>> Tested on x86 using Qemu/KVM.
>>
>> Working but crude matching patch to iproute2 sent earlier to the list.
>>
>> Patchset based on original work by Ori Finkelman and Yony Amit
>> from ComSleep Ltd.
>>
>> The author wishes to thank Eric Dumazaet, William Allen Simpson, 
>> Bill Fink and Ilpo Jarvinen for their feedback.
>>
>>
>> Gilad Ben-Yossef (7):
>>   Only parse time stamp TCP option in time wait sock
>>   Allow tcp_parse_options to consult dst entry
>>   Add dst_feature to query route entry features
>>   Add the no SACK route option feature
>>   Allow disabling TCP timestamp options per route
>>   Allow to turn off TCP window scale opt per route
>>   Allow disabling of DSACK TCP option per route
>>
>>  include/linux/rtnetlink.h |    6 ++++--
>>  include/net/dst.h         |    8 +++++++-
>>  include/net/tcp.h         |    3 ++-
>>  net/ipv4/syncookies.c     |   27 ++++++++++++++-------------
>>  net/ipv4/tcp_input.c      |   26 ++++++++++++++++++--------
>>  net/ipv4/tcp_ipv4.c       |   21 ++++++++++++---------
>>  net/ipv4/tcp_minisocks.c  |    9 ++++++---
>>  net/ipv4/tcp_output.c     |   18 +++++++++++++-----
>>  net/ipv6/syncookies.c     |   28 +++++++++++++++-------------
>>  net/ipv6/tcp_ipv6.c       |    3 ++-
>>  10 files changed, 93 insertions(+), 56 deletions(-)
>>
>>     
>
> I am a bit lost. What exactly changed in this new version, versus v3 ?
>
>   


Code wise only the first patch - in tcp_timewait_state_process I
was calling tcp_parse_options() without actually initializing
properly the tstamp_ok field of the truct tcp_options_received
parameter to the that function.

I failed to noticed that since being a non initalized structure
on the stack it can actually produce the required behavior
depending on whatever random data was there.

I spotted it thanks to Williams A.S. review of my changes in that area.

Since I had the opportunity, I also edited the patch set description to
better document some of the things that seemed to bug some
of the reviewers (namely Bill F. and Williams A.S ).

Sorry for the confusion.

Thanks,
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker & CTO
Codefidence Ltd.

Web:   http://codefidence.com
Cell:  +972-52-8260388
Skype: gilad_codefidence
Tel:   +972-8-9316883 ext. 201
Fax:   +972-8-9316884
Email: gilad@codefidence.com

Check out our Open Source technology and training blog - http://tuxology.net

	"The biggest risk you can take it is to take no risk."
		-- Mark Zuckerberg and probably others