bpf: fix NULL pointer dereference in

PATCH Superseded Yonghong Song 2 participants 6 messages Mar 19, 2021

Subsystems (3)

Networking BPF BPF core

lore

Share & tools

lore

Yonghong Song

author

2021-03-19 00:19 UTC #0 lore raw

cover bpf: fix NULL pointer dereference in

Jiri Olsa reported a bug in
https://lore.kernel.org/bpf/CAKH8qBuXCfUz=w8L+Fj74OaUpbosO29niYwTki7e3Ag044_aww@mail.gmail.com/T
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.
Issue (1) has been fixed and this patch set fixed issue (2). Please see details of Patch #1 for detailed fix.
Patch #2 fixed bpf_prog_test_run to use new signature for bpf_cgroup_storage_set().

Patch #1 can be backported to bpf tree and Patch #2 cannot due to refactoring done in bpf-next only.
I did not put a Fix tag as I am not able to find a proper one. The original commit which changed from preempt_disable to migrate_disable in bpf.h is just a wrapper where migrate_disable still calls preempt_disable. The real migrate_disable change happens later in kernel/sched/*.

Yonghong Song (2):
bpf: fix NULL pointer dereference in bpf_get_local_storage() helper
bpf: fix bpf_cgroup_storage_set() usage in test_run

 include/linux/bpf-cgroup.h | 53 +++++++++++++++++++++++++++++++++-----
 include/linux/bpf.h        | 22 +++++++++++++---
 kernel/bpf/helpers.c       | 15 ++++++++---
 kernel/bpf/local_storage.c |  3 ++-
 net/bpf/test_run.c         |  6 ++++-
 5 files changed, 83 insertions(+), 16 deletions(-)

Signature · 1 line

2.30.2

Yonghong Song

author

2021-03-19 00:19 UTC #1 lore raw

PATCH 1/2 bpf: fix NULL pointer dereference in bpf_get_local_storage() helper

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.

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/CAKH8qBuXCfUz=w8L+Fj74OaUpbosO29niYwTki7e3Ag044_aww@mail.gmail.com/T

Cc Jiri Olsa <[email protected]>

Cc Roman Gushchin <[email protected]>

Signed-off-by Yonghong Song <[email protected]>

---
 include/linux/bpf-cgroup.h | 53 +++++++++++++++++++++++++++++++++-----
 include/linux/bpf.h        | 22 +++++++++++++---
 kernel/bpf/helpers.c       | 15 ++++++++---
 kernel/bpf/local_storage.c |  3 ++-
 4 files changed, 78 insertions(+), 15 deletions(-)

include/linux/bpf-cgroup.h +47 -6

index c42e02b4d84b..96fe26509489 100644
@@ -20,14 +20,25 @@ struct bpf_sock_ops_kern;
 struct bpf_cgroup_storage;
 struct ctl_table;
 struct ctl_table_header;
+struct task_struct;
 
 #ifdef CONFIG_CGROUP_BPF
 
 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])
 
-DECLARE_PER_CPU(struct bpf_cgroup_storage*,
-		bpf_cgroup_storage[MAX_BPF_CGROUP_STORAGE_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++)

@@ -161,13 +172,43 @@ static inline enum bpf_cgroup_storage_type cgroup_storage_type(
 	return BPF_CGROUP_STORAGE_SHARED;
 }
 
-static inline void bpf_cgroup_storage_set(struct bpf_cgroup_storage
-					  *storage[MAX_BPF_CGROUP_STORAGE_TYPE])
+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;
+
+	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]);
+		break;
+	}
+	preempt_enable();
+
+	if (i == BPF_CGROUP_STORAGE_NEST_MAX) {
+		WARN_ON_ONCE(1);
+		return -EBUSY;
+	}
+	return 0;
+}
+
+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;
 
-	for_each_cgroup_storage_type(stype)
-		this_cpu_write(bpf_cgroup_storage[stype], storage[stype]);
+		this_cpu_write(bpf_cgroup_storage_info[i].task, NULL);
+		return;
+	}
 }
 
 struct bpf_cgroup_storage *

include/linux/bpf.h +18 -4

index a47285cd39c2..3a6ae69743ff 100644
@@ -1090,6 +1090,13 @@ int bpf_prog_array_copy(struct bpf_prog_array *old_array,
 /* 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;			\

@@ -1102,10 +1109,12 @@ int bpf_prog_array_copy(struct bpf_prog_array *old_array,
 		_array = rcu_dereference(array);			\
 		_item = &_array->items[0];				\
 		while ((_prog = READ_ONCE(_item->prog))) {		\
-			bpf_cgroup_storage_set(_item->cgroup_storage);	\
+			if (unlikely(bpf_cgroup_storage_set(_item->cgroup_storage)))	\
+				break;					\
 			func_ret = func(_prog, ctx);			\
 			_ret &= (func_ret & 1);				\
 			*(ret_flags) |= (func_ret >> 1);			\
+			bpf_cgroup_storage_unset();			\
 			_item++;					\
 		}							\
 		rcu_read_unlock();					\

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

kernel/bpf/helpers.c +11 -4

index 074800226327..f306611c4ddf 100644
@@ -382,8 +382,8 @@ const struct bpf_func_proto bpf_get_current_ancestor_cgroup_id_proto = {
 };
 
 #ifdef CONFIG_CGROUP_BPF
-DECLARE_PER_CPU(struct bpf_cgroup_storage*,
-		bpf_cgroup_storage[MAX_BPF_CGROUP_STORAGE_TYPE]);
+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)
 {

@@ -392,10 +392,17 @@ 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;
+	struct bpf_cgroup_storage *storage = NULL;
 	void *ptr;
+	int i;
 
-	storage = this_cpu_read(bpf_cgroup_storage[stype]);
+	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;
+	}
 
 	if (stype == BPF_CGROUP_STORAGE_SHARED)
 		ptr = &READ_ONCE(storage->buf)->data[0];

kernel/bpf/local_storage.c +2 -1

index 2d4f9ac12377..174350524577 100644
@@ -9,7 +9,8 @@
 #include <linux/slab.h>
 #include <uapi/linux/btf.h>
 
-DEFINE_PER_CPU(struct bpf_cgroup_storage*, bpf_cgroup_storage[MAX_BPF_CGROUP_STORAGE_TYPE]);
+DEFINE_PER_CPU(struct bpf_cgroup_storage_info,
+	       bpf_cgroup_storage_info[BPF_CGROUP_STORAGE_NEST_MAX]);
 
 #ifdef CONFIG_CGROUP_BPF

Signature · 1 line

2.30.2

kernel test robot

build bot

2021-03-19 01:37 UTC #2 lore raw

Re: [PATCH bpf-next 1/2] bpf: fix NULL pointer dereference in bpf_get_local_storage() helper

I love your patch! Yet something to improve:

Hi Yonghong,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20210318]
[cannot apply to bpf-next/master bpf/master v5.12-rc3 v5.12-rc2 v5.12-rc1 v5.12-rc3] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Yonghong-Song/bpf-fix-NULL-pointer-dereference-in/20210319-082140
base:    ba5b053ab3ac674b91a6669086139819359a5e6e
config: powerpc-randconfig-r034-20210318 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project fcc1ce00931751ac02498986feb37744e9ace8de)

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 powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/38b56e03d083be940fe9dc231c5f6c8f4f282f15
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Yonghong-Song/bpf-fix-NULL-pointer-dereference-in/20210319-082140 git checkout 38b56e03d083be940fe9dc231c5f6c8f4f282f15
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'

                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:210:1: note: expanded from here
   __do_insb
   ^
   arch/powerpc/include/asm/io.h:556:56: note: expanded from macro '__do_insb'
   #define __do_insb(p, b, n)      readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from kernel/bpf/local_storage.c:6:
   In file included from include/linux/filter.h:13:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:212:1: note: expanded from here
   __do_insw
   ^
   arch/powerpc/include/asm/io.h:557:56: note: expanded from macro '__do_insw'
   #define __do_insw(p, b, n)      readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from kernel/bpf/local_storage.c:6:
   In file included from include/linux/filter.h:13:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:214:1: note: expanded from here
   __do_insl
   ^
   arch/powerpc/include/asm/io.h:558:56: note: expanded from macro '__do_insl'
   #define __do_insl(p, b, n)      readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from kernel/bpf/local_storage.c:6:
   In file included from include/linux/filter.h:13:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:216:1: note: expanded from here
   __do_outsb
   ^
   arch/powerpc/include/asm/io.h:559:58: note: expanded from macro '__do_outsb'
   #define __do_outsb(p, b, n)     writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
   In file included from kernel/bpf/local_storage.c:6:
   In file included from include/linux/filter.h:13:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:218:1: note: expanded from here
   __do_outsw
   ^
   arch/powerpc/include/asm/io.h:560:58: note: expanded from macro '__do_outsw'
   #define __do_outsw(p, b, n)     writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
   In file included from kernel/bpf/local_storage.c:6:
   In file included from include/linux/filter.h:13:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:220:1: note: expanded from here
   __do_outsl
   ^
   arch/powerpc/include/asm/io.h:561:58: note: expanded from macro '__do_outsl'
   #define __do_outsl(p, b, n)     writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
>> kernel/bpf/local_storage.c:13:33: error: use of undeclared identifier 'BPF_CGROUP_STORAGE_NEST_MAX'
                  bpf_cgroup_storage_info[BPF_CGROUP_STORAGE_NEST_MAX]);
                                          ^

6 warnings and 1 error generated.
--
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'

                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:200:1: note: expanded from here
   __do_insb
   ^
   arch/powerpc/include/asm/io.h:556:56: note: expanded from macro '__do_insb'
   #define __do_insb(p, b, n)      readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from kernel/trace/bpf_trace.c:11:
   In file included from include/linux/filter.h:13:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:202:1: note: expanded from here
   __do_insw
   ^
   arch/powerpc/include/asm/io.h:557:56: note: expanded from macro '__do_insw'
   #define __do_insw(p, b, n)      readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from kernel/trace/bpf_trace.c:11:
   In file included from include/linux/filter.h:13:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:204:1: note: expanded from here
   __do_insl
   ^
   arch/powerpc/include/asm/io.h:558:56: note: expanded from macro '__do_insl'
   #define __do_insl(p, b, n)      readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from kernel/trace/bpf_trace.c:11:
   In file included from include/linux/filter.h:13:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:206:1: note: expanded from here
   __do_outsb
   ^
   arch/powerpc/include/asm/io.h:559:58: note: expanded from macro '__do_outsb'
   #define __do_outsb(p, b, n)     writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
   In file included from kernel/trace/bpf_trace.c:11:
   In file included from include/linux/filter.h:13:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:208:1: note: expanded from here
   __do_outsw
   ^
   arch/powerpc/include/asm/io.h:560:58: note: expanded from macro '__do_outsw'
   #define __do_outsw(p, b, n)     writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
   In file included from kernel/trace/bpf_trace.c:11:
   In file included from include/linux/filter.h:13:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:210:1: note: expanded from here
   __do_outsl
   ^
   arch/powerpc/include/asm/io.h:561:58: note: expanded from macro '__do_outsl'
   #define __do_outsl(p, b, n)     writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
>> kernel/trace/bpf_trace.c:127:8: error: invalid argument type 'void' to unary expression
           ret = BPF_PROG_RUN_ARRAY_CHECK(call->prog_array, ctx, BPF_PROG_RUN);
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/bpf.h:1194:2: note: expanded from macro 'BPF_PROG_RUN_ARRAY_CHECK'
           __BPF_PROG_RUN_ARRAY(array, ctx, func, true, false)
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/bpf.h:1141:9: note: expanded from macro '__BPF_PROG_RUN_ARRAY'
                                   if (unlikely(bpf_cgroup_storage_set(_item->cgroup_storage)))    \
                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:78:40: note: expanded from macro 'unlikely'
   # define unlikely(x)    __builtin_expect(!!(x), 0)
                                             ^~~~
>> kernel/trace/bpf_trace.c:127:8: error: implicit declaration of function 'bpf_cgroup_storage_unset' [-Werror,-Wimplicit-function-declaration]
   include/linux/bpf.h:1194:2: note: expanded from macro 'BPF_PROG_RUN_ARRAY_CHECK'
           __BPF_PROG_RUN_ARRAY(array, ctx, func, true, false)
           ^
   include/linux/bpf.h:1144:5: note: expanded from macro '__BPF_PROG_RUN_ARRAY'
                                   bpf_cgroup_storage_unset();     \
                                   ^
   kernel/trace/bpf_trace.c:127:8: note: did you mean 'bpf_cgroup_storage_set'?
   include/linux/bpf.h:1194:2: note: expanded from macro 'BPF_PROG_RUN_ARRAY_CHECK'
           __BPF_PROG_RUN_ARRAY(array, ctx, func, true, false)
           ^
   include/linux/bpf.h:1144:5: note: expanded from macro '__BPF_PROG_RUN_ARRAY'
                                   bpf_cgroup_storage_unset();     \
                                   ^
   include/linux/bpf-cgroup.h:492:20: note: 'bpf_cgroup_storage_set' declared here
   static inline void bpf_cgroup_storage_set(
                      ^

6 warnings and 2 errors generated.

vim +/BPF_CGROUP_STORAGE_NEST_MAX +13 kernel/bpf/local_storage.c

11
12 DEFINE_PER_CPU(struct bpf_cgroup_storage_info,

  > 13		       bpf_cgroup_storage_info[BPF_CGROUP_STORAGE_NEST_MAX]);
    14

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

kernel test robot

build bot

2021-03-19 02:40 UTC #3 lore raw

Re: [PATCH bpf-next 1/2] bpf: fix NULL pointer dereference in bpf_get_local_storage() helper

In reply to Yonghong Song

I love your patch! Yet something to improve:

Hi Yonghong,

I love your patch! Yet something to improve:

url:    https://github.com/0day-ci/linux/commits/Yonghong-Song/bpf-fix-NULL-pointer-dereference-in/20210319-082140
base:    ba5b053ab3ac674b91a6669086139819359a5e6e
config: i386-randconfig-a005-20210318 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/38b56e03d083be940fe9dc231c5f6c8f4f282f15
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Yonghong-Song/bpf-fix-NULL-pointer-dereference-in/20210319-082140
        git checkout 38b56e03d083be940fe9dc231c5f6c8f4f282f15
        # save the attached .config to linux build tree
        make W=1 ARCH=i386

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

In file included from include/asm-generic/percpu.h:7,

                    from arch/x86/include/asm/percpu.h:390,
                    from arch/x86/include/asm/current.h:6,
                    from arch/x86/include/asm/processor.h:17,
                    from arch/x86/include/asm/timex.h:5,
                    from include/linux/timex.h:65,
                    from include/linux/time32.h:13,
                    from include/linux/time.h:60,
                    from include/linux/ktime.h:24,
                    from include/linux/timer.h:6,
                    from include/linux/workqueue.h:9,
                    from include/linux/bpf.h:9,
                    from include/linux/bpf-cgroup.h:5,
                    from kernel/bpf/local_storage.c:2:
>> kernel/bpf/local_storage.c:13:33: error: 'BPF_CGROUP_STORAGE_NEST_MAX' undeclared here (not in a function); did you mean 'BPF_CGROUP_STORAGE_PERCPU'?
      13 |         bpf_cgroup_storage_info[BPF_CGROUP_STORAGE_NEST_MAX]);
         |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/percpu-defs.h:104:37: note: in definition of macro 'DEFINE_PER_CPU_SECTION'
     104 |  __PCPU_ATTRS(sec) __typeof__(type) name
         |                                     ^~~~

kernel/bpf/local_storage.c:12:1: note: in expansion of macro 'DEFINE_PER_CPU'
12 | DEFINE_PER_CPU(struct bpf_cgroup_storage_info,
| ^~~~~~~~~~~~~~

vim +13 kernel/bpf/local_storage.c

11
12 DEFINE_PER_CPU(struct bpf_cgroup_storage_info,

  > 13		       bpf_cgroup_storage_info[BPF_CGROUP_STORAGE_NEST_MAX]);
    14

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

Yonghong Song

author

2021-03-19 00:19 UTC #4 lore raw

PATCH 2/2 bpf: fix bpf_cgroup_storage_set() usage in test_run

In reply to Yonghong Song

In bpf_test_run(), check the return value of bpf_cgroup_storage_set() and do bpf_cgroup_storate_unset() properly.

Cc Jiri Olsa <[email protected]>

Cc Roman Gushchin <[email protected]>

Signed-off-by Yonghong Song <[email protected]>

---
 net/bpf/test_run.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

net/bpf/test_run.c +5 -1

index 0abdd67f44b1..4aabf71cd95d 100644
@@ -106,12 +106,16 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
 
 	bpf_test_timer_enter(&t);
 	do {
-		bpf_cgroup_storage_set(storage);
+		ret = bpf_cgroup_storage_set(storage);
+		if (ret)
+			break;
 
 		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_test_timer_leave(&t);

Signature · 1 line

2.30.2

kernel test robot

build bot

2021-03-19 02:11 UTC #5 lore raw

Re: [PATCH bpf-next 2/2] bpf: fix bpf_cgroup_storage_set() usage in test_run

I love your patch! Yet something to improve:

Hi Yonghong,

I love your patch! Yet something to improve:

url:    https://github.com/0day-ci/linux/commits/Yonghong-Song/bpf-fix-NULL-pointer-dereference-in/20210319-082140
base:    ba5b053ab3ac674b91a6669086139819359a5e6e
config: arm-randconfig-r014-20210318 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project fcc1ce00931751ac02498986feb37744e9ace8de)

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 arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/0day-ci/linux/commit/4d3510a809c631ed53b78f7baa50fd4cc2730587
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Yonghong-Song/bpf-fix-NULL-pointer-dereference-in/20210319-082140 git checkout 4d3510a809c631ed53b78f7baa50fd4cc2730587
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

>> net/bpf/test_run.c:109:7: error: assigning to 'int' from incompatible type 'void'
                   ret = bpf_cgroup_storage_set(storage);
                       ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> net/bpf/test_run.c:118:3: error: implicit declaration of function 'bpf_cgroup_storage_unset' [-Werror,-Wimplicit-function-declaration]
                   bpf_cgroup_storage_unset();
                   ^
   net/bpf/test_run.c:118:3: note: did you mean 'bpf_cgroup_storage_set'?
   include/linux/bpf-cgroup.h:492:20: note: 'bpf_cgroup_storage_set' declared here
   static inline void bpf_cgroup_storage_set(
                      ^
   net/bpf/test_run.c:167:14: warning: no previous prototype for function 'bpf_fentry_test1' [-Wmissing-prototypes]
   int noinline bpf_fentry_test1(int a)
                ^
   net/bpf/test_run.c:167:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test1(int a)
   ^
   static 
   net/bpf/test_run.c:172:14: warning: no previous prototype for function 'bpf_fentry_test2' [-Wmissing-prototypes]
   int noinline bpf_fentry_test2(int a, u64 b)
                ^
   net/bpf/test_run.c:172:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test2(int a, u64 b)
   ^
   static 
   net/bpf/test_run.c:177:14: warning: no previous prototype for function 'bpf_fentry_test3' [-Wmissing-prototypes]
   int noinline bpf_fentry_test3(char a, int b, u64 c)
                ^
   net/bpf/test_run.c:177:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test3(char a, int b, u64 c)
   ^
   static 
   net/bpf/test_run.c:182:14: warning: no previous prototype for function 'bpf_fentry_test4' [-Wmissing-prototypes]
   int noinline bpf_fentry_test4(void *a, char b, int c, u64 d)
                ^
   net/bpf/test_run.c:182:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test4(void *a, char b, int c, u64 d)
   ^
   static 
   net/bpf/test_run.c:187:14: warning: no previous prototype for function 'bpf_fentry_test5' [-Wmissing-prototypes]
   int noinline bpf_fentry_test5(u64 a, void *b, short c, int d, u64 e)
                ^
   net/bpf/test_run.c:187:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test5(u64 a, void *b, short c, int d, u64 e)
   ^
   static 
   net/bpf/test_run.c:192:14: warning: no previous prototype for function 'bpf_fentry_test6' [-Wmissing-prototypes]
   int noinline bpf_fentry_test6(u64 a, void *b, short c, int d, void *e, u64 f)
                ^
   net/bpf/test_run.c:192:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test6(u64 a, void *b, short c, int d, void *e, u64 f)
   ^
   static 
   net/bpf/test_run.c:201:14: warning: no previous prototype for function 'bpf_fentry_test7' [-Wmissing-prototypes]
   int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg)
                ^
   net/bpf/test_run.c:201:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg)
   ^
   static 
   net/bpf/test_run.c:206:14: warning: no previous prototype for function 'bpf_fentry_test8' [-Wmissing-prototypes]
   int noinline bpf_fentry_test8(struct bpf_fentry_test_t *arg)
                ^
   net/bpf/test_run.c:206:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test8(struct bpf_fentry_test_t *arg)
   ^
   static 
   net/bpf/test_run.c:211:14: warning: no previous prototype for function 'bpf_modify_return_test' [-Wmissing-prototypes]
   int noinline bpf_modify_return_test(int a, int *b)
                ^

net/bpf/test_run.c:211:1: note: declare 'static' if the function is not intended to be used outside of this translation unit int noinline bpf_modify_return_test(int a, int *b)
^
static
9 warnings and 2 errors generated.

vim +109 net/bpf/test_run.c

    85	
    86	static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
    87				u32 *retval, u32 *time, bool xdp)
    88	{
    89		struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
    90		struct bpf_test_timer t = { NO_MIGRATE };
    91		enum bpf_cgroup_storage_type stype;
    92		int ret;
    93	
    94		for_each_cgroup_storage_type(stype) {
    95			storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
    96			if (IS_ERR(storage[stype])) {
    97				storage[stype] = NULL;
    98				for_each_cgroup_storage_type(stype)
    99					bpf_cgroup_storage_free(storage[stype]);
   100				return -ENOMEM;
   101			}
   102		}
   103	
   104		if (!repeat)
   105			repeat = 1;
   106	
   107		bpf_test_timer_enter(&t);
   108		do {
 > 109			ret = bpf_cgroup_storage_set(storage);
   110			if (ret)
   111				break;
   112	
   113			if (xdp)
   114				*retval = bpf_prog_run_xdp(prog, ctx);
   115			else
   116				*retval = BPF_PROG_RUN(prog, ctx);
   117	
 > 118			bpf_cgroup_storage_unset();

119 } while (bpf_test_timer_continue(&t, repeat, &ret, time));
120 bpf_test_timer_leave(&t);
121
122 for_each_cgroup_storage_type(stype)
123 bpf_cgroup_storage_free(storage[stype]);
124
125 return ret;
126 }
127

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