| Message ID | 20221206231000.3180914-3-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | BPF rbtree next-gen datastructure | expand |
On Tue, Dec 06, 2022 at 03:09:49PM -0800, 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 s/groot/ghead/ ? > 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@fb.com> > cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> > 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; > + > + if (map->record) { > int i; > > if (!bpf_capable()) { > -- > 2.30.2 >
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@fb.com> > cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> > 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.
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@fb.com> > > cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > 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.
On 12/7/22 2:05 PM, Alexei Starovoitov wrote: > 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@fb.com> >>> cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> >>> 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. In v2 I addressed these comments by just dropping this patch. No additional logic is needed for "error at verification time", since btf_parse_fields doesn't create a btf_record, and thus the first insn that expects the map_val to have one will cause verification to fail. For my "list_head __contains rb_node" case, the first insn is usually bpf_spin_lock call, which also needs a populated btf_record for spin_lock. Unfortunately this doesn't really achieve "proper verifier log", since spin_lock definition isn't the root cause here, but verifier error msg can only complain about spin_lock. Not that the error message coming from BTF parse or check failing is any better. Anyways, I think there's some path forward here that results in a good error message. But semantics work how we want them to without this commit, so it can be delayed for followups.
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; + + if (map->record) { int i; if (!bpf_capable()) {
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@fb.com> cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> Fixes: aa3496accc41 ("bpf: Refactor kptr_off_tab into btf_record") --- kernel/bpf/syscall.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)