Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
MIME-Version: 1.0
References: <20231121020111.1143180-1-dima@arista.com> <20231121020111.1143180-7-dima@arista.com>
In-Reply-To: <20231121020111.1143180-7-dima@arista.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Tue, 21 Nov 2023 09:08:49 +0100
Message-ID: <CANn89iLEANNvZ45PaPL8miZeyMUTAcLoVR4WS55gbtfiMPbueQ@mail.gmail.com>
Subject: Re: [PATCH 6/7] net/tcp: ACCESS_ONCE() on snd/rcv SNEs
To:     Dmitry Safonov <dima@arista.com>
Cc:     David Ahern <dsahern@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        linux-kernel@vger.kernel.org,
        Dmitry Safonov <0x7f454c46@gmail.com>,
        Francesco Ruggeri <fruggeri05@gmail.com>,
        Salam Noureddine <noureddine@arista.com>,
        Simon Horman <horms@kernel.org>, netdev@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Nov 21, 2023 at 3:01=E2=80=AFAM Dmitry Safonov <dima@arista.com> wr=
ote:
>
> SNEs need READ_ONCE()/WRITE_ONCE() for access as they can be written and
> read at the same time.
>
> This is actually a shame: I planned to send it in TCP-AO patches, but
> it seems I've chosen a wrong commit to git-commit-fixup some time ago.
> It ended up in a commit that adds a selftest. Human factor.
>
> Fixes: 64382c71a557 ("net/tcp: Add TCP-AO SNE support")
> Signed-off-by: Dmitry Safonov <dima@arista.com>
> ---
>  net/ipv4/tcp_ao.c    | 4 ++--
>  net/ipv4/tcp_input.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c
> index 122ff58168ee..9b7f1970c2e9 100644
> --- a/net/ipv4/tcp_ao.c
> +++ b/net/ipv4/tcp_ao.c
> @@ -956,8 +956,8 @@ tcp_inbound_ao_hash(struct sock *sk, const struct sk_=
buff *skb,
>                 if (unlikely(th->syn && !th->ack))
>                         goto verify_hash;
>
> -               sne =3D tcp_ao_compute_sne(info->rcv_sne, tcp_sk(sk)->rcv=
_nxt,
> -                                        ntohl(th->seq));
> +               sne =3D tcp_ao_compute_sne(READ_ONCE(info->rcv_sne),
> +                                        tcp_sk(sk)->rcv_nxt, ntohl(th->s=
eq));


I think this is a wrong fix. Something is definitely fishy here.

Update side should only happen for an established socket ?

And the read side should have locked the socket before calling
tcp_inbound_ao_hash(),
otherwise reading other fields (like tcp_sk(sk)->rcv_nxt) would be racy any=
way.


>                 /* Established socket, traffic key are cached */
>                 traffic_key =3D rcv_other_key(key);
>                 err =3D tcp_ao_verify_hash(sk, skb, family, info, aoh, ke=
y,
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index bcb55d98004c..78896c8be0d4 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c

tcp_snd_sne_update() definitely only deals with full sockets
(TCP_AO_ESTABLISHED)

> @@ -3583,7 +3583,7 @@ static void tcp_snd_sne_update(struct tcp_sock *tp,=
 u32 ack)
>         ao =3D rcu_dereference_protected(tp->ao_info,
>                                        lockdep_sock_is_held((struct sock =
*)tp));
>         if (ao && ack < tp->snd_una)
> -               ao->snd_sne++;
> +               WRITE_ONCE(ao->snd_sne, ao->snd_sne + 1);
>  #endif
>  }
>
> @@ -3609,7 +3609,7 @@ static void tcp_rcv_sne_update(struct tcp_sock *tp,=
 u32 seq)
>         ao =3D rcu_dereference_protected(tp->ao_info,
>                                        lockdep_sock_is_held((struct sock =
*)tp));
>         if (ao && seq < tp->rcv_nxt)
> -               ao->rcv_sne++;
> +               WRITE_ONCE(ao->rcv_sne, ao->rcv_sne + 1);
>  #endif
>  }
>
> --
> 2.42.0
>