On Wed, Dec 07, 2022 at 04:39:49AM IST, Dave Marchevsky wrote:
> map_check_btf calls btf_parse_fields to create a btf_record for its
> value_type. If there are no special fields in the value_type
> btf_parse_fields returns NULL, whereas if there special value_type
> fields but they are invalid in some way an error is returned.
>
> An example invalid state would be:
>
> struct node_data {
> struct bpf_rb_node node;
> int data;
> };
>
> private(A) struct bpf_spin_lock glock;
> private(A) struct bpf_list_head ghead __contains(node_data, node);
>
> groot should be invalid as its __contains tag points to a field with
> type != "bpf_list_node".
>
> Before this patch, such a scenario would result in btf_parse_fields
> returning an error ptr, subsequent !IS_ERR_OR_NULL check failing,
> and btf_check_and_fixup_fields returning 0, which would then be
> returned by map_check_btf.
>
> After this patch's changes, -EINVAL would be returned by map_check_btf
> and the map would correctly fail to load.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
> cc: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> Fixes: aa3496accc41 ("bpf: Refactor kptr_off_tab into btf_record")
> ---
> kernel/bpf/syscall.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 35972afb6850..c3599a7902f0 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1007,7 +1007,10 @@ static int map_check_btf(struct bpf_map *map, const struct btf *btf,
> map->record = btf_parse_fields(btf, value_type,
> BPF_SPIN_LOCK | BPF_TIMER | BPF_KPTR | BPF_LIST_HEAD,
> map->value_size);
> - if (!IS_ERR_OR_NULL(map->record)) {
> + if (IS_ERR(map->record))
> + return -EINVAL;
> +
I didn't do this on purpose, because of backward compatibility concerns. An
error has not been returned in earlier kernel versions during map creation time
and those fields acted like normal non-special regions, with errors on use of
helpers that act on those fields.
Especially that bpf_spin_lock and bpf_timer are part of the unified btf_record.
If we are doing such a change, then you should also drop the checks for IS_ERR
in verifier.c, since that shouldn't be possible anymore. But I think we need to
think carefully before changing this.
One possible example is: If we introduce bpf_foo in the future and program
already has that defined in map value, using it for some other purpose, with
different alignment and size, their map creation will start failing.
