Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210420120043.48151-1-zhaoya.gaius@bytedance.com>
In-Reply-To: <20210420120043.48151-1-zhaoya.gaius@bytedance.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Tue, 20 Apr 2021 15:45:17 +0200
Message-ID: <CANn89i+CH1uHem_QaCxoWxKVWyM8qsz_zi4Dhm26Tni2EsW71A@mail.gmail.com>
Subject: Re: [PATCH] tcp: prevent loss of bytes when using syncookie
To:     =?UTF-8?B?6LW15Lqa?= <zhaoya.gaius@bytedance.com>
Cc:     David Miller <davem@davemloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        netdev <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Apr 20, 2021 at 2:00 PM zhaoya <zhaoya.gaius@bytedance.com> wrote:
>
> When using syncookie, since $MSSID is spliced into cookie and
> the legal index of msstab  is 0,1,2,3, this gives client 3 bytes
> of freedom, resulting in at most 3 bytes of silent loss.
>
> C ------------seq=12345-------------> S
> C <------seq=cookie/ack=12346-------- S S generated the cookie
>                                         [RFC4987 Appendix A]
> C ---seq=123456/ack=cookie+1-->X      S The first byte was loss.
> C -----seq=123457/ack=cookie+1------> S The second byte was received and
>                                         cookie-check was still okay and
>                                         handshake was finished.
> C <--------seq=.../ack=12348--------- S acknowledge the second byte.
>
> Here is a POC:
>
> $ cat poc.c
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> #include <pthread.h>
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <netinet/in.h>
> #include <arpa/inet.h>
>
> void *serverfunc(void *arg)
> {
>         int sd = -1;
>         int csd = -1;
>         struct sockaddr_in servaddr, cliaddr;
>         int len = sizeof(cliaddr);
>
>         sd = socket(AF_INET, SOCK_STREAM, 0);
>         servaddr.sin_family = AF_INET;
>         servaddr.sin_addr.s_addr = htonl(INADDR_ANY);
>         servaddr.sin_port = htons(1234);
>         bind(sd, (struct sockaddr *)&servaddr, sizeof(servaddr));
>         listen(sd, 1);
>
>         while (1) {
>                 char buf[2];
>                 int ret;
>                 csd = accept(sd, (struct sockaddr *)&cliaddr, &len);
>                 memset(buf, 0, 2);
>                 ret = recv(csd, buf, 1, 0);
>                 // but unexpected char is 'b'
>                 if (ret && strncmp(buf, "a", 1)) {
>                         printf("unexpected:%s\n", buf);
>                         close(csd);
>                         exit(0);
>                 }
>                 close(csd);
>         }
> }
>
> void *connectfunc(void *arg)
> {
>         struct sockaddr_in addr;
>         int sd;
>         int i;
>
>         for (i = 0; i < 500; i++) {
>                 sd = socket(AF_INET, SOCK_STREAM, 0);
>                 addr.sin_family = AF_INET;
>                 addr.sin_addr.s_addr = inet_addr("127.0.0.1");
>                 addr.sin_port = htons(1234);
>
>                 connect(sd, (struct sockaddr *)&addr, sizeof(addr));
>
>                 send(sd, "a", 1, 0); // expected char is 'a'
>                 send(sd, "b", 1, 0);
>                 close(sd);
>         }
>         return NULL;
> }
>
> int main(int argc, char *argv[])
> {
>         int i;
>         pthread_t id;
>
>         pthread_create(&id, NULL, serverfunc, NULL);
>         sleep(1);
>         for (i = 0; i < 500; i++) {
>                 pthread_create(&id, NULL, connectfunc, NULL);
>         }
>         sleep(5);
> }
>
> $ sudo gcc poc.c -lpthread
> $ sudo sysctl -w net.ipv4.tcp_syncookies=1
> $ sudo ./a.out # please try as many times.
>

POC is distracting, you could instead give a link to
https://kognitio.com/blog/syn-cookies-ate-my-dog-breaking-tcp-on-linux/
where all the details can be found.

Also it gives credits to past work.

If you feel a program needs to be written and recorded for posterity,
add a selftest. (in tools/testing/selftests/net )

> This problem can be fixed by having $SSEQ itself participate in the
> hash operation.
>
> The feature is protected by a sysctl so that admins can choose
> the preferred behavior.
>
> Signed-off-by: zhaoya <zhaoya.gaius@bytedance.com>
> ---
>  Documentation/networking/ip-sysctl.rst |  9 ++++++++
>  include/linux/netfilter_ipv6.h         | 24 +++++++++++++-------
>  include/net/netns/ipv4.h               |  1 +
>  include/net/tcp.h                      | 20 ++++++++++-------
>  net/core/filter.c                      |  6 +++--
>  net/ipv4/syncookies.c                  | 31 ++++++++++++++++----------
>  net/ipv4/sysctl_net_ipv4.c             |  7 ++++++
>  net/ipv4/tcp_ipv4.c                    |  4 +++-
>  net/ipv6/syncookies.c                  | 29 +++++++++++++++---------
>  net/ipv6/tcp_ipv6.c                    |  3 ++-
>  net/netfilter/nf_synproxy_core.c       | 10 +++++----
>  11 files changed, 97 insertions(+), 47 deletions(-)
>
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index c7952ac5bd2f..a430e8736c2b 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -732,6 +732,15 @@ tcp_syncookies - INTEGER
>         network connections you can set this knob to 2 to enable
>         unconditionally generation of syncookies.
>
> +tcp_syncookies_enorder - INTEGER

enorder ? What does it mean ?

> +       Only valid when the kernel was compiled with CONFIG_SYN_COOKIES
> +       Prevent the loss of at most 3 bytes of which sent by client when
> +       syncookies as generated to complete TCP-Handshake.
> +       Default: 0
> +
> +       Note that if it was enabled, any out-of-order bytes sent by client
> +       to complete TCP-Handshake could get the session killed.

Technically speaking, the client does not send out-of-order packets.

The issue here is that loss of packets sent after 3WHS can lead to RST
and session being killed.


> +
>  tcp_fastopen - INTEGER
>         Enable TCP Fast Open (RFC7413) to send and accept data in the opening
>         SYN packet.
> diff --git a/include/linux/netfilter_ipv6.h b/include/linux/netfilter_ipv6.h
> index 48314ade1506..97b67672dee7 100644
> --- a/include/linux/netfilter_ipv6.h
> +++ b/include/linux/netfilter_ipv6.h
> @@ -49,9 +49,11 @@ struct nf_ipv6_ops {
>         int (*route)(struct net *net, struct dst_entry **dst, struct flowi *fl,
>                      bool strict);
>         u32 (*cookie_init_sequence)(const struct ipv6hdr *iph,
> -                                   const struct tcphdr *th, u16 *mssp);
> +                                   const struct tcphdr *th, u16 *mssp,
> +                                   int enorder);
>

Patch is too big, and passing either a ' struct net'  or an ' int
enorder' is not pretty/consistent.

Please send a patch series :

1) Add ' struct net'  pointers to all helpers which will need it
later. This is a pure preparation work, easy to review.

2) Patch itself, adding the sysctl.

    I would prefer you add a helper like this and use it every where
you need to use the sysctl so that it is factorized

  static inline u32 tcp_cond_seq(const struct net *net, u32 seq)
{
    return net->ipv4.sysctl_tcp_syncookie_no_eat ? seq : 0;
}

And use this helper  in functions right before using (or not) the seq
in hash functions/
(Do not pass around the result of the helper)

Thanks.