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

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

[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

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

[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

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

[Andrii Nakryiko]

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
>

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

[Yonghong Song]

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
>>

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

[Andrii Nakryiko]

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
> >>

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

[Yonghong Song]

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
>>>>

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

[Andrii Nakryiko]

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
> >>>>

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

[Alexei Starovoitov]

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.

[PATCH bpf-next 2/2] tools/bpf: selftests: add map lookup to test_map_in_map bpf prog

[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

Re: [PATCH bpf-next 2/2] tools/bpf: selftests: add map lookup to test_map_in_map bpf prog

[Andrii Nakryiko]

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>

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

[Alexei Starovoitov]

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