netdev - Re: [RFC PATCH bpf-next 2/2] bpf, cpumap: Clean up bpf_cpu_map_entry directly in cpu_map

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: <87fs4rfb8t.fsf@toke.dk>
Date: Thu, 10 Aug 2023 12:22:10 +0200
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Hou Tao <houtao@...weicloud.com>, bpf@...r.kernel.org
Cc: 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>,
 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>, houtao1@...wei.com
Subject: Re: [RFC PATCH bpf-next 2/2] bpf, cpumap: Clean up
 bpf_cpu_map_entry directly in cpu_map_free

Hou Tao <houtao@...weicloud.com> writes:

> From: Hou Tao <houtao1@...wei.com>
>
> After synchronize_rcu(), both the dettached XDP program and
> xdp_do_flush() are completed, and the only user of bpf_cpu_map_entry
> will be cpu_map_kthread_run(), so instead of calling
> __cpu_map_entry_replace() to empty queue and do cleanup after a RCU
> grace period, do these things directly.
>
> Signed-off-by: Hou Tao <houtao1@...wei.com>

With one nit below:

Reviewed-by: Toke Høiland-Jørgensen <toke@...hat.com>

> ---
>  kernel/bpf/cpumap.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> index 24f39c37526f..f8e2b24320c0 100644
> --- a/kernel/bpf/cpumap.c
> +++ b/kernel/bpf/cpumap.c
> @@ -554,16 +554,15 @@ static void cpu_map_free(struct bpf_map *map)
>  	/* At this point bpf_prog->aux->refcnt == 0 and this map->refcnt == 0,
>  	 * so the bpf programs (can be more than one that used this map) were
>  	 * disconnected from events. Wait for outstanding critical sections in
> -	 * these programs to complete. The rcu critical section only guarantees
> -	 * no further "XDP/bpf-side" reads against bpf_cpu_map->cpu_map.
> -	 * It does __not__ ensure pending flush operations (if any) are
> -	 * complete.
> +	 * these programs to complete. synchronize_rcu() below not only
> +	 * guarantees no further "XDP/bpf-side" reads against
> +	 * bpf_cpu_map->cpu_map, but also ensure pending flush operations
> +	 * (if any) are complete.
>  	 */
> -
>  	synchronize_rcu();
>  
> -	/* For cpu_map the remote CPUs can still be using the entries
> -	 * (struct bpf_cpu_map_entry).
> +	/* The only possible user of bpf_cpu_map_entry is
> +	 * cpu_map_kthread_run().
>  	 */
>  	for (i = 0; i < cmap->map.max_entries; i++) {
>  		struct bpf_cpu_map_entry *rcpu;
> @@ -572,8 +571,8 @@ static void cpu_map_free(struct bpf_map *map)
>  		if (!rcpu)
>  			continue;
>  
> -		/* bq flush and cleanup happens after RCU grace-period */
> -		__cpu_map_entry_replace(cmap, i, NULL); /* call_rcu */
> +		/* Empty queue and do cleanup directly */

The "empty queue" here is a bit ambiguous, maybe "Stop kthread and
cleanup entry"?

> +		__cpu_map_entry_free(&rcpu->free_work.work);
>  	}
>  	bpf_map_area_free(cmap->cpu_map);
>  	bpf_map_area_free(cmap);
> -- 
> 2.29.2