[PATCH v2] net/smc: fix refcount bug in sk_psock_get (2)

[PATCH v2] net/smc: fix refcount bug in sk_psock_get (2)

[Hawkins Jiawei]

Syzkaller reports refcount bug as follows:
------------[ cut here ]------------
refcount_t: saturated; leaking memory.
WARNING: CPU: 1 PID: 3605 at lib/refcount.c:19 refcount_warn_saturate+0xf4/0x1e0 lib/refcount.c:19
Modules linked in:
CPU: 1 PID: 3605 Comm: syz-executor208 Not tainted 5.18.0-syzkaller-03023-g7e062cda7d90 #0
...
Call Trace:
 <TASK>
 __refcount_add_not_zero include/linux/refcount.h:163 [inline]
 __refcount_inc_not_zero include/linux/refcount.h:227 [inline]
 refcount_inc_not_zero include/linux/refcount.h:245 [inline]
 sk_psock_get+0x3bc/0x410 include/linux/skmsg.h:439
 tls_data_ready+0x6d/0x1b0 net/tls/tls_sw.c:2091
 tcp_data_ready+0x106/0x520 net/ipv4/tcp_input.c:4983
 tcp_data_queue+0x25f2/0x4c90 net/ipv4/tcp_input.c:5057
 tcp_rcv_state_process+0x1774/0x4e80 net/ipv4/tcp_input.c:6659
 tcp_v4_do_rcv+0x339/0x980 net/ipv4/tcp_ipv4.c:1682
 sk_backlog_rcv include/net/sock.h:1061 [inline]
 __release_sock+0x134/0x3b0 net/core/sock.c:2849
 release_sock+0x54/0x1b0 net/core/sock.c:3404
 inet_shutdown+0x1e0/0x430 net/ipv4/af_inet.c:909
 __sys_shutdown_sock net/socket.c:2331 [inline]
 __sys_shutdown_sock net/socket.c:2325 [inline]
 __sys_shutdown+0xf1/0x1b0 net/socket.c:2343
 __do_sys_shutdown net/socket.c:2351 [inline]
 __se_sys_shutdown net/socket.c:2349 [inline]
 __x64_sys_shutdown+0x50/0x70 net/socket.c:2349
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x46/0xb0
 </TASK>

During SMC fallback process in connect syscall, kernel will
replaces TCP with SMC. In order to forward wakeup
smc socket waitqueue after fallback, kernel will sets
clcsk->sk_user_data to origin smc socket in
smc_fback_replace_callbacks().

Later, in shutdown syscall, kernel will calls
sk_psock_get(), which treats the clcsk->sk_user_data
as sk_psock type, triggering the refcnt warning.

So, the root cause is that smc and psock, both will use
sk_user_data field. So they will mismatch this field
easily.

This patch solves it by using another bit(defined as
SK_USER_DATA_NOTPSOCK) in PTRMASK, to mark whether
sk_user_data points to a sk_psock object or not.
This patch depends on a PTRMASK introduced in commit f1ff5ce2cd5e
("net, sk_msg: Clear sk_user_data pointer on clone if tagged").

