[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANn89iJAZ+OYVebm-x4pJgjYTdj8RiXc8iLnQC8X6JC3RUywuA@mail.gmail.com>
Date: Mon, 8 May 2023 19:31:12 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, kuba@...nel.org, kuni1840@...il.com,
netdev@...r.kernel.org, pabeni@...hat.com, syzkaller@...glegroups.com
Subject: Re: [PATCH v2 net] net: Fix sk->sk_stamp race in sock_recv_cmsgs().
On Mon, May 8, 2023 at 7:20 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Eric Dumazet <edumazet@...gle.com>
> Date: Mon, 8 May 2023 19:08:58 +0200
> > On Mon, May 8, 2023 at 6:58 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> > >
> > > KCSAN found a data race in sock_recv_cmsgs() [0] where the read access
> > > to sk->sk_stamp needs READ_ONCE().
> > >
> > > Also, there is another race like below. If the torn load of the high
> > > 32-bits precedes WRITE_ONCE(sk, skb->tstamp) and later the written
> > > lower 32-bits happens to match with SK_DEFAULT_STAMP, the final result
> > > of sk->sk_stamp could be 0.
> > >
> > > sock_recv_cmsgs() ioctl(SIOCGSTAMP) sock_recv_cmsgs()
> > > | | |
> > > |- if (sock_flag(sk, SOCK_TIMESTAMP)) |
> > > | | |
> > > | `- sock_set_flag(sk, SOCK_TIMESTAMP)
> > > | |
> > > | `- if (sock_flag(sk, SOCK_TIMESTAMP))
> > > `- if (sk->sk_stamp == SK_DEFAULT_STAMP) `- sock_write_timestamp(sk, skb->tstamp)
> > > `- sock_write_timestamp(sk, 0)
> > >
> > > Even with READ_ONCE(), we could get the same result if READ_ONCE() precedes
> > > WRITE_ONCE() because the SK_DEFAULT_STAMP check and WRITE_ONCE(sk_stamp, 0)
> > > are not atomic.
> > >
> > > Let's avoid the race by cmpxchg() on 64-bits architecture or seqlock on
> > > 32-bits machines.
> > >
> >
> > I disagree. Please use WRITE_ONCE(), even if we know it is racy on 32bit.
> >
> > sock_read_timestamp() and sock_write_timestamp() already are racy, and
> > we do not care.
>
> I think it's not racy since commit 3a0ed3e96197 ("sock: Make sock->sk_stamp
> thread-safe"), which introduced seqlock in sock_read_timestamp() and
> sock_write_timestamp().
Please note I do not care of 32bit.
It is definitely racy on 64bit.
Please look at
commit f75359f3ac855940c5718af10ba089b8977bf339
Author: Eric Dumazet <edumazet@...gle.com>
Date: Mon Nov 4 21:38:43 2019 -0800
net: prevent load/store tearing on sk->sk_stamp
We can not use cmpxchg() only in one place and not the others.
cmpxchg() is expensive, we do not want it here on our fast path.
Thanks.
Powered by blists - more mailing lists