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: <CAL+tcoBEPLUN1Zj0YNR8VUWXrEwG0ba2a6GahW9wspBBwGMR3g@mail.gmail.com>
Date: Fri, 5 Apr 2024 22:57:31 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Eric Dumazet <edumazet@...gle.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 10:49 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> 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.

Oh right, thanks. The socket will be locked as soon as the skb enters
into the TCP layer.