Re: [PATCH bpf v2 2/7] bpf: Add assertion for the size of bpf_link_type_strs[]

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Oct 21, 2024 at 09:39:59AM +0800, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> If a corresponding link type doesn't invoke BPF_LINK_TYPE(), accessing
> bpf_link_type_strs[link->type] may result in out-of-bound access.
> 
> To prevent such missed invocations in the future, the following static
> assertion seems feasible:
> 
>   BUILD_BUG_ON(ARRAY_SIZE(bpf_link_type_strs) != __MAX_BPF_LINK_TYPE)
> 
> However, this doesn't work well. The reason is that the invocation of
> BPF_LINK_TYPE() for one link type is optional due to its CONFIG_XXX
> dependency and the elements in bpf_link_type_strs[] will be sparse. For
> example, if CONFIG_NET is disabled, the size of bpf_link_type_strs will
> be BPF_LINK_TYPE_UPROBE_MULTI + 1.
> 
> Therefore, in addition to the static assertion, remove all CONFIG_XXX
> conditions for the invocation of BPF_LINK_TYPE(). If these CONFIG_XXX
> conditions become necessary later, the fix may need to be revised (e.g.,
> to check the validity of link_type in bpf_link_show_fdinfo()).
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  include/linux/bpf_types.h | 6 ------
>  kernel/bpf/syscall.c      | 2 ++
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> index fa78f49d4a9a..6b7eabe9a115 100644
> --- a/include/linux/bpf_types.h
> +++ b/include/linux/bpf_types.h
> @@ -136,21 +136,15 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_ARENA, arena_map_ops)
>  
>  BPF_LINK_TYPE(BPF_LINK_TYPE_RAW_TRACEPOINT, raw_tracepoint)
>  BPF_LINK_TYPE(BPF_LINK_TYPE_TRACING, tracing)
> -#ifdef CONFIG_CGROUP_BPF
>  BPF_LINK_TYPE(BPF_LINK_TYPE_CGROUP, cgroup)
> -#endif
>  BPF_LINK_TYPE(BPF_LINK_TYPE_ITER, iter)
> -#ifdef CONFIG_NET
>  BPF_LINK_TYPE(BPF_LINK_TYPE_NETNS, netns)
>  BPF_LINK_TYPE(BPF_LINK_TYPE_XDP, xdp)
>  BPF_LINK_TYPE(BPF_LINK_TYPE_NETFILTER, netfilter)
>  BPF_LINK_TYPE(BPF_LINK_TYPE_TCX, tcx)
>  BPF_LINK_TYPE(BPF_LINK_TYPE_NETKIT, netkit)
>  BPF_LINK_TYPE(BPF_LINK_TYPE_SOCKMAP, sockmap)
> -#endif
> -#ifdef CONFIG_PERF_EVENTS
>  BPF_LINK_TYPE(BPF_LINK_TYPE_PERF_EVENT, perf)
> -#endif
>  BPF_LINK_TYPE(BPF_LINK_TYPE_KPROBE_MULTI, kprobe_multi)
>  BPF_LINK_TYPE(BPF_LINK_TYPE_STRUCT_OPS, struct_ops)
>  BPF_LINK_TYPE(BPF_LINK_TYPE_UPROBE_MULTI, uprobe_multi)
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 8cfa7183d2ef..9f335c379b05 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3071,6 +3071,8 @@ static void bpf_link_show_fdinfo(struct seq_file *m, struct file *filp)
>  	const struct bpf_prog *prog = link->prog;
>  	char prog_tag[sizeof(prog->tag) * 2 + 1] = { };
>  
> +	BUILD_BUG_ON(ARRAY_SIZE(bpf_link_type_strs) != __MAX_BPF_LINK_TYPE);

I wonder it'd be simpler to just kill BPF_LINK_TYPE completely
and add link names directly to bpf_link_type_strs array..
it seems it's the only purpose of the BPF_LINK_TYPE macro

jirka