| Message ID | 20221114134720.1057939-2-xukuohai@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Bug fix and test case for special map value field | expand |
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.
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.
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.
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.
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; }
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(-)