Hi,
On 10/22/2024 7:02 AM, Andrii Nakryiko wrote:
> On Mon, Oct 21, 2024 at 1:18 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>> On Mon, Oct 21, 2024 at 09:39:59AM +0800, Hou Tao wrote:
>>> From: Hou Tao <houtao1@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> ---
>>> 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
> I'm not sure what's the implication here, but I'd avoid doing that.
> But see below.
OK.
>>> 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);
> If this is useless, why are you adding it?
It will work after removing these CONFIG_XXX dependencies for
BPF_LINK_TYPE() invocations.
>
> Let's instead do a NULL check inside bpf_link_show_fdinfo() to handle
> sparsity. And to avoid out-of-bounds, just add
>
> [__MAX_BPF_LINK_TYPE] = NULL,
>
> into the definition of bpf_link_type_strs
Instead of outputting a null string for a link_type which didn't invoke
BPF_LINK_TYPE, is outputting the numerical value of link->type more
reasonable as shown below ?
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 2873302faf39..9a02cd914ed8 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3073,14 +3073,15 @@ static void bpf_link_show_fdinfo(struct seq_file
*m, struct file *filp)
const struct bpf_link *link = filp->private_data;
const struct bpf_prog *prog = link->prog;
char prog_tag[sizeof(prog->tag) * 2 + 1] = { };
+ enum bpf_link_type type;
+ if (type < ARRAY_SIZE(bpf_link_type_strs) &&
bpf_link_type_strs[type])
+ seq_printf(m, "link_type:\t%s\n", bpf_link_type_strs[type]);
+ else
+ seq_printf(m, "link_type:\t[%d]\n", type);
+
+ seq_printf(m, "link_id:\t%u\n", link->id);
- seq_printf(m,
- "link_type:\t%s\n"
- "link_id:\t%u\n",
- bpf_link_type_strs[link->type],
- link->id);
>
> pw-bot: cr
>
>> 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
>>
> This seems like a bit too short-sighted approach, let's not go there just yet.
>
>> jirka
