diff mbox series

[net-next] net: annotate data-races around sk->sk_lingertime

Message ID 20230818021132.2796092-1-edumazet@google.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: annotate data-races around sk->sk_lingertime | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1338 this patch: 1338
netdev/cc_maintainers warning 16 maintainers not CCed: kgraul@linux.ibm.com luiz.dentz@gmail.com jhs@mojatatu.com alibuda@linux.alibaba.com wenjia@linux.ibm.com linux-s390@vger.kernel.org johan.hedberg@gmail.com kuniyu@amazon.com marcel@holtmann.org jaka@linux.ibm.com martin.lau@kernel.org tonylu@linux.alibaba.com linux-bluetooth@vger.kernel.org xiyou.wangcong@gmail.com jiri@resnulli.us guwen@linux.alibaba.com
netdev/build_clang fail Errors and warnings before: 1353 this patch: 1355
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1361 this patch: 1361
netdev/checkpatch warning WARNING: line length of 84 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Eric Dumazet Aug. 18, 2023, 2:11 a.m. UTC 
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(-)

Comments

Jason Xing Aug. 18, 2023, 6:40 a.m. UTC | #1 
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
>
>
Jakub Kicinski Aug. 19, 2023, 2:28 a.m. UTC | #2 
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?
Eric Dumazet Aug. 19, 2023, 3:03 a.m. UTC | #3 
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.
Jakub Kicinski Aug. 19, 2023, 3:04 a.m. UTC | #4 
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.
Eric Dumazet Aug. 19, 2023, 4:02 a.m. UTC | #5 
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.
diff mbox series

Patch

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 */