diff mbox series

[net-next] unix: Improve locking scheme in unix_show_fdinfo()

Message ID 9d951e81-2051-5b67-a394-2cb819e5bf57@ya.ru (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next] unix: Improve locking scheme in unix_show_fdinfo() | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang fail Errors and warnings before: 0 this patch: 3
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 9 this patch: 9
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 32 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Kirill Tkhai Jan. 14, 2023, 12:20 a.m. UTC 
After switching to TCP_ESTABLISHED or TCP_LISTEN sk_state, alive SOCK_STREAM
and SOCK_SEQPACKET sockets can't change it anymore (since commit 3ff8bff704f4
"unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()").

Thus, we do not need to take lock here.

Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
---
 net/unix/af_unix.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

kernel test robot Jan. 14, 2023, 5:30 a.m. UTC | #1 
Hi Kirill,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Kirill-Tkhai/unix-Improve-locking-scheme-in-unix_show_fdinfo/20230114-082118
patch link:    https://lore.kernel.org/r/9d951e81-2051-5b67-a394-2cb819e5bf57%40ya.ru
patch subject: [PATCH net-next] unix: Improve locking scheme in unix_show_fdinfo()
config: x86_64-rhel-8.3-rust
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/1c3b5ffa3da1bc362d28489fc860432b09e8a451
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kirill-Tkhai/unix-Improve-locking-scheme-in-unix_show_fdinfo/20230114-082118
        git checkout 1c3b5ffa3da1bc362d28489fc860432b09e8a451
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash net/unix/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/unix/af_unix.c:824:12: warning: variable 'nr_fds' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
                   else if (s_state == TCP_LISTEN)
                            ^~~~~~~~~~~~~~~~~~~~~
   net/unix/af_unix.c:827:34: note: uninitialized use occurs here
                   seq_printf(m, "scm_fds: %u\n", nr_fds);
                                                  ^~~~~~
   net/unix/af_unix.c:824:8: note: remove the 'if' if its condition is always true
                   else if (s_state == TCP_LISTEN)
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~
   net/unix/af_unix.c:812:12: note: initialize the variable 'nr_fds' to silence this warning
           int nr_fds;
                     ^
                      = 0
   1 warning generated.


vim +824 net/unix/af_unix.c

   806	
   807	static void unix_show_fdinfo(struct seq_file *m, struct socket *sock)
   808	{
   809		struct sock *sk = sock->sk;
   810		unsigned char s_state;
   811		struct unix_sock *u;
   812		int nr_fds;
   813	
   814		if (sk) {
   815			s_state = READ_ONCE(sk->sk_state);
   816			u = unix_sk(sk);
   817	
   818			/* SOCK_STREAM and SOCK_SEQPACKET sockets never change their
   819			 * sk_state after switching to TCP_ESTABLISHED or TCP_LISTEN.
   820			 * SOCK_DGRAM is ordinary. So, no lock is needed.
   821			 */
   822			if (sock->type == SOCK_DGRAM || s_state == TCP_ESTABLISHED)
   823				nr_fds = atomic_read(&u->scm_stat.nr_fds);
 > 824			else if (s_state == TCP_LISTEN)
   825				nr_fds = unix_count_nr_fds(sk);
   826	
   827			seq_printf(m, "scm_fds: %u\n", nr_fds);
   828		}
   829	}
   830	#else
   831	#define unix_show_fdinfo NULL
   832	#endif
   833
