| Message ID | 20230729095107.1722450-3-houtao@huaweicloud.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | Two fixes for cpu-map | expand |
On 29/07/2023 11.51, Hou Tao wrote: > From: Hou Tao <houtao1@huawei.com> > > The following warning was reported when running xdp_redirect_cpu with > both skb-mode and stress-mode enabled: > > ------------[ cut here ]------------ > Incorrect XDP memory type (-2128176192) usage I'm happy to see that WARN "Incorrect XDP memory type" caught this. > WARNING: CPU: 7 PID: 1442 at net/core/xdp.c:405 > Modules linked in: > CPU: 7 PID: 1442 Comm: kworker/7:0 Tainted: G 6.5.0-rc2+ #1 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996) > Workqueue: events __cpu_map_entry_free > RIP: 0010:__xdp_return+0x1e4/0x4a0 > ...... > Call Trace: > <TASK> > ? show_regs+0x65/0x70 > ? __warn+0xa5/0x240 > ? __xdp_return+0x1e4/0x4a0 > ...... > xdp_return_frame+0x4d/0x150 > __cpu_map_entry_free+0xf9/0x230 > 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 reason for the warning is twofold. One is due to the kthread > cpu_map_kthread_run() is stopped prematurely. Another one is > __cpu_map_ring_cleanup() doesn't handle skb mode and treats skbs in > ptr_ring as XDP frames. > > Prematurely-stopped kthread will be fixed by the preceding patch and > ptr_ring will be empty when __cpu_map_ring_cleanup() is called. But > as the comments in __cpu_map_ring_cleanup() said, handling and freeing > skbs in ptr_ring as well to "catch any broken behaviour gracefully". > > Fixes: 11941f8a8536 ("bpf: cpumap: Implement generic cpumap") > Signed-off-by: Hou Tao <houtao1@huawei.com> > --- Acked-by: Jesper Dangaard Brouer <hawk@kernel.org> I think we should fix this code, even-though patch 1 have made it harder to trigger. > kernel/bpf/cpumap.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c > index 08351e0863e5..ef28c64f1eb1 100644 > --- a/kernel/bpf/cpumap.c > +++ b/kernel/bpf/cpumap.c > @@ -129,11 +129,17 @@ static void __cpu_map_ring_cleanup(struct ptr_ring *ring) > * invoked cpu_map_kthread_stop(). Catch any broken behaviour > * gracefully and warn once. > */ > - struct xdp_frame *xdpf; > + void *ptr; > > - while ((xdpf = ptr_ring_consume(ring))) > - if (WARN_ON_ONCE(xdpf)) > - xdp_return_frame(xdpf); > + while ((ptr = ptr_ring_consume(ring))) { > + WARN_ON_ONCE(1); > + if (unlikely(__ptr_test_bit(0, &ptr))) { > + __ptr_clear_bit(0, &ptr); > + kfree_skb(ptr); > + continue; > + } > + xdp_return_frame(ptr); > + } > } > > static void put_cpu_map_entry(struct bpf_cpu_map_entry *rcpu)
diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c index 08351e0863e5..ef28c64f1eb1 100644 --- a/kernel/bpf/cpumap.c +++ b/kernel/bpf/cpumap.c @@ -129,11 +129,17 @@ static void __cpu_map_ring_cleanup(struct ptr_ring *ring) * invoked cpu_map_kthread_stop(). Catch any broken behaviour * gracefully and warn once. */ - struct xdp_frame *xdpf; + void *ptr; - while ((xdpf = ptr_ring_consume(ring))) - if (WARN_ON_ONCE(xdpf)) - xdp_return_frame(xdpf); + while ((ptr = ptr_ring_consume(ring))) { + WARN_ON_ONCE(1); + if (unlikely(__ptr_test_bit(0, &ptr))) { + __ptr_clear_bit(0, &ptr); + kfree_skb(ptr); + continue; + } + xdp_return_frame(ptr); + } } static void put_cpu_map_entry(struct bpf_cpu_map_entry *rcpu)