Skip to content

Commit 7eeeb29

Browse files
Hou TaoFishwaldo
authored andcommitted
bpf, cpumap: Make sure kthread is running before map update returns
commit 640a604 upstream. The following warning was reported when running stress-mode enabled xdp_redirect_cpu with some RT threads: ------------[ cut here ]------------ WARNING: CPU: 4 PID: 65 at kernel/bpf/cpumap.c:135 CPU: 4 PID: 65 Comm: kworker/4:1 Not tainted 6.5.0-rc2+ #1 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996) Workqueue: events cpu_map_kthread_stop RIP: 0010:put_cpu_map_entry+0xda/0x220 ...... Call Trace: <TASK> ? show_regs+0x65/0x70 ? __warn+0xa5/0x240 ...... ? put_cpu_map_entry+0xda/0x220 cpu_map_kthread_stop+0x41/0x60 process_one_work+0x6b0/0xb80 worker_thread+0x96/0x720 kthread+0x1a5/0x1f0 ret_from_fork+0x3a/0x70 ret_from_fork_asm+0x1b/0x30 </TASK> The root cause is the same as commit 4369016 ("bpf: cpumap: Fix memory leak in cpu_map_update_elem"). The kthread is stopped prematurely by kthread_stop() in cpu_map_kthread_stop(), and kthread() doesn't call cpu_map_kthread_run() at all but XDP program has already queued some frames or skbs into ptr_ring. So when __cpu_map_ring_cleanup() checks the ptr_ring, it will find it was not emptied and report a warning. An alternative fix is to use __cpu_map_ring_cleanup() to drop these pending frames or skbs when kthread_stop() returns -EINTR, but it may confuse the user, because these frames or skbs have been handled correctly by XDP program. So instead of dropping these frames or skbs, just make sure the per-cpu kthread is running before __cpu_map_entry_alloc() returns. After apply the fix, the error handle for kthread_stop() will be unnecessary because it will always return 0, so just remove it. Fixes: 6710e11 ("bpf: introduce new bpf cpu map type BPF_MAP_TYPE_CPUMAP") Signed-off-by: Hou Tao <houtao1@huawei.com> Reviewed-by: Pu Lehui <pulehui@huawei.com> Acked-by: Jesper Dangaard Brouer <hawk@kernel.org> Link: https://lore.kernel.org/r/20230729095107.1722450-2-houtao@huaweicloud.com Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent d42e9ed commit 7eeeb29

File tree

1 file changed

+11
-10
lines changed

1 file changed

+11
-10
lines changed

kernel/bpf/cpumap.c

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include <linux/workqueue.h>
2727
#include <linux/kthread.h>
2828
#include <linux/capability.h>
29+
#include <linux/completion.h>
2930
#include <trace/events/xdp.h>
3031

3132
#include <linux/netdevice.h> /* netif_receive_skb_list */
@@ -70,6 +71,7 @@ struct bpf_cpu_map_entry {
7071
struct rcu_head rcu;
7172

7273
struct work_struct kthread_stop_wq;
74+
struct completion kthread_running;
7375
};
7476

7577
struct bpf_cpu_map {
@@ -163,7 +165,6 @@ static void put_cpu_map_entry(struct bpf_cpu_map_entry *rcpu)
163165
static void cpu_map_kthread_stop(struct work_struct *work)
164166
{
165167
struct bpf_cpu_map_entry *rcpu;
166-
int err;
167168

168169
rcpu = container_of(work, struct bpf_cpu_map_entry, kthread_stop_wq);
169170

@@ -173,14 +174,7 @@ static void cpu_map_kthread_stop(struct work_struct *work)
173174
rcu_barrier();
174175

175176
/* kthread_stop will wake_up_process and wait for it to complete */
176-
err = kthread_stop(rcpu->kthread);
177-
if (err) {
178-
/* kthread_stop may be called before cpu_map_kthread_run
179-
* is executed, so we need to release the memory related
180-
* to rcpu.
181-
*/
182-
put_cpu_map_entry(rcpu);
183-
}
177+
kthread_stop(rcpu->kthread);
184178
}
185179

186180
static void cpu_map_bpf_prog_run_skb(struct bpf_cpu_map_entry *rcpu,
@@ -308,11 +302,11 @@ static int cpu_map_bpf_prog_run(struct bpf_cpu_map_entry *rcpu, void **frames,
308302
return nframes;
309303
}
310304

311-
312305
static int cpu_map_kthread_run(void *data)
313306
{
314307
struct bpf_cpu_map_entry *rcpu = data;
315308

309+
complete(&rcpu->kthread_running);
316310
set_current_state(TASK_INTERRUPTIBLE);
317311

318312
/* When kthread gives stop order, then rcpu have been disconnected
@@ -475,6 +469,7 @@ __cpu_map_entry_alloc(struct bpf_map *map, struct bpf_cpumap_val *value,
475469
goto free_ptr_ring;
476470

477471
/* Setup kthread */
472+
init_completion(&rcpu->kthread_running);
478473
rcpu->kthread = kthread_create_on_node(cpu_map_kthread_run, rcpu, numa,
479474
"cpumap/%d/map:%d", cpu,
480475
map->id);
@@ -488,6 +483,12 @@ __cpu_map_entry_alloc(struct bpf_map *map, struct bpf_cpumap_val *value,
488483
kthread_bind(rcpu->kthread, cpu);
489484
wake_up_process(rcpu->kthread);
490485

486+
/* Make sure kthread has been running, so kthread_stop() will not
487+
* stop the kthread prematurely and all pending frames or skbs
488+
* will be handled by the kthread before kthread_stop() returns.
489+
*/
490+
wait_for_completion(&rcpu->kthread_running);
491+
491492
return rcpu;
492493

493494
free_prog:

0 commit comments

Comments
 (0)