Re: [PATCH bpf 1/2] bpf: Check timer_off for map_in_map only when map value have timer

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Sun, Nov 27, 2022 at 6:42 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> Hi, Alexei:
>
> On 2022/11/28 08:44, Alexei Starovoitov wrote:
> > On Sat, Nov 26, 2022 at 2:54 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
> >>
> >> The timer_off value could be -EINVAL or -ENOENT when map value of
> >> inner map is struct and contains no bpf_timer. The EINVAL case happens
> >> when the map is created without BTF key/value info, map->timer_off
> >> is set to -EINVAL in map_create(). The ENOENT case happens when
> >> the map is created with BTF key/value info (e.g. from BPF skeleton),
> >> map->timer_off is set to -ENOENT as what btf_find_timer() returns.
> >> In bpf_map_meta_equal(), we expect timer_off to be equal even if
> >> map value does not contains bpf_timer. This rejects map_in_map created
> >> with BTF key/value info to be updated using inner map without BTF
> >> key/value info in case inner map value is struct. This commit lifts
> >> such restriction.
> >
> > Sorry, but I prefer to label this issue as 'wont-fix'.
> > Mixing BTF enabled and non-BTF inner maps is a corner case
>
> We do have such usecase. The BPF progs and maps are pinned to bpffs
> using BPF object file. And the map_in_map is updated by some other
> process which don't have access to such BTF info.
>
> > that is not worth fixing.
>
> Is there a way to get this fixed for v5.x series only ?
>
> > At some point we will require all programs and maps to contain BTF.
> > It's necessary for introspection.
>
> We don't care much about BTF for introspection. In production, we always
> have a version field and some reserved fields in the map value for backward
> compatibility. The interpretation of such map values are left to upper layer.

All the BTF stuff aside, wouldn't this be the best and most minimal
fix? It seems to define correct semantic meaning: no timer is found
(because no BTF in this case). Easy to backport, solves the immediate
problem. This code seems to be completely reworked in bpf-next,
though, so I don't know what the situation is there.

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 7b373a5e861f..9e38cc1e136c 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1118,7 +1118,7 @@ static int map_create(union bpf_attr *attr)
        spin_lock_init(&map->owner.lock);

        map->spin_lock_off = -EINVAL;
-       map->timer_off = -EINVAL;
+       map->timer_off = -ENOENT;
        if (attr->btf_key_type_id || attr->btf_value_type_id ||
            /* Even the map's value is a kernel's struct,
             * the bpf_prog.o must have BTF to begin with




>
> > The maps as blobs of data should not be used.
> > Much so adding support for mixed use as inner maps.