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]

Message-ID: <CAEf4BzbCd9ZWyunwEu78PCK14UA+kR0b7Xp8Lt46Xb2HqUdbQA@mail.gmail.com>
Date:   Wed, 27 Feb 2019 15:34:11 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     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 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>
> ---
>  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?

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?

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