netdev - Re: [PATCH bpf 2/2] bpf, cpumap: Handle skb as well when clean up ptr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6e55ab8c-0b84-4de7-0f92-c8789732dbdb@redhat.com>
Date: Mon, 31 Jul 2023 12:09:45 +0200
From: Jesper Dangaard Brouer <jbrouer@...hat.com>
To: Hou Tao <houtao@...weicloud.com>, bpf@...r.kernel.org
Cc: brouer@...hat.com, netdev@...r.kernel.org,
 "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
 Jesper Dangaard Brouer <hawk@...nel.org>,
 John Fastabend <john.fastabend@...il.com>,
 Björn Töpel <bjorn.topel@...il.com>,
 Toke Høiland-Jørgensen <toke@...hat.com>,
 Martin KaFai Lau <martin.lau@...ux.dev>,
 Alexei Starovoitov <alexei.starovoitov@...il.com>,
 Andrii Nakryiko <andrii@...nel.org>, Song Liu <song@...nel.org>,
 Hao Luo <haoluo@...gle.com>, Yonghong Song <yonghong.song@...ux.dev>,
 Daniel Borkmann <daniel@...earbox.net>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
 Pu Lehui <pulehui@...wei.com>, houtao1@...wei.com
Subject: Re: [PATCH bpf 2/2] bpf, cpumap: Handle skb as well when clean up
 ptr_ring

On 29/07/2023 11.51, Hou Tao wrote:
> From: Hou Tao <houtao1@...wei.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@...wei.com>
> ---

Acked-by: Jesper Dangaard Brouer <hawk@...nel.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)