Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH bpf-next] bpf: add ambient BPF runtime context stored in current
@ 2021-07-10  1:11 Andrii Nakryiko
  2021-07-10  3:43 ` kernel test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Andrii Nakryiko @ 2021-07-10  1:11 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel, linux-kernel; +Cc: andrii, kernel-team, Yonghong Song

b910eaaaa4b8 ("bpf: Fix NULL pointer dereference in bpf_get_local_storage()
helper") fixed the problem with cgroup-local storage use in BPF by
pre-allocating per-CPU array of 8 cgroup storage pointers to accommodate
possible BPF program preemptions and nested executions.

While this seems to work good in practice, it introduces new and unnecessary
failure mode in which not all BPF programs might be executed if we fail to
find an unused slot for cgroup storage, however unlikely it is. It might also
not be so unlikely when/if we allow sleepable cgroup BPF programs in the
future.

Further, the way that cgroup storage is implemented as ambiently-available
property during entire BPF program execution is a convenient way to pass extra
information to BPF program and helpers without requiring user code to pass
around extra arguments explicitly. So it would be good to have a generic
solution that can allow implementing this without arbitrary restrictions.
Ideally, such solution would work for both preemptable and sleepable BPF
programs in exactly the same way.

This patch introduces such solution, bpf_run_ctx. It adds one pointer field
(bpf_ctx) to task_struct. This field is maintained by BPF_PROG_RUN family of
macros in such a way that it always stays valid throughout BPF program
execution. BPF program preemption is handled by remembering previous
current->bpf_ctx value locally while executing nested BPF program and
restoring old value after nested BPF program finishes. This is handled by two
helper functions, bpf_set_run_ctx() and bpf_reset_run_ctx(), which are
supposed to be used before and after BPF program runs, respectively.

Restoring old value of the pointer handles preemption, while bpf_run_ctx
pointer being a property of current task_struct naturally solves this problem
for sleepable BPF programs by "following" BPF program execution as it is
scheduled in and out of CPU. It would even allow CPU migration of BPF
programs, even though it's not currently allowed by BPF infra.

This patch cleans up cgroup local storage handling as a first application. The
design itself is generic, though, with bpf_run_ctx being an empty struct that
is supposed to be embedded into a specific struct for a given BPF program type
(bpf_cg_run_ctx in this case). Follow up patches are planned that will expand
this mechanism for other uses within tracing BPF programs.

To verify that this change doesn't revert the fix to the original cgroup
storage issue, I ran the same repro as in the original report ([0]) and didn't
get any problems. Replacing bpf_reset_run_ctx(old_run_ctx) with
bpf_reset_run_ctx(NULL) triggers the issue pretty quickly (so repro does work).

  [0] https://lore.kernel.org/bpf/YEEvBUiJl2pJkxTd@krava/

Cc: Yonghong Song <yhs@fb.com>
Fixes: b910eaaaa4b8 ("bpf: Fix NULL pointer dereference in bpf_get_local_storage() helper")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/bpf-cgroup.h | 54 --------------------------------------
 include/linux/bpf.h        | 54 ++++++++++++++++++++++++--------------
 include/linux/sched.h      |  3 +++
 kernel/bpf/helpers.c       | 16 ++++-------
 kernel/bpf/local_storage.c |  3 ---
 kernel/fork.c              |  1 +
 net/bpf/test_run.c         | 23 ++++++++--------
 7 files changed, 54 insertions(+), 100 deletions(-)

diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index 8b77d08d4b47..a74cd1c3bd87 100644
--- a/include/linux/bpf-cgroup.h
+++ b/include/linux/bpf-cgroup.h
@@ -27,19 +27,6 @@ struct task_struct;
 extern struct static_key_false cgroup_bpf_enabled_key[MAX_BPF_ATTACH_TYPE];
 #define cgroup_bpf_enabled(type) static_branch_unlikely(&cgroup_bpf_enabled_key[type])
 
-#define BPF_CGROUP_STORAGE_NEST_MAX	8
-
-struct bpf_cgroup_storage_info {
-	struct task_struct *task;
-	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE];
-};
-
-/* For each cpu, permit maximum BPF_CGROUP_STORAGE_NEST_MAX number of tasks
- * to use bpf cgroup storage simultaneously.
- */
-DECLARE_PER_CPU(struct bpf_cgroup_storage_info,
-		bpf_cgroup_storage_info[BPF_CGROUP_STORAGE_NEST_MAX]);
-
 #define for_each_cgroup_storage_type(stype) \
 	for (stype = 0; stype < MAX_BPF_CGROUP_STORAGE_TYPE; stype++)
 
@@ -172,44 +159,6 @@ static inline enum bpf_cgroup_storage_type cgroup_storage_type(
 	return BPF_CGROUP_STORAGE_SHARED;
 }
 
-static inline int bpf_cgroup_storage_set(struct bpf_cgroup_storage
-					 *storage[MAX_BPF_CGROUP_STORAGE_TYPE])
-{
-	enum bpf_cgroup_storage_type stype;
-	int i, err = 0;
-
-	preempt_disable();
-	for (i = 0; i < BPF_CGROUP_STORAGE_NEST_MAX; i++) {
-		if (unlikely(this_cpu_read(bpf_cgroup_storage_info[i].task) != NULL))
-			continue;
-
-		this_cpu_write(bpf_cgroup_storage_info[i].task, current);
-		for_each_cgroup_storage_type(stype)
-			this_cpu_write(bpf_cgroup_storage_info[i].storage[stype],
-				       storage[stype]);
-		goto out;
-	}
-	err = -EBUSY;
-	WARN_ON_ONCE(1);
-
-out:
-	preempt_enable();
-	return err;
-}
-
-static inline void bpf_cgroup_storage_unset(void)
-{
-	int i;
-
-	for (i = 0; i < BPF_CGROUP_STORAGE_NEST_MAX; i++) {
-		if (unlikely(this_cpu_read(bpf_cgroup_storage_info[i].task) != current))
-			continue;
-
-		this_cpu_write(bpf_cgroup_storage_info[i].task, NULL);
-		return;
-	}
-}
-
 struct bpf_cgroup_storage *
 cgroup_storage_lookup(struct bpf_cgroup_storage_map *map,
 		      void *key, bool locked);
@@ -487,9 +436,6 @@ static inline int cgroup_bpf_prog_query(const union bpf_attr *attr,
 	return -EINVAL;
 }
 
-static inline int bpf_cgroup_storage_set(
-	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE]) { return 0; }
-static inline void bpf_cgroup_storage_unset(void) {}
 static inline int bpf_cgroup_storage_assign(struct bpf_prog_aux *aux,
 					    struct bpf_map *map) { return 0; }
 static inline struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 4afbff308ca3..8d72fdfba7bc 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1111,38 +1111,54 @@ int bpf_prog_array_copy(struct bpf_prog_array *old_array,
 			struct bpf_prog *include_prog,
 			struct bpf_prog_array **new_array);
 
+struct bpf_run_ctx {};
+
+static inline struct bpf_run_ctx *bpf_set_run_ctx(struct bpf_run_ctx *new_ctx)
+{
+	struct bpf_run_ctx *old_ctx;
+
+	old_ctx = current->bpf_ctx;
+	current->bpf_ctx = new_ctx;
+	return old_ctx;
+}
+
+static inline void bpf_reset_run_ctx(struct bpf_run_ctx *old_ctx)
+{
+	current->bpf_ctx = old_ctx;
+}
+
+struct bpf_cg_run_ctx {
+	struct bpf_run_ctx run_ctx;
+	struct bpf_prog_array_item *prog_item;
+};
+
 /* BPF program asks to bypass CAP_NET_BIND_SERVICE in bind. */
 #define BPF_RET_BIND_NO_CAP_NET_BIND_SERVICE			(1 << 0)
 /* BPF program asks to set CN on the packet. */
 #define BPF_RET_SET_CN						(1 << 0)
 
-/* For BPF_PROG_RUN_ARRAY_FLAGS and __BPF_PROG_RUN_ARRAY,
- * if bpf_cgroup_storage_set() failed, the rest of programs
- * will not execute. This should be a really rare scenario
- * as it requires BPF_CGROUP_STORAGE_NEST_MAX number of
- * preemptions all between bpf_cgroup_storage_set() and
- * bpf_cgroup_storage_unset() on the same cpu.
- */
 #define BPF_PROG_RUN_ARRAY_FLAGS(array, ctx, func, ret_flags)		\
 	({								\
 		struct bpf_prog_array_item *_item;			\
 		struct bpf_prog *_prog;					\
 		struct bpf_prog_array *_array;				\
+		struct bpf_run_ctx *old_run_ctx;			\
+		struct bpf_cg_run_ctx run_ctx;				\
 		u32 _ret = 1;						\
 		u32 func_ret;						\
 		migrate_disable();					\
 		rcu_read_lock();					\
 		_array = rcu_dereference(array);			\
 		_item = &_array->items[0];				\
+		old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);	\
 		while ((_prog = READ_ONCE(_item->prog))) {		\
-			if (unlikely(bpf_cgroup_storage_set(_item->cgroup_storage)))	\
-				break;					\
+			run_ctx.prog_item = _item;			\
 			func_ret = func(_prog, ctx);			\
 			_ret &= (func_ret & 1);				\
-			*(ret_flags) |= (func_ret >> 1);			\
-			bpf_cgroup_storage_unset();			\
+			*(ret_flags) |= (func_ret >> 1);		\
 			_item++;					\
 		}							\
+		bpf_reset_run_ctx(old_run_ctx);				\
 		rcu_read_unlock();					\
 		migrate_enable();					\
 		_ret;							\
@@ -1153,6 +1169,8 @@ int bpf_prog_array_copy(struct bpf_prog_array *old_array,
 		struct bpf_prog_array_item *_item;	\
 		struct bpf_prog *_prog;			\
 		struct bpf_prog_array *_array;		\
+		struct bpf_run_ctx *old_run_ctx;	\
+		struct bpf_cg_run_ctx run_ctx;		\
 		u32 _ret = 1;				\
 		migrate_disable();			\
 		rcu_read_lock();			\
@@ -1160,17 +1178,13 @@ int bpf_prog_array_copy(struct bpf_prog_array *old_array,
 		if (unlikely(check_non_null && !_array))\
 			goto _out;			\
 		_item = &_array->items[0];		\
-		while ((_prog = READ_ONCE(_item->prog))) {		\
-			if (!set_cg_storage) {			\
-				_ret &= func(_prog, ctx);	\
-			} else {				\
-				if (unlikely(bpf_cgroup_storage_set(_item->cgroup_storage)))	\
-					break;			\
-				_ret &= func(_prog, ctx);	\
-				bpf_cgroup_storage_unset();	\
-			}				\
+		old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);\
+		while ((_prog = READ_ONCE(_item->prog))) {	\
+			run_ctx.prog_item = _item;	\
+			_ret &= func(_prog, ctx);	\
 			_item++;			\
 		}					\
+		bpf_reset_run_ctx(old_run_ctx);		\
 _out:							\
 		rcu_read_unlock();			\
 		migrate_enable();			\
diff --git a/include/linux/sched.h b/include/linux/sched.h
index ec8d07d88641..c64119aa2e60 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -42,6 +42,7 @@ struct backing_dev_info;
 struct bio_list;
 struct blk_plug;
 struct bpf_local_storage;
+struct bpf_run_ctx;
 struct capture_control;
 struct cfs_rq;
 struct fs_struct;
@@ -1379,6 +1380,8 @@ struct task_struct {
 #ifdef CONFIG_BPF_SYSCALL
 	/* Used by BPF task local storage */
 	struct bpf_local_storage __rcu	*bpf_storage;
+	/* Used for BPF run context */
+	struct bpf_run_ctx		*bpf_ctx;
 #endif
 
 #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 62cf00383910..3d05674f4f85 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -383,8 +383,6 @@ const struct bpf_func_proto bpf_get_current_ancestor_cgroup_id_proto = {
 };
 
 #ifdef CONFIG_CGROUP_BPF
-DECLARE_PER_CPU(struct bpf_cgroup_storage_info,
-		bpf_cgroup_storage_info[BPF_CGROUP_STORAGE_NEST_MAX]);
 
 BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags)
 {
@@ -393,17 +391,13 @@ BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags)
 	 * verifier checks that its value is correct.
 	 */
 	enum bpf_cgroup_storage_type stype = cgroup_storage_type(map);
-	struct bpf_cgroup_storage *storage = NULL;
+	struct bpf_cgroup_storage *storage;
+	struct bpf_cg_run_ctx *ctx;
 	void *ptr;
-	int i;
 
-	for (i = 0; i < BPF_CGROUP_STORAGE_NEST_MAX; i++) {
-		if (unlikely(this_cpu_read(bpf_cgroup_storage_info[i].task) != current))
-			continue;
-
-		storage = this_cpu_read(bpf_cgroup_storage_info[i].storage[stype]);
-		break;
-	}
+	/* get current cgroup storage from BPF run context */
+	ctx = container_of(current->bpf_ctx, struct bpf_cg_run_ctx, run_ctx);
+	storage = ctx->prog_item->cgroup_storage[stype];
 
 	if (stype == BPF_CGROUP_STORAGE_SHARED)
 		ptr = &READ_ONCE(storage->buf)->data[0];
diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
index bd11db9774c3..1ef12f320a9b 100644
--- a/kernel/bpf/local_storage.c
+++ b/kernel/bpf/local_storage.c
@@ -11,9 +11,6 @@
 
 #ifdef CONFIG_CGROUP_BPF
 
-DEFINE_PER_CPU(struct bpf_cgroup_storage_info,
-	       bpf_cgroup_storage_info[BPF_CGROUP_STORAGE_NEST_MAX]);
-
 #include "../cgroup/cgroup-internal.h"
 
 #define LOCAL_STORAGE_CREATE_FLAG_MASK					\
diff --git a/kernel/fork.c b/kernel/fork.c
index bc94b2cc5995..e8b41e212110 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2083,6 +2083,7 @@ static __latent_entropy struct task_struct *copy_process(
 #endif
 #ifdef CONFIG_BPF_SYSCALL
 	RCU_INIT_POINTER(p->bpf_storage, NULL);
+	p->bpf_ctx = NULL;
 #endif
 
 	/* Perform scheduler related setup. Assign this task to a CPU. */
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index cda8375bbbaf..8d46e2962786 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -88,17 +88,19 @@ static bool bpf_test_timer_continue(struct bpf_test_timer *t, u32 repeat, int *e
 static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
 			u32 *retval, u32 *time, bool xdp)
 {
-	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
+	struct bpf_prog_array_item item = {.prog = prog};
+	struct bpf_run_ctx *old_ctx;
+	struct bpf_cg_run_ctx run_ctx;
 	struct bpf_test_timer t = { NO_MIGRATE };
 	enum bpf_cgroup_storage_type stype;
 	int ret;
 
 	for_each_cgroup_storage_type(stype) {
-		storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
-		if (IS_ERR(storage[stype])) {
-			storage[stype] = NULL;
+		item.cgroup_storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
+		if (IS_ERR(item.cgroup_storage[stype])) {
+			item.cgroup_storage[stype] = NULL;
 			for_each_cgroup_storage_type(stype)
-				bpf_cgroup_storage_free(storage[stype]);
+				bpf_cgroup_storage_free(item.cgroup_storage[stype]);
 			return -ENOMEM;
 		}
 	}
@@ -107,22 +109,19 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
 		repeat = 1;
 
 	bpf_test_timer_enter(&t);
+	old_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
 	do {
-		ret = bpf_cgroup_storage_set(storage);
-		if (ret)
-			break;
-
+		run_ctx.prog_item = &item;
 		if (xdp)
 			*retval = bpf_prog_run_xdp(prog, ctx);
 		else
 			*retval = BPF_PROG_RUN(prog, ctx);
-
-		bpf_cgroup_storage_unset();
 	} while (bpf_test_timer_continue(&t, repeat, &ret, time));
+	bpf_reset_run_ctx(old_ctx);
 	bpf_test_timer_leave(&t);
 
 	for_each_cgroup_storage_type(stype)
-		bpf_cgroup_storage_free(storage[stype]);
+		bpf_cgroup_storage_free(item.cgroup_storage[stype]);
 
 	return ret;
 }
-- 
2.30.2


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH bpf-next] bpf: add ambient BPF runtime context stored in current
  2021-07-10  1:11 [PATCH bpf-next] bpf: add ambient BPF runtime context stored in current Andrii Nakryiko
@ 2021-07-10  3:43 ` kernel test robot
  2021-07-10  4:34 ` kernel test robot
  2021-07-12 15:49 ` Yonghong Song
  2 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-07-10  3:43 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, netdev, ast, daniel, linux-kernel
  Cc: kbuild-all, andrii, kernel-team, Yonghong Song

