netdev - Re: [PATCH net-next] tcp: annotate data-races around tp->window

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]

Message-ID: <CANn89iK9pDX=dA78Bk-sm8p4xxSno6XgHT=s0epSes=WLwxOZA@mail.gmail.com>
Date: Fri, 5 Apr 2024 16:49:07 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH net-next] tcp: annotate data-races around tp->window_clamp

On Fri, Apr 5, 2024 at 4:29 PM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> On Thu, Apr 4, 2024 at 7:53 PM Eric Dumazet <edumazet@...gle.com> wrote:
> >
> > tp->window_clamp can be read locklessly, add READ_ONCE()
> > and WRITE_ONCE() annotations.
> >
> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > ---
> >  net/ipv4/syncookies.c |  3 ++-
> >  net/ipv4/tcp.c        |  8 ++++----
> >  net/ipv4/tcp_input.c  | 17 ++++++++++-------
> >  net/ipv4/tcp_output.c | 18 ++++++++++--------
> >  net/ipv6/syncookies.c |  2 +-
> >  net/mptcp/protocol.c  |  2 +-
> >  net/mptcp/sockopt.c   |  2 +-
> >  7 files changed, 29 insertions(+), 23 deletions(-)
> >
> > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> > index 500f665f98cbce4a3d681f8e39ecd368fe4013b1..b61d36810fe3fd62b1e5c5885bbaf20185f1abf0 100644
> > --- a/net/ipv4/syncookies.c
> > +++ b/net/ipv4/syncookies.c
> > @@ -462,7 +462,8 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
> >         }
> >
> >         /* Try to redo what tcp_v4_send_synack did. */
> > -       req->rsk_window_clamp = tp->window_clamp ? :dst_metric(&rt->dst, RTAX_WINDOW);
> > +       req->rsk_window_clamp = READ_ONCE(tp->window_clamp) ? :
> > +                               dst_metric(&rt->dst, RTAX_WINDOW);
> >         /* limit the window selection if the user enforce a smaller rx buffer */
> >         full_space = tcp_full_space(sk);
> >         if (sk->sk_userlocks & SOCK_RCVBUF_LOCK &&
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index e767721b3a588b5d56567ae7badf5dffcd35a76a..92ee60492314a1483cfbfa2f73d32fcad5632773 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -1721,7 +1721,7 @@ int tcp_set_rcvlowat(struct sock *sk, int val)
> >         space = tcp_space_from_win(sk, val);
> >         if (space > sk->sk_rcvbuf) {
> >                 WRITE_ONCE(sk->sk_rcvbuf, space);
> > -               tcp_sk(sk)->window_clamp = val;
> > +               WRITE_ONCE(tcp_sk(sk)->window_clamp, val);
> >         }
> >         return 0;
> >  }
> > @@ -3379,7 +3379,7 @@ int tcp_set_window_clamp(struct sock *sk, int val)
> >         if (!val) {
> >                 if (sk->sk_state != TCP_CLOSE)
> >                         return -EINVAL;
> > -               tp->window_clamp = 0;
> > +               WRITE_ONCE(tp->window_clamp, 0);
> >         } else {
> >                 u32 new_rcv_ssthresh, old_window_clamp = tp->window_clamp;
> >                 u32 new_window_clamp = val < SOCK_MIN_RCVBUF / 2 ?
> > @@ -3388,7 +3388,7 @@ int tcp_set_window_clamp(struct sock *sk, int val)
> >                 if (new_window_clamp == old_window_clamp)
> >                         return 0;
> >
> > -               tp->window_clamp = new_window_clamp;
> > +               WRITE_ONCE(tp->window_clamp, new_window_clamp);
> >                 if (new_window_clamp < old_window_clamp) {
> >                         /* need to apply the reserved mem provisioning only
> >                          * when shrinking the window clamp
> > @@ -4057,7 +4057,7 @@ int do_tcp_getsockopt(struct sock *sk, int level,
> >                                       TCP_RTO_MAX / HZ);
> >                 break;
> >         case TCP_WINDOW_CLAMP:
> > -               val = tp->window_clamp;
> > +               val = READ_ONCE(tp->window_clamp);
> >                 break;
> >         case TCP_INFO: {
> >                 struct tcp_info info;
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 1b6cd384001202df5f8e8e8c73adff0db89ece63..8d44ab5671eacd4bc06647c7cca387a79e346618 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -563,19 +563,20 @@ static void tcp_init_buffer_space(struct sock *sk)
> >         maxwin = tcp_full_space(sk);
> >
> >         if (tp->window_clamp >= maxwin) {
>
> I wonder if it is necessary to locklessly protect the above line with
> READ_ONCE() because I saw the full reader protection in the
> tcp_select_initial_window()? There are some other places like this.
> Any special reason?

We hold the socket lock at this point.

READ_ONCE() is only needed if another thread can potentially change
the value under us.