Re: [PATCH bpf-next 02/13] bpf: map_check_btf should fail if btf_parse_fields fails

[Dave Marchevsky <davemarchevsky@meta.com>]

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.