On Wed, Dec 07, 2022 at 10:19:00PM +0530, Kumar Kartikeya Dwivedi wrote:
> 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.
That's a good point.
If we can error on such misconstructed map at the program verification time that's better
anyway, since there will be a proper verifier log instead of EINVAL from map_create.