Fixes: 341adeec9ada ("net/smc: Forward wakeup to smc socket waitqueue after fallback")
Fixes: a60a2b1e0af1 ("net/smc: reduce active tcp_listen workers")
Reported-and-tested-by: syzbot+5f26f85569bd179c18ce@syzkaller.appspotmail.com
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Acked-by: Wen Gu <guwen@linux.alibaba.com>
Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
v1 -> v2: 
  - add bit in PTRMASK to patch the bug

 include/linux/skmsg.h |  2 +-
 include/net/sock.h    | 27 +++++++++++++++++++++++++--
 net/smc/af_smc.c      |  6 ++++--
 net/smc/smc.h         |  2 +-
 4 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index c5a2d6f50f25..81bfa1a33623 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -277,7 +277,7 @@ static inline void sk_msg_sg_copy_clear(struct sk_msg *msg, u32 start)
 
 static inline struct sk_psock *sk_psock(const struct sock *sk)
 {
-	return rcu_dereference_sk_user_data(sk);
+	return rcu_dereference_sk_user_data_psock(sk);
 }
 
 static inline void sk_psock_set_state(struct sk_psock *psock,
diff --git a/include/net/sock.h b/include/net/sock.h
index 9fa54762e077..316c0313b2bf 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -549,10 +549,17 @@ enum sk_pacing {
  * when cloning the socket. For instance, it can point to a reference
  * counted object. sk_user_data bottom bit is set if pointer must not
  * be copied.
+ *
+ * SK_USER_DATA_NOCOPY   - test if pointer must not copied
+ * SK_USER_DATA_BPF      - managed by BPF
+ * SK_USER_DATA_NOTPSOCK - test if pointer points to psock
  */
 #define SK_USER_DATA_NOCOPY	1UL
-#define SK_USER_DATA_BPF	2UL	/* Managed by BPF */
-#define SK_USER_DATA_PTRMASK	~(SK_USER_DATA_NOCOPY | SK_USER_DATA_BPF)
+#define SK_USER_DATA_BPF	2UL
+#define SK_USER_DATA_NOTPSOCK	4UL
+#define SK_USER_DATA_PTRMASK	~(SK_USER_DATA_NOCOPY | SK_USER_DATA_BPF |\
+				  SK_USER_DATA_NOTPSOCK)
+
 
 /**
  * sk_user_data_is_nocopy - Test if sk_user_data pointer must not be copied
@@ -584,6 +591,22 @@ static inline bool sk_user_data_is_nocopy(const struct sock *sk)
 			   __tmp | SK_USER_DATA_NOCOPY);		\
 })
 
+/**
+ * rcu_dereference_sk_user_data_psock - return psock if sk_user_data points
+ * to the psock
+ * @sk: socket
+ */
+static inline
+struct sk_psock *rcu_dereference_sk_user_data_psock(const struct sock *sk)
+{
+	uintptr_t __tmp = (uintptr_t)rcu_dereference(__sk_user_data((sk)));
+
+	if (__tmp & SK_USER_DATA_NOTPSOCK)
+		return NULL;
+	return (struct sk_psock *)(__tmp & SK_USER_DATA_PTRMASK);
+}
+
+
 static inline
 struct net *sock_net(const struct sock *sk)
 {
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 433bb5a7df31..d0feccf824c8 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -812,7 +812,8 @@ static void smc_fback_replace_callbacks(struct smc_sock *smc)
 	struct sock *clcsk = smc->clcsock->sk;
 
 	write_lock_bh(&clcsk->sk_callback_lock);
-	clcsk->sk_user_data = (void *)((uintptr_t)smc | SK_USER_DATA_NOCOPY);
+	clcsk->sk_user_data = (void *)((uintptr_t)smc | SK_USER_DATA_NOCOPY |
+				       SK_USER_DATA_NOTPSOCK);
 
 	smc_clcsock_replace_cb(&clcsk->sk_state_change, smc_fback_state_change,
 			       &smc->clcsk_state_change);
@@ -2470,7 +2471,8 @@ static int smc_listen(struct socket *sock, int backlog)
 	 */
 	write_lock_bh(&smc->clcsock->sk->sk_callback_lock);
 	smc->clcsock->sk->sk_user_data =
-		(void *)((uintptr_t)smc | SK_USER_DATA_NOCOPY);
+		(void *)((uintptr_t)smc | SK_USER_DATA_NOCOPY |
+			 SK_USER_DATA_NOTPSOCK);
 	smc_clcsock_replace_cb(&smc->clcsock->sk->sk_data_ready,
 			       smc_clcsock_data_ready, &smc->clcsk_data_ready);
 	write_unlock_bh(&smc->clcsock->sk->sk_callback_lock);
diff --git a/net/smc/smc.h b/net/smc/smc.h
index 5ed765ea0c73..c24d0469d267 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -299,7 +299,7 @@ static inline void smc_init_saved_callbacks(struct smc_sock *smc)
 static inline struct smc_sock *smc_clcsock_user_data(const struct sock *clcsk)
 {
 	return (struct smc_sock *)
-	       ((uintptr_t)clcsk->sk_user_data & ~SK_USER_DATA_NOCOPY);
+	       ((uintptr_t)clcsk->sk_user_data & SK_USER_DATA_PTRMASK);
 }
 
 /* save target_cb in saved_cb, and replace target_cb with new_cb */
-- 
2.25.1

Re: [PATCH v2] net/smc: fix refcount bug in sk_psock_get (2)

[Jakub Sitnicki]

Hi,

On Sat, Jul 30, 2022 at 04:56 PM +08, Hawkins Jiawei wrote:
> Syzkaller reports refcount bug as follows:
> ------------[ cut here ]------------
> refcount_t: saturated; leaking memory.
> WARNING: CPU: 1 PID: 3605 at lib/refcount.c:19 refcount_warn_saturate+0xf4/0x1e0 lib/refcount.c:19
> Modules linked in:
> CPU: 1 PID: 3605 Comm: syz-executor208 Not tainted 5.18.0-syzkaller-03023-g7e062cda7d90 #0
> ...
> Call Trace:
>  <TASK>
>  __refcount_add_not_zero include/linux/refcount.h:163 [inline]
>  __refcount_inc_not_zero include/linux/refcount.h:227 [inline]
>  refcount_inc_not_zero include/linux/refcount.h:245 [inline]
>  sk_psock_get+0x3bc/0x410 include/linux/skmsg.h:439
>  tls_data_ready+0x6d/0x1b0 net/tls/tls_sw.c:2091
>  tcp_data_ready+0x106/0x520 net/ipv4/tcp_input.c:4983
>  tcp_data_queue+0x25f2/0x4c90 net/ipv4/tcp_input.c:5057
>  tcp_rcv_state_process+0x1774/0x4e80 net/ipv4/tcp_input.c:6659
>  tcp_v4_do_rcv+0x339/0x980 net/ipv4/tcp_ipv4.c:1682
>  sk_backlog_rcv include/net/sock.h:1061 [inline]
>  __release_sock+0x134/0x3b0 net/core/sock.c:2849
>  release_sock+0x54/0x1b0 net/core/sock.c:3404
>  inet_shutdown+0x1e0/0x430 net/ipv4/af_inet.c:909
>  __sys_shutdown_sock net/socket.c:2331 [inline]
>  __sys_shutdown_sock net/socket.c:2325 [inline]
>  __sys_shutdown+0xf1/0x1b0 net/socket.c:2343
>  __do_sys_shutdown net/socket.c:2351 [inline]
>  __se_sys_shutdown net/socket.c:2349 [inline]
>  __x64_sys_shutdown+0x50/0x70 net/socket.c:2349
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x46/0xb0
>  </TASK>
>
> During SMC fallback process in connect syscall, kernel will
> replaces TCP with SMC. In order to forward wakeup
> smc socket waitqueue after fallback, kernel will sets
> clcsk->sk_user_data to origin smc socket in
> smc_fback_replace_callbacks().
>
> Later, in shutdown syscall, kernel will calls
> sk_psock_get(), which treats the clcsk->sk_user_data
> as sk_psock type, triggering the refcnt warning.
>
> So, the root cause is that smc and psock, both will use
> sk_user_data field. So they will mismatch this field
> easily.
>
> This patch solves it by using another bit(defined as
> SK_USER_DATA_NOTPSOCK) in PTRMASK, to mark whether
> sk_user_data points to a sk_psock object or not.
> This patch depends on a PTRMASK introduced in commit f1ff5ce2cd5e
> ("net, sk_msg: Clear sk_user_data pointer on clone if tagged").
>
> Fixes: 341adeec9ada ("net/smc: Forward wakeup to smc socket waitqueue after fallback")
> Fixes: a60a2b1e0af1 ("net/smc: reduce active tcp_listen workers")
> Reported-and-tested-by: syzbot+5f26f85569bd179c18ce@syzkaller.appspotmail.com
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Acked-by: Wen Gu <guwen@linux.alibaba.com>
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> ---

Since using psock is not the common case, I'm wondering if it makes more
sense to have an inverse flag - SK_USER_DATA_PSOCK. Flag would be set by
the psock code on assignment to sk_user_data.

This way we would also avoid some confusion. With the change below, the
SK_USER_DATA_NOTPSOCK is not *always* set when sk_user_data holds a
non-psock pointer. Only when SMC sets it.

If we go with the current approach, the rest of sites, execpt for psock,
that assign to sk_user_data should be updated to set
SK_USER_DATA_NOTPSOCK as well, IMO.

That is why I'd do it the other way.

[...]

Re: [PATCH v2] net/smc: fix refcount bug in sk_psock_get (2)

[Hawkins Jiawei]

Thanks for your suggestion!

On Mon, 1 Aug 2022 at 17:16, Jakub Sitnicki <jakub@cloudflare.com> wrote:
> This way we would also avoid some confusion. With the change below, the
> SK_USER_DATA_NOTPSOCK is not *always* set when sk_user_data holds a
> non-psock pointer. Only when SMC sets it.
>
> If we go with the current approach, the rest of sites, execpt for psock,
> that assign to sk_user_data should be updated to set
> SK_USER_DATA_NOTPSOCK as well, IMO.
Yes, as you point out, in this patch, this flag's name should be
*SK_USER_DATA_NEEDCHECK_NOTPSOCK*, which is more clearly.

To be more specific, we don't need to set this flag for
every sk_user_data who holds non-psock pointer. Only set the flag for
the site that has been reported involved with the type-mismatch bug
like this bug.
> > During SMC fallback process in connect syscall, kernel will
> > replaces TCP with SMC. In order to forward wakeup
> > smc socket waitqueue after fallback, kernel will sets
> > clcsk->sk_user_data to origin smc socket in
> > smc_fback_replace_callbacks().
> > 
> > Later, in shutdown syscall, kernel will calls
> > sk_psock_get(), which treats the clcsk->sk_user_data
> > as sk_psock type, triggering the refcnt warning.

For other sites, this patch is actually transparent to them, because
the *SK_USER_DATA_NEEDCHECK_NOTPSOCK* flag is always unset. So this
patch won't affect them, which won't introduce any extra
potential bugs.
> +/**
> + * rcu_dereference_sk_user_data_psock - return psock if sk_user_data points
> + * to the psock
> + * @sk: socket
> + */
> +static inline
> +struct sk_psock *rcu_dereference_sk_user_data_psock(const struct sock *sk)
> +{
> +	uintptr_t __tmp = (uintptr_t)rcu_dereference(__sk_user_data((sk)));
> +
> +	if (__tmp & SK_USER_DATA_NOTPSOCK)
> +		return NULL;
> +	return (struct sk_psock *)(__tmp & SK_USER_DATA_PTRMASK);
> +}

>
> Hi,
> Since using psock is not the common case, I'm wondering if it makes more
> sense to have an inverse flag - SK_USER_DATA_PSOCK. Flag would be set by
> the psock code on assignment to sk_user_data.
However, your suggestion seems more elegant. For my patch, as you point
out, when anyone reports a new type-mismatch bug, the relative assign to
sk_user_data should be updated to set *SK_USER_DATA_NEEDCHECK_NOTPSOCK*
flag.

For your suggestion, you seems avoid above situation. What's more, as I
use git grep to search, there seems no direct access to sk_user_data,
all via a small amount macros and wrapper functions. So we can keep
transparent by only update those macros and wrapper functions, which
also won't introduce any extra potential bugs.

I will patch as you suggest in v3 patch.