* [PATCH net-next] net: annotate data-races around sk->sk_lingertime
@ 2023-08-18 2:11 Eric Dumazet
2023-08-18 6:40 ` Jason Xing
2023-08-19 2:28 ` Jakub Kicinski
0 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2023-08-18 2:11 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: netdev, eric.dumazet, Eric Dumazet
sk_getsockopt() runs locklessly. This means sk->sk_lingertime
can be read while other threads are changing its value.
Other reads also happen without socket lock being held,
and must be annotated.
Remove preprocessor logic using BITS_PER_LONG, compilers
are smart enough to figure this by themselves.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/bluetooth/iso.c | 2 +-
net/bluetooth/sco.c | 2 +-
net/core/sock.c | 18 +++++++++---------
net/sched/em_meta.c | 2 +-
net/smc/af_smc.c | 2 +-
5 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 6b66d6a88b9a22adf208b24e0a31d6f236355d9b..3c03e49422c7519167a0a2a6f5bdc8af5b2c0cd0 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1475,7 +1475,7 @@ static int iso_sock_release(struct socket *sock)
iso_sock_close(sk);
- if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime &&
+ if (sock_flag(sk, SOCK_LINGER) && READ_ONCE(sk->sk_lingertime) &&
!(current->flags & PF_EXITING)) {
lock_sock(sk);
err = bt_sock_wait_state(sk, BT_CLOSED, sk->sk_lingertime);
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 50ad5935ae47a31cb3d11a8b56f7d462cbaf2366..c736186aba26beadccd76c66f0af72835d740551 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -1245,7 +1245,7 @@ static int sco_sock_release(struct socket *sock)
sco_sock_close(sk);
- if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime &&
+ if (sock_flag(sk, SOCK_LINGER) && READ_ONCE(sk->sk_lingertime) &&
!(current->flags & PF_EXITING)) {
lock_sock(sk);
err = bt_sock_wait_state(sk, BT_CLOSED, sk->sk_lingertime);
diff --git a/net/core/sock.c b/net/core/sock.c
index 22d94394335fb75f12da65368e87c5a65167cc0e..e11952aee3777a5df51abdf70d30fbd3ec3a50fc 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -797,7 +797,7 @@ EXPORT_SYMBOL(sock_set_reuseport);
void sock_no_linger(struct sock *sk)
{
lock_sock(sk);
- sk->sk_lingertime = 0;
+ WRITE_ONCE(sk->sk_lingertime, 0);
sock_set_flag(sk, SOCK_LINGER);
release_sock(sk);
}
@@ -1230,15 +1230,15 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
ret = -EFAULT;
break;
}
- if (!ling.l_onoff)
+ if (!ling.l_onoff) {
sock_reset_flag(sk, SOCK_LINGER);
- else {
-#if (BITS_PER_LONG == 32)
- if ((unsigned int)ling.l_linger >= MAX_SCHEDULE_TIMEOUT/HZ)
- sk->sk_lingertime = MAX_SCHEDULE_TIMEOUT;
+ } else {
+ unsigned int t_sec = ling.l_linger;
+
+ if (t_sec >= MAX_SCHEDULE_TIMEOUT / HZ)
+ WRITE_ONCE(sk->sk_lingertime, MAX_SCHEDULE_TIMEOUT);
else
-#endif
- sk->sk_lingertime = (unsigned int)ling.l_linger * HZ;
+ WRITE_ONCE(sk->sk_lingertime, t_sec * HZ);
sock_set_flag(sk, SOCK_LINGER);
}
break;
@@ -1692,7 +1692,7 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
case SO_LINGER:
lv = sizeof(v.ling);
v.ling.l_onoff = sock_flag(sk, SOCK_LINGER);
- v.ling.l_linger = sk->sk_lingertime / HZ;
+ v.ling.l_linger = READ_ONCE(sk->sk_lingertime) / HZ;
break;
case SO_BSDCOMPAT:
diff --git a/net/sched/em_meta.c b/net/sched/em_meta.c
index 6fdba069f6bfd306fa68fc2e68bdcaf0cf4d4e9e..da34fd4c92695f453f1d6547c6e4e8d3afe7a116 100644
--- a/net/sched/em_meta.c
+++ b/net/sched/em_meta.c
@@ -502,7 +502,7 @@ META_COLLECTOR(int_sk_lingertime)
*err = -1;
return;
}
- dst->value = sk->sk_lingertime / HZ;
+ dst->value = READ_ONCE(sk->sk_lingertime) / HZ;
}
META_COLLECTOR(int_sk_err_qlen)
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index f5834af5fad535c420381827548cecdf0d03b0d5..7c77565c39d19c7f1baf1184c4a5bf950c9cfe33 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1820,7 +1820,7 @@ void smc_close_non_accepted(struct sock *sk)
lock_sock(sk);
if (!sk->sk_lingertime)
/* wait for peer closing */
- sk->sk_lingertime = SMC_MAX_STREAM_WAIT_TIMEOUT;
+ WRITE_ONCE(sk->sk_lingertime, SMC_MAX_STREAM_WAIT_TIMEOUT);
__smc_release(smc);
release_sock(sk);
sock_put(sk); /* sock_hold above */
--
2.42.0.rc1.204.g551eb34607-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net-next] net: annotate data-races around sk->sk_lingertime
2023-08-18 2:11 [PATCH net-next] net: annotate data-races around sk->sk_lingertime Eric Dumazet
@ 2023-08-18 6:40 ` Jason Xing
2023-08-19 2:28 ` Jakub Kicinski
1 sibling, 0 replies; 6+ messages in thread
From: Jason Xing @ 2023-08-18 6:40 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet
On Fri, Aug 18, 2023 at 10:11 AM Eric Dumazet <edumazet@google.com> wrote:
>
> sk_getsockopt() runs locklessly. This means sk->sk_lingertime
> can be read while other threads are changing its value.
>
> Other reads also happen without socket lock being held,
> and must be annotated.
>
> Remove preprocessor logic using BITS_PER_LONG, compilers
> are smart enough to figure this by themselves.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>
Thanks!
> ---
> net/bluetooth/iso.c | 2 +-
> net/bluetooth/sco.c | 2 +-
> net/core/sock.c | 18 +++++++++---------
> net/sched/em_meta.c | 2 +-
> net/smc/af_smc.c | 2 +-
> 5 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 6b66d6a88b9a22adf208b24e0a31d6f236355d9b..3c03e49422c7519167a0a2a6f5bdc8af5b2c0cd0 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -1475,7 +1475,7 @@ static int iso_sock_release(struct socket *sock)
>
> iso_sock_close(sk);
>
> - if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime &&
> + if (sock_flag(sk, SOCK_LINGER) && READ_ONCE(sk->sk_lingertime) &&
> !(current->flags & PF_EXITING)) {
> lock_sock(sk);
> err = bt_sock_wait_state(sk, BT_CLOSED, sk->sk_lingertime);
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 50ad5935ae47a31cb3d11a8b56f7d462cbaf2366..c736186aba26beadccd76c66f0af72835d740551 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -1245,7 +1245,7 @@ static int sco_sock_release(struct socket *sock)
>
> sco_sock_close(sk);
>
> - if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime &&
> + if (sock_flag(sk, SOCK_LINGER) && READ_ONCE(sk->sk_lingertime) &&
> !(current->flags & PF_EXITING)) {
> lock_sock(sk);
> err = bt_sock_wait_state(sk, BT_CLOSED, sk->sk_lingertime);
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 22d94394335fb75f12da65368e87c5a65167cc0e..e11952aee3777a5df51abdf70d30fbd3ec3a50fc 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -797,7 +797,7 @@ EXPORT_SYMBOL(sock_set_reuseport);
> void sock_no_linger(struct sock *sk)
> {
> lock_sock(sk);
> - sk->sk_lingertime = 0;
> + WRITE_ONCE(sk->sk_lingertime, 0);
> sock_set_flag(sk, SOCK_LINGER);
> release_sock(sk);
> }
> @@ -1230,15 +1230,15 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
> ret = -EFAULT;
> break;
> }
> - if (!ling.l_onoff)
> + if (!ling.l_onoff) {
> sock_reset_flag(sk, SOCK_LINGER);
> - else {
> -#if (BITS_PER_LONG == 32)
> - if ((unsigned int)ling.l_linger >= MAX_SCHEDULE_TIMEOUT/HZ)
> - sk->sk_lingertime = MAX_SCHEDULE_TIMEOUT;
> + } else {
> + unsigned int t_sec = ling.l_linger;
> +
> + if (t_sec >= MAX_SCHEDULE_TIMEOUT / HZ)
> + WRITE_ONCE(sk->sk_lingertime, MAX_SCHEDULE_TIMEOUT);
> else
> -#endif
> - sk->sk_lingertime = (unsigned int)ling.l_linger * HZ;
> + WRITE_ONCE(sk->sk_lingertime, t_sec * HZ);
> sock_set_flag(sk, SOCK_LINGER);
> }
> break;
> @@ -1692,7 +1692,7 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
> case SO_LINGER:
> lv = sizeof(v.ling);
> v.ling.l_onoff = sock_flag(sk, SOCK_LINGER);
> - v.ling.l_linger = sk->sk_lingertime / HZ;
> + v.ling.l_linger = READ_ONCE(sk->sk_lingertime) / HZ;
> break;
>
> case SO_BSDCOMPAT:
> diff --git a/net/sched/em_meta.c b/net/sched/em_meta.c
> index 6fdba069f6bfd306fa68fc2e68bdcaf0cf4d4e9e..da34fd4c92695f453f1d6547c6e4e8d3afe7a116 100644
> --- a/net/sched/em_meta.c
> +++ b/net/sched/em_meta.c
> @@ -502,7 +502,7 @@ META_COLLECTOR(int_sk_lingertime)
> *err = -1;
> return;
> }
> - dst->value = sk->sk_lingertime / HZ;
> + dst->value = READ_ONCE(sk->sk_lingertime) / HZ;
> }
>
> META_COLLECTOR(int_sk_err_qlen)
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index f5834af5fad535c420381827548cecdf0d03b0d5..7c77565c39d19c7f1baf1184c4a5bf950c9cfe33 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1820,7 +1820,7 @@ void smc_close_non_accepted(struct sock *sk)
> lock_sock(sk);
> if (!sk->sk_lingertime)
> /* wait for peer closing */
> - sk->sk_lingertime = SMC_MAX_STREAM_WAIT_TIMEOUT;
> + WRITE_ONCE(sk->sk_lingertime, SMC_MAX_STREAM_WAIT_TIMEOUT);
> __smc_release(smc);
> release_sock(sk);
> sock_put(sk); /* sock_hold above */
> --
> 2.42.0.rc1.204.g551eb34607-goog
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] net: annotate data-races around sk->sk_lingertime
2023-08-18 2:11 [PATCH net-next] net: annotate data-races around sk->sk_lingertime Eric Dumazet
2023-08-18 6:40 ` Jason Xing
@ 2023-08-19 2:28 ` Jakub Kicinski
2023-08-19 3:03 ` Eric Dumazet
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2023-08-19 2:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, Paolo Abeni, netdev, eric.dumazet
On Fri, 18 Aug 2023 02:11:32 +0000 Eric Dumazet wrote:
> Remove preprocessor logic using BITS_PER_LONG, compilers
> are smart enough to figure this by themselves.
clang does complain that we're basically comparing an in to a MAX_LONG:
net/core/sock.c:1238:14: warning: result of comparison of constant 36893488147419103 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
if (t_sec >= MAX_SCHEDULE_TIMEOUT / HZ)
~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
Can we shut this up somehow? Or don't care?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] net: annotate data-races around sk->sk_lingertime
2023-08-19 2:28 ` Jakub Kicinski
@ 2023-08-19 3:03 ` Eric Dumazet
2023-08-19 3:04 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2023-08-19 3:03 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: David S . Miller, Paolo Abeni, netdev, eric.dumazet
On Sat, Aug 19, 2023 at 4:28 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 18 Aug 2023 02:11:32 +0000 Eric Dumazet wrote:
> > Remove preprocessor logic using BITS_PER_LONG, compilers
> > are smart enough to figure this by themselves.
>
> clang does complain that we're basically comparing an in to a MAX_LONG:
>
> net/core/sock.c:1238:14: warning: result of comparison of constant 36893488147419103 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
> if (t_sec >= MAX_SCHEDULE_TIMEOUT / HZ)
> ~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
>
Ah... I thought I was using clang.... Let me check again.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: annotate data-races around sk->sk_lingertime
2023-08-19 3:03 ` Eric Dumazet
@ 2023-08-19 3:04 ` Jakub Kicinski
2023-08-19 4:02 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2023-08-19 3:04 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, Paolo Abeni, netdev, eric.dumazet
On Sat, 19 Aug 2023 05:03:47 +0200 Eric Dumazet wrote:
> > net/core/sock.c:1238:14: warning: result of comparison of constant 36893488147419103 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
> > if (t_sec >= MAX_SCHEDULE_TIMEOUT / HZ)
> > ~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
> >
>
> Ah... I thought I was using clang.... Let me check again.
Could be a W=1 warning.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: annotate data-races around sk->sk_lingertime
2023-08-19 3:04 ` Jakub Kicinski
@ 2023-08-19 4:02 ` Eric Dumazet
0 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2023-08-19 4:02 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: David S . Miller, Paolo Abeni, netdev, eric.dumazet
On Sat, Aug 19, 2023 at 5:04 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 19 Aug 2023 05:03:47 +0200 Eric Dumazet wrote:
> > > net/core/sock.c:1238:14: warning: result of comparison of constant 36893488147419103 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
> > > if (t_sec >= MAX_SCHEDULE_TIMEOUT / HZ)
> > > ~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> >
> > Ah... I thought I was using clang.... Let me check again.
>
> Could be a W=1 warning.
Indeed, I will use an "unsigned long t_sec" to remove the warning,
(compiler also removes the dead branch in 64bit builds)
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-19 4:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-18 2:11 [PATCH net-next] net: annotate data-races around sk->sk_lingertime Eric Dumazet
2023-08-18 6:40 ` Jason Xing
2023-08-19 2:28 ` Jakub Kicinski
2023-08-19 3:03 ` Eric Dumazet
2023-08-19 3:04 ` Jakub Kicinski
2023-08-19 4:02 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).