Jiri Olsa reported a bug ([1]) in kernel where cgroup local storage pointer may be NULL in bpf_get_local_storage() helper. There are two issues uncovered by this bug: (1). kprobe or tracepoint prog incorrectly sets cgroup local storage before prog run, (2). due to change from preempt_disable to migrate_disable, preemption is possible and percpu storage might be overwritten by other tasks. This issue (1) is fixed in [2]. This patch tried to address issue (2). The following shows how things can go wrong: task 1: bpf_cgroup_storage_set() for percpu local storage preemption happens task 2: bpf_cgroup_storage_set() for percpu local storage preemption happens task 1: run bpf program task 1 will effectively use the percpu local storage setting by task 2 which will be either NULL or incorrect ones. Instead of just one common local storage per cpu, this patch fixed the issue by permitting 8 local storages per cpu and each local storage is identified by a task_struct pointer. This way, we allow at most 8 nested preemption between bpf_cgroup_storage_set() and bpf_cgroup_storage_unset(). The percpu local storage slot is released (calling bpf_cgroup_storage_unset()) by the same task after bpf program finished running. bpf_test_run() is also fixed to use the new bpf_cgroup_storage_set() interface. The patch is tested on top of [2] with reproducer in [1]. Without this patch, kernel will emit error in 2-3 minutes. With this patch, after one hour, still no error. [1] https://lore.kernel.org/bpf/CAKH8qBuXCfUz=w8L+Fj74OaUpbosO29niYwTki7e3Ag044_aww@mail.gmail.com/T [2] https://lore.kernel.org/bpf/20210309185028.3763817-1-yhs@fb.com
Showing the changes to kernel (2 of 5 files). Show all
Unified · Split
| @@ -382,8 +382,8 @@ const struct bpf_func_proto bpf_get_current_ancestor_cgroup_id_proto = { · 381 unmodified lines | |||
| 382 | }; | 382 | }; |
| 383 | 383 | ||
| 384 | #ifdef CONFIG_CGROUP_BPF | 384 | #ifdef CONFIG_CGROUP_BPF |
| 385 | DECLARE_PER_CPU(struct bpf_cgroup_storage*, | 385 | DECLARE_PER_CPU(struct bpf_cgroup_storage_info, |
| 386 | bpf_cgroup_storage[MAX_BPF_CGROUP_STORAGE_TYPE]); | 386 | bpf_cgroup_storage_info[BPF_CGROUP_STORAGE_NEST_MAX]); |
| 387 | 387 | ||
| 388 | BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags) | 388 | BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags) |
| 389 | { | 389 | { |
| @@ -392,10 +392,17 @@ BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags) · 2 unmodified lines | |||
| 392 | * verifier checks that its value is correct. | 392 | * verifier checks that its value is correct. |
| 393 | */ | 393 | */ |
| 394 | enum bpf_cgroup_storage_type stype = cgroup_storage_type(map); | 394 | enum bpf_cgroup_storage_type stype = cgroup_storage_type(map); |
| 395 | struct bpf_cgroup_storage *storage; | 395 | struct bpf_cgroup_storage *storage = NULL; |
| 396 | void *ptr; | 396 | void *ptr; |
| 397 | int i; | ||
| 397 | 398 | ||
| 398 | storage = this_cpu_read(bpf_cgroup_storage[stype]); | 399 | for (i = 0; i < BPF_CGROUP_STORAGE_NEST_MAX; i++) { |
| 400 | if (unlikely(this_cpu_read(bpf_cgroup_storage_info[i].task) != current)) | ||
| 401 | continue; | ||
| 402 | |||
| 403 | storage = this_cpu_read(bpf_cgroup_storage_info[i].storage[stype]); | ||
| 404 | break; | ||
| 405 | } | ||
| 399 | 406 | ||
| 400 | if (stype == BPF_CGROUP_STORAGE_SHARED) | 407 | if (stype == BPF_CGROUP_STORAGE_SHARED) |
| 401 | ptr = &READ_ONCE(storage->buf)->data[0]; | 408 | ptr = &READ_ONCE(storage->buf)->data[0]; |
| @@ -9,10 +9,11 @@ · 8 unmodified lines | |||
| 9 | #include <linux/slab.h> | 9 | #include <linux/slab.h> |
| 10 | #include <uapi/linux/btf.h> | 10 | #include <uapi/linux/btf.h> |
| 11 | 11 | ||
| 12 | DEFINE_PER_CPU(struct bpf_cgroup_storage*, bpf_cgroup_storage[MAX_BPF_CGROUP_STORAGE_TYPE]); | ||
| 13 | |||
| 14 | #ifdef CONFIG_CGROUP_BPF | 12 | #ifdef CONFIG_CGROUP_BPF |
| 15 | 13 | ||
| 14 | DEFINE_PER_CPU(struct bpf_cgroup_storage_info, | ||
| 15 | bpf_cgroup_storage_info[BPF_CGROUP_STORAGE_NEST_MAX]); | ||
| 16 | |||
| 16 | #include "../cgroup/cgroup-internal.h" | 17 | #include "../cgroup/cgroup-internal.h" |
| 17 | 18 | ||
| 18 | #define LOCAL_STORAGE_CREATE_FLAG_MASK \ | 19 | #define LOCAL_STORAGE_CREATE_FLAG_MASK \ |