Skip to contentSkip to conversation

bpf: don't do bpf_cgroup_storage_set() for kuprobe/tp programs

PATCH In review Yonghong Song 3 participants 3 messages Mar 9, 2021

Subsystems (2)
BPF BPF core
lore
Share & tools
lore

Yonghong Song

author
#0 lore raw

PATCH v2 1/1 bpf: don't do bpf_cgroup_storage_set() for kuprobe/tp programs

For kuprobe and tracepoint bpf programs, kernel calls trace_call_bpf() which calls BPF_PROG_RUN_ARRAY_CHECK() to run the program array. Currently, BPF_PROG_RUN_ARRAY_CHECK() also calls bpf_cgroup_storage_set() to set percpu cgroup local storage with NULL value. This is
due to Commit 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the cgroup storage") which modified __BPF_PROG_RUN_ARRAY() to call bpf_cgroup_storage_set() and this macro is also used by BPF_PROG_RUN_ARRAY_CHECK().

kuprobe and tracepoint programs are not allowed to call bpf_get_local_storage() helper hence does not access percpu cgroup local storage. Let us change BPF_PROG_RUN_ARRAY_CHECK() not to modify percpu cgroup local storage.

The issue is observed when I tried to debug [1] where percpu data is overwritten due to
preempt_disable -> migration_disable
change. This patch does not completely fix the above issue, which will be addressed separately, e.g., multiple cgroup prog runs may preempt each other. But it does fix any potential issue caused by tracing program overwriting percpu cgroup storage:
- in a busy system, a tracing program is to run between bpf_cgroup_storage_set() and the cgroup prog run.
- a kprobe program is triggered by a helper in cgroup prog before bpf_get_local_storage() is called.

[1] https://lore.kernel.org/bpf/CAKH8qBuXCfUz=w8L+Fj74OaUpbosO29niYwTki7e3Ag044_aww@mail.gmail.com/T

Cc Roman Gushchin <[email protected]>
Fixes 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the cgroup storage")
Signed-off-by Yonghong Song <[email protected]>
---
 include/linux/bpf.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
include/linux/bpf.h +6 -4
index c931bc97019d..b037cb698fa6 100644
@@ -1110,7 +1110,7 @@ int bpf_prog_array_copy(struct bpf_prog_array *old_array,
 		_ret;							\
 	 })
 
-#define __BPF_PROG_RUN_ARRAY(array, ctx, func, check_non_null)	\
+#define __BPF_PROG_RUN_ARRAY(array, ctx, func, check_non_null, set_cg_storage)	\
 	({						\
 		struct bpf_prog_array_item *_item;	\
 		struct bpf_prog *_prog;			\

@@ -1123,7 +1123,9 @@ int bpf_prog_array_copy(struct bpf_prog_array *old_array,
 			goto _out;			\
 		_item = &_array->items[0];		\
 		while ((_prog = READ_ONCE(_item->prog))) {		\
-			bpf_cgroup_storage_set(_item->cgroup_storage);	\
+			if (set_cg_storage) {		\
+				bpf_cgroup_storage_set(_item->cgroup_storage);	\
+			}				\
 			_ret &= func(_prog, ctx);	\
 			_item++;			\
 		}					\

@@ -1170,10 +1172,10 @@ _out:							\
 	})
 
 #define BPF_PROG_RUN_ARRAY(array, ctx, func)		\
-	__BPF_PROG_RUN_ARRAY(array, ctx, func, false)
+	__BPF_PROG_RUN_ARRAY(array, ctx, func, false, true)
 
 #define BPF_PROG_RUN_ARRAY_CHECK(array, ctx, func)	\
-	__BPF_PROG_RUN_ARRAY(array, ctx, func, true)
+	__BPF_PROG_RUN_ARRAY(array, ctx, func, true, false)
 
 #ifdef CONFIG_BPF_SYSCALL
 DECLARE_PER_CPU(int, bpf_prog_active);
Signature · 1 line
2.24.1
Acked-by on v2 1/1
#1 lore raw
Quoted 29 lines · Yonghong Song

For kuprobe and tracepoint bpf programs, kernel calls trace_call_bpf() which calls BPF_PROG_RUN_ARRAY_CHECK() to run the program array. Currently, BPF_PROG_RUN_ARRAY_CHECK() also calls bpf_cgroup_storage_set() to set percpu cgroup local storage with NULL value. This is
due to Commit 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the cgroup storage") which modified __BPF_PROG_RUN_ARRAY() to call bpf_cgroup_storage_set() and this macro is also used by BPF_PROG_RUN_ARRAY_CHECK().

kuprobe and tracepoint programs are not allowed to call bpf_get_local_storage() helper hence does not access percpu cgroup local storage. Let us change BPF_PROG_RUN_ARRAY_CHECK() not to modify percpu cgroup local storage.

The issue is observed when I tried to debug [1] where percpu data is overwritten due to
preempt_disable -> migration_disable
change. This patch does not completely fix the above issue, which will be addressed separately, e.g., multiple cgroup prog runs may preempt each other. But it does fix any potential issue caused by tracing program overwriting percpu cgroup storage:
- in a busy system, a tracing program is to run between bpf_cgroup_storage_set() and the cgroup prog run.
- a kprobe program is triggered by a helper in cgroup prog before bpf_get_local_storage() is called.

Quoted 5 lines

[1] https://lore.kernel.org/bpf/CAKH8qBuXCfUz=w8L+Fj74OaUpbosO29niYwTki7e3Ag044_aww@mail.gmail.com/T

Cc Roman Gushchin <[email protected]>
Fixes 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the cgroup storage")
Signed-off-by Yonghong Song <[email protected]>
Acked-by Roman Gushchin <[email protected]>

Thanks, Yonghong!

#2 lore raw
In reply to Yonghong Song

include/linux/bpf.h int bpf_prog_array_copy(struct bpf_prog_array *old_array, · line 1128

Quoted 5 lines · Yonghong Song
                while ((_prog = READ_ONCE(_item->prog))) {              \
include/linux/bpf.h +3 -1
-                       bpf_cgroup_storage_set(_item->cgroup_storage);  \
+                       if (set_cg_storage) {           \
+                               bpf_cgroup_storage_set(_item->cgroup_storage);  \
+                       }                               \

Extra {} are unnecessary. I removed them while applying. Thanks!