From: John Fastabend <john.fastabend@gmail.com>
To: Michal Luczaj <mhal@rbox.co>,
Andrii Nakryiko <andrii@kernel.org>,
Eduard Zingerman <eddyz87@gmail.com>,
Mykola Lysenko <mykolal@fb.com>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Martin KaFai Lau <martin.lau@linux.dev>,
Song Liu <song@kernel.org>,
Yonghong Song <yonghong.song@linux.dev>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@kernel.org>,
Stanislav Fomichev <sdf@fomichev.me>,
Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
Shuah Khan <shuah@kernel.org>,
Jakub Sitnicki <jakub@cloudflare.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>,
Paolo Abeni <pabeni@redhat.com>,
Simon Horman <horms@kernel.org>
Cc: bpf@vger.kernel.org, linux-kselftest@vger.kernel.org,
netdev@vger.kernel.org, Michal Luczaj <mhal@rbox.co>
Subject: RE: [PATCH bpf 3/3] bpf, sockmap: Fix race between element replace and close()
Date: Sun, 08 Dec 2024 22:11:26 -0800 [thread overview]
Message-ID: <67568a0ed36d3_1abf20818@john.notmuch> (raw)
In-Reply-To: <20241202-sockmap-replace-v1-3-1e88579e7bd5@rbox.co>
Michal Luczaj wrote:
> Element replace (with a socket different from the one stored) may race with
> socket's close() link popping & unlinking. __sock_map_delete()
> unconditionally unrefs the (wrong) element:
>
> // set map[0] = s0
> map_update_elem(map, 0, s0)
>
> // drop fd of s0
> close(s0)
> sock_map_close()
> lock_sock(sk) (s0!)
> sock_map_remove_links(sk)
> link = sk_psock_link_pop()
> sock_map_unlink(sk, link)
> sock_map_delete_from_link
> // replace map[0] with s1
> map_update_elem(map, 0, s1)
> sock_map_update_elem
> (s1!) lock_sock(sk)
> sock_map_update_common
> psock = sk_psock(sk)
> spin_lock(&stab->lock)
> osk = stab->sks[idx]
> sock_map_add_link(..., &stab->sks[idx])
> sock_map_unref(osk, &stab->sks[idx])
> psock = sk_psock(osk)
> sk_psock_put(sk, psock)
> if (refcount_dec_and_test(&psock))
> sk_psock_drop(sk, psock)
> spin_unlock(&stab->lock)
> unlock_sock(sk)
> __sock_map_delete
> spin_lock(&stab->lock)
> sk = *psk // s1 replaced s0; sk == s1
> if (!sk_test || sk_test == sk) // sk_test (s0) != sk (s1); no branch
> sk = xchg(psk, NULL)
> if (sk)
> sock_map_unref(sk, psk) // unref s1; sks[idx] will dangle
> psock = sk_psock(sk)
> sk_psock_put(sk, psock)
> if (refcount_dec_and_test())
> sk_psock_drop(sk, psock)
> spin_unlock(&stab->lock)
> release_sock(sk)
>
> Then close(map) enqueues bpf_map_free_deferred, which finally calls
> sock_map_free(). This results in some refcount_t warnings along with a
> KASAN splat[1].
>
[...]
> Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
> net/core/sock_map.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 20b348b1964a10a1b0bfbe1a90a4a4cd99715b81..f1b9b3958792cd599efcb591742874e9b3f4a76b 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -412,12 +412,11 @@ static void *sock_map_lookup_sys(struct bpf_map *map, void *key)
> static int __sock_map_delete(struct bpf_stab *stab, struct sock *sk_test,
> struct sock **psk)
> {
> - struct sock *sk;
> + struct sock *sk = NULL;
> int err = 0;
>
> spin_lock_bh(&stab->lock);
> - sk = *psk;
> - if (!sk_test || sk_test == sk)
> + if (!sk_test || sk_test == *psk)
> sk = xchg(psk, NULL);
>
> if (likely(sk))
>
> --
> 2.46.2
>
Reviewed-by: John Fastabend <john.fastabend@gmail.com>
next prev parent reply other threads:[~2024-12-09 6:11 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-02 11:29 [PATCH bpf 0/3] bpf, sockmap: Fix the element replace Michal Luczaj
2024-12-02 11:29 ` [PATCH bpf 1/3] bpf, sockmap: Fix update element with same Michal Luczaj
2024-12-09 5:47 ` John Fastabend
2024-12-09 9:54 ` Michal Luczaj
2024-12-02 11:29 ` [PATCH bpf 2/3] selftest/bpf: Extend test for sockmap update " Michal Luczaj
2024-12-09 5:50 ` John Fastabend
2024-12-02 11:29 ` [PATCH bpf 3/3] bpf, sockmap: Fix race between element replace and close() Michal Luczaj
2024-12-09 6:11 ` John Fastabend [this message]
2024-12-10 16:50 ` [PATCH bpf 0/3] bpf, sockmap: Fix the element replace patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=67568a0ed36d3_1abf20818@john.notmuch \
--to=john.fastabend@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=eddyz87@gmail.com \
--cc=edumazet@google.com \
--cc=haoluo@google.com \
--cc=horms@kernel.org \
--cc=jakub@cloudflare.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=martin.lau@linux.dev \
--cc=mhal@rbox.co \
--cc=mykolal@fb.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=sdf@fomichev.me \
--cc=shuah@kernel.org \
--cc=song@kernel.org \
--cc=yonghong.song@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.