On Tue, Apr 20, 2021 at 2:00 PM zhaoya <zhaoya.ga...@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.ga...@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.