netdev - Re: [PATCH bpf-next 01/10] tcp: Use a struct to represent a saved

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]
Message-ID: <20200630232406.3ozanjlyx5a2mv6i@kafai-mbp.dhcp.thefacebook.com>
Date:   Tue, 30 Jun 2020 16:24:06 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Eric Dumazet <edumazet@...gle.com>
CC:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        kernel-team <kernel-team@...com>,
        Lawrence Brakmo <brakmo@...com>,
        Neal Cardwell <ncardwell@...gle.com>,
        netdev <netdev@...r.kernel.org>,
        Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [PATCH bpf-next 01/10] tcp: Use a struct to represent a saved_syn

On Sat, Jun 27, 2020 at 10:41:32AM -0700, Eric Dumazet wrote:
> On Fri, Jun 26, 2020 at 10:55 AM Martin KaFai Lau <kafai@...com> wrote:
> >
> > The total length of the saved syn packet is currently stored in
> > the first 4 bytes (u32) and the actual packet data is stored after that.
> >
> > A latter patch will also want to store an offset (bpf_hdr_opt_off) to
> > a TCP header option which the bpf program will be interested in parsing.
> > Instead of anonymously storing this offset into the second 4 bytes,
> > this patch creates a struct for the existing saved_syn.
> > It can give a readable name to the stored lengths instead of implicitly
> > using the first few u32(s) to do that.
> >
> > The new TCP bpf header offset (bpf_hdr_opt_off) added in a latter patch is
> > an offset from the tcp header instead of from the network header.
> > It will make the bpf programming side easier.  Thus, this patch stores
> > the network header length instead of the total length of the syn
> > header.  The total length can be obtained by the
> > "network header len + tcp_hdrlen".  The latter patch can
> > then also gets the offset to the TCP bpf header option by
> > "network header len + bpf_hdr_opt_off".
> >
> > Signed-off-by: Martin KaFai Lau <kafai@...com>
> > ---
> >  include/linux/tcp.h        | 11 ++++++++++-
> >  include/net/request_sock.h |  7 ++++++-
> >  net/core/filter.c          |  4 ++--
> >  net/ipv4/tcp.c             |  9 +++++----
> >  net/ipv4/tcp_input.c       | 12 ++++++------
> >  5 files changed, 29 insertions(+), 14 deletions(-)
> >
> > diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> > index 3bdec31ce8f4..9d50132d95e6 100644
> > --- a/include/linux/tcp.h
> > +++ b/include/linux/tcp.h
> > @@ -404,7 +404,7 @@ struct tcp_sock {
> >          * socket. Used to retransmit SYNACKs etc.
> >          */
> >         struct request_sock __rcu *fastopen_rsk;
> > -       u32     *saved_syn;
> > +       struct saved_syn *saved_syn;
> >  };
> >
> >  enum tsq_enum {
> > @@ -482,6 +482,15 @@ static inline void tcp_saved_syn_free(struct tcp_sock *tp)
> >         tp->saved_syn = NULL;
> >  }
> >
> > +static inline u32 tcp_saved_syn_len(const struct saved_syn *saved_syn)
> > +{
> > +       const struct tcphdr *th;
> > +
> > +       th = (void *)saved_syn->data + saved_syn->network_hdrlen;
> > +
> > +       return saved_syn->network_hdrlen + __tcp_hdrlen(th);
> > +}
> 
> 
> Ah... We have a patch extending TCP_SAVE_SYN to save all headers, so
> keeping the length in a proper field would be better than going back
> to TCP header to get __tcp_hdrlen(th)
> 
> I am not sure why trying to save 4 bytes in this saved_syn would matter ;)
I will use another 4 bytes to store __tcp_hdrlen().

> 
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index c9fcfa4ec43f3f0d75763e2bc6773e15bd38d68f..8ecdc5f87788439c7a08d3b72f9567e6369e7c4e
> 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -258,7 +258,7 @@ struct tcp_sock {
>                 fastopen_connect:1, /* FASTOPEN_CONNECT sockopt */
>                 fastopen_no_cookie:1, /* Allow send/recv SYN+data
> without a cookie */
>                 is_sack_reneg:1,    /* in recovery from loss with SACK reneg? */
> -               unused:2;
> +               save_syn:2;     /* Save headers of SYN packet */
Interesting idea.  I don't have an immediate use case in the mac header of
the SYN but I think it may be useful going forward.

Although bpf_hdr_opt_off may be no longer needed in v2,
it will still be convenient for the bpf prog to be able to get the TCP header
only instead of reparsing from the mac/ip[46] and also save some stack space
of the bpf prog.  Thus, storing the length of each hdr would still be nice
so that the bpf helper can directly offset to the start of the required
header.

Do you prefer to incorporate this "save_syn:2" idea in this set
so that mac hdrlen can be stored in the "struct saved_syn"?

This "unused:2" bits have already been used by "fastopen_client_fail:2".
May be get 2 bits from repair_queue?


>         u8      nonagle     : 4,/* Disable Nagle algorithm?             */
>                 thin_lto    : 1,/* Use linear timeouts for thin streams */
>                 recvmsg_inq : 1,/* Indicate # of bytes in queue upon recvmsg */
> @@ -270,7 +270,7 @@ struct tcp_sock {
>                 syn_fastopen_exp:1,/* SYN includes Fast Open exp. option */
>                 syn_fastopen_ch:1, /* Active TFO re-enabling probe */
>                 syn_data_acked:1,/* data in SYN is acked by SYN-ACK */
> -               save_syn:1,     /* Save headers of SYN packet */
> +               unused_save_syn:1,      /* Moved above */
>                 is_cwnd_limited:1,/* forward progress limited by snd_cwnd? */
>                 syn_smc:1;      /* SYN includes SMC */
>         u32     tlp_high_seq;   /* snd_nxt at the time of TLP retransmit. */