| Message ID | 20220709024659.6671-1-yin31149@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | smc: fix refcount bug in sk_psock_get (2) | expand |
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(); > +
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(); > > +
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.
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
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.
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.
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 >
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.
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));