* [PATCH bpf-next 0/2] bpf: set inner_map_meta->spin_lock_off correctly
@ 2019-02-27 21:22 Yonghong Song
2019-02-27 21:22 ` [PATCH bpf-next 1/2] " Yonghong Song
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Yonghong Song @ 2019-02-27 21:22 UTC (permalink / raw)
To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Yonghong Song
The inner_map_meta->spin_lock_off is not set correctly during
map creation for BPF_MAP_TYPE_ARRAY_OF_MAPS and BPF_MAP_TYPE_HASH_OF_MAPS.
This may lead verifier error due to misinformation.
This patch set fixed the issue with Patch #1 for the kernel change
and Patch #2 for enhanced selftest test_maps.
Yonghong Song (2):
bpf: set inner_map_meta->spin_lock_off correctly
tools/bpf: selftests: add map lookup to test_map_in_map bpf prog
kernel/bpf/map_in_map.c | 1 +
tools/testing/selftests/bpf/progs/test_map_in_map.c | 4 ++++
2 files changed, 5 insertions(+)
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
2019-02-27 21:22 [PATCH bpf-next 0/2] bpf: set inner_map_meta->spin_lock_off correctly Yonghong Song
@ 2019-02-27 21:22 ` Yonghong Song
2019-02-27 23:34 ` Andrii Nakryiko
2019-02-27 21:22 ` [PATCH bpf-next 2/2] tools/bpf: selftests: add map lookup to test_map_in_map bpf prog Yonghong Song
2019-02-28 1:07 ` [PATCH bpf-next 0/2] bpf: set inner_map_meta->spin_lock_off correctly Alexei Starovoitov
2 siblings, 1 reply; 11+ messages in thread
From: Yonghong Song @ 2019-02-27 21:22 UTC (permalink / raw)
To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Yonghong Song
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>
---
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;
/* Misc members not needed in bpf_map_meta_equal() check. */
inner_map_meta->ops = inner_map->ops;
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH bpf-next 2/2] tools/bpf: selftests: add map lookup to test_map_in_map bpf prog
2019-02-27 21:22 [PATCH bpf-next 0/2] bpf: set inner_map_meta->spin_lock_off correctly Yonghong Song
2019-02-27 21:22 ` [PATCH bpf-next 1/2] " Yonghong Song
@ 2019-02-27 21:22 ` Yonghong Song
2019-02-27 23:36 ` Andrii Nakryiko
2019-02-28 1:07 ` [PATCH bpf-next 0/2] bpf: set inner_map_meta->spin_lock_off correctly Alexei Starovoitov
2 siblings, 1 reply; 11+ messages in thread
From: Yonghong Song @ 2019-02-27 21:22 UTC (permalink / raw)
To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Yonghong Song
The bpf_map_lookup_elem is added in the bpf program.
Without previous patch, the test change will trigger the
following error:
$ ./test_maps
...
; value_p = bpf_map_lookup_elem(map, &key);
20: (bf) r1 = r7
21: (bf) r2 = r8
22: (85) call bpf_map_lookup_elem#1
; if (!value_p || *value_p != 123)
23: (15) if r0 == 0x0 goto pc+16
R0=map_value(id=2,off=0,ks=4,vs=4,imm=0) R6=inv1 R7=map_ptr(id=0,off=0,ks=4,vs=4,imm=0)
R8=fp-8,call_-1 R10=fp0,call_-1 fp-8=mmmmmmmm
; if (!value_p || *value_p != 123)
24: (61) r1 = *(u32 *)(r0 +0)
R0=map_value(id=2,off=0,ks=4,vs=4,imm=0) R6=inv1 R7=map_ptr(id=0,off=0,ks=4,vs=4,imm=0)
R8=fp-8,call_-1 R10=fp0,call_-1 fp-8=mmmmmmmm
bpf_spin_lock cannot be accessed directly by load/store
With the kernel fix in the previous commit, the error goes away.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/testing/selftests/bpf/progs/test_map_in_map.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map.c b/tools/testing/selftests/bpf/progs/test_map_in_map.c
index ce923e67e08e..2985f262846e 100644
--- a/tools/testing/selftests/bpf/progs/test_map_in_map.c
+++ b/tools/testing/selftests/bpf/progs/test_map_in_map.c
@@ -27,6 +27,7 @@ SEC("xdp_mimtest")
int xdp_mimtest0(struct xdp_md *ctx)
{
int value = 123;
+ int *value_p;
int key = 0;
void *map;
@@ -35,6 +36,9 @@ int xdp_mimtest0(struct xdp_md *ctx)
return XDP_DROP;
bpf_map_update_elem(map, &key, &value, 0);
+ value_p = bpf_map_lookup_elem(map, &key);
+ if (!value_p || *value_p != 123)
+ return XDP_DROP;
map = bpf_map_lookup_elem(&mim_hash, &key);
if (!map)
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
2019-02-27 21:22 ` [PATCH bpf-next 1/2] " Yonghong Song
@ 2019-02-27 23:34 ` Andrii Nakryiko
2019-02-28 0:19 ` Yonghong Song
0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2019-02-27 23:34 UTC (permalink / raw)
To: Yonghong Song; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team
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>
> ---
> 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?
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?
>
> /* Misc members not needed in bpf_map_meta_equal() check. */
> inner_map_meta->ops = inner_map->ops;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 2/2] tools/bpf: selftests: add map lookup to test_map_in_map bpf prog
2019-02-27 21:22 ` [PATCH bpf-next 2/2] tools/bpf: selftests: add map lookup to test_map_in_map bpf prog Yonghong Song
@ 2019-02-27 23:36 ` Andrii Nakryiko
0 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2019-02-27 23:36 UTC (permalink / raw)
To: Yonghong Song; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Wed, Feb 27, 2019 at 1:24 PM Yonghong Song <yhs@fb.com> wrote:
>
> The bpf_map_lookup_elem is added in the bpf program.
> Without previous patch, the test change will trigger the
> following error:
> $ ./test_maps
> ...
> ; value_p = bpf_map_lookup_elem(map, &key);
> 20: (bf) r1 = r7
> 21: (bf) r2 = r8
> 22: (85) call bpf_map_lookup_elem#1
> ; if (!value_p || *value_p != 123)
> 23: (15) if r0 == 0x0 goto pc+16
> R0=map_value(id=2,off=0,ks=4,vs=4,imm=0) R6=inv1 R7=map_ptr(id=0,off=0,ks=4,vs=4,imm=0)
> R8=fp-8,call_-1 R10=fp0,call_-1 fp-8=mmmmmmmm
> ; if (!value_p || *value_p != 123)
> 24: (61) r1 = *(u32 *)(r0 +0)
> R0=map_value(id=2,off=0,ks=4,vs=4,imm=0) R6=inv1 R7=map_ptr(id=0,off=0,ks=4,vs=4,imm=0)
> R8=fp-8,call_-1 R10=fp0,call_-1 fp-8=mmmmmmmm
> bpf_spin_lock cannot be accessed directly by load/store
>
> With the kernel fix in the previous commit, the error goes away.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> tools/testing/selftests/bpf/progs/test_map_in_map.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map.c b/tools/testing/selftests/bpf/progs/test_map_in_map.c
> index ce923e67e08e..2985f262846e 100644
> --- a/tools/testing/selftests/bpf/progs/test_map_in_map.c
> +++ b/tools/testing/selftests/bpf/progs/test_map_in_map.c
> @@ -27,6 +27,7 @@ SEC("xdp_mimtest")
> int xdp_mimtest0(struct xdp_md *ctx)
> {
> int value = 123;
> + int *value_p;
> int key = 0;
> void *map;
>
> @@ -35,6 +36,9 @@ int xdp_mimtest0(struct xdp_md *ctx)
> return XDP_DROP;
>
> bpf_map_update_elem(map, &key, &value, 0);
> + value_p = bpf_map_lookup_elem(map, &key);
> + if (!value_p || *value_p != 123)
> + return XDP_DROP;
>
> map = bpf_map_lookup_elem(&mim_hash, &key);
> if (!map)
> --
> 2.17.1
>
LGTM.
Acked-by: Andrii Nakryiko <andriin@fb.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
2019-02-27 23:34 ` Andrii Nakryiko
@ 2019-02-28 0:19 ` Yonghong Song
2019-02-28 0:28 ` Andrii Nakryiko
0 siblings, 1 reply; 11+ messages in thread
From: Yonghong Song @ 2019-02-28 0:19 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: netdev@vger.kernel.org, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
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>
>> ---
>> 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
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
2019-02-28 0:19 ` Yonghong Song
@ 2019-02-28 0:28 ` Andrii Nakryiko
2019-02-28 0:40 ` Yonghong Song
2019-02-28 0:56 ` Alexei Starovoitov
0 siblings, 2 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2019-02-28 0:28 UTC (permalink / raw)
To: Yonghong Song
Cc: netdev@vger.kernel.org, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
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.
>
> >
> > 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);
}
Maybe Alexei can clarify?
>
> >
> >>
> >> /* Misc members not needed in bpf_map_meta_equal() check. */
> >> inner_map_meta->ops = inner_map->ops;
> >> --
> >> 2.17.1
> >>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
2019-02-28 0:28 ` Andrii Nakryiko
@ 2019-02-28 0:40 ` Yonghong Song
2019-02-28 0:43 ` Andrii Nakryiko
2019-02-28 0:56 ` Alexei Starovoitov
1 sibling, 1 reply; 11+ messages in thread
From: Yonghong Song @ 2019-02-28 0:40 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: netdev@vger.kernel.org, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
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
>>>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
2019-02-28 0:40 ` Yonghong Song
@ 2019-02-28 0:43 ` Andrii Nakryiko
0 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2019-02-28 0:43 UTC (permalink / raw)
To: Yonghong Song
Cc: netdev@vger.kernel.org, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
On Wed, Feb 27, 2019 at 4:40 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> 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.
You are right, I stand corrected. In both cases the effect should be
the same (no way to use spinlock).
>
> >
> >>
> >>>
> >>> 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 :-)
Ah, got it, yeah, maybe.
>
> >
> > Maybe Alexei can clarify?
> >
> >
> >>
> >>>
> >>>>
> >>>> /* Misc members not needed in bpf_map_meta_equal() check. */
> >>>> inner_map_meta->ops = inner_map->ops;
> >>>> --
> >>>> 2.17.1
> >>>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
2019-02-28 0:28 ` Andrii Nakryiko
2019-02-28 0:40 ` Yonghong Song
@ 2019-02-28 0:56 ` Alexei Starovoitov
1 sibling, 0 replies; 11+ messages in thread
From: Alexei Starovoitov @ 2019-02-28 0:56 UTC (permalink / raw)
To: Andrii Nakryiko, Yonghong Song
Cc: netdev@vger.kernel.org, Daniel Borkmann, Kernel Team
On 2/27/19 4:28 PM, Andrii Nakryiko wrote:
> 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);
> }
>
> Maybe Alexei can clarify?
inner map can be replaced while progs are running.
I don't think added complexity is worth it.
Hence it's disallowed for time being.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 0/2] bpf: set inner_map_meta->spin_lock_off correctly
2019-02-27 21:22 [PATCH bpf-next 0/2] bpf: set inner_map_meta->spin_lock_off correctly Yonghong Song
2019-02-27 21:22 ` [PATCH bpf-next 1/2] " Yonghong Song
2019-02-27 21:22 ` [PATCH bpf-next 2/2] tools/bpf: selftests: add map lookup to test_map_in_map bpf prog Yonghong Song
@ 2019-02-28 1:07 ` Alexei Starovoitov
2 siblings, 0 replies; 11+ messages in thread
From: Alexei Starovoitov @ 2019-02-28 1:07 UTC (permalink / raw)
To: Yonghong Song; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team
On Wed, Feb 27, 2019 at 01:22:56PM -0800, Yonghong Song wrote:
> The inner_map_meta->spin_lock_off is not set correctly during
> map creation for BPF_MAP_TYPE_ARRAY_OF_MAPS and BPF_MAP_TYPE_HASH_OF_MAPS.
> This may lead verifier error due to misinformation.
> This patch set fixed the issue with Patch #1 for the kernel change
> and Patch #2 for enhanced selftest test_maps.
Applied, Thanks
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-02-28 1:07 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-27 21:22 [PATCH bpf-next 0/2] bpf: set inner_map_meta->spin_lock_off correctly Yonghong Song
2019-02-27 21:22 ` [PATCH bpf-next 1/2] " Yonghong Song
2019-02-27 23:34 ` Andrii Nakryiko
2019-02-28 0:19 ` Yonghong Song
2019-02-28 0:28 ` Andrii Nakryiko
2019-02-28 0:40 ` Yonghong Song
2019-02-28 0:43 ` Andrii Nakryiko
2019-02-28 0:56 ` Alexei Starovoitov
2019-02-27 21:22 ` [PATCH bpf-next 2/2] tools/bpf: selftests: add map lookup to test_map_in_map bpf prog Yonghong Song
2019-02-27 23:36 ` Andrii Nakryiko
2019-02-28 1:07 ` [PATCH bpf-next 0/2] bpf: set inner_map_meta->spin_lock_off correctly Alexei Starovoitov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).