Re: [PATCH bpf 1/2] bpf: Check timer_off for map_in_map only when map value have timer

[Hengqi Chen <hengqi.chen@gmail.com>]

Hi, Yonghong:

On 2022/11/27 11:21, Yonghong Song wrote:
> 
> 
> On 11/26/22 2:53 AM, Hengqi Chen wrote:
>> The timer_off value could be -EINVAL or -ENOENT when map value of
>> inner map is struct and contains no bpf_timer. The EINVAL case happens
>> when the map is created without BTF key/value info, map->timer_off
>> is set to -EINVAL in map_create(). The ENOENT case happens when
>> the map is created with BTF key/value info (e.g. from BPF skeleton),
>> map->timer_off is set to -ENOENT as what btf_find_timer() returns.
>> In bpf_map_meta_equal(), we expect timer_off to be equal even if
>> map value does not contains bpf_timer. This rejects map_in_map created
>> with BTF key/value info to be updated using inner map without BTF
>> key/value info in case inner map value is struct. This commit lifts
>> such restriction.
>>
>> Fixes: 68134668c17f ("bpf: Add map side support for bpf timers.")
>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>> ---
>>   kernel/bpf/map_in_map.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
>> index 135205d0d560..0840872de486 100644
>> --- a/kernel/bpf/map_in_map.c
>> +++ b/kernel/bpf/map_in_map.c
>> @@ -80,11 +80,18 @@ void bpf_map_meta_free(struct bpf_map *map_meta)
>>   bool bpf_map_meta_equal(const struct bpf_map *meta0,
>>               const struct bpf_map *meta1)
>>   {
>> +    bool timer_off_equal;
>> +
>> +    if (!map_value_has_timer(meta0) && !map_value_has_timer(meta1))
>> +        timer_off_equal = true;
>> +    else
>> +        timer_off_equal = meta0->timer_off == meta1->timer_off;
>> +
> 
> Is it possible we assign -1 to meta->timer_off directly instead of
> -EINVAL or -ENOENT, to indicate it does not exist? This will make
> this and possible other future timer_off comparison much easier?
> 

These error codes are checked in verifier, so didn't touch it.

>>       /* No need to compare ops because it is covered by map_type */
>>       return meta0->map_type == meta1->map_type &&
>>           meta0->key_size == meta1->key_size &&
>>           meta0->value_size == meta1->value_size &&
>> -        meta0->timer_off == meta1->timer_off &&
>> +        timer_off_equal &&
>>           meta0->map_flags == meta1->map_flags &&
>>           bpf_map_equal_kptr_off_tab(meta0, meta1);
>>   }
>> -- 
>> 2.34.1