bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hawkins Jiawei <yin31149@gmail.com>
To: syzbot+5f26f85569bd179c18ce@syzkaller.appspotmail.com
Cc: 18801353760@163.com, andrii@kernel.org, ast@kernel.org,
	borisp@nvidia.com, bpf@vger.kernel.org, daniel@iogearbox.net,
	davem@davemloft.net, edumazet@google.com, jakub@cloudflare.com,
	john.fastabend@gmail.com, kafai@fb.com, kgraul@linux.ibm.com,
	kpsingh@kernel.org, kuba@kernel.org,
	linux-kernel-mentees@lists.linuxfoundation.org,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	pabeni@redhat.com, paskripkin@gmail.com,
	skhan@linuxfoundation.org, songliubraving@fb.com,
	syzkaller-bugs@googlegroups.com, yhs@fb.com, yin31149@gmail.com,
	Wen Gu <guwen@linux.alibaba.com>
Subject: [PATCH v4] net: fix refcount bug in sk_psock_get (2)
Date: Wed,  3 Aug 2022 20:41:22 +0800	[thread overview]
Message-ID: <20220803124121.173303-1-yin31149@gmail.com> (raw)
In-Reply-To: <00000000000026328205e08cdbeb@google.com>

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
 <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 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_PSOCK) in PTRMASK, to mark whether
sk_user_data points to a 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").

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>
---
v3 -> v4:
  - change new subject
  - fix diff content, which has been edit accidentally

v2 -> v3:
  - use SK_USER_DATA_PSOCK instead of SK_USER_DATA_NOTPSOCK
to patch the bug
  - refactor the code on assigning to sk_user_data field
in psock part
  - refactor the code on getting and setting the flag
with sk_user_data field

v1 -> v2:
  - add bit in PTRMASK to patch the bug

 include/linux/skmsg.h |  2 +-
 include/net/sock.h    | 58 +++++++++++++++++++++++++++++++------------
 net/core/skmsg.c      |  3 ++-
 3 files changed, 45 insertions(+), 18 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..d010910d5879 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -545,14 +545,24 @@ enum sk_pacing {
 	SK_PACING_FQ		= 2,
 };
 
-/* Pointer stored in sk_user_data might not be suitable for copying
- * 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.
+/* flag bits in sk_user_data
+ *
+ * SK_USER_DATA_NOCOPY - Pointer stored in sk_user_data might
+ * not be suitable for copying 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_BPF    - Managed by BPF
+ *
+ * SK_USER_DATA_PSOCK  - Mark whether pointer stored in sk_user_data points
+ * to psock type. This bit should be set when sk_user_data is
+ * assigned to a psock object.
  */
 #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_PSOCK	4UL
