Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly

[Yonghong Song <yhs@fb.com>]

On 2/27/19 4:28 PM, Andrii Nakryiko wrote:
> On Wed, Feb 27, 2019 at 4:19 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 2/27/19 3:34 PM, Andrii Nakryiko wrote:
>>> On Wed, Feb 27, 2019 at 1:23 PM Yonghong Song <yhs@fb.com> wrote:
>>>>
>>>> Commit d83525ca62cf ("bpf: introduce bpf_spin_lock")
>>>> introduced bpf_spin_lock and the field spin_lock_off
>>>> in kernel internal structure bpf_map has the following
>>>> meaning:
>>>>     >=0 valid offset, <0 error
>>>>
>>>> For every map created, the kernel will ensure
>>>> spin_lock_off has correct value.
>>>>
>>>> Currently, bpf_map->spin_lock_off is not copied
>>>> from the inner map to the map_in_map inner_map_meta
>>>> during a map_in_map type map creation, so
>>>> inner_map_meta->spin_lock_off = 0.
>>>> This will give verifier wrong information that
>>>> inner_map has bpf_spin_lock and the bpf_spin_lock
>>>> is defined at offset 0. An access to offset 0
>>>> of a value pointer will trigger the following error:
>>>>      bpf_spin_lock cannot be accessed directly by load/store
>>>>
>>>> This patch fixed the issue by copy inner map's spin_lock_off
>>>> value to inner_map_meta->spin_lock_off.
>>>>
>>>> Fixes: d83525ca62cf ("bpf: introduce bpf_spin_lock")
>>>> Signed-off-by: Yonghong Song <yhs@fb.com>
> 
> Acked-by: Andrii Nakryiko <andriin@fb.com>
> 
>>>> ---
>>>>    kernel/bpf/map_in_map.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
>>>> index 583346a0ab29..3dff41403583 100644
>>>> --- a/kernel/bpf/map_in_map.c
>>>> +++ b/kernel/bpf/map_in_map.c
>>>> @@ -58,6 +58,7 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
>>>>           inner_map_meta->value_size = inner_map->value_size;
>>>>           inner_map_meta->map_flags = inner_map->map_flags;
>>>>           inner_map_meta->max_entries = inner_map->max_entries;
>>>> +       inner_map_meta->spin_lock_off = inner_map->spin_lock_off;
>>>
>>> Looks like spinlock inside inner map is not supported: there is
>>> specific check few lines above returning -ENOSUPP for such case. In
>>> that case, maybe assign -1 here to make this explicit?
>>
>> -1 (-EPERM) probably not the best choice. The verifier already has
>> knowledge that a particular tracked map is an inner map or not. So
>> keeping the original error code (mostly -EINVAL) is preferred I think.
> 
> Ah, I actually missed the fact that verifier actually checks those
> values (so it's not just >= 0 or < 0), so yeah, let's just pass
> through. Btw, the value when there is no spinlock is actually -ENOENT.

If there is no BTF, it will be -EINVAL. If there is BTF and no spinlock 
member, mostly -ENOENT.

> 
>>
>>>
>>> Though I guess that also brings up the question: is there any harm in
>>> supporting spin lock for inner map and why it was disabled in the
>>> first place?
>>
>> Not exactly sure about the reason. Maybe with this patch, it can get
>> proper support. Not 100% sure.
> 
> No, it won't, because bpf_map_meta_alloc explicitly tests for it:
> 
>          if (map_value_has_spin_lock(inner_map)) {
>                  fdput(f);
>                  return ERR_PTR(-ENOTSUPP);
>          }

I mean that this can be removed after my patch and it may work :-)

> 
> Maybe Alexei can clarify?
> 
> 
>>
>>>
>>>>
>>>>           /* Misc members not needed in bpf_map_meta_equal() check. */
>>>>           inner_map_meta->ops = inner_map->ops;
>>>> --
>>>> 2.17.1
>>>>