Kirill Tkhai Jan. 14, 2023, 9:30 a.m. UTC | #2 
On 14.01.2023 08:30, kernel test robot wrote:
> Hi Kirill,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on net-next/master]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Kirill-Tkhai/unix-Improve-locking-scheme-in-unix_show_fdinfo/20230114-082118
> patch link:    https://lore.kernel.org/r/9d951e81-2051-5b67-a394-2cb819e5bf57%40ya.ru
> patch subject: [PATCH net-next] unix: Improve locking scheme in unix_show_fdinfo()
> config: x86_64-rhel-8.3-rust
> compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/1c3b5ffa3da1bc362d28489fc860432b09e8a451
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Kirill-Tkhai/unix-Improve-locking-scheme-in-unix_show_fdinfo/20230114-082118
>         git checkout 1c3b5ffa3da1bc362d28489fc860432b09e8a451
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash net/unix/
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
>>> net/unix/af_unix.c:824:12: warning: variable 'nr_fds' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>                    else if (s_state == TCP_LISTEN)
>                             ^~~~~~~~~~~~~~~~~~~~~
>    net/unix/af_unix.c:827:34: note: uninitialized use occurs here
>                    seq_printf(m, "scm_fds: %u\n", nr_fds);
>                                                   ^~~~~~
>    net/unix/af_unix.c:824:8: note: remove the 'if' if its condition is always true
>                    else if (s_state == TCP_LISTEN)
>                         ^~~~~~~~~~~~~~~~~~~~~~~~~~
>    net/unix/af_unix.c:812:12: note: initialize the variable 'nr_fds' to silence this warning
>            int nr_fds;
>                      ^
>                       = 0

Strange, my gcc didn't warn me... I will send v2.

>    1 warning generated.
> 
> 
> vim +824 net/unix/af_unix.c
> 
>    806	
>    807	static void unix_show_fdinfo(struct seq_file *m, struct socket *sock)
>    808	{
>    809		struct sock *sk = sock->sk;
>    810		unsigned char s_state;
>    811		struct unix_sock *u;
>    812		int nr_fds;
>    813	
>    814		if (sk) {
>    815			s_state = READ_ONCE(sk->sk_state);
>    816			u = unix_sk(sk);
>    817	
>    818			/* SOCK_STREAM and SOCK_SEQPACKET sockets never change their
>    819			 * sk_state after switching to TCP_ESTABLISHED or TCP_LISTEN.
>    820			 * SOCK_DGRAM is ordinary. So, no lock is needed.
>    821			 */
>    822			if (sock->type == SOCK_DGRAM || s_state == TCP_ESTABLISHED)
>    823				nr_fds = atomic_read(&u->scm_stat.nr_fds);
>  > 824			else if (s_state == TCP_LISTEN)
>    825				nr_fds = unix_count_nr_fds(sk);
>    826	
>    827			seq_printf(m, "scm_fds: %u\n", nr_fds);
>    828		}
>    829	}
>    830	#else
>    831	#define unix_show_fdinfo NULL
>    832	#endif
>    833	
>
diff mbox series

Patch

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index f0c2293f1d3b..f98d03fe3942 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -807,23 +807,23 @@  static int unix_count_nr_fds(struct sock *sk)
 static void unix_show_fdinfo(struct seq_file *m, struct socket *sock)
 {
 	struct sock *sk = sock->sk;
+	unsigned char s_state;
 	struct unix_sock *u;
 	int nr_fds;
 
 	if (sk) {
+		s_state = READ_ONCE(sk->sk_state);
 		u = unix_sk(sk);
-		if (sock->type == SOCK_DGRAM) {
-			nr_fds = atomic_read(&u->scm_stat.nr_fds);
-			goto out_print;
-		}
 
-		unix_state_lock(sk);
-		if (sk->sk_state != TCP_LISTEN)
+		/* SOCK_STREAM and SOCK_SEQPACKET sockets never change their
+		 * sk_state after switching to TCP_ESTABLISHED or TCP_LISTEN.
+		 * SOCK_DGRAM is ordinary. So, no lock is needed.
+		 */
+		if (sock->type == SOCK_DGRAM || s_state == TCP_ESTABLISHED)
 			nr_fds = atomic_read(&u->scm_stat.nr_fds);
-		else
+		else if (s_state == TCP_LISTEN)
 			nr_fds = unix_count_nr_fds(sk);
-		unix_state_unlock(sk);
-out_print:
+
 		seq_printf(m, "scm_fds: %u\n", nr_fds);
 	}
 }