| Message ID | tencent_F35C58B90E47D014455212BC7110EDBB2106@qq.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: use READ_ONCE() to read in concurrent environment | expand |
From: linke li <lilinke99@qq.com> Date: Tue, 23 Jan 2024 04:24:46 +0800 > In function sk_stream_wait_memory(), reads of sk->sk_err and sk->sk_shutdown > is protected using READ_ONCE() in line 145, 146. > 145: ret = sk_wait_event(sk, ¤t_timeo, READ_ONCE(sk->sk_err) || > 146: (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN) || > > But reads in line 133 are not protected. This may cause unexpected error > when other threads change sk->sk_err and sk->sk_shutdown. Function > sk_stream_wait_connect() has same problem. > > There is patch similar to this. https://github.com/torvalds/linux/commit/c1c0ce31b2420d5c173228a2132a492ede03d81f > This patch find two read of same variable while one is protected, another > is not. And READ_ONCE() is added to protect. This is not sufficient. You need to add WRITE_ONCE() on the writer side as well. Also, you can disambiguate Subject by mentioning sk_state or sk_stream_wait_connect(). Thanks! > > Signed-off-by: linke li <lilinke99@qq.com> > --- > net/core/stream.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/stream.c b/net/core/stream.c > index b16dfa568a2d..7e67a2bf4480 100644 > --- a/net/core/stream.c > +++ b/net/core/stream.c > @@ -63,7 +63,7 @@ int sk_stream_wait_connect(struct sock *sk, long *timeo_p) > int err = sock_error(sk); > if (err) > return err; > - if ((1 << sk->sk_state) & ~(TCPF_SYN_SENT | TCPF_SYN_RECV)) > + if ((1 << READ_ONCE(sk->sk_state)) & ~(TCPF_SYN_SENT | TCPF_SYN_RECV)) > return -EPIPE; > if (!*timeo_p) > return -EAGAIN; > @@ -130,7 +130,7 @@ int sk_stream_wait_memory(struct sock *sk, long *timeo_p) > while (1) { > sk_set_bit(SOCKWQ_ASYNC_NOSPACE, sk); > > - if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN)) > + if (READ_ONCE(sk->sk_err) || (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN)) > goto do_error; > if (!*timeo_p) > goto do_eagain; > -- > 2.39.3 (Apple Git-145)
On Tue, 2024-01-23 at 04:24 +0800, linke li wrote: > In function sk_stream_wait_memory(), reads of sk->sk_err and sk->sk_shutdown > is protected using READ_ONCE() in line 145, 146. > 145: ret = sk_wait_event(sk, ¤t_timeo, READ_ONCE(sk->sk_err) || > 146: (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN) || The above read happens outside the sk socket lock, see the sk_wait_event() macro definition... > > But reads in line 133 are not protected. ... while the above one happens under the sk socket lock. The _ONCE annotation is not needed. > This may cause unexpected error > when other threads change sk->sk_err and sk->sk_shutdown. Function > sk_stream_wait_connect() has same problem. Same as above, the access with the _ONCE() annotation is outside the socket lock and vice versa. I think this patch is not needed. Thanks, Paolo
diff --git a/net/core/stream.c b/net/core/stream.c index b16dfa568a2d..7e67a2bf4480 100644 --- a/net/core/stream.c +++ b/net/core/stream.c @@ -63,7 +63,7 @@ int sk_stream_wait_connect(struct sock *sk, long *timeo_p) int err = sock_error(sk); if (err) return err; - if ((1 << sk->sk_state) & ~(TCPF_SYN_SENT | TCPF_SYN_RECV)) + if ((1 << READ_ONCE(sk->sk_state)) & ~(TCPF_SYN_SENT | TCPF_SYN_RECV)) return -EPIPE; if (!*timeo_p) return -EAGAIN; @@ -130,7 +130,7 @@ int sk_stream_wait_memory(struct sock *sk, long *timeo_p) while (1) { sk_set_bit(SOCKWQ_ASYNC_NOSPACE, sk); - if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN)) + if (READ_ONCE(sk->sk_err) || (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN)) goto do_error; if (!*timeo_p) goto do_eagain;
In function sk_stream_wait_memory(), reads of sk->sk_err and sk->sk_shutdown is protected using READ_ONCE() in line 145, 146. 145: ret = sk_wait_event(sk, ¤t_timeo, READ_ONCE(sk->sk_err) || 146: (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN) || But reads in line 133 are not protected. This may cause unexpected error when other threads change sk->sk_err and sk->sk_shutdown. Function sk_stream_wait_connect() has same problem. There is patch similar to this. https://github.com/torvalds/linux/commit/c1c0ce31b2420d5c173228a2132a492ede03d81f This patch find two read of same variable while one is protected, another is not. And READ_ONCE() is added to protect. Signed-off-by: linke li <lilinke99@qq.com> --- net/core/stream.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)