* [PATCH bpf 0/2] Bug fix and test case for special map value field
@ 2022-11-14 13:47 Xu Kuohai
2022-11-14 13:47 ` [PATCH bpf 1/2] bpf: Do not copy spin lock field from user in bpf_selem_alloc Xu Kuohai
2022-11-14 13:47 ` [PATCH bpf 2/2] bpf: Set and check spin lock value in sk_storage_map_test Xu Kuohai
0 siblings, 2 replies; 7+ messages in thread
From: Xu Kuohai @ 2022-11-14 13:47 UTC (permalink / raw)
To: bpf, linux-kernel, linux-kselftest
Cc: Martin KaFai Lau, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Kumar Kartikeya Dwivedi, Mykola Lysenko, Shuah Khan
This series is a follow-up to [0]. patch 1 updates sk_storage_map_test to
ensure special map value fields are not copied between user and kernel.
patch 0 fixes a bug found by the updated test.
[0] https://lore.kernel.org/bpf/1ca2e4e8-ed7e-9174-01f6-c14539b8b8b2@huawei.com/
Xu Kuohai (2):
bpf: Do not copy spin lock field from user in bpf_selem_alloc
bpf: Set and check spin lock value in sk_storage_map_test
kernel/bpf/bpf_local_storage.c | 2 +-
.../selftests/bpf/map_tests/sk_storage_map.c | 36 ++++++++++---------
2 files changed, 21 insertions(+), 17 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf 1/2] bpf: Do not copy spin lock field from user in bpf_selem_alloc
2022-11-14 13:47 [PATCH bpf 0/2] Bug fix and test case for special map value field Xu Kuohai
@ 2022-11-14 13:47 ` Xu Kuohai
2022-11-16 5:27 ` Alexei Starovoitov
2022-11-14 13:47 ` [PATCH bpf 2/2] bpf: Set and check spin lock value in sk_storage_map_test Xu Kuohai
1 sibling, 1 reply; 7+ messages in thread
From: Xu Kuohai @ 2022-11-14 13:47 UTC (permalink / raw)
To: bpf, linux-kernel, linux-kselftest
Cc: Martin KaFai Lau, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Kumar Kartikeya Dwivedi, Mykola Lysenko, Shuah Khan
bpf_selem_alloc function is used by inode_storage, sk_storage and
task_storage maps to set map value, for these map types, there may
be a spin lock in the map value, so if we use memcpy to copy the whole
map value from user, the spin lock field may be initialized incorrectly.
Since the spin lock field is zeroed by kzalloc, call copy_map_value
instead of memcpy to skip copying the spin lock field to fix it.
Fixes: 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
kernel/bpf/bpf_local_storage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
index 802fc15b0d73..f27fa5ba7d72 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -74,7 +74,7 @@ bpf_selem_alloc(struct bpf_local_storage_map *smap, void *owner,
gfp_flags | __GFP_NOWARN);
if (selem) {
if (value)
- memcpy(SDATA(selem)->data, value, smap->map.value_size);
+ copy_map_value(&smap->map, SDATA(selem)->data, value);
return selem;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf 2/2] bpf: Set and check spin lock value in sk_storage_map_test
2022-11-14 13:47 [PATCH bpf 0/2] Bug fix and test case for special map value field Xu Kuohai
2022-11-14 13:47 ` [PATCH bpf 1/2] bpf: Do not copy spin lock field from user in bpf_selem_alloc Xu Kuohai
@ 2022-11-14 13:47 ` Xu Kuohai
1 sibling, 0 replies; 7+ messages in thread
From: Xu Kuohai @ 2022-11-14 13:47 UTC (permalink / raw)
To: bpf, linux-kernel, linux-kselftest
Cc: Martin KaFai Lau, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Kumar Kartikeya Dwivedi, Mykola Lysenko, Shuah Khan
Update sk_storage_map_test to make sure kernel does not copy user
non-zero value spin lock to kernel, and does not copy kernel spin
lock value to user.
If user spin lock value is copied to kernel, this test case will
make kernel spin on the copied lock, resulting in rcu stall and
softlockup.
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
.../selftests/bpf/map_tests/sk_storage_map.c | 36 ++++++++++---------
1 file changed, 20 insertions(+), 16 deletions(-)
diff --git a/tools/testing/selftests/bpf/map_tests/sk_storage_map.c b/tools/testing/selftests/bpf/map_tests/sk_storage_map.c
index 099eb4dfd4f7..18405c3b7cee 100644
--- a/tools/testing/selftests/bpf/map_tests/sk_storage_map.c
+++ b/tools/testing/selftests/bpf/map_tests/sk_storage_map.c
@@ -458,7 +458,7 @@ static void test_sk_storage_map_basic(void)
struct {
int cnt;
int lock;
- } value = { .cnt = 0xeB9f, .lock = 0, }, lookup_value;
+ } value = { .cnt = 0xeB9f, .lock = 1, }, lookup_value;
struct bpf_map_create_opts bad_xattr;
int btf_fd, map_fd, sk_fd, err;
@@ -483,38 +483,41 @@ static void test_sk_storage_map_basic(void)
"err:%d errno:%d\n", err, errno);
err = bpf_map_lookup_elem_flags(map_fd, &sk_fd, &lookup_value,
BPF_F_LOCK);
- CHECK(err || lookup_value.cnt != value.cnt,
+ CHECK(err || lookup_value.lock || lookup_value.cnt != value.cnt,
"bpf_map_lookup_elem_flags(BPF_F_LOCK)",
- "err:%d errno:%d cnt:%x(%x)\n",
- err, errno, lookup_value.cnt, value.cnt);
+ "err:%d errno:%d lock:%x cnt:%x(%x)\n",
+ err, errno, lookup_value.lock, lookup_value.cnt, value.cnt);
/* Bump the cnt and update with BPF_EXIST | BPF_F_LOCK */
value.cnt += 1;
+ value.lock = 2;
err = bpf_map_update_elem(map_fd, &sk_fd, &value,
BPF_EXIST | BPF_F_LOCK);
CHECK(err, "bpf_map_update_elem(BPF_EXIST|BPF_F_LOCK)",
"err:%d errno:%d\n", err, errno);
err = bpf_map_lookup_elem_flags(map_fd, &sk_fd, &lookup_value,
BPF_F_LOCK);
- CHECK(err || lookup_value.cnt != value.cnt,
+ CHECK(err || lookup_value.lock || lookup_value.cnt != value.cnt,
"bpf_map_lookup_elem_flags(BPF_F_LOCK)",
- "err:%d errno:%d cnt:%x(%x)\n",
- err, errno, lookup_value.cnt, value.cnt);
+ "err:%d errno:%d lock:%x cnt:%x(%x)\n",
+ err, errno, lookup_value.lock, lookup_value.cnt, value.cnt);
/* Bump the cnt and update with BPF_EXIST */
value.cnt += 1;
+ value.lock = 2;
err = bpf_map_update_elem(map_fd, &sk_fd, &value, BPF_EXIST);
CHECK(err, "bpf_map_update_elem(BPF_EXIST)",
"err:%d errno:%d\n", err, errno);
err = bpf_map_lookup_elem_flags(map_fd, &sk_fd, &lookup_value,
BPF_F_LOCK);
- CHECK(err || lookup_value.cnt != value.cnt,
+ CHECK(err || lookup_value.lock || lookup_value.cnt != value.cnt,
"bpf_map_lookup_elem_flags(BPF_F_LOCK)",
- "err:%d errno:%d cnt:%x(%x)\n",
- err, errno, lookup_value.cnt, value.cnt);
+ "err:%d errno:%d lock:%x cnt:%x(%x)\n",
+ err, errno, lookup_value.lock, lookup_value.cnt, value.cnt);
/* Update with BPF_NOEXIST */
value.cnt += 1;
+ value.lock = 2;
err = bpf_map_update_elem(map_fd, &sk_fd, &value,
BPF_NOEXIST | BPF_F_LOCK);
CHECK(!err || errno != EEXIST,
@@ -526,22 +529,23 @@ static void test_sk_storage_map_basic(void)
value.cnt -= 1;
err = bpf_map_lookup_elem_flags(map_fd, &sk_fd, &lookup_value,
BPF_F_LOCK);
- CHECK(err || lookup_value.cnt != value.cnt,
+ CHECK(err || lookup_value.lock || lookup_value.cnt != value.cnt,
"bpf_map_lookup_elem_flags(BPF_F_LOCK)",
- "err:%d errno:%d cnt:%x(%x)\n",
- err, errno, lookup_value.cnt, value.cnt);
+ "err:%d errno:%d lock:%x cnt:%x(%x)\n",
+ err, errno, lookup_value.lock, lookup_value.cnt, value.cnt);
/* Bump the cnt again and update with map_flags == 0 */
value.cnt += 1;
+ value.lock = 2;
err = bpf_map_update_elem(map_fd, &sk_fd, &value, 0);
CHECK(err, "bpf_map_update_elem()", "err:%d errno:%d\n",
err, errno);
err = bpf_map_lookup_elem_flags(map_fd, &sk_fd, &lookup_value,
BPF_F_LOCK);
- CHECK(err || lookup_value.cnt != value.cnt,
+ CHECK(err || lookup_value.lock || lookup_value.cnt != value.cnt,
"bpf_map_lookup_elem_flags(BPF_F_LOCK)",
- "err:%d errno:%d cnt:%x(%x)\n",
- err, errno, lookup_value.cnt, value.cnt);
+ "err:%d errno:%d lock:%x cnt:%x(%x)\n",
+ err, errno, lookup_value.lock, lookup_value.cnt, value.cnt);
/* Test delete elem */
err = bpf_map_delete_elem(map_fd, &sk_fd);
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf 1/2] bpf: Do not copy spin lock field from user in bpf_selem_alloc
2022-11-14 13:47 ` [PATCH bpf 1/2] bpf: Do not copy spin lock field from user in bpf_selem_alloc Xu Kuohai
@ 2022-11-16 5:27 ` Alexei Starovoitov
2022-11-16 8:07 ` Xu Kuohai
0 siblings, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2022-11-16 5:27 UTC (permalink / raw)
To: Xu Kuohai
Cc: bpf, LKML, open list:KERNEL SELFTEST FRAMEWORK, Martin KaFai Lau,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Kumar Kartikeya Dwivedi, Mykola Lysenko,
Shuah Khan
On Mon, Nov 14, 2022 at 5:31 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>
> bpf_selem_alloc function is used by inode_storage, sk_storage and
> task_storage maps to set map value, for these map types, there may
> be a spin lock in the map value, so if we use memcpy to copy the whole
> map value from user, the spin lock field may be initialized incorrectly.
>
> Since the spin lock field is zeroed by kzalloc, call copy_map_value
> instead of memcpy to skip copying the spin lock field to fix it.
>
> Fixes: 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage")
The tag is wrong. When local storage was introduced it was not
possible to use spin_locks there.
Pls resubmit.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf 1/2] bpf: Do not copy spin lock field from user in bpf_selem_alloc
2022-11-16 5:27 ` Alexei Starovoitov
@ 2022-11-16 8:07 ` Xu Kuohai
2022-11-21 11:30 ` Xu Kuohai
0 siblings, 1 reply; 7+ messages in thread
From: Xu Kuohai @ 2022-11-16 8:07 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: bpf, LKML, open list:KERNEL SELFTEST FRAMEWORK, Martin KaFai Lau,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Kumar Kartikeya Dwivedi, Mykola Lysenko,
Shuah Khan
On 11/16/2022 1:27 PM, Alexei Starovoitov wrote:
> On Mon, Nov 14, 2022 at 5:31 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>>
>> bpf_selem_alloc function is used by inode_storage, sk_storage and
>> task_storage maps to set map value, for these map types, there may
>> be a spin lock in the map value, so if we use memcpy to copy the whole
>> map value from user, the spin lock field may be initialized incorrectly.
>>
>> Since the spin lock field is zeroed by kzalloc, call copy_map_value
>> instead of memcpy to skip copying the spin lock field to fix it.
>>
>> Fixes: 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage")
>
> The tag is wrong. When local storage was introduced it was not
> possible to use spin_locks there.
> Pls resubmit.
> .
No, spin_lock was introduced by d83525ca62cf ("bpf: introduce bpf_spin_lock"),
before 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage").
To confirm this, I built a kernel image on comit 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage")
and run test case posted in patch 2, a softlockup was triggered. Then I picked
this patch and tried again, nothing failed.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf 1/2] bpf: Do not copy spin lock field from user in bpf_selem_alloc
2022-11-16 8:07 ` Xu Kuohai
@ 2022-11-21 11:30 ` Xu Kuohai
2022-11-21 19:50 ` Alexei Starovoitov
0 siblings, 1 reply; 7+ messages in thread
From: Xu Kuohai @ 2022-11-21 11:30 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: bpf, LKML, open list:KERNEL SELFTEST FRAMEWORK, Martin KaFai Lau,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Kumar Kartikeya Dwivedi, Mykola Lysenko,
Shuah Khan
On 11/16/2022 4:07 PM, Xu Kuohai wrote:
> On 11/16/2022 1:27 PM, Alexei Starovoitov wrote:
>> On Mon, Nov 14, 2022 at 5:31 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>>>
>>> bpf_selem_alloc function is used by inode_storage, sk_storage and
>>> task_storage maps to set map value, for these map types, there may
>>> be a spin lock in the map value, so if we use memcpy to copy the whole
>>> map value from user, the spin lock field may be initialized incorrectly.
>>>
>>> Since the spin lock field is zeroed by kzalloc, call copy_map_value
>>> instead of memcpy to skip copying the spin lock field to fix it.
>>>
>>> Fixes: 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage")
>>
>> The tag is wrong. When local storage was introduced it was not
>> possible to use spin_locks there.
>> Pls resubmit.
>> .
>
> No, spin_lock was introduced by d83525ca62cf ("bpf: introduce bpf_spin_lock"),
> before 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage").
>
> To confirm this, I built a kernel image on comit 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage")
> and run test case posted in patch 2, a softlockup was triggered. Then I picked
> this patch and tried again, nothing failed.
Hello, am I right? Or could you please give the correct fix-tag? Thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf 1/2] bpf: Do not copy spin lock field from user in bpf_selem_alloc
2022-11-21 11:30 ` Xu Kuohai
@ 2022-11-21 19:50 ` Alexei Starovoitov
0 siblings, 0 replies; 7+ messages in thread
From: Alexei Starovoitov @ 2022-11-21 19:50 UTC (permalink / raw)
To: Xu Kuohai
Cc: bpf, LKML, open list:KERNEL SELFTEST FRAMEWORK, Martin KaFai Lau,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Kumar Kartikeya Dwivedi, Mykola Lysenko,
Shuah Khan
On Mon, Nov 21, 2022 at 3:30 AM Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>
> On 11/16/2022 4:07 PM, Xu Kuohai wrote:
> > On 11/16/2022 1:27 PM, Alexei Starovoitov wrote:
> >> On Mon, Nov 14, 2022 at 5:31 AM Xu Kuohai <xukuohai@huawei.com> wrote:
> >>>
> >>> bpf_selem_alloc function is used by inode_storage, sk_storage and
> >>> task_storage maps to set map value, for these map types, there may
> >>> be a spin lock in the map value, so if we use memcpy to copy the whole
> >>> map value from user, the spin lock field may be initialized incorrectly.
> >>>
> >>> Since the spin lock field is zeroed by kzalloc, call copy_map_value
> >>> instead of memcpy to skip copying the spin lock field to fix it.
> >>>
> >>> Fixes: 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage")
> >>
> >> The tag is wrong. When local storage was introduced it was not
> >> possible to use spin_locks there.
> >> Pls resubmit.
> >> .
> >
> > No, spin_lock was introduced by d83525ca62cf ("bpf: introduce bpf_spin_lock"),
> > before 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage").
> >
> > To confirm this, I built a kernel image on comit 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage")
> > and run test case posted in patch 2, a softlockup was triggered. Then I picked
> > this patch and tried again, nothing failed.
>
> Hello, am I right? Or could you please give the correct fix-tag? Thanks.
I see. I was under the impression that bpf_spin_lock was enabled
in the local storage later.
Ok. Applied as-is.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-21 19:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-14 13:47 [PATCH bpf 0/2] Bug fix and test case for special map value field Xu Kuohai
2022-11-14 13:47 ` [PATCH bpf 1/2] bpf: Do not copy spin lock field from user in bpf_selem_alloc Xu Kuohai
2022-11-16 5:27 ` Alexei Starovoitov
2022-11-16 8:07 ` Xu Kuohai
2022-11-21 11:30 ` Xu Kuohai
2022-11-21 19:50 ` Alexei Starovoitov
2022-11-14 13:47 ` [PATCH bpf 2/2] bpf: Set and check spin lock value in sk_storage_map_test Xu Kuohai
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).