On 2/27/19 3:34 PM, Andrii Nakryiko wrote:
> On Wed, Feb 27, 2019 at 1:23 PM Yonghong Song <y...@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 <y...@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.
>
> 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.
>
>>
>> /* Misc members not needed in bpf_map_meta_equal() check. */
>> inner_map_meta->ops = inner_map->ops;
>> --
>> 2.17.1
>>
