| Message ID | c6c7084c-56c7-cd37-befe-df718e080597@ya.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b27401a30ee466c4476b035487be0580767ba1fb |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] unix: Improve locking scheme in unix_show_fdinfo() | expand |
From: Kirill Tkhai <tkhai@ya.ru> Date: Sat, 14 Jan 2023 12:35:02 +0300 > 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> > --- > v2: Initialize "nr_fds = 0". Yes, this is necessary because the new if-else does not cover (SOCK_STREAM, TCP_CLOSE), and in such a case, nr_fds is uninitialised val with the v1 patch. This version looks good. Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com> > > net/unix/af_unix.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index f0c2293f1d3b..009616fa0256 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; > + int nr_fds = 0; > > 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); > } > }
Hello: This patch was applied to netdev/net-next.git (master) by David S. Miller <davem@davemloft.net>: On Sat, 14 Jan 2023 12:35:02 +0300 you wrote: > 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> > > [...] Here is the summary with links: - [net-next,v2] unix: Improve locking scheme in unix_show_fdinfo() https://git.kernel.org/netdev/net-next/c/b27401a30ee4 You are awesome, thank you!
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index f0c2293f1d3b..009616fa0256 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; + int nr_fds = 0; 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); } }
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> --- v2: Initialize "nr_fds = 0". net/unix/af_unix.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)