* [PATCH] smc: fix refcount bug in sk_psock_get (2)
2022-06-03 15:34 [syzbot] WARNING: refcount bug in sk_psock_get (2) syzbot
@ 2022-07-09 2:46 ` Hawkins Jiawei
2022-07-09 3:06 ` Jakub Kicinski
` (2 more replies)
2022-07-30 8:56 ` [PATCH v2] net/smc: " Hawkins Jiawei
` (3 subsequent siblings)
4 siblings, 3 replies; 29+ messages in thread
From: Hawkins Jiawei @ 2022-07-09 2:46 UTC (permalink / raw)
To: syzbot+5f26f85569bd179c18ce
Cc: andrii, ast, borisp, bpf, daniel, davem, edumazet, guwen,
yoshfuji, dsahern, john.fastabend, kafai, kgraul, kpsingh, kuba,
linux-kernel, netdev, pabeni, songliubraving, syzkaller-bugs,
yhs, skhan, 18801353760, paskripkin, linux-kernel-mentees
From: hawk <18801353760@163.com>
Syzkaller reportes 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>
syzbot is try to setup TLS on a SMC socket.
During SMC fallback process in connect syscall, kernel will sets the
smc->sk.sk_socket->file->private_data to smc->clcsock
in smc_switch_to_fallback(), and set smc->clcsock->sk_user_data
to origin smc in smc_fback_replace_callbacks().
When syzbot makes a setsockopt syscall, its argument sockfd
actually points to smc->clcsock, which is not a smc_sock type,
So it won't call smc_setsockopt() in setsockopt syscall,
instead it will call do_tcp_setsockopt() to setup TLS, which
bypasses the fixes 734942cc4ea6, its content is shown as below
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index be3e80b3e27f1..5eff7cccceffc 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -2161,6 +2161,9 @@ static int smc_setsockopt(struct socket *sock,
> int level, int optname,
> struct smc_sock *smc;
> int val, rc;
>
> + if (level == SOL_TCP && optname == TCP_ULP)
> + return -EOPNOTSUPP;
> +
> smc = smc_sk(sk);
>
> /* generic setsockopts reaching us here always apply to the
> @@ -2185,7 +2188,6 @@ static int smc_setsockopt(struct socket *sock,
> int level, int optname,
> if (rc || smc->use_fallback)
> goto out;
> switch (optname) {
> - case TCP_ULP:
> case TCP_FASTOPEN:
> case TCP_FASTOPEN_CONNECT:
> case TCP_FASTOPEN_KEY:
> --
Later, sk_psock_get() will treat the smc->clcsock->sk_user_data
as sk_psock type, which triggers the refcnt warning.
So Just disallow this setup in do_tcp_setsockopt() is OK,
by checking whether sk_user_data points to a SMC socket.
Reported-and-tested-by: syzbot+5f26f85569bd179c18ce@syzkaller.appspotmail.com
Signed-off-by: hawk <18801353760@163.com>
---
net/ipv4/tcp.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 9984d23a7f3e..a1e6cab2c748 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3395,10 +3395,23 @@ static int do_tcp_setsockopt(struct sock *sk, int level, int optname,
}
case TCP_ULP: {
char name[TCP_ULP_NAME_MAX];
+ struct sock *smc_sock;
if (optlen < 1)
return -EINVAL;
+ /* SMC sk_user_data may be treated as psock,
+ * which triggers a refcnt warning.
+ */
+ rcu_read_lock();
+ smc_sock = rcu_dereference_sk_user_data(sk);
+ if (level == SOL_TCP && smc_sock &&
+ smc_sock->__sk_common.skc_family == AF_SMC) {
+ rcu_read_unlock();
+ return -EOPNOTSUPP;
+ }
+ rcu_read_unlock();
+
val = strncpy_from_sockptr(name, optval,
min_t(long, TCP_ULP_NAME_MAX - 1,
optlen));
--
2.25.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH] smc: fix refcount bug in sk_psock_get (2)
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-12 9:47 ` Dan Carpenter
2 siblings, 1 reply; 29+ messages in thread
From: Jakub Kicinski @ 2022-07-09 3:06 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: syzbot+5f26f85569bd179c18ce, andrii, ast, borisp, bpf, daniel,
davem, edumazet, guwen, yoshfuji, dsahern, john.fastabend, kafai,
kgraul, kpsingh, linux-kernel, netdev, pabeni, songliubraving,
syzkaller-bugs, yhs, skhan, 18801353760, paskripkin,
linux-kernel-mentees
On Sat, 9 Jul 2022 10:46:59 +0800 Hawkins Jiawei wrote:
> Reported-and-tested-by: syzbot+5f26f85569bd179c18ce@syzkaller.appspotmail.com
> Signed-off-by: hawk <18801353760@163.com>
> ---
> net/ipv4/tcp.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 9984d23a7f3e..a1e6cab2c748 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -3395,10 +3395,23 @@ static int do_tcp_setsockopt(struct sock *sk, int level, int optname,
> }
> case TCP_ULP: {
> char name[TCP_ULP_NAME_MAX];
> + struct sock *smc_sock;
>
> if (optlen < 1)
> return -EINVAL;
>
> + /* SMC sk_user_data may be treated as psock,
> + * which triggers a refcnt warning.
> + */
> + rcu_read_lock();
> + smc_sock = rcu_dereference_sk_user_data(sk);
> + if (level == SOL_TCP && smc_sock &&
> + smc_sock->__sk_common.skc_family == AF_SMC) {
This should prolly be under the socket lock?
Can we add a bit to SK_USER_DATA_PTRMASK and have ULP-compatible
users (sockmap) opt into ULP cooperation? Modifying TCP is backwards,
layer-wise.
> + rcu_read_unlock();
> + return -EOPNOTSUPP;
> + }
> + rcu_read_unlock();
> +
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] smc: fix refcount bug in sk_psock_get (2)
2022-07-09 3:06 ` Jakub Kicinski
@ 2022-07-09 8:36 ` Hawkins Jiawei
0 siblings, 0 replies; 29+ messages in thread
From: Hawkins Jiawei @ 2022-07-09 8:36 UTC (permalink / raw)
To: kuba
Cc: 18801353760, andrii, ast, borisp, bpf, daniel, davem, dsahern,
edumazet, guwen, john.fastabend, kafai, kgraul, kpsingh,
linux-kernel-mentees, linux-kernel, netdev, pabeni, paskripkin,
skhan, songliubraving, syzbot+5f26f85569bd179c18ce,
syzkaller-bugs, yhs, yin31149, yoshfuji
On Sat, 9 Jul 2022 at 11:06, Jakub Kicinski <kuba@kernel.org> wrote:
> On Sat, 9 Jul 2022 10:46:59 +0800 Hawkins Jiawei wrote:
> > Reported-and-tested-by: syzbot+5f26f85569bd179c18ce@syzkaller.appspotmail.com
> > Signed-off-by: hawk <18801353760@163.com>
> > ---
> > net/ipv4/tcp.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 9984d23a7f3e..a1e6cab2c748 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -3395,10 +3395,23 @@ static int do_tcp_setsockopt(struct sock *sk, int level, int optname,
> > }
> > case TCP_ULP: {
> > char name[TCP_ULP_NAME_MAX];
> > + struct sock *smc_sock;
> >
> > if (optlen < 1)
> > return -EINVAL;
> >
> > + /* SMC sk_user_data may be treated as psock,
> > + * which triggers a refcnt warning.
> > + */
> > + rcu_read_lock();
> > + smc_sock = rcu_dereference_sk_user_data(sk);
> > + if (level == SOL_TCP && smc_sock &&
> > + smc_sock->__sk_common.skc_family == AF_SMC) {
>
> This should prolly be under the socket lock?
>
> Can we add a bit to SK_USER_DATA_PTRMASK and have ULP-compatible
> users (sockmap) opt into ULP cooperation? Modifying TCP is backwards,
> layer-wise.
Thanks for your suggestion, I also agree that modifying TCP directly
is not wise.
I am sorry that I can't follow you on haveing ULP-compatible
users (sockmap) opt into ULP cooperation, yet adding a bit to
SK_USER_DATA_PTRMASK seems like a good way.
I plan to add a mask bit, and check it during sk_psock_get(),
in v2 patch
>
> > + rcu_read_unlock();
> > + return -EOPNOTSUPP;
> > + }
> > + rcu_read_unlock();
> > +
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] smc: fix refcount bug in sk_psock_get (2)
2022-07-09 2:46 ` [PATCH] smc: fix " Hawkins Jiawei
2022-07-09 3:06 ` Jakub Kicinski
@ 2022-07-11 7:21 ` Wen Gu
2022-07-13 3:10 ` Hawkins Jiawei
2022-07-12 9:47 ` Dan Carpenter
2 siblings, 1 reply; 29+ messages in thread
From: Wen Gu @ 2022-07-11 7:21 UTC (permalink / raw)
To: Hawkins Jiawei, syzbot+5f26f85569bd179c18ce
Cc: andrii, ast, borisp, bpf, daniel, davem, edumazet, yoshfuji,
dsahern, john.fastabend, kafai, kgraul, kpsingh, kuba,
linux-kernel, netdev, pabeni, songliubraving, syzkaller-bugs,
yhs, skhan, 18801353760, paskripkin, linux-kernel-mentees
On 2022/7/9 10:46 am, Hawkins Jiawei wrote:
>
> syzbot is try to setup TLS on a SMC socket.
>
> During SMC fallback process in connect syscall, kernel will sets the
> smc->sk.sk_socket->file->private_data to smc->clcsock
> in smc_switch_to_fallback(), and set smc->clcsock->sk_user_data
> to origin smc in smc_fback_replace_callbacks().
>
> Later, sk_psock_get() will treat the smc->clcsock->sk_user_data
> as sk_psock type, which triggers the refcnt warning.
>
Thanks for your analysis.
Although syzbot found this issue in SMC, seems that it is a generic
issue about sk_user_data usage? Fixing it from SK_USER_DATA_PTRMASK
as you plan should be a right way.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] smc: fix refcount bug in sk_psock_get (2)
2022-07-11 7:21 ` Wen Gu
@ 2022-07-13 3:10 ` Hawkins Jiawei
2022-07-13 3:33 ` Jakub Kicinski
0 siblings, 1 reply; 29+ messages in thread
From: Hawkins Jiawei @ 2022-07-13 3:10 UTC (permalink / raw)
To: guwen
Cc: 18801353760, andrii, ast, borisp, bpf, daniel, davem, dsahern,
edumazet, john.fastabend, kafai, kgraul, kpsingh, kuba,
linux-kernel-mentees, linux-kernel, netdev, pabeni, paskripkin,
skhan, songliubraving, syzbot+5f26f85569bd179c18ce,
syzkaller-bugs, yhs, yin31149, yoshfuji, chuck.lever
On Mon, 11 Jul 2022 at 15:21, Wen Gu <guwen@linux.alibaba.com> wrote:
>Although syzbot found this issue in SMC, seems that it is a generic
>issue about sk_user_data usage? Fixing it from SK_USER_DATA_PTRMASK
>as you plan should be a right way.
Thanks for your advice. In fact, I found a more
general patch, but it seems that it has not
been merged until now.
In this bug, the problem is that smc and psock, both use
sk_user_data field to save their private data. So they
will treat field in their own way.
>> in smc_switch_to_fallback(), and set smc->clcsock->sk_user_data
>> to origin smc in smc_fback_replace_callbacks().
>>
>> Later, sk_psock_get() will treat the smc->clcsock->sk_user_data
>> as sk_psock type, which triggers the refcnt warning.
So in the patch [PATCH RFC 1/5] net: Add distinct sk_psock field,
psock private data will be moved to the sk_psock field, shown as
below
> The sk_psock facility populates the sk_user_data field with the
> address of an extra bit of metadata. User space sockets never
> populate the sk_user_data field, so this has worked out fine.
>
> However, kernel consumers such as the RPC client and server do
> populate the sk_user_data field. The sk_psock() function cannot tell
> that the content of sk_user_data does not point to psock metadata,
> so it will happily return a pointer to something else, cast to a
> struct sk_psock.
>
> Thus kernel consumers and psock currently cannot co-exist.
>
> We could educate sk_psock() to return NULL if sk_user_data does
> not point to a struct sk_psock. However, a more general solution
> that enables full co-existence psock and other uses of sk_user_data
> might be more interesting.
>
> Move the struct sk_psock address to its own pointer field so that
> the contents of the sk_user_data field is preserved.
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> include/linux/skmsg.h | 2 +-
> include/net/sock.h | 4 +++-
> net/core/skmsg.c | 6 +++---
> 3 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> index c5a2d6f50f25..5ef3a07c5b6c 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->sk_psock);
> }
>
> static inline void sk_psock_set_state(struct sk_psock *psock,
> diff --git a/include/net/sock.h b/include/net/sock.h
> index c4b91fc19b9c..d2a513169527 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -327,7 +327,8 @@ struct sk_filter;
> * @sk_tskey: counter to disambiguate concurrent tstamp requests
> * @sk_zckey: counter to order MSG_ZEROCOPY notifications
> * @sk_socket: Identd and reporting IO signals
> - * @sk_user_data: RPC layer private data
> + * @sk_user_data: Upper layer private data
> + * @sk_psock: socket policy data (bpf)
> * @sk_frag: cached page frag
> * @sk_peek_off: current peek_offset value
> * @sk_send_head: front of stuff to transmit
> @@ -519,6 +520,7 @@ struct sock {
>
> struct socket *sk_socket;
> void *sk_user_data;
> + struct sk_psock __rcu *sk_psock;
> #ifdef CONFIG_SECURITY
> void *sk_security;
> #endif
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index cc381165ea08..2b3d01d92790 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -695,7 +695,7 @@ struct sk_psock *sk_psock_init(struct sock *sk,
> int node)
>
> write_lock_bh(&sk->sk_callback_lock);
>
> - if (sk->sk_user_data) {
> + if (sk->sk_psock) {
> psock = ERR_PTR(-EBUSY);
> goto out;
> }
> @@ -726,7 +726,7 @@ 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_pointer(sk->sk_psock, psock);
> sock_hold(sk);
>
> out:
> @@ -825,7 +825,7 @@ void sk_psock_drop(struct sock *sk,
> struct sk_psock *psock)
> {
> write_lock_bh(&sk->sk_callback_lock);
> sk_psock_restore_proto(sk, psock);
> - rcu_assign_sk_user_data(sk, NULL);
> + rcu_assign_pointer(sk->sk_psock, NULL);
> if (psock->progs.stream_parser)
> sk_psock_stop_strp(sk, psock);
> else if (psock->progs.stream_verdict || psock->progs.skb_verdict)
I have tested this patch and the reproducer did not trigger any issue.
In Patchwork website, this patch fails the checks on
netdev/cc_maintainers. If this patch fails for some other reasons,
I will still fix this bug from SK_USER_DATA_PTRMASK,
as a temporary solution.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH] smc: fix refcount bug in sk_psock_get (2)
2022-07-13 3:10 ` Hawkins Jiawei
@ 2022-07-13 3:33 ` Jakub Kicinski
2022-07-13 3:53 ` Hawkins Jiawei
0 siblings, 1 reply; 29+ messages in thread
From: Jakub Kicinski @ 2022-07-13 3:33 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: guwen, 18801353760, andrii, ast, borisp, bpf, daniel, davem,
dsahern, edumazet, john.fastabend, kafai, kgraul, kpsingh,
linux-kernel-mentees, linux-kernel, netdev, pabeni, paskripkin,
skhan, songliubraving, syzbot+5f26f85569bd179c18ce,
syzkaller-bugs, yhs, yoshfuji, chuck.lever
On Wed, 13 Jul 2022 11:10:05 +0800 Hawkins Jiawei wrote:
> In Patchwork website, this patch fails the checks on
> netdev/cc_maintainers. If this patch fails for some other reasons,
> I will still fix this bug from SK_USER_DATA_PTRMASK,
> as a temporary solution.
That check just runs scripts/get_maintainer.pl so make sure you CC
folks pointed out by that script and you should be fine.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] smc: fix refcount bug in sk_psock_get (2)
2022-07-13 3:33 ` Jakub Kicinski
@ 2022-07-13 3:53 ` Hawkins Jiawei
0 siblings, 0 replies; 29+ messages in thread
From: Hawkins Jiawei @ 2022-07-13 3:53 UTC (permalink / raw)
To: kuba
Cc: 18801353760, andrii, ast, borisp, bpf, chuck.lever, daniel,
davem, dsahern, edumazet, guwen, john.fastabend, kafai, kgraul,
kpsingh, linux-kernel-mentees, linux-kernel, netdev, pabeni,
paskripkin, skhan, songliubraving, syzbot+5f26f85569bd179c18ce,
syzkaller-bugs, yhs, yin31149, yoshfuji
On Wed, 13 Jul 2022 at 11:33, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 13 Jul 2022 11:10:05 +0800 Hawkins Jiawei wrote:
> > In Patchwork website, this patch fails the checks on
> > netdev/cc_maintainers. If this patch fails for some other reasons,
> > I will still fix this bug from SK_USER_DATA_PTRMASK,
> > as a temporary solution.
>
> That check just runs scripts/get_maintainer.pl so make sure you CC
> folks pointed out by that script and you should be fine.
Thanks for your reply, yet I am not the patch's author, I
found this patch during my bug analysis.
I will reply the relative email to remind the patch's author.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] smc: fix refcount bug in sk_psock_get (2)
2022-07-09 2:46 ` [PATCH] smc: fix " Hawkins Jiawei
2022-07-09 3:06 ` Jakub Kicinski
2022-07-11 7:21 ` Wen Gu
@ 2022-07-12 9:47 ` Dan Carpenter
2022-07-13 3:35 ` Hawkins Jiawei
2 siblings, 1 reply; 29+ messages in thread
From: Dan Carpenter @ 2022-07-12 9:47 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: syzbot+5f26f85569bd179c18ce, andrii, ast, borisp, bpf, daniel,
davem, edumazet, guwen, yoshfuji, dsahern, john.fastabend, kafai,
kgraul, kpsingh, kuba, linux-kernel, netdev, pabeni,
songliubraving, syzkaller-bugs, yhs, skhan, 18801353760,
paskripkin, linux-kernel-mentees
On Sat, Jul 09, 2022 at 10:46:59AM +0800, Hawkins Jiawei wrote:
> From: hawk <18801353760@163.com>
Please use your legal name like you would for signing a legal document.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] smc: fix refcount bug in sk_psock_get (2)
2022-07-12 9:47 ` Dan Carpenter
@ 2022-07-13 3:35 ` Hawkins Jiawei
0 siblings, 0 replies; 29+ messages in thread
From: Hawkins Jiawei @ 2022-07-13 3:35 UTC (permalink / raw)
To: dan.carpenter
Cc: 18801353760, andrii, ast, borisp, bpf, daniel, davem, dsahern,
edumazet, guwen, john.fastabend, kafai, kgraul, kpsingh, kuba,
linux-kernel-mentees, linux-kernel, netdev, pabeni, paskripkin,
skhan, songliubraving, syzbot+5f26f85569bd179c18ce,
syzkaller-bugs, yhs, yin31149, yoshfuji
On Tue, 12 Jul 2022 at 17:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Sat, Jul 09, 2022 at 10:46:59AM +0800, Hawkins Jiawei wrote:
> > From: hawk <18801353760@163.com>
>
> Please use your legal name like you would for signing a legal document.
Thanks, I will pay attention to it in the future.
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2] net/smc: fix refcount bug in sk_psock_get (2)
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-30 8:56 ` Hawkins Jiawei
2022-08-01 9:09 ` Jakub Sitnicki
2022-08-03 8:03 ` [PATCH v3] " Hawkins Jiawei
` (2 subsequent siblings)
4 siblings, 1 reply; 29+ messages in thread
From: Hawkins Jiawei @ 2022-07-30 8:56 UTC (permalink / raw)
To: syzbot+5f26f85569bd179c18ce
Cc: andrii, ast, jakub, borisp, bpf, wenjia, ubraun, daniel, guvenc,
davem, edumazet, guwen, john.fastabend, kafai, kgraul, kpsingh,
kuba, linux-kernel, netdev, pabeni, songliubraving,
syzkaller-bugs, linux-s390, yhs, yin31149, 18801353760,
paskripkin, skhan, linux-kernel-mentees
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v2] net/smc: fix refcount bug in sk_psock_get (2)
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
0 siblings, 1 reply; 29+ messages in thread
From: Jakub Sitnicki @ 2022-08-01 9:09 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: syzbot+5f26f85569bd179c18ce, andrii, ast, borisp, bpf, wenjia,
ubraun, daniel, guvenc, davem, edumazet, guwen, john.fastabend,
kafai, kgraul, kpsingh, kuba, linux-kernel, netdev, pabeni,
songliubraving, syzkaller-bugs, linux-s390, yhs, 18801353760,
paskripkin, skhan, linux-kernel-mentees
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.
[...]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2] net/smc: fix refcount bug in sk_psock_get (2)
2022-08-01 9:09 ` Jakub Sitnicki
@ 2022-08-02 14:32 ` Hawkins Jiawei
0 siblings, 0 replies; 29+ messages in thread
From: Hawkins Jiawei @ 2022-08-02 14:32 UTC (permalink / raw)
To: jakub
Cc: 18801353760, andrii, ast, borisp, bpf, daniel, davem, edumazet,
guvenc, guwen, john.fastabend, kafai, kgraul, kpsingh, kuba,
linux-kernel-mentees, linux-kernel, linux-s390, netdev, pabeni,
paskripkin, skhan, songliubraving, syzbot+5f26f85569bd179c18ce,
syzkaller-bugs, ubraun, wenjia, yhs, yin31149
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3] net/smc: fix refcount bug in sk_psock_get (2)
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-30 8:56 ` [PATCH v2] net/smc: " Hawkins Jiawei
@ 2022-08-03 8:03 ` Hawkins Jiawei
2022-08-03 11:27 ` Wen Gu
2022-08-03 12:41 ` [PATCH v4] net: " Hawkins Jiawei
2022-08-05 7:36 ` [PATCH net v5 0/2] net: enhancements to sk_user_data field Hawkins Jiawei
4 siblings, 1 reply; 29+ messages in thread
From: Hawkins Jiawei @ 2022-08-03 8:03 UTC (permalink / raw)
To: syzbot+5f26f85569bd179c18ce
Cc: andrii, ast, borisp, bpf, daniel, davem, edumazet, guwen,
john.fastabend, kafai, kgraul, kpsingh, kuba, linux-kernel,
netdev, pabeni, songliubraving, syzkaller-bugs, yhs, 18801353760,
linux-kernel-mentees, jakub, paskripkin, skhan, 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
<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>
---
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3] net/smc: fix refcount bug in sk_psock_get (2)
2022-08-03 8:03 ` [PATCH v3] " Hawkins Jiawei
@ 2022-08-03 11:27 ` Wen Gu
2022-08-03 12:07 ` Hawkins Jiawei
0 siblings, 1 reply; 29+ messages in thread
From: Wen Gu @ 2022-08-03 11:27 UTC (permalink / raw)
To: Hawkins Jiawei, syzbot+5f26f85569bd179c18ce
Cc: andrii, ast, borisp, bpf, daniel, davem, edumazet,
john.fastabend, kafai, kgraul, kpsingh, kuba, linux-kernel,
netdev, pabeni, songliubraving, syzkaller-bugs, yhs, 18801353760,
linux-kernel-mentees, jakub, paskripkin, skhan
On 2022/8/3 16:03, 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
> <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>
> ---
> 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(-)
>
Hi Hawkins,
Since the fix v3 doesn't involved smc codes any more, I wonder if it's still
appropriate to use 'net/smc:' in subject?
Cheers,
Wen Gu
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH v3] net/smc: fix refcount bug in sk_psock_get (2)
2022-08-03 11:27 ` Wen Gu
@ 2022-08-03 12:07 ` Hawkins Jiawei
0 siblings, 0 replies; 29+ messages in thread
From: Hawkins Jiawei @ 2022-08-03 12:07 UTC (permalink / raw)
To: guwen
Cc: 18801353760, andrii, ast, borisp, bpf, daniel, davem, edumazet,
jakub, john.fastabend, kafai, kgraul, kpsingh, kuba,
linux-kernel-mentees, linux-kernel, netdev, pabeni, paskripkin,
skhan, songliubraving, syzbot+5f26f85569bd179c18ce,
syzkaller-bugs, yhs, yin31149
On Wed, 3 Aug 2022 at 19:27, Wen Gu <guwen@linux.alibaba.com> wrote:
> Hi Hawkins,
>
> Since the fix v3 doesn't involved smc codes any more, I wonder if it's still
> appropriate to use 'net/smc:' in subject?
>
> Cheers,
> Wen Gu
Thank you for reminding me, I will send the v4 patch with new subject.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4] net: fix refcount bug in sk_psock_get (2)
2022-06-03 15:34 [syzbot] WARNING: refcount bug in sk_psock_get (2) syzbot
` (2 preceding siblings ...)
2022-08-03 8:03 ` [PATCH v3] " Hawkins Jiawei
@ 2022-08-03 12:41 ` Hawkins Jiawei
2022-08-03 15:14 ` Jakub Kicinski
2022-08-05 7:36 ` [PATCH net v5 0/2] net: enhancements to sk_user_data field Hawkins Jiawei
4 siblings, 1 reply; 29+ messages in thread
From: Hawkins Jiawei @ 2022-08-03 12:41 UTC (permalink / raw)
To: syzbot+5f26f85569bd179c18ce
Cc: 18801353760, andrii, ast, borisp, bpf, daniel, davem, edumazet,
jakub, john.fastabend, kafai, kgraul, kpsingh, kuba,
linux-kernel-mentees, linux-kernel, netdev, pabeni, paskripkin,
skhan, songliubraving, syzkaller-bugs, yhs, yin31149, Wen Gu
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v4] net: fix refcount bug in sk_psock_get (2)
2022-08-03 12:41 ` [PATCH v4] net: " Hawkins Jiawei
@ 2022-08-03 15:14 ` Jakub Kicinski
2022-08-03 15:37 ` Martin KaFai Lau
0 siblings, 1 reply; 29+ messages in thread
From: Jakub Kicinski @ 2022-08-03 15:14 UTC (permalink / raw)
To: Hawkins Jiawei, kafai
Cc: syzbot+5f26f85569bd179c18ce, 18801353760, andrii, ast, borisp,
bpf, daniel, davem, edumazet, jakub, john.fastabend, kgraul,
kpsingh, linux-kernel-mentees, linux-kernel, netdev, pabeni,
paskripkin, skhan, songliubraving, syzkaller-bugs, yhs, Wen Gu
On Wed, 3 Aug 2022 20:41:22 +0800 Hawkins Jiawei wrote:
> -/* 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
I'd use this opportunity to add more info here, BPF is too general.
Maybe "Pointer is used by a BPF reuseport array"? Martin, WDYT?
> + * 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.
> +/**
> + * 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)
nit: the return type more commonly goes on the same line as "static
inline"
> +{
> + 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;
> +}
As a follow up we can probably generalize this into
__rcu_dereference_sk_user_data_cond(sk, bit)
and make the psock just call that:
static inline struct sk_psock *
rcu_dereference_sk_user_data_psock(const struct sock *sk)
{
return __rcu_dereference_sk_user_data_cond(sk, SK_USER_DATA_PSOCK);
}
then reuseport can also benefit, maybe:
diff --git a/kernel/bpf/reuseport_array.c b/kernel/bpf/reuseport_array.c
index e2618fb5870e..ad5c447a690c 100644
--- a/kernel/bpf/reuseport_array.c
+++ b/kernel/bpf/reuseport_array.c
@@ -21,14 +21,11 @@ static struct reuseport_array *reuseport_array(struct bpf_map *map)
/* The caller must hold the reuseport_lock */
void bpf_sk_reuseport_detach(struct sock *sk)
{
- uintptr_t sk_user_data;
+ struct sock __rcu **socks;
write_lock_bh(&sk->sk_callback_lock);
- sk_user_data = (uintptr_t)sk->sk_user_data;
- if (sk_user_data & SK_USER_DATA_BPF) {
- struct sock __rcu **socks;
-
- socks = (void *)(sk_user_data & SK_USER_DATA_PTRMASK);
+ socks = __rcu_dereference_sk_user_data_cond(sk, SK_USER_DATA_BPF);
+ if (socks) {
WRITE_ONCE(sk->sk_user_data, NULL);
/*
* Do not move this NULL assignment outside of
But that must be a separate patch, not part of this fix.
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v4] net: fix refcount bug in sk_psock_get (2)
2022-08-03 15:14 ` Jakub Kicinski
@ 2022-08-03 15:37 ` Martin KaFai Lau
2022-08-04 3:05 ` Hawkins Jiawei
0 siblings, 1 reply; 29+ messages in thread
From: Martin KaFai Lau @ 2022-08-03 15:37 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Hawkins Jiawei, syzbot+5f26f85569bd179c18ce, 18801353760, andrii,
ast, borisp, bpf, daniel, davem, edumazet, jakub, john.fastabend,
kgraul, kpsingh, linux-kernel-mentees, linux-kernel, netdev,
pabeni, paskripkin, skhan, songliubraving, syzkaller-bugs, yhs,
Wen Gu
On Wed, Aug 03, 2022 at 08:14:13AM -0700, Jakub Kicinski wrote:
> On Wed, 3 Aug 2022 20:41:22 +0800 Hawkins Jiawei wrote:
> > -/* 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
>
> I'd use this opportunity to add more info here, BPF is too general.
> Maybe "Pointer is used by a BPF reuseport array"? Martin, WDYT?
SGTM. Thanks.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4] net: fix refcount bug in sk_psock_get (2)
2022-08-03 15:37 ` Martin KaFai Lau
@ 2022-08-04 3:05 ` Hawkins Jiawei
2022-08-04 15:29 ` Jakub Kicinski
0 siblings, 1 reply; 29+ messages in thread
From: Hawkins Jiawei @ 2022-08-04 3:05 UTC (permalink / raw)
To: kafai
Cc: 18801353760, andrii, ast, borisp, bpf, daniel, davem, edumazet,
guwen, jakub, john.fastabend, kgraul, kpsingh, kuba,
linux-kernel-mentees, linux-kernel, netdev, pabeni, paskripkin,
skhan, songliubraving, syzbot+5f26f85569bd179c18ce,
syzkaller-bugs, yhs, yin31149
On Wed, 3 Aug 2022 at 23:37, Martin KaFai Lau <kafai@fb.com> wrote:
> > > + * SK_USER_DATA_BPF - Managed by BPF
> >
> > I'd use this opportunity to add more info here, BPF is too general.
> > Maybe "Pointer is used by a BPF reuseport array"? Martin, WDYT?
> SGTM. Thanks.
OK. It seems that this flag is introduced from
c9a368f1c0fb ("bpf: net: Avoid incorrect bpf_sk_reuseport_detach call").
I will search for more detailed description in this commit.
> >
> > > +/**
> > > + * 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)
> >
> > nit: the return type more commonly goes on the same line as "static
> > inline"
Ok. I will correct it.
> >
> > > +{
> > > + 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;
> > > +}
> >
> > As a follow up we can probably generalize this into
> > __rcu_dereference_sk_user_data_cond(sk, bit)
> >
> > and make the psock just call that:
> >
> > static inline struct sk_psock *
> > rcu_dereference_sk_user_data_psock(const struct sock *sk)
> > {
> > return __rcu_dereference_sk_user_data_cond(sk, SK_USER_DATA_PSOCK);
> > }
Yes. I will refactor it in this way.
> >
> > diff --git a/kernel/bpf/reuseport_array.c b/kernel/bpf/reuseport_array.c
> > index e2618fb5870e..ad5c447a690c 100644
> > --- a/kernel/bpf/reuseport_array.c
> > +++ b/kernel/bpf/reuseport_array.c
> > @@ -21,14 +21,11 @@ static struct reuseport_array *reuseport_array(struct bpf_map *map)
> > /* The caller must hold the reuseport_lock */
> > void bpf_sk_reuseport_detach(struct sock *sk)
> > {
> > - uintptr_t sk_user_data;
> > + struct sock __rcu **socks;
> >
> > write_lock_bh(&sk->sk_callback_lock);
> > - sk_user_data = (uintptr_t)sk->sk_user_data;
> > - if (sk_user_data & SK_USER_DATA_BPF) {
> > - struct sock __rcu **socks;
> > -
> > - socks = (void *)(sk_user_data & SK_USER_DATA_PTRMASK);
> > + socks = __rcu_dereference_sk_user_data_cond(sk, SK_USER_DATA_BPF);
> > + if (socks) {
> > WRITE_ONCE(sk->sk_user_data, NULL);
> > /*
> > * Do not move this NULL assignment outside of
> >
> >
> > But that must be a separate patch, not part of this fix.
I wonder if it is proper to gather these together in a patchset, for
they are all about flags in sk_user_data, maybe:
[PATCH v5 0/2] net: enhancement to flags in sk_user_data field
- introduce the patchset
[PATCH v5 1/2] net: clean up code for flags in sk_user_data field
- refactor the things in include/linux/skmsg.h and
include/net/sock.h
- refactor the flags's usage by other code, such as
net/core/skmsg.c and kernel/bpf/reuseport_array.c
[PATCH v5 2/2] net: fix refcount bug in sk_psock_get (2)
- add SK_USER_DATA_PSOCK flag in sk_user_data field
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v4] net: fix refcount bug in sk_psock_get (2)
2022-08-04 3:05 ` Hawkins Jiawei
@ 2022-08-04 15:29 ` Jakub Kicinski
2022-08-05 6:28 ` Hawkins Jiawei
0 siblings, 1 reply; 29+ messages in thread
From: Jakub Kicinski @ 2022-08-04 15:29 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: kafai, 18801353760, andrii, ast, borisp, bpf, daniel, davem,
edumazet, guwen, jakub, john.fastabend, kgraul, kpsingh,
linux-kernel-mentees, linux-kernel, netdev, pabeni, paskripkin,
skhan, songliubraving, syzbot+5f26f85569bd179c18ce,
syzkaller-bugs, yhs
On Thu, 4 Aug 2022 11:05:15 +0800 Hawkins Jiawei wrote:
> I wonder if it is proper to gather these together in a patchset, for
> they are all about flags in sk_user_data, maybe:
>
> [PATCH v5 0/2] net: enhancement to flags in sk_user_data field
> - introduce the patchset
>
> [PATCH v5 1/2] net: clean up code for flags in sk_user_data field
> - refactor the things in include/linux/skmsg.h and
> include/net/sock.h
> - refactor the flags's usage by other code, such as
> net/core/skmsg.c and kernel/bpf/reuseport_array.c
>
> [PATCH v5 2/2] net: fix refcount bug in sk_psock_get (2)
> - add SK_USER_DATA_PSOCK flag in sk_user_data field
Usually the fix comes first, because it needs to be backported to
stable, and we don't want to have to pull extra commits into stable
and risk regressions in code which was not directly involved in the bug.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4] net: fix refcount bug in sk_psock_get (2)
2022-08-04 15:29 ` Jakub Kicinski
@ 2022-08-05 6:28 ` Hawkins Jiawei
0 siblings, 0 replies; 29+ messages in thread
From: Hawkins Jiawei @ 2022-08-05 6:28 UTC (permalink / raw)
To: kuba
Cc: 18801353760, andrii, ast, borisp, bpf, daniel, davem, edumazet,
guwen, jakub, john.fastabend, kafai, kgraul, kpsingh,
linux-kernel-mentees, linux-kernel, netdev, pabeni, paskripkin,
skhan, songliubraving, syzbot+5f26f85569bd179c18ce,
syzkaller-bugs, yhs, yin31149
On Thu, 4 Aug 2022 at 23:29, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 4 Aug 2022 11:05:15 +0800 Hawkins Jiawei wrote:
> > I wonder if it is proper to gather these together in a patchset, for
> > they are all about flags in sk_user_data, maybe:
> >
> > [PATCH v5 0/2] net: enhancement to flags in sk_user_data field
> > - introduce the patchset
> >
> > [PATCH v5 1/2] net: clean up code for flags in sk_user_data field
> > - refactor the things in include/linux/skmsg.h and
> > include/net/sock.h
> > - refactor the flags's usage by other code, such as
> > net/core/skmsg.c and kernel/bpf/reuseport_array.c
> >
> > [PATCH v5 2/2] net: fix refcount bug in sk_psock_get (2)
> > - add SK_USER_DATA_PSOCK flag in sk_user_data field
>
> Usually the fix comes first, because it needs to be backported to
> stable, and we don't want to have to pull extra commits into stable
> and risk regressions in code which was not directly involved in the bug.
Ok, I got it. Thanks for the explanation.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH net v5 0/2] net: enhancements to sk_user_data field
2022-06-03 15:34 [syzbot] WARNING: refcount bug in sk_psock_get (2) syzbot
` (3 preceding siblings ...)
2022-08-03 12:41 ` [PATCH v4] net: " Hawkins Jiawei
@ 2022-08-05 7:36 ` Hawkins Jiawei
2022-08-05 7:48 ` [PATCH net v5 1/2] net: fix refcount bug in sk_psock_get (2) Hawkins Jiawei
` (3 more replies)
4 siblings, 4 replies; 29+ messages in thread
From: Hawkins Jiawei @ 2022-08-05 7:36 UTC (permalink / raw)
To: syzbot+5f26f85569bd179c18ce
Cc: paskripkin, skhan, linux-kernel-mentees, 18801353760,
Hawkins Jiawei, bpf
This patchset fixes refcount bug by adding SK_USER_DATA_PSOCK flag bit in
sk_user_data field. The bug cause following info:
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>
To improve code maintainability, this patchset refactors sk_user_data
flags code to be more generic.
Hawkins Jiawei (2):
net: fix refcount bug in sk_psock_get (2)
net: refactor bpf_sk_reuseport_detach()
include/linux/skmsg.h | 3 +-
include/net/sock.h | 68 +++++++++++++++++++++++++-----------
kernel/bpf/reuseport_array.c | 9 ++---
net/core/skmsg.c | 4 ++-
4 files changed, 56 insertions(+), 28 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH net v5 1/2] net: fix refcount bug in sk_psock_get (2)
2022-08-05 7:36 ` [PATCH net v5 0/2] net: enhancements to sk_user_data field Hawkins Jiawei
@ 2022-08-05 7:48 ` Hawkins Jiawei
2022-08-05 7:48 ` [PATCH net v5 2/2] net: refactor bpf_sk_reuseport_detach() Hawkins Jiawei
` (2 subsequent siblings)
3 siblings, 0 replies; 29+ messages in thread
From: Hawkins Jiawei @ 2022-08-05 7:48 UTC (permalink / raw)
To: syzbot+5f26f85569bd179c18ce, John Fastabend, Jakub Sitnicki,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: paskripkin, skhan, linux-kernel-mentees, 18801353760,
Hawkins Jiawei, Wen Gu, netdev, bpf, linux-kernel
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").
For there will possibly be more flags in the sk_user_data field,
this patch also refactor sk_user_data flags code to be more generic
to improve its maintainability.
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>
---
v4 -> v5:
- add more info about SK_USER_DATA_BPF in comment
- correct the function definition format
- generalize rcu_deference_sk_user_data_psock() into
__rcu_deference_sk_user_data_with_flags()
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 | 3 +-
include/net/sock.h | 68 ++++++++++++++++++++++++++++++-------------
net/core/skmsg.c | 4 ++-
3 files changed, 53 insertions(+), 22 deletions(-)
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index c5a2d6f50f25..c5bdc5975a5c 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -277,7 +277,8 @@ 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_with_flags(sk,
+ SK_USER_DATA_PSOCK);
}
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..a5ebc3b28804 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -545,14 +545,26 @@ 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: Mark whether sk_user_data field is
+ * managed/owned by a BPF reuseport array. This bit should be set
+ * when sk_user_data's sk is added to the bpf's reuseport_array.
+ *
+ * - 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
@@ -565,24 +577,40 @@ static inline bool sk_user_data_is_nocopy(const struct sock *sk)
#define __sk_user_data(sk) ((*((void __rcu **)&(sk)->sk_user_data)))
+/**
+ * __rcu_dereference_sk_user_data_with_flags - return the pointer
+ * only if argument flags all has been set in sk_user_data. Otherwise
+ * return NULL
+ *
+ * @sk: socket
+ * @flags: flag bits
+ */
+static inline void *
+__rcu_dereference_sk_user_data_with_flags(const struct sock *sk,
+ uintptr_t flags)
+{
+ uintptr_t sk_user_data = (uintptr_t)rcu_dereference(__sk_user_data(sk));
+
+ WARN_ON_ONCE(flags & SK_USER_DATA_PTRMASK);
+
+ if ((sk_user_data & flags) == flags)
+ return (void *)(sk_user_data & SK_USER_DATA_PTRMASK);
+ return NULL;
+}
+
#define rcu_dereference_sk_user_data(sk) \
+ __rcu_dereference_sk_user_data_with_flags(sk, 0)
+#define __rcu_assign_sk_user_data_with_flags(sk, ptr, flags) \
({ \
- void *__tmp = rcu_dereference(__sk_user_data((sk))); \
- (void *)((uintptr_t)__tmp & SK_USER_DATA_PTRMASK); \
-})
-#define rcu_assign_sk_user_data(sk, ptr) \
-({ \
- 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)
static inline
struct net *sock_net(const struct sock *sk)
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index b0fcd0200e84..10f7afa3d7a5 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -735,7 +735,9 @@ 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
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH net v5 2/2] net: refactor bpf_sk_reuseport_detach()
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 ` 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-11 5:30 ` patchwork-bot+netdevbpf
3 siblings, 1 reply; 29+ messages in thread
From: Hawkins Jiawei @ 2022-08-05 7:48 UTC (permalink / raw)
To: syzbot+5f26f85569bd179c18ce, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa
Cc: paskripkin, skhan, linux-kernel-mentees, 18801353760,
Hawkins Jiawei, Jakub Kicinski, bpf, linux-kernel
Refactor sk_user_data dereference using more generic function
__rcu_dereference_sk_user_data_with_flags(), which improve its
maintainability
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
kernel/bpf/reuseport_array.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/kernel/bpf/reuseport_array.c b/kernel/bpf/reuseport_array.c
index e2618fb5870e..85fa9dbfa8bf 100644
--- a/kernel/bpf/reuseport_array.c
+++ b/kernel/bpf/reuseport_array.c
@@ -21,14 +21,11 @@ static struct reuseport_array *reuseport_array(struct bpf_map *map)
/* The caller must hold the reuseport_lock */
void bpf_sk_reuseport_detach(struct sock *sk)
{
- uintptr_t sk_user_data;
+ struct sock __rcu **socks;
write_lock_bh(&sk->sk_callback_lock);
- sk_user_data = (uintptr_t)sk->sk_user_data;
- if (sk_user_data & SK_USER_DATA_BPF) {
- struct sock __rcu **socks;
-
- socks = (void *)(sk_user_data & SK_USER_DATA_PTRMASK);
+ socks = __rcu_dereference_sk_user_data_with_flags(sk, SK_USER_DATA_BPF);
+ if (socks) {
WRITE_ONCE(sk->sk_user_data, NULL);
/*
* Do not move this NULL assignment outside of
--
2.25.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH net v5 2/2] net: refactor bpf_sk_reuseport_detach()
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
0 siblings, 0 replies; 29+ messages in thread
From: Martin KaFai Lau @ 2022-08-15 18:24 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: syzbot+5f26f85569bd179c18ce, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
paskripkin, skhan, linux-kernel-mentees, 18801353760,
Jakub Kicinski, bpf, linux-kernel
On Fri, Aug 05, 2022 at 03:48:36PM +0800, Hawkins Jiawei wrote:
> Refactor sk_user_data dereference using more generic function
> __rcu_dereference_sk_user_data_with_flags(), which improve its
> maintainability
>
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> ---
> kernel/bpf/reuseport_array.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/bpf/reuseport_array.c b/kernel/bpf/reuseport_array.c
> index e2618fb5870e..85fa9dbfa8bf 100644
> --- a/kernel/bpf/reuseport_array.c
> +++ b/kernel/bpf/reuseport_array.c
> @@ -21,14 +21,11 @@ static struct reuseport_array *reuseport_array(struct bpf_map *map)
> /* The caller must hold the reuseport_lock */
> void bpf_sk_reuseport_detach(struct sock *sk)
> {
> - uintptr_t sk_user_data;
> + struct sock __rcu **socks;
>
> write_lock_bh(&sk->sk_callback_lock);
> - sk_user_data = (uintptr_t)sk->sk_user_data;
> - if (sk_user_data & SK_USER_DATA_BPF) {
> - struct sock __rcu **socks;
> -
> - socks = (void *)(sk_user_data & SK_USER_DATA_PTRMASK);
> + socks = __rcu_dereference_sk_user_data_with_flags(sk, SK_USER_DATA_BPF);
syzbot reports 'suspicious rcu_dereference_check() usage':
https://lore.kernel.org/netdev/0000000000007902fc05e6458697@google.com/
rcu_read_lock() does not need to be held here.
One option is to use rcu_access_pointer.
Another option is to use rcu_dereference_check() and pass the
lockdep_is_held(&sk->sk_callback_lock) from here.
> + if (socks) {
> WRITE_ONCE(sk->sk_user_data, NULL);
> /*
> * Do not move this NULL assignment outside of
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH net v5 0/2] net: enhancements to sk_user_data field
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-05 10:29 ` Jakub Sitnicki
2022-08-05 15:36 ` Hawkins Jiawei
2022-08-11 5:30 ` patchwork-bot+netdevbpf
3 siblings, 1 reply; 29+ messages in thread
From: Jakub Sitnicki @ 2022-08-05 10:29 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: syzbot+5f26f85569bd179c18ce, paskripkin, skhan,
linux-kernel-mentees, 18801353760, bpf
On Fri, Aug 05, 2022 at 03:36 PM +08, Hawkins Jiawei wrote:
> This patchset fixes refcount bug by adding SK_USER_DATA_PSOCK flag bit in
> sk_user_data field. The bug cause following info:
>
> 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>
>
> To improve code maintainability, this patchset refactors sk_user_data
> flags code to be more generic.
>
> Hawkins Jiawei (2):
> net: fix refcount bug in sk_psock_get (2)
> net: refactor bpf_sk_reuseport_detach()
>
> include/linux/skmsg.h | 3 +-
> include/net/sock.h | 68 +++++++++++++++++++++++++-----------
> kernel/bpf/reuseport_array.c | 9 ++---
> net/core/skmsg.c | 4 ++-
> 4 files changed, 56 insertions(+), 28 deletions(-)
Thank you for the fix.
For the series:
Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH net v5 0/2] net: enhancements to sk_user_data field
2022-08-05 7:36 ` [PATCH net v5 0/2] net: enhancements to sk_user_data field Hawkins Jiawei
` (2 preceding siblings ...)
2022-08-05 10:29 ` [PATCH net v5 0/2] net: enhancements to sk_user_data field Jakub Sitnicki
@ 2022-08-11 5:30 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 29+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-08-11 5:30 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: syzbot+5f26f85569bd179c18ce, paskripkin, skhan,
linux-kernel-mentees, 18801353760, bpf
Hello:
This series was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Fri, 5 Aug 2022 15:36:48 +0800 you wrote:
> This patchset fixes refcount bug by adding SK_USER_DATA_PSOCK flag bit in
> sk_user_data field. The bug cause following info:
>
> 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>
>
> [...]
Here is the summary with links:
- [net,v5,1/2] net: fix refcount bug in sk_psock_get (2)
https://git.kernel.org/netdev/net/c/2a0133723f9e
- [net,v5,2/2] net: refactor bpf_sk_reuseport_detach()
https://git.kernel.org/netdev/net/c/cf8c1e967224
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 29+ messages in thread