+#define SK_USER_DATA_PTRMASK	~(SK_USER_DATA_NOCOPY | SK_USER_DATA_BPF |\
+				  SK_USER_DATA_PSOCK)
 
 /**
  * sk_user_data_is_nocopy - Test if sk_user_data pointer must not be copied
@@ -570,19 +580,35 @@ static inline bool sk_user_data_is_nocopy(const struct sock *sk)
 	void *__tmp = rcu_dereference(__sk_user_data((sk)));		\
 	(void *)((uintptr_t)__tmp & SK_USER_DATA_PTRMASK);		\
 })
-#define rcu_assign_sk_user_data(sk, ptr)				\
+#define rcu_assign_sk_user_data_with_flags(sk, ptr, flags)		\
 ({									\
-	uintptr_t __tmp = (uintptr_t)(ptr);				\
-	WARN_ON_ONCE(__tmp & ~SK_USER_DATA_PTRMASK);			\
-	rcu_assign_pointer(__sk_user_data((sk)), __tmp);		\
-})
-#define rcu_assign_sk_user_data_nocopy(sk, ptr)				\
-({									\
-	uintptr_t __tmp = (uintptr_t)(ptr);				\
-	WARN_ON_ONCE(__tmp & ~SK_USER_DATA_PTRMASK);			\
+	uintptr_t __tmp1 = (uintptr_t)(ptr),				\
+		  __tmp2 = (uintptr_t)(flags);				\
+	WARN_ON_ONCE(__tmp1 & ~SK_USER_DATA_PTRMASK);			\
+	WARN_ON_ONCE(__tmp2 & SK_USER_DATA_PTRMASK);			\
 	rcu_assign_pointer(__sk_user_data((sk)),			\
-			   __tmp | SK_USER_DATA_NOCOPY);		\
+			   __tmp1 | __tmp2);				\
 })
+#define rcu_assign_sk_user_data(sk, ptr)				\
+	rcu_assign_sk_user_data_with_flags(sk, ptr, 0)
+
+/**
+ * rcu_dereference_sk_user_data_psock - return psock if sk_user_data
+ * points to the psock type(SK_USER_DATA_PSOCK flag is set), otherwise
+ * return NULL
+ *
+ * @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_PSOCK)
+		return (struct sk_psock *)(__tmp & SK_USER_DATA_PTRMASK);
+
+	return NULL;
+}
 
 static inline
 struct net *sock_net(const struct sock *sk)
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index b0fcd0200e84..d174897dbb4b 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -735,7 +735,8 @@ struct sk_psock *sk_psock_init(struct sock *sk, int node)
 	sk_psock_set_state(psock, SK_PSOCK_TX_ENABLED);
 	refcount_set(&psock->refcnt, 1);
 
-	rcu_assign_sk_user_data_nocopy(sk, psock);
+	rcu_assign_sk_user_data_with_flags(sk, psock, SK_USER_DATA_NOCOPY |
+						      SK_USER_DATA_PSOCK);
 	sock_hold(sk);
 
 out:
-- 
2.25.1


  parent reply	other threads:[~2022-08-03 12:44 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-03 15:34 [syzbot] WARNING: refcount bug in sk_psock_get (2) syzbot
2022-07-09  2:46 ` [PATCH] smc: fix " Hawkins Jiawei
2022-07-09  3:06   ` Jakub Kicinski
2022-07-09  8:36     ` Hawkins Jiawei
2022-07-11  7:21   ` Wen Gu
2022-07-13  3:10     ` Hawkins Jiawei
2022-07-13  3:33       ` Jakub Kicinski
2022-07-13  3:53         ` Hawkins Jiawei
2022-07-12  9:47   ` Dan Carpenter
2022-07-13  3:35     ` Hawkins Jiawei
2022-07-30  8:56 ` [PATCH v2] net/smc: " Hawkins Jiawei
2022-08-01  9:09   ` Jakub Sitnicki
2022-08-02 14:32     ` Hawkins Jiawei
2022-08-03  8:03 ` [PATCH v3] " Hawkins Jiawei
2022-08-03 11:27   ` Wen Gu
2022-08-03 12:07     ` Hawkins Jiawei
2022-08-03 12:41 ` Hawkins Jiawei [this message]
2022-08-03 15:14   ` [PATCH v4] net: " Jakub Kicinski
2022-08-03 15:37     ` Martin KaFai Lau
2022-08-04  3:05       ` Hawkins Jiawei
2022-08-04 15:29         ` Jakub Kicinski
2022-08-05  6:28           ` Hawkins Jiawei
2022-08-05  7:36 ` [PATCH net v5 0/2] net: enhancements to sk_user_data field Hawkins Jiawei
2022-08-05  7:48   ` [PATCH net v5 1/2] net: fix refcount bug in sk_psock_get (2) Hawkins Jiawei
2022-08-05  7:48   ` [PATCH net v5 2/2] net: refactor bpf_sk_reuseport_detach() Hawkins Jiawei
2022-08-15 18:24     ` Martin KaFai Lau
2022-08-05 10:29   ` [PATCH net v5 0/2] net: enhancements to sk_user_data field Jakub Sitnicki
2022-08-05 15:36     ` Hawkins Jiawei
2022-08-11  5:30   ` patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220803124121.173303-1-yin31149@gmail.com \
    --to=yin31149@gmail.com \
    --cc=18801353760@163.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=borisp@nvidia.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=guwen@linux.alibaba.com \
    --cc=jakub@cloudflare.com \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kgraul@linux.ibm.com \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=paskripkin@gmail.com \
    --cc=skhan@linuxfoundation.org \
    --cc=songliubraving@fb.com \
    --cc=syzbot+5f26f85569bd179c18ce@syzkaller.appspotmail.com \
    --cc=syzkaller-bugs@googlegroups.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).