| Message ID | 20220730085654.2598-1-yin31149@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net/smc: fix refcount bug in sk_psock_get (2) | expand |
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. [...]
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.
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 */
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(-)