Re: [PATCH bpf-next v4] bpf: Fix a race condition between btf_put() and map_free()

[Yonghong Song <yonghong.song@linux.dev>]

On 12/8/23 12:16 AM, Martin KaFai Lau wrote:
> On 12/7/23 7:59 PM, Yonghong Song wrote:
>>>
>>> I am trying to avoid making a special case for "bool has_btf_ref;" 
>>> and "bool from_map_check". It seems to a bit too much to deal with 
>>> the error path for btf_parse().
>>>
>>> Would doing the refcount_set(&btf->refcnt, 1) earlier in btf_parse 
>>> help?
>>
>> No, it does not. The core reason is what Hao is mentioned in
>> https://lore.kernel.org/bpf/47ee3265-23f7-2130-ff28-27bfaf3f7877@huaweicloud.com/ 
>>
>> We simply cannot take btf reference if called from btf_parse().
>> Let us say we move refcount_set(&btf->refcnt, 1) earlier in btf_parse()
>> so we take ref for btf during btf_parse_fields(), then we have
>>       btf_put <=== expect refcount == 0 to start the destruction process
>>         ...
>>           btf_record_free <=== in which if graph_root, a btf 
>> reference will be hold
>> so btf_put will never be able to actually free btf data.
>
> ah. There is a loop like btf->struct_meta_tab->...btf.
>
>> Yes, the kasan problem will be resolved but we leak memory.
>>
>>>
>>>> It is also unnecessary to take a reference since the value_rec is
>>>> referring to a record in struct_meta_tab.
>>>
>>> If we optimize for not taking a refcnt, how about not taking a 
>>> refcnt for all cases and postpone the btf_put(), instead of taking 
>>> refcnt in one case but not another. Like your fix in v1. The failed 
>>> selftest can be changed or even removed if it does not make sense 
>>> anymore.
>>
>> After a couple of iterations, I think taking necessary reference 
>> approach sounds better
>> and this will be consistent with how kptr is handled. For kptr, 
>> btf_parse will ignore it.
>
> Got it. It is why kptr.btf got away with the loop.
>
> On the other hand, am I reading it correctly that kptr.btf only needs 
> to take the refcnt for btf that is btf_is_kernel()?

No. besides vmlinux and module btf, it also takes reference for prog btf, see

static int btf_parse_kptr(const struct btf *btf, struct btf_field *field,
                           struct btf_field_info *info)
{
...
         if (id == -ENOENT) {
                 /* btf_parse_kptr should only be called w/ btf = program BTF */
                 WARN_ON_ONCE(btf_is_kernel(btf));
                 
                 /* Type exists only in program BTF. Assume that it's a MEM_ALLOC
                  * kptr allocated via bpf_obj_new
                  */
                 field->kptr.dtor = NULL;
                 id = info->kptr.type_id;
                 kptr_btf = (struct btf *)btf;
                 btf_get(kptr_btf);
                 goto found_dtor;
         }
...
}

>
>> Unfortunately, for graph_root (list_head, rb_root), btf_parse and 
>> map_check will both
>> process it and that adds a little bit complexity.
>> Alexei also suggested the same taking reference approach:
>> https://lore.kernel.org/bpf/CAADnVQL+uc6VV65_Ezgzw3WH=ME9z1Fdy8Pd6xd0oOq8rgwh7g@mail.gmail.com/ 
>>
>