netdev - Re: [PATCH v2 net-next 8/8] tcp: Factorise cookie-dependent fields initialisation in cookie_v[46]

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, 28 Nov 2023 16:42:38 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 8/8] tcp: Factorise cookie-dependent fields
 initialisation in cookie_v[46]_check()

On Sat, Nov 25, 2023 at 2:20 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> We will support arbitrary SYN Cookie with BPF, and then kfunc at
> TC will preallocate reqsk and initialise some fields that should
> not be overwritten later by cookie_v[46]_check().
>
> To simplify the flow in cookie_v[46]_check(), we move such fields'
> initialisation to cookie_tcp_reqsk_alloc() and factorise non-BPF
> SYN Cookie handling into cookie_tcp_check(), where we validate the
> cookie and allocate reqsk, as done by kfunc later.
>
> Note that we set ireq->ecn_ok in two steps, the latter of which will
> be shared by the BPF case.  As cookie_ecn_ok() is one-liner, now
> it's inlined.
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> Reviewed-by: Simon Horman <horms@...nel.org>
> ---
>  include/net/tcp.h     |  13 ++++--
>  net/ipv4/syncookies.c | 106 +++++++++++++++++++++++-------------------
>  net/ipv6/syncookies.c |  61 ++++++++++++------------
>  3 files changed, 99 insertions(+), 81 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index d4d0e9763175..973555cb1d3f 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -494,7 +494,10 @@ struct sock *tcp_get_cookie_sock(struct sock *sk, struct sk_buff *skb,
>  int __cookie_v4_check(const struct iphdr *iph, const struct tcphdr *th);
>  struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb);
>  struct request_sock *cookie_tcp_reqsk_alloc(const struct request_sock_ops *ops,
> -                                           struct sock *sk, struct sk_buff *skb);
> +                                           struct sock *sk, struct sk_buff *skb,
> +                                           struct tcp_options_received *tcp_opt,
> +                                           int mss, u32 tsoff);
> +
>  #ifdef CONFIG_SYN_COOKIES
>
>  /* Syncookies use a monotonic timer which increments every 60 seconds.
> @@ -580,8 +583,12 @@ __u32 cookie_v4_init_sequence(const struct sk_buff *skb, __u16 *mss);
>  u64 cookie_init_timestamp(struct request_sock *req, u64 now);
>  bool cookie_timestamp_decode(const struct net *net,
>                              struct tcp_options_received *opt);
> -bool cookie_ecn_ok(const struct tcp_options_received *opt,
> -                  const struct net *net, const struct dst_entry *dst);
> +
> +static inline bool cookie_ecn_ok(const struct net *net, const struct dst_entry *dst)
> +{
> +       return READ_ONCE(net->ipv4.sysctl_tcp_ecn) ||
> +               dst_feature(dst, RTAX_FEATURE_ECN);
> +}
>
>  /* From net/ipv6/syncookies.c */
>  int __cookie_v6_check(const struct ipv6hdr *iph, const struct tcphdr *th);
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index f4bcd4822fe0..5be12f186c26 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -270,21 +270,6 @@ bool cookie_timestamp_decode(const struct net *net,
>  }
>  EXPORT_SYMBOL(cookie_timestamp_decode);
>
> -bool cookie_ecn_ok(const struct tcp_options_received *tcp_opt,
> -                  const struct net *net, const struct dst_entry *dst)
> -{
> -       bool ecn_ok = tcp_opt->rcv_tsecr & TS_OPT_ECN;
> -
> -       if (!ecn_ok)
> -               return false;
> -
> -       if (READ_ONCE(net->ipv4.sysctl_tcp_ecn))
> -               return true;
> -
> -       return dst_feature(dst, RTAX_FEATURE_ECN);
> -}
> -EXPORT_SYMBOL(cookie_ecn_ok);
> -
>  static int cookie_tcp_reqsk_init(struct sock *sk, struct sk_buff *skb,
>                                  struct request_sock *req)
>  {
> @@ -320,8 +305,12 @@ static int cookie_tcp_reqsk_init(struct sock *sk, struct sk_buff *skb,
>  }
>
>  struct request_sock *cookie_tcp_reqsk_alloc(const struct request_sock_ops *ops,
> -                                           struct sock *sk, struct sk_buff *skb)
> +                                           struct sock *sk, struct sk_buff *skb,
> +                                           struct tcp_options_received *tcp_opt,
> +                                           int mss, u32 tsoff)
>  {
> +       struct inet_request_sock *ireq;
> +       struct tcp_request_sock *treq;
>         struct request_sock *req;
>
>         if (sk_is_mptcp(sk))
> @@ -337,40 +326,36 @@ struct request_sock *cookie_tcp_reqsk_alloc(const struct request_sock_ops *ops,
>                 return NULL;
>         }
>
> +       ireq = inet_rsk(req);
> +       treq = tcp_rsk(req);
> +
> +       req->mss = mss;
> +       req->ts_recent = tcp_opt->saw_tstamp ? tcp_opt->rcv_tsval : 0;
> +
> +       ireq->snd_wscale = tcp_opt->snd_wscale;
> +       ireq->tstamp_ok = tcp_opt->saw_tstamp;
> +       ireq->sack_ok = tcp_opt->sack_ok;
> +       ireq->wscale_ok = tcp_opt->wscale_ok;
> +       ireq->ecn_ok = tcp_opt->rcv_tsecr & TS_OPT_ECN;

I doubt this will do what you/we want, because ireq->ecn is not a
bool, it is a one bit field
and TS_OPT_ECN != 1.

I would have used instead :

 ireq->ecn_ok = !!(tcp_opt->rcv_tsecr & TS_OPT_ECN);

> +
> +       treq->ts_off = tsoff;
> +
>         return req;
>  }