| Message ID | 20220620191353.1184629-2-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e34a07c0ae3906f97eb18df50902e2a01c1015b6 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/2] Revert "net/tls: fix tls_sk_proto_close executed repeatedly" | expand |
Jakub Kicinski wrote: > Commit 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()") > has moved the inet_csk_has_ulp(sk) check from sk_psock_init() to > the new tcp_bpf_update_proto() function. I'm guessing that this > was done to allow creating psocks for non-inet sockets. > > Unfortunately the destruction path for psock includes the ULP > unwind, so we need to fail the sk_psock_init() itself. > Otherwise if ULP is already present we'll notice that later, > and call tcp_update_ulp() with the sk_proto of the ULP > itself, which will most likely result in the ULP looping > its callbacks. > > Fixes: 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()") > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > CC: john.fastabend@gmail.com > CC: jakub@cloudflare.com > CC: yoshfuji@linux-ipv6.org > CC: dsahern@kernel.org > CC: ast@kernel.org > CC: daniel@iogearbox.net > CC: andrii@kernel.org > CC: kafai@fb.com > CC: songliubraving@fb.com > CC: yhs@fb.com > CC: kpsingh@kernel.org > CC: borisp@nvidia.com > CC: cong.wang@bytedance.com > CC: bpf@vger.kernel.org > --- > include/net/inet_sock.h | 5 +++++ > net/core/skmsg.c | 5 +++++ > net/ipv4/tcp_bpf.c | 3 --- > net/tls/tls_main.c | 2 ++ > 4 files changed, 12 insertions(+), 3 deletions(-) Thanks Jakub this looks correct to me. I can't remember at the moment or dig up in the email or git why it was originally moved into the update logic. Maybe Cong can comment seeing it was his original patch? I seemed to have missed the error path on original review :( From my side though, Reviewed-by: John Fastabend <john.fastabend@gmail.com>
On Mon, Jun 20, 2022 at 12:13 PM -07, Jakub Kicinski wrote: > Commit 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()") > has moved the inet_csk_has_ulp(sk) check from sk_psock_init() to > the new tcp_bpf_update_proto() function. I'm guessing that this > was done to allow creating psocks for non-inet sockets. > > Unfortunately the destruction path for psock includes the ULP > unwind, so we need to fail the sk_psock_init() itself. > Otherwise if ULP is already present we'll notice that later, > and call tcp_update_ulp() with the sk_proto of the ULP > itself, which will most likely result in the ULP looping > its callbacks. > > Fixes: 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()") > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- I followed up with a regression test, if you would like to pick it up through net tree. Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com> Tested-by: Jakub Sitnicki <jakub@cloudflare.com>
On Wed, 22 Jun 2022 19:24:16 +0200 Jakub Sitnicki wrote: > I followed up with a regression test, if you would like to pick it up > through net tree. Sweet, thanks! > Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com> > Tested-by: Jakub Sitnicki <jakub@cloudflare.com>
diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h index c1b5dcd6597c..daead5fb389a 100644 --- a/include/net/inet_sock.h +++ b/include/net/inet_sock.h @@ -253,6 +253,11 @@ struct inet_sock { #define IP_CMSG_CHECKSUM BIT(7) #define IP_CMSG_RECVFRAGSIZE BIT(8) +static inline bool sk_is_inet(struct sock *sk) +{ + return sk->sk_family == AF_INET || sk->sk_family == AF_INET6; +} + /** * sk_to_full_sk - Access to a full socket * @sk: pointer to a socket diff --git a/net/core/skmsg.c b/net/core/skmsg.c index 22b983ade0e7..b0fcd0200e84 100644 --- a/net/core/skmsg.c +++ b/net/core/skmsg.c @@ -699,6 +699,11 @@ struct sk_psock *sk_psock_init(struct sock *sk, int node) write_lock_bh(&sk->sk_callback_lock); + if (sk_is_inet(sk) && inet_csk_has_ulp(sk)) { + psock = ERR_PTR(-EINVAL); + goto out; + } + if (sk->sk_user_data) { psock = ERR_PTR(-EBUSY); goto out; diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c index be3947e70fec..0d3f68bb51c0 100644 --- a/net/ipv4/tcp_bpf.c +++ b/net/ipv4/tcp_bpf.c @@ -611,9 +611,6 @@ int tcp_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool restore) return 0; } - if (inet_csk_has_ulp(sk)) - return -EINVAL; - if (sk->sk_family == AF_INET6) { if (tcp_bpf_assert_proto_ops(psock->sk_proto)) return -EINVAL; diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c index da176411c1b5..2ffede463e4a 100644 --- a/net/tls/tls_main.c +++ b/net/tls/tls_main.c @@ -921,6 +921,8 @@ static void tls_update(struct sock *sk, struct proto *p, { struct tls_context *ctx; + WARN_ON_ONCE(sk->sk_prot == p); + ctx = tls_get_ctx(sk); if (likely(ctx)) { ctx->sk_write_space = write_space;
Commit 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()") has moved the inet_csk_has_ulp(sk) check from sk_psock_init() to the new tcp_bpf_update_proto() function. I'm guessing that this was done to allow creating psocks for non-inet sockets. Unfortunately the destruction path for psock includes the ULP unwind, so we need to fail the sk_psock_init() itself. Otherwise if ULP is already present we'll notice that later, and call tcp_update_ulp() with the sk_proto of the ULP itself, which will most likely result in the ULP looping its callbacks. Fixes: 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()") Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- CC: john.fastabend@gmail.com CC: jakub@cloudflare.com CC: yoshfuji@linux-ipv6.org CC: dsahern@kernel.org CC: ast@kernel.org CC: daniel@iogearbox.net CC: andrii@kernel.org CC: kafai@fb.com CC: songliubraving@fb.com CC: yhs@fb.com CC: kpsingh@kernel.org CC: borisp@nvidia.com CC: cong.wang@bytedance.com CC: bpf@vger.kernel.org --- include/net/inet_sock.h | 5 +++++ net/core/skmsg.c | 5 +++++ net/ipv4/tcp_bpf.c | 3 --- net/tls/tls_main.c | 2 ++ 4 files changed, 12 insertions(+), 3 deletions(-)