Hi,
On 10/11/2024 2:02 AM, Eduard Zingerman wrote:
> On Tue, 2024-10-08 at 17:14 +0800, Hou Tao wrote:
>
> [...]
>
>> diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
>> index 645bd30bc9a9..a072835dc645 100644
>> --- a/kernel/bpf/map_in_map.c
>> +++ b/kernel/bpf/map_in_map.c
> [...]
>
>> @@ -45,9 +45,13 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
>> * invalid/empty/valid, but ERR_PTR in case of errors. During
>> * equality NULL or IS_ERR is equivalent.
>> */
>> - struct bpf_map *ret = ERR_CAST(inner_map_meta->record);
>> - kfree(inner_map_meta);
>> - return ret;
>> + ret = ERR_CAST(inner_map_meta->record);
>> + goto free;
>> + }
>> + inner_map_meta->key_record = btf_record_dup(inner_map->key_record);
>> + if (IS_ERR(inner_map_meta->key_record)) {
>> + ret = ERR_CAST(inner_map_meta->key_record);
>> + goto free;
> The 'goto free' executes a call to bpf_map_meta_free() which does
> btf_put(map_meta->btf), but corresponding btf_get(inner_map->btf) only
> happens on the lines below => in case when 'free' branch is taken we
> 'put' BTF object that was not 'get' by us.
Yes, but map_meta->btf is NULL, so calling btf_put(NULL) incurs no harm.
My purpose was trying to simplify the error handling, but it seems that
it leads to confusion. Will only undo the done part in next revision.
>
>> }
>> /* Note: We must use the same BTF, as we also used btf_record_dup above
>> * which relies on BTF being same for both maps, as some members like
>> @@ -71,6 +75,10 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
>> inner_map_meta->bypass_spec_v1 = inner_map->bypass_spec_v1;
>> }
>> return inner_map_meta;
>> +
>> +free:
>> + bpf_map_meta_free(inner_map_meta);
>> + return ret;
>> }
>>
>> void bpf_map_meta_free(struct bpf_map *map_meta)
> [...]
