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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 22 Oct 2019 11:17:39 -0700
From:   Yuchung Cheng <ycheng@...gle.com>
To:     Neal Cardwell <ncardwell@...gle.com>
Cc:     Jason Baron <jbaron@...mai.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 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?


> > 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.



>
> The pahole tool shows the following small holes in tcp_sock:
>
>         u8                         compressed_ack;       /*  1530     1 */
>
>         /* XXX 1 byte hole, try to pack */
>
>         u32                        chrono_start;         /*  1532     4 */
> ...
>         u8                         bpf_sock_ops_cb_flags; /*  2076     1 */
>
>         /* XXX 3 bytes hole, try to pack */
>
>         u32                        rcv_ooopack;          /*  2080     4 */
>
> ...and the following hole in tcp_info:
>         __u8                       tcpi_delivery_rate_app_limited:1;
> /*     7: 7  1 */
>
>         /* XXX 7 bits hole, try to pack */
>
>         __u32                      tcpi_rto;             /*     8     4 */
>
> cheers,
> neal