| 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
| ||
|
Message-ID: <CAL+tcoAqvQPw6PXYa-4hz6B=krgOYxw8jdFNCzQRcZnOVT_i+w@mail.gmail.com>
Date: Wed, 30 Oct 2024 11:27:56 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, dsahern@...nel.org, willemb@...gle.com, ast@...nel.org,
daniel@...earbox.net, andrii@...nel.org, martin.lau@...ux.dev,
eddyz87@...il.com, song@...nel.org, yonghong.song@...ux.dev,
john.fastabend@...il.com, kpsingh@...nel.org, sdf@...ichev.me,
haoluo@...gle.com, jolsa@...nel.org, shuah@...nel.org, ykolal@...com,
bpf@...r.kernel.org, netdev@...r.kernel.org,
Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v3 10/14] net-timestamp: add basic support with
tskey offset
On Wed, Oct 30, 2024 at 3:45 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> > > > > > +static long int sock_calculate_tskey_offset(struct sock *sk, int val, int bpf_type)
> > > > > > +{
> > > > > > + u32 tskey;
> > > > > > +
> > > > > > + if (sk_is_tcp(sk)) {
> > > > > > + if ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN))
> > > > > > + return -EINVAL;
> > > > > > +
> > > > > > + if (val & SOF_TIMESTAMPING_OPT_ID_TCP)
> > > > > > + tskey = tcp_sk(sk)->write_seq;
> > > > > > + else
> > > > > > + tskey = tcp_sk(sk)->snd_una;
> > > > > > + } else {
> > > > > > + tskey = 0;
> > > > > > + }
> > > > > > +
> > > > > > + if (bpf_type && (sk->sk_tsflags & SOF_TIMESTAMPING_OPT_ID)) {
> > > > > > + sk->sk_tskey_bpf_offset = tskey - atomic_read(&sk->sk_tskey);
> > > > > > + return 0;
> > > > > > + } else if (!bpf_type && (sk->sk_tsflags_bpf & SOF_TIMESTAMPING_OPT_ID)) {
> > > > > > + sk->sk_tskey_bpf_offset = atomic_read(&sk->sk_tskey) - tskey;
> > > > > > + } else {
> > > > > > + sk->sk_tskey_bpf_offset = 0;
> > > > > > + }
> > > > > > +
> > > > > > + return tskey;
> > > > > > +}
> > > > > > +
> > > > > > int sock_set_tskey(struct sock *sk, int val, int bpf_type)
> > > > > > {
> > > > > > u32 tsflags = bpf_type ? sk->sk_tsflags_bpf : sk->sk_tsflags;
> > > > > > @@ -901,17 +944,13 @@ int sock_set_tskey(struct sock *sk, int val, int bpf_type)
> > > > > >
> > > > > > if (val & SOF_TIMESTAMPING_OPT_ID &&
> > > > > > !(tsflags & SOF_TIMESTAMPING_OPT_ID)) {
> > > > > > - if (sk_is_tcp(sk)) {
> > > > > > - if ((1 << sk->sk_state) &
> > > > > > - (TCPF_CLOSE | TCPF_LISTEN))
> > > > > > - return -EINVAL;
> > > > > > - if (val & SOF_TIMESTAMPING_OPT_ID_TCP)
> > > > > > - atomic_set(&sk->sk_tskey, tcp_sk(sk)->write_seq);
> > > > > > - else
> > > > > > - atomic_set(&sk->sk_tskey, tcp_sk(sk)->snd_una);
> > > > > > - } else {
> > > > > > - atomic_set(&sk->sk_tskey, 0);
> > > > > > - }
> > > > > > + long int ret;
> > > > > > +
> > > > > > + ret = sock_calculate_tskey_offset(sk, val, bpf_type);
> > > > > > + if (ret <= 0)
> > > > > > + return ret;
> > > > > > +
> > > > > > + atomic_set(&sk->sk_tskey, ret);
> > > > > > }
> > > > > >
> > > > > > return 0;
> > > > > > @@ -956,10 +995,15 @@ static int sock_set_timestamping_bpf(struct sock *sk,
> > > > > > struct so_timestamping timestamping)
> > > > > > {
> > > > > > u32 flags = timestamping.flags;
> > > > > > + int ret;
> > > > > >
> > > > > > if (flags & ~SOF_TIMESTAMPING_BPF_SUPPPORTED_MASK)
> > > > > > return -EINVAL;
> > > > > >
> > > > > > + ret = sock_set_tskey(sk, flags, 1);
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > > > +
> > > > > > WRITE_ONCE(sk->sk_tsflags_bpf, flags);
> > > > > >
> > > > > > return 0;
> > > > >
> > > > > I'm a bit hazy on when this can be called. We can assume that this new
> > > > > BPF operation cannot race with the existing setsockopt nor with the
> > > > > datapath that might touch the atomic fields, right?
> > > >
> > > > It surely can race with the existing setsockopt.
> > > >
> > > > 1)
> > > > if (only existing setsockopt works) {
> > > > then sk->sk_tskey is set through setsockopt, sk_tskey_bpf_offset is 0.
> > > > }
> > > >
> > > > 2)
> > > > if (only bpf setsockopt works) {
> > > > then sk->sk_tskey is set through bpf_setsockopt,
> > > > sk_tskey_bpf_offset is 0.
> > > > }
> > > >
> > > > 3)
> > > > if (existing setsockopt already started, here we enable the bpf feature) {
> > > > then sk->sk_tskey will not change, but the sk_tskey_bpf_offset
> > > > will be calculated.
> > > > }
> > > >
> > > > 4)
> > > > if (bpf setsockopt already started, here we enable the application feature) {
> > > > then sk->sk_tskey will re-initialized/overridden by
> > > > setsockopt, and the sk_tskey_bpf_offset will be calculated.
> > > > }
> >
> > I will copy the above to the commit message next time in order to
> > provide a clear design to future readers.
> >
> > > >
> > > > Then the skb tskey will use the sk->sk_tskey like before.
> > >
> > > I mean race as in the setsockopt and bpf setsockopt and datapath
> > > running concurrently.
> > >
> > > As long as both variants of setsockopt hold the socket lock, that
> > > won't happen.
> > >
> > > The datapath is lockless for UDP, so atomic_inc sk_tskey can race
> > > with calculating the difference. But this is a known issue. A process
> > > that cares should not run setsockopt and send concurrently. So this is
> > > fine too.
> >
> > Oh, now I see. Thanks for the detailed explanation! So Do you feel if
> > we need to take care of this in the future, I mean, after this series
> > gets merged...?
>
> If there is a race condition, then that cannot be fixed up later.
>
> But from my admittedly brief analysis, it seems that there is nothing
> here that needs to be fixed: control plane operations (setsockopt)
> hold the socket lock. A setsockopt that conflicts with a lockless
> datapath update will have a slightly ambiguous offset. It is under
> controlof and up to the user to avoid that if they care.
I got it. Thanks.
Powered by blists - more mailing lists