netdev - Re: [net-next] tcp: add TCP_INFO status for failed client TFO

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 22 Oct 2019 12:45:32 -0700
From:   Yuchung Cheng <ycheng@...gle.com>
To:     Jason Baron <jbaron@...mai.com>
Cc:     Neal Cardwell <ncardwell@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Christoph Paasch <cpaasch@...le.com>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [net-next] tcp: add TCP_INFO status for failed client TFO

On Tue, Oct 22, 2019 at 12:34 PM Jason Baron <jbaron@...mai.com> wrote:
>
>
>
> On 10/22/19 2:17 PM, Yuchung Cheng wrote:
> > On Mon, Oct 21, 2019 at 7:14 PM Neal Cardwell <ncardwell@...gle.com> wrote:
> >>
> >> On Mon, Oct 21, 2019 at 5:11 PM Jason Baron <jbaron@...mai.com> wrote:
> >>>
> >>>
> >>>
> >>> On 10/21/19 4:36 PM, Eric Dumazet wrote:
> >>>> On Mon, Oct 21, 2019 at 12:53 PM Christoph Paasch <cpaasch@...le.com> wrote:
> >>>>>
> >>>>
> >>>>> Actually, longterm I hope we would be able to get rid of the
> >>>>> blackhole-detection and fallback heuristics. In a far distant future where
> >>>>> these middleboxes have been weeded out ;-)
> >>>>>
> >>>>> So, do we really want to eternalize this as part of the API in tcp_info ?
> >>>>>
> >>>>
> >>>> A getsockopt() option won't be available for netlink diag (ss command).
> >>>>
> >>>> So it all depends on plans to use this FASTOPEN information ?
> >>>>
> >>>
> >>> The original proposal I had 4 states of interest:
> >>>
> >>> First, we are interested in knowing when a socket has TFO set but
> >>> actually requires a retransmit of a non-TFO syn to become established.
> >>> In this case, we'd be better off not using TFO.
> >>>
> >>> A second case is when the server immediately rejects the DATA and just
> >>> acks the syn (but not the data). Again in that case, we don't want to be
> >>> sending syn+data.
> >>>
> >>> The third case was whether or not we sent a cookie. Perhaps, the server
> >>> doesn't have TFO enabled in which case, it really doesn't make make
> >>> sense to enable TFO in the first place. Or if one also controls the
> >>> server its helpful in understanding if the server is mis-configured. So
> >>> that was the motivation I had for the original four states that I
> >>> proposed (final state was a catch-all state).
> >>>
> >>> Yuchung suggested dividing the 3rd case into 2 for - no cookie sent
> >>> because of blackhole or no cookie sent because its not in cache. And
> >>> then dropping the second state because we already have the
> >>> TCPI_OPT_SYN_DATA bit. However, the TCPI_OPT_SYN_DATA may not be set
> >>> because we may fallback in tcp_send_syn_data() due to send failure. So
> > but sendto would return -1 w/ EINPROGRESS in this case already so the
> > application shouldn't expect TCPI_OPT_SYN_DATA?
>
> Ok, but let's say the sk_stream_alloc_skb() fails in
> tcp_send_syn_data(), in that case we aren't going to send a TFO cookie
> (just a regular syn). The user isn't going to get any error and would
> expect TCPI_OPT_SYN_DATA. Now, TCPI_OPT_SYN_DATA wouldn't be set but we
> can't assume then that a SYN+data was sent and the SYN_ACK didn't cover
> the data part. Instead, the reason for failure is really -ENOMEM, which
> in the proposed states would fall into TFO_STATUS_UNSPEC, but it does
> mean that I think we shouldn't have a TFO_DATA_NOT_ACKED state otherwise
> we can't differentiate the two.
>
> >
> >
> >>> I'm inclined to say that the second state is valuable. And since
> >>> blackhole is already a global thing via MIB, I didn't see a strong need
> >>> for it. But it sounded like Yuchung had an interest in it, and I'd
> >>> obviously like a set of states that is generally useful.
> >>
> >> I have not kept up with all the proposals in this thread, but would it
> >> work to include all of the cases proposed in this thread? It seems
> >> like there are enough bits available in holes in tcp_sock and tcp_info
> >> to have up to 7 bits of information saved in tcp_sock and exported to
> >> tcp_info. That seems like more than enough?
> > I would rather use only at most 2-bits for TFO errors to be
> > parsimonious on (bloated) tcp sock. I don't mind if the next patch
> > skip my idea of BH detection.
> > my experience is reading host global stats for most applications are a
> > hassle (or sometimes not even feasible). they mostly care about
> > information of their own sockets.
>
> hmmm, if you are ok without having the BH failure state which Christoph
> also pushed back on a bit, but we still only want 2 bits as you
> suggested how about:
>
> 1) TFO_STATUS_UNSPEC - includes black hole state and other failures
> 2) TFO_COOKIE_UNAVAILABLE - we don't have a cookie in the cache
> 3) TFO_DATA_NOT_ACKED - syn+data sent but no ack for data part
> 4) TFO_SYN_RETRANSMITTED - same as 3 but at least 1 additional syn sent
sounds good to me. thanks
>
> Thanks,
>
> -Jason