[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <08ffafc3-ddbb-48a4-99f7-d5694184f686@rbox.co>
Date: Wed, 25 Sep 2024 02:22:39 +0200
From: Michal Luczaj <mhal@...x.co>
To: Paolo Abeni <pabeni@...hat.com>, Dmitry Antipov <dmantipov@...dex.ru>,
John Fastabend <john.fastabend@...il.com>,
Jakub Sitnicki <jakub@...udflare.com>
Cc: Cong Wang <xiyou.wangcong@...il.com>, Jakub Kicinski <kuba@...nel.org>,
netdev@...r.kernel.org, lvc-project@...uxtesting.org,
syzbot+f363afac6b0ace576f45@...kaller.appspotmail.com,
Kuniyuki Iwashima <kuniyu@...zon.com>
Subject: Re: [PATCH net v2] net: sockmap: avoid race between
sock_map_destroy() and sk_psock_put()
On 9/24/24 10:23, Paolo Abeni wrote:
> ...
> I guess that the main point in Cong's feedback is that a sockmap update
> is not supposed to race with sock_map_destroy() (???) @Cong, @John,
> @JakubS: any comments on that?
In somewhat related news: sock_map_unhash() races with the update, hitting
WARN_ON_ONCE(saved_unhash == sock_map_unhash).
CPU0 CPU1
==== ====
BPF_MAP_DELETE_ELEM
sk_psock_drop()
sk_psock_restore_proto
rcu_assign_sk_user_data(NULL)
shutdown()
sock_map_unhash()
psock = sk_psock(sk)
if (unlikely(!psock)) {
BPF_MAP_UPDATE_ELEM
sock_map_init_proto()
sock_replace_proto
saved_unhash = READ_ONCE(sk->sk_prot)->unhash;
}
if (WARN_ON_ONCE(saved_unhash == sock_map_unhash))
return;
[ 20.860668] WARNING: CPU: 1 PID: 1238 at net/core/sock_map.c:1641 sock_map_unhash+0xa1/0x220
[ 20.860686] CPU: 1 UID: 0 PID: 1238 Comm: a.out Not tainted 6.11.0+ #59
[ 20.860688] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.3-1-1 04/01/2014
[ 20.860705] Call Trace:
[ 20.860706] <TASK>
[ 20.860725] unix_shutdown+0xb0/0x470
[ 20.860728] __sys_shutdown+0x7a/0xa0
[ 20.860731] __x64_sys_shutdown+0x10/0x20
[ 20.860733] do_syscall_64+0x93/0x180
[ 20.860788] entry_SYSCALL_64_after_hwframe+0x76/0x7e
Under VM it takes about 10 minutes to trigger the splat:
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <unistd.h>
#include <pthread.h>
#include <sys/un.h>
#include <sys/syscall.h>
#include <sys/socket.h>
#include <linux/bpf.h>
int s[2], sockmap;
static void die(char *msg)
{
perror(msg);
exit(-1);
}
static int create_sockmap(int key_size, int value_size, int max_entries)
{
union bpf_attr attr = {
.map_type = BPF_MAP_TYPE_SOCKMAP,
.key_size = key_size,
.value_size = value_size,
.max_entries = max_entries
};
int map = syscall(SYS_bpf, BPF_MAP_CREATE, &attr, sizeof(attr));
if (map < 0)
die("bpf_create_map");
return map;
}
static void map_update_elem(int map_fd, int key, void *value, uint64_t flags)
{
union bpf_attr attr = {
.map_fd = map_fd,
.key = (uint64_t)&key,
.value = (uint64_t)value,
.flags = flags
};
syscall(SYS_bpf, BPF_MAP_UPDATE_ELEM, &attr, sizeof(attr));
}
static void map_del_elem(int map_fd, int key)
{
union bpf_attr attr = {
.map_fd = map_fd,
.key = (uint64_t)&key
};
syscall(SYS_bpf, BPF_MAP_DELETE_ELEM, &attr, sizeof(attr));
}
static void *racer_del(void *unused)
{
for (;;)
map_del_elem(sockmap, 0);
return NULL;
}
static void *racer_update(void *unused)
{
for (;;)
map_update_elem(sockmap, 0, &s[0], BPF_ANY);
return NULL;
}
int main(void)
{
pthread_t t0, t1;
if (pthread_create(&t0, NULL, racer_del, NULL))
die("pthread_create");
if (pthread_create(&t1, NULL, racer_update, NULL))
die("pthread_create");
sockmap = create_sockmap(sizeof(int), sizeof(int), 1);
for (;;) {
if (socketpair(AF_UNIX, SOCK_STREAM, 0, s) < 0)
die("socketpair");
map_update_elem(sockmap, 0, &s[0], BPF_ANY);
shutdown(s[1], 0);
close(s[0]);
close(s[1]);
}
}
With mdelay(1) it's a matter of seconds:
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 724b6856fcc3..98a964399813 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1631,6 +1631,7 @@ void sock_map_unhash(struct sock *sk)
psock = sk_psock(sk);
if (unlikely(!psock)) {
rcu_read_unlock();
+ mdelay(1);
saved_unhash = READ_ONCE(sk->sk_prot)->unhash;
} else {
saved_unhash = psock->saved_unhash;
I've tried the patch below and it seems to do the trick
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 724b6856fcc3..a384771a66e8 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1627,6 +1627,7 @@ void sock_map_unhash(struct sock *sk)
void (*saved_unhash)(struct sock *sk);
struct sk_psock *psock;
+ lock_sock(sk);
rcu_read_lock();
psock = sk_psock(sk);
if (unlikely(!psock)) {
@@ -1637,6 +1638,7 @@ void sock_map_unhash(struct sock *sk)
sock_map_remove_links(sk, psock);
rcu_read_unlock();
}
+ release_sock(sk);
if (WARN_ON_ONCE(saved_unhash == sock_map_unhash))
return;
if (saved_unhash)
but perhaps what needs to be fixed instead is af_unix shutdown()?
CCing Kuniyuki.
thanks,
Michal
Powered by blists - more mailing lists