netdev - Re: [PATCH v1 net] TCP_USER_TIMEOUT and tcp_keepalive should conform to RFC5482

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Aug 2017 14:05:37 -0700
From:   Rao Shoaib <rao.shoaib@...cle.com>
To:     Jerry Chu <hkchu@...gle.com>
Cc:     David Miller <davem@...emloft.net>, codesoldier1@...il.com,
        Yuchung Cheng <ycheng@...gle.com>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v1 net] TCP_USER_TIMEOUT and tcp_keepalive should conform
 to RFC5482



On 08/09/2017 09:59 PM, Jerry Chu wrote:
> On Wed, Aug 9, 2017 at 8:32 PM, Jerry Chu <hkchu@...gle.com> wrote:
>> On Wed, Aug 9, 2017 at 5:47 PM, Rao Shoaib <rao.shoaib@...cle.com> wrote:
>>>
>>> On 08/09/2017 05:30 PM, David Miller wrote:
>>>> From: Joe Smith <codesoldier1@...il.com>
>>>> Date: Wed, 9 Aug 2017 17:20:32 -0700
>>>>
>>>>> Making Linux conform to standards and behavior that is logical seems
>>>>> like a good enough reason.
>>>> That's an awesome attitude to have when we're implementing something
>>>> new and don't have the facility already.
>>>>
>>>> But when we have something already the only important consideration is
>>>> not breaking existing apps which rely on that behavior.
>>>>
>>>> That is much, much, more important than standards compliance.
>>>>
>>>> If users are confused, just fix the documentation.
>>> David,
>>>
>>> If it was just confusion than sure fixing the documentation is fine. What if
>>> the logic is incorrect, does not conform to the standard that is says it is
>> Not sure what part of logic is "incorrect" when it was a homegrown Linux API
>> with no need to conform with any "standard"? Note that the new API was invented
>> 7 years ago not out of need for RFC5482. In fact I initially call the option
>> TCP_FAILFAST and did not even know the existence of RFC5482 until someone
>> around the same time proposed a UTO option specifically for RFC5482 and I
>> thought the two can be combined. (This is roughly the memory I can
>> recollect so far.)
>>
>> So you see my focus back then was to devise a "failfast" option whereas RFC5482
>> was meant for a "failslow" case. I think that explains why I let the
>> option override
>> keepalive so a TCP connection can "fail fast" while RFC5482 4.2 tries to prevent
>> keepalive failure ahead of UTO, favoring "fail slow".
>>
>> If we start from a clean slate then perhaps one can argue the semantic
>> either way
>> but we do not have a clean slate. For that I still slightly favor not
>> changing the code
>> because the risk of breakage is definitely non-zero and the issue you're having
>> seem to be only related to documentation.
We all make mistakes and over look things, that seems to be the case 
here. If this was so important than I am sure there was a use case. None 
has been presented. Without a use case I do not understand why we have 
to live with broken logic when we have a chance to fix it and make the 
code better.

If this change does break something (very very unlikely) we will 
understand the use case and provide an appropriate solution.

> One more thing - the proposed patch compares TCP_KEEPIDLE against
> TCP_USER_TIMEOUT. But I don't think TCP_KEEPIDLE is what the
> "keep-alive
> timer" referred to in RFC5482. Linux keepalive implementation seems to use #
> of retries (TCP_KEEPCNT) rather than time duration (keep-alive time) to
> determine when to quit. If that is the case then your proposed change is not
> fully "compliant" either and the best is probably just don't change.
Did you look at the patch and what it changes ?

Take a look at the TCP_KEEPIDLE socket option and see what it does or 
just look at the man page of tcp(7)

TCP_KEEPIDLE (since Linux 2.4) The time (in seconds) the connection 
needs to remain idle before TCP starts sending keepalive probes, if the 
socket option SO_KEEPALIVE has been set on this socket. This option 
should not be used in code intended to be portable.

Shoaib.
>
>> Jerry
>>
>>> implementing and easy to fix with little or no risk of breakage.
>>>
>>> The proposed patch changes a feature that no one uses. It also imposes the
>>> relation ship between keepalive and timeout values that is required by the
>>> RFC and make sense.
>>>
>>> You are the final authority, if you say we should just fix the documentation
>>> than that is fine.
>>>
>>> Shoaib
>>>