[-- Attachment #1: Type: text/plain, Size: 4150 bytes --]

Hi Andrii,

I love your patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Andrii-Nakryiko/bpf-add-ambient-BPF-runtime-context-stored-in-current/20210710-091156
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: nios2-randconfig-p001-20210710 (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e66ab92194c9a54bde72690e56df1c2602b06ae4
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andrii-Nakryiko/bpf-add-ambient-BPF-runtime-context-stored-in-current/20210710-091156
        git checkout e66ab92194c9a54bde72690e56df1c2602b06ae4
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross O=build_dir ARCH=nios2 SHELL=/bin/bash drivers/iio/humidity/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/bpf-cgroup.h:5,
                    from include/linux/cgroup-defs.h:22,
                    from include/linux/cgroup.h:28,
                    from include/linux/memcontrol.h:13,
                    from include/linux/swap.h:9,
                    from include/linux/suspend.h:5,
                    from include/linux/regulator/consumer.h:35,
                    from drivers/iio/humidity/hts221.h:16,
                    from drivers/iio/humidity/hts221_core.c:19:
   include/linux/bpf.h: In function 'bpf_set_run_ctx':
>> include/linux/bpf.h:1120:19: error: 'struct task_struct' has no member named 'bpf_ctx'
    1120 |  old_ctx = current->bpf_ctx;
         |                   ^~
   include/linux/bpf.h:1121:9: error: 'struct task_struct' has no member named 'bpf_ctx'
    1121 |  current->bpf_ctx = new_ctx;
         |         ^~
   include/linux/bpf.h: In function 'bpf_reset_run_ctx':
   include/linux/bpf.h:1127:9: error: 'struct task_struct' has no member named 'bpf_ctx'
    1127 |  current->bpf_ctx = old_ctx;
         |         ^~
--
   In file included from include/linux/bpf-cgroup.h:5,
                    from include/linux/cgroup-defs.h:22,
                    from include/linux/cgroup.h:28,
                    from include/linux/memcontrol.h:13,
                    from include/linux/swap.h:9,
                    from include/linux/suspend.h:5,
                    from include/linux/regulator/consumer.h:35,
                    from drivers/iio/humidity/hts221.h:16,
                    from drivers/iio/humidity/hts221_i2c.c:17:
   include/linux/bpf.h: In function 'bpf_set_run_ctx':
>> include/linux/bpf.h:1120:19: error: 'struct task_struct' has no member named 'bpf_ctx'
    1120 |  old_ctx = current->bpf_ctx;
         |                   ^~
   include/linux/bpf.h:1121:9: error: 'struct task_struct' has no member named 'bpf_ctx'
    1121 |  current->bpf_ctx = new_ctx;
         |         ^~
   include/linux/bpf.h: In function 'bpf_reset_run_ctx':
   include/linux/bpf.h:1127:9: error: 'struct task_struct' has no member named 'bpf_ctx'
    1127 |  current->bpf_ctx = old_ctx;
         |         ^~
   At top level:
   drivers/iio/humidity/hts221_i2c.c:44:36: warning: 'hts221_acpi_match' defined but not used [-Wunused-const-variable=]
      44 | static const struct acpi_device_id hts221_acpi_match[] = {
         |                                    ^~~~~~~~~~~~~~~~~


vim +1120 include/linux/bpf.h

  1115	
  1116	static inline struct bpf_run_ctx *bpf_set_run_ctx(struct bpf_run_ctx *new_ctx)
  1117	{
  1118		struct bpf_run_ctx *old_ctx;
  1119	
> 1120		old_ctx = current->bpf_ctx;
  1121		current->bpf_ctx = new_ctx;
  1122		return old_ctx;
  1123	}
  1124	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33607 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH bpf-next] bpf: add ambient BPF runtime context stored in current
  2021-07-10  1:11 [PATCH bpf-next] bpf: add ambient BPF runtime context stored in current Andrii Nakryiko
  2021-07-10  3:43 ` kernel test robot
@ 2021-07-10  4:34 ` kernel test robot
  2021-07-12 15:49 ` Yonghong Song
  2 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-07-10  4:34 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, netdev, ast, daniel, linux-kernel
  Cc: clang-built-linux, kbuild-all, andrii, kernel-team, Yonghong Song

[-- Attachment #1: Type: text/plain, Size: 15444 bytes --]

Hi Andrii,

I love your patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Andrii-Nakryiko/bpf-add-ambient-BPF-runtime-context-stored-in-current/20210710-091156
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: riscv-buildonly-randconfig-r004-20210709 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 8d69635ed9ecf36fd0ca85906bfde17949671cbe)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/e66ab92194c9a54bde72690e56df1c2602b06ae4
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andrii-Nakryiko/bpf-add-ambient-BPF-runtime-context-stored-in-current/20210710-091156
        git checkout e66ab92194c9a54bde72690e56df1c2602b06ae4
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=riscv SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/gpu/drm/tve200/tve200_drv.c:36:
   In file included from include/linux/shmem_fs.h:6:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:28:
   In file included from include/linux/cgroup-defs.h:22:
   In file included from include/linux/bpf-cgroup.h:5:
>> include/linux/bpf.h:1120:21: error: no member named 'bpf_ctx' in 'struct task_struct'
           old_ctx = current->bpf_ctx;
                     ~~~~~~~  ^
   include/linux/bpf.h:1121:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = new_ctx;
           ~~~~~~~  ^
   include/linux/bpf.h:1127:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = old_ctx;
           ~~~~~~~  ^
   3 errors generated.
--
   In file included from drivers/input/touchscreen/mms114.c:15:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:28:
   In file included from include/linux/cgroup-defs.h:22:
   In file included from include/linux/bpf-cgroup.h:5:
>> include/linux/bpf.h:1120:21: error: no member named 'bpf_ctx' in 'struct task_struct'
           old_ctx = current->bpf_ctx;
                     ~~~~~~~  ^
   include/linux/bpf.h:1121:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = new_ctx;
           ~~~~~~~  ^
   include/linux/bpf.h:1127:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = old_ctx;
           ~~~~~~~  ^
   drivers/input/touchscreen/mms114.c:468:15: warning: cast to smaller integer type 'enum mms_type' from 'const void *' [-Wvoid-pointer-to-enum-cast]
           data->type = (enum mms_type)match_data;
                        ^~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 3 errors generated.
--
   In file included from drivers/regulator/lp872x.c:15:
   In file included from include/linux/regulator/lp872x.h:11:
   In file included from include/linux/regulator/machine.h:15:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:28:
   In file included from include/linux/cgroup-defs.h:22:
   In file included from include/linux/bpf-cgroup.h:5:
>> include/linux/bpf.h:1120:21: error: no member named 'bpf_ctx' in 'struct task_struct'
           old_ctx = current->bpf_ctx;
                     ~~~~~~~  ^
   include/linux/bpf.h:1121:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = new_ctx;
           ~~~~~~~  ^
   include/linux/bpf.h:1127:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = old_ctx;
           ~~~~~~~  ^
   drivers/regulator/lp872x.c:876:5: warning: cast to smaller integer type 'enum lp872x_regulator_id' from 'void *' [-Wvoid-pointer-to-enum-cast]
                                   (enum lp872x_regulator_id)match[i].driver_data;
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 3 errors generated.
--
   In file included from drivers/regulator/ltc3589.c:15:
   In file included from include/linux/regulator/driver.h:18:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:28:
   In file included from include/linux/cgroup-defs.h:22:
   In file included from include/linux/bpf-cgroup.h:5:
>> include/linux/bpf.h:1120:21: error: no member named 'bpf_ctx' in 'struct task_struct'
           old_ctx = current->bpf_ctx;
                     ~~~~~~~  ^
   include/linux/bpf.h:1121:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = new_ctx;
           ~~~~~~~  ^
   include/linux/bpf.h:1127:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = old_ctx;
           ~~~~~~~  ^
   drivers/regulator/ltc3589.c:395:22: warning: cast to smaller integer type 'enum ltc3589_variant' from 'const void *' [-Wvoid-pointer-to-enum-cast]
                   ltc3589->variant = (enum ltc3589_variant)
                                      ^~~~~~~~~~~~~~~~~~~~~~
   1 warning and 3 errors generated.
--
   In file included from drivers/char/random.c:335:
   In file included from include/linux/syscalls.h:87:
   In file included from include/trace/syscall.h:7:
   In file included from include/linux/trace_events.h:10:
   In file included from include/linux/perf_event.h:57:
   In file included from include/linux/cgroup.h:28:
   In file included from include/linux/cgroup-defs.h:22:
   In file included from include/linux/bpf-cgroup.h:5:
>> include/linux/bpf.h:1120:21: error: no member named 'bpf_ctx' in 'struct task_struct'
           old_ctx = current->bpf_ctx;
                     ~~~~~~~  ^
   include/linux/bpf.h:1121:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = new_ctx;
           ~~~~~~~  ^
   include/linux/bpf.h:1127:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = old_ctx;
           ~~~~~~~  ^
   drivers/char/random.c:2272:6: warning: no previous prototype for function 'add_hwgenerator_randomness' [-Wmissing-prototypes]
   void add_hwgenerator_randomness(const char *buffer, size_t count,
        ^
   drivers/char/random.c:2272:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void add_hwgenerator_randomness(const char *buffer, size_t count,
   ^
   static 
   1 warning and 3 errors generated.
--
   In file included from drivers/char/tpm/tpm-interface.c:26:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:28:
   In file included from include/linux/cgroup-defs.h:22:
   In file included from include/linux/bpf-cgroup.h:5:
>> include/linux/bpf.h:1120:21: error: no member named 'bpf_ctx' in 'struct task_struct'
           old_ctx = current->bpf_ctx;
                     ~~~~~~~  ^
   include/linux/bpf.h:1121:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = new_ctx;
           ~~~~~~~  ^
   include/linux/bpf.h:1127:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = old_ctx;
           ~~~~~~~  ^
   In file included from drivers/char/tpm/tpm-interface.c:28:
   include/linux/tpm_eventlog.h:167:6: warning: variable 'mapping_size' set but not used [-Wunused-but-set-variable]
           int mapping_size;
               ^
   1 warning and 3 errors generated.
--
   In file included from drivers/iio/dac/mcp4725.c:18:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:28:
   In file included from include/linux/cgroup-defs.h:22:
   In file included from include/linux/bpf-cgroup.h:5:
>> include/linux/bpf.h:1120:21: error: no member named 'bpf_ctx' in 'struct task_struct'
           old_ctx = current->bpf_ctx;
                     ~~~~~~~  ^
   include/linux/bpf.h:1121:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = new_ctx;
           ~~~~~~~  ^
   include/linux/bpf.h:1127:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = old_ctx;
           ~~~~~~~  ^
   drivers/iio/dac/mcp4725.c:389:14: warning: cast to smaller integer type 'enum chip_id' from 'const void *' [-Wvoid-pointer-to-enum-cast]
                   data->id = (enum chip_id)device_get_match_data(&client->dev);
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 3 errors generated.
--
   In file included from drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c:14:
   In file included from drivers/iio/imu/inv_icm42600/inv_icm42600.h:13:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:28:
   In file included from include/linux/cgroup-defs.h:22:
   In file included from include/linux/bpf-cgroup.h:5:
>> include/linux/bpf.h:1120:21: error: no member named 'bpf_ctx' in 'struct task_struct'
           old_ctx = current->bpf_ctx;
                     ~~~~~~~  ^
   include/linux/bpf.h:1121:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = new_ctx;
           ~~~~~~~  ^
   include/linux/bpf.h:1127:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = old_ctx;
           ~~~~~~~  ^
   drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c:61:9: warning: cast to smaller integer type 'enum inv_icm42600_chip' from 'const void *' [-Wvoid-pointer-to-enum-cast]
           chip = (enum inv_icm42600_chip)match;
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 3 errors generated.
--
   In file included from drivers/iio/magnetometer/ak8975.c:21:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:28:
   In file included from include/linux/cgroup-defs.h:22:
   In file included from include/linux/bpf-cgroup.h:5:
>> include/linux/bpf.h:1120:21: error: no member named 'bpf_ctx' in 'struct task_struct'
           old_ctx = current->bpf_ctx;
                     ~~~~~~~  ^
   include/linux/bpf.h:1121:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = new_ctx;
           ~~~~~~~  ^
   include/linux/bpf.h:1127:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = old_ctx;
           ~~~~~~~  ^
   drivers/iio/magnetometer/ak8975.c:900:13: warning: cast to smaller integer type 'enum asahi_compass_chipset' from 'const void *' [-Wvoid-pointer-to-enum-cast]
                   chipset = (enum asahi_compass_chipset)(match);
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 3 errors generated.
--
   In file included from drivers/mfd/lp87565.c:14:
   In file included from include/linux/mfd/lp87565.h:12:
   In file included from include/linux/regulator/driver.h:18:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:28:
   In file included from include/linux/cgroup-defs.h:22:
   In file included from include/linux/bpf-cgroup.h:5:
>> include/linux/bpf.h:1120:21: error: no member named 'bpf_ctx' in 'struct task_struct'
           old_ctx = current->bpf_ctx;
                     ~~~~~~~  ^
   include/linux/bpf.h:1121:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = new_ctx;
           ~~~~~~~  ^
   include/linux/bpf.h:1127:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = old_ctx;
           ~~~~~~~  ^
   drivers/mfd/lp87565.c:77:23: warning: cast to smaller integer type 'enum lp87565_device_type' from 'const void *' [-Wvoid-pointer-to-enum-cast]
                   lp87565->dev_type = (enum lp87565_device_type)of_id->data;
                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 3 errors generated.
--
   In file included from drivers/mfd/wm8994-core.c:21:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:28:
   In file included from include/linux/cgroup-defs.h:22:
   In file included from include/linux/bpf-cgroup.h:5:
>> include/linux/bpf.h:1120:21: error: no member named 'bpf_ctx' in 'struct task_struct'
           old_ctx = current->bpf_ctx;
                     ~~~~~~~  ^
   include/linux/bpf.h:1121:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = new_ctx;
           ~~~~~~~  ^
   include/linux/bpf.h:1127:11: error: no member named 'bpf_ctx' in 'struct task_struct'
           current->bpf_ctx = old_ctx;
           ~~~~~~~  ^
   drivers/mfd/wm8994-core.c:644:19: warning: cast to smaller integer type 'enum wm8994_type' from 'const void *' [-Wvoid-pointer-to-enum-cast]
                           wm8994->type = (enum wm8994_type)of_id->data;
                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 3 errors generated.
..


vim +1120 include/linux/bpf.h

  1115	
  1116	static inline struct bpf_run_ctx *bpf_set_run_ctx(struct bpf_run_ctx *new_ctx)
  1117	{
  1118		struct bpf_run_ctx *old_ctx;
  1119	
> 1120		old_ctx = current->bpf_ctx;
  1121		current->bpf_ctx = new_ctx;
  1122		return old_ctx;
  1123	}
  1124	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40864 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH bpf-next] bpf: add ambient BPF runtime context stored in current
  2021-07-10  1:11 [PATCH bpf-next] bpf: add ambient BPF runtime context stored in current Andrii Nakryiko
  2021-07-10  3:43 ` kernel test robot
  2021-07-10  4:34 ` kernel test robot
@ 2021-07-12 15:49 ` Yonghong Song
  2 siblings, 0 replies; 4+ messages in thread
From: Yonghong Song @ 2021-07-12 15:49 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, netdev, ast, daniel, linux-kernel; +Cc: kernel-team



On 7/9/21 6:11 PM, Andrii Nakryiko wrote:
> b910eaaaa4b8 ("bpf: Fix NULL pointer dereference in bpf_get_local_storage()
> helper") fixed the problem with cgroup-local storage use in BPF by
> pre-allocating per-CPU array of 8 cgroup storage pointers to accommodate
> possible BPF program preemptions and nested executions.
> 
> While this seems to work good in practice, it introduces new and unnecessary
> failure mode in which not all BPF programs might be executed if we fail to
> find an unused slot for cgroup storage, however unlikely it is. It might also
> not be so unlikely when/if we allow sleepable cgroup BPF programs in the
> future.
> 
> Further, the way that cgroup storage is implemented as ambiently-available
> property during entire BPF program execution is a convenient way to pass extra
> information to BPF program and helpers without requiring user code to pass
> around extra arguments explicitly. So it would be good to have a generic
> solution that can allow implementing this without arbitrary restrictions.
> Ideally, such solution would work for both preemptable and sleepable BPF
> programs in exactly the same way.
> 
> This patch introduces such solution, bpf_run_ctx. It adds one pointer field
> (bpf_ctx) to task_struct. This field is maintained by BPF_PROG_RUN family of
> macros in such a way that it always stays valid throughout BPF program
> execution. BPF program preemption is handled by remembering previous
> current->bpf_ctx value locally while executing nested BPF program and
> restoring old value after nested BPF program finishes. This is handled by two
> helper functions, bpf_set_run_ctx() and bpf_reset_run_ctx(), which are
> supposed to be used before and after BPF program runs, respectively.
> 
> Restoring old value of the pointer handles preemption, while bpf_run_ctx
> pointer being a property of current task_struct naturally solves this problem
> for sleepable BPF programs by "following" BPF program execution as it is
> scheduled in and out of CPU. It would even allow CPU migration of BPF
> programs, even though it's not currently allowed by BPF infra.
> 
> This patch cleans up cgroup local storage handling as a first application. The
> design itself is generic, though, with bpf_run_ctx being an empty struct that
> is supposed to be embedded into a specific struct for a given BPF program type
> (bpf_cg_run_ctx in this case). Follow up patches are planned that will expand
> this mechanism for other uses within tracing BPF programs.
> 
> To verify that this change doesn't revert the fix to the original cgroup
> storage issue, I ran the same repro as in the original report ([0]) and didn't
> get any problems. Replacing bpf_reset_run_ctx(old_run_ctx) with
> bpf_reset_run_ctx(NULL) triggers the issue pretty quickly (so repro does work).
> 
>    [0] https://lore.kernel.org/bpf/YEEvBUiJl2pJkxTd@krava/
> 
> Cc: Yonghong Song <yhs@fb.com>
> Fixes: b910eaaaa4b8 ("bpf: Fix NULL pointer dereference in bpf_get_local_storage() helper")
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Except a config related issue reported by kernel test robot, the
patch looks good to me.

Acked-by: Yonghong Song <yhs@fb.com>

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-07-12 15:49 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-10  1:11 [PATCH bpf-next] bpf: add ambient BPF runtime context stored in current Andrii Nakryiko
2021-07-10  3:43 ` kernel test robot
2021-07-10  4:34 ` kernel test robot
2021-07-12 15:49 ` Yonghong Song

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