netdev - Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock

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]

Date:   Wed, 27 Feb 2019 16:43:48 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly

On Wed, Feb 27, 2019 at 4:40 PM Yonghong Song <yhs@...com> wrote:
>
>
>
> On 2/27/19 4:28 PM, Andrii Nakryiko wrote:
> > On Wed, Feb 27, 2019 at 4:19 PM Yonghong Song <yhs@...com> wrote:
> >>
> >>
> >>
> >> On 2/27/19 3:34 PM, Andrii Nakryiko wrote:
> >>> On Wed, Feb 27, 2019 at 1:23 PM Yonghong Song <yhs@...com> wrote:
> >>>>
> >>>> Commit d83525ca62cf ("bpf: introduce bpf_spin_lock")
> >>>> introduced bpf_spin_lock and the field spin_lock_off
> >>>> in kernel internal structure bpf_map has the following
> >>>> meaning:
> >>>>     >=0 valid offset, <0 error
> >>>>
> >>>> For every map created, the kernel will ensure
> >>>> spin_lock_off has correct value.
> >>>>
> >>>> Currently, bpf_map->spin_lock_off is not copied
> >>>> from the inner map to the map_in_map inner_map_meta
> >>>> during a map_in_map type map creation, so
> >>>> inner_map_meta->spin_lock_off = 0.
> >>>> This will give verifier wrong information that
> >>>> inner_map has bpf_spin_lock and the bpf_spin_lock
> >>>> is defined at offset 0. An access to offset 0
> >>>> of a value pointer will trigger the following error:
> >>>>      bpf_spin_lock cannot be accessed directly by load/store
> >>>>
> >>>> This patch fixed the issue by copy inner map's spin_lock_off
> >>>> value to inner_map_meta->spin_lock_off.
> >>>>
> >>>> Fixes: d83525ca62cf ("bpf: introduce bpf_spin_lock")
> >>>> Signed-off-by: Yonghong Song <yhs@...com>
> >
> > Acked-by: Andrii Nakryiko <andriin@...com>
> >
> >>>> ---
> >>>>    kernel/bpf/map_in_map.c | 1 +
> >>>>    1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
> >>>> index 583346a0ab29..3dff41403583 100644
> >>>> --- a/kernel/bpf/map_in_map.c
> >>>> +++ b/kernel/bpf/map_in_map.c
> >>>> @@ -58,6 +58,7 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
> >>>>           inner_map_meta->value_size = inner_map->value_size;
> >>>>           inner_map_meta->map_flags = inner_map->map_flags;
> >>>>           inner_map_meta->max_entries = inner_map->max_entries;
> >>>> +       inner_map_meta->spin_lock_off = inner_map->spin_lock_off;
> >>>
> >>> Looks like spinlock inside inner map is not supported: there is
> >>> specific check few lines above returning -ENOSUPP for such case. In
> >>> that case, maybe assign -1 here to make this explicit?
> >>
> >> -1 (-EPERM) probably not the best choice. The verifier already has
> >> knowledge that a particular tracked map is an inner map or not. So
> >> keeping the original error code (mostly -EINVAL) is preferred I think.
> >
> > Ah, I actually missed the fact that verifier actually checks those
> > values (so it's not just >= 0 or < 0), so yeah, let's just pass
> > through. Btw, the value when there is no spinlock is actually -ENOENT.
>
> If there is no BTF, it will be -EINVAL. If there is BTF and no spinlock
> member, mostly -ENOENT.

You are right, I stand corrected. In both cases the effect should be
the same (no way to use spinlock).

>
> >
> >>
> >>>
> >>> Though I guess that also brings up the question: is there any harm in
> >>> supporting spin lock for inner map and why it was disabled in the
> >>> first place?
> >>
> >> Not exactly sure about the reason. Maybe with this patch, it can get
> >> proper support. Not 100% sure.
> >
> > No, it won't, because bpf_map_meta_alloc explicitly tests for it:
> >
> >          if (map_value_has_spin_lock(inner_map)) {
> >                  fdput(f);
> >                  return ERR_PTR(-ENOTSUPP);
> >          }
>
> I mean that this can be removed after my patch and it may work :-)

Ah, got it, yeah, maybe.

>
> >
> > Maybe Alexei can clarify?
> >
> >
> >>
> >>>
> >>>>
> >>>>           /* Misc members not needed in bpf_map_meta_equal() check. */
> >>>>           inner_map_meta->ops = inner_map->ops;
> >>>> --
> >>>> 2.17.1
> >>>>