diff mbox series

[bpf-next,v6,11/26] bpf: Allow locking bpf_spin_lock in allocated objects

Message ID 20221111193224.876706-12-memxor@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Allocated objects, BPF linked lists | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count fail Series longer than 15 patches (and no cover letter)
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 17 this patch: 18
netdev/cc_maintainers warning 8 maintainers not CCed: sdf@google.com kpsingh@kernel.org haoluo@google.com yhs@fb.com jolsa@kernel.org martin.lau@linux.dev song@kernel.org john.fastabend@gmail.com
netdev/build_clang fail Errors and warnings before: 5 this patch: 7
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 17 this patch: 18
netdev/checkpatch warning WARNING: Missing a blank line after declarations WARNING: line length of 103 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns WARNING: line length of 99 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-4 fail Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-9 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-7 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-5 fail Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 fail Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-2 fail Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 fail Logs for build for aarch64 with llvm-16

Commit Message

Kumar Kartikeya Dwivedi Nov. 11, 2022, 7:32 p.m. UTC 
Allow locking a bpf_spin_lock in an allocated object, in addition to
already support map value pointers. The handling is similar to that of
map values, by just preserving the reg->id of PTR_TO_BTF_ID | MEM_ALLOC
as well, and adjusting process_spin_lock to work with them  and remember
the id in verifier state.

Refactor the existing process_spin_lock to work with PTR_TO_BTF_ID |
MEM_ALLOC in addition to PTR_TO_MAP_VALUE. We need to update the
reg_may_point_to_spin_lock which is used in mark_ptr_or_null_reg to
preserve reg->id, that will be used in env->cur_state->active_spin_lock
to remember the currently held spin lock.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/helpers.c  |  2 ++
 kernel/bpf/verifier.c | 70 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 55 insertions(+), 17 deletions(-)

Comments

Dan Carpenter Nov. 14, 2022, 8:25 a.m. UTC | #1 
Hi Kumar,

url:    https://github.com/intel-lab-lkp/linux/commits/Kumar-Kartikeya-Dwivedi/Allocated-objects-BPF-linked-lists/20221112-033643
base:   e5659e4e19e49f1eac58bb07ce8bc2d78a89fe65
patch link:    https://lore.kernel.org/r/20221111193224.876706-12-memxor%40gmail.com
patch subject: [PATCH bpf-next v6 11/26] bpf: Allow locking bpf_spin_lock in allocated objects
config: x86_64-randconfig-m001
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>

smatch warnings:
kernel/bpf/verifier.c:5623 process_spin_lock() error: uninitialized symbol 'rec'.

vim +/rec +5623 kernel/bpf/verifier.c

d83525ca62cf8e Alexei Starovoitov      2019-01-31  5588  static int process_spin_lock(struct bpf_verifier_env *env, int regno,
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5589  			     bool is_lock)
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5590  {
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5591  	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5592  	struct bpf_verifier_state *cur = env->cur_state;
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5593  	bool is_const = tnum_is_const(reg->var_off);
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5594  	u64 val = reg->var_off.value;
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5595  	struct bpf_map *map = NULL;
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5596  	struct btf_record *rec;
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5597  	struct btf *btf = NULL;
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5598  
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5599  	if (!is_const) {
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5600  		verbose(env,
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5601  			"R%d doesn't have constant offset. bpf_spin_lock has to be at the constant offset\n",
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5602  			regno);
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5603  		return -EINVAL;
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5604  	}
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5605  	if (reg->type == PTR_TO_MAP_VALUE) {
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5606  		map = reg->map_ptr;
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5607  		if (!map->btf) {
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5608  			verbose(env,
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5609  				"map '%s' has to have BTF in order to use bpf_spin_lock\n",
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5610  				map->name);
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5611  			return -EINVAL;
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5612  		}
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5613  		rec = map->record;
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5614  	} else {
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5615  		struct btf_struct_meta *meta;
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5616  
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5617  		btf = reg->btf;
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5618  		meta = btf_find_struct_meta(reg->btf, reg->btf_id);
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5619  		if (meta)
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5620  			rec = meta->record;

No else path.

425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5621  	}
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5622  
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12 @5623  	if (!btf_record_has_field(rec, BPF_SPIN_LOCK)) {
                                                                                          ^^^

425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5624  		verbose(env, "%s '%s' has no valid bpf_spin_lock\n", map ? "map" : "local",
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5625  			map ? map->name : "kptr");
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5626  		return -EINVAL;
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5627  	}
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5628  	if (rec->spin_lock_off != val + reg->off) {
db559117828d24 Kumar Kartikeya Dwivedi 2022-11-04  5629  		verbose(env, "off %lld doesn't point to 'struct bpf_spin_lock' that is at %d\n",
425ce908da14d5 Kumar Kartikeya Dwivedi 2022-11-12  5630  			val + reg->off, rec->spin_lock_off);
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5631  		return -EINVAL;
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5632  	}
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5633  	if (is_lock) {
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5634  		if (cur->active_spin_lock) {
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5635  			verbose(env,
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5636  				"Locking two bpf_spin_locks are not allowed\n");
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5637  			return -EINVAL;
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5638  		}
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5639  		cur->active_spin_lock = reg->id;
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5640  	} else {
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5641  		if (!cur->active_spin_lock) {
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5642  			verbose(env, "bpf_spin_unlock without taking a lock\n");
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5643  			return -EINVAL;
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5644  		}
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5645  		if (cur->active_spin_lock != reg->id) {
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5646  			verbose(env, "bpf_spin_unlock of different lock\n");
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5647  			return -EINVAL;
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5648  		}
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5649  		cur->active_spin_lock = 0;
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5650  	}
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5651  	return 0;
d83525ca62cf8e Alexei Starovoitov      2019-01-31  5652  }
Kumar Kartikeya Dwivedi Nov. 14, 2022, 9:11 a.m. UTC | #2 
On Mon, Nov 14, 2022 at 01:55:01PM IST, Dan Carpenter wrote:
> Hi Kumar,
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Kumar-Kartikeya-Dwivedi/Allocated-objects-BPF-linked-lists/20221112-033643
> base:   e5659e4e19e49f1eac58bb07ce8bc2d78a89fe65
> patch link:    https://lore.kernel.org/r/20221111193224.876706-12-memxor%40gmail.com
> patch subject: [PATCH bpf-next v6 11/26] bpf: Allow locking bpf_spin_lock in allocated objects
> config: x86_64-randconfig-m001
> compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
>
> smatch warnings:
> kernel/bpf/verifier.c:5623 process_spin_lock() error: uninitialized symbol 'rec'.
>
> vim +/rec +5623 kernel/bpf/verifier.c

Hi Dan,

Thanks for the report! I noticed it yesterday night.

How should I be crediting you in the respin? Not sure Reported-by: is ok if I
incorporate the fix into the patch and resend it. Would Fixed-by: be better?

> [...]
Dan Carpenter Nov. 14, 2022, 9:38 a.m. UTC | #3 
On Mon, Nov 14, 2022 at 02:41:00PM +0530, Kumar Kartikeya Dwivedi wrote:
> On Mon, Nov 14, 2022 at 01:55:01PM IST, Dan Carpenter wrote:
> > Hi Kumar,
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Kumar-Kartikeya-Dwivedi/Allocated-objects-BPF-linked-lists/20221112-033643
> > base:   e5659e4e19e49f1eac58bb07ce8bc2d78a89fe65
> > patch link:    https://lore.kernel.org/r/20221111193224.876706-12-memxor%40gmail.com
> > patch subject: [PATCH bpf-next v6 11/26] bpf: Allow locking bpf_spin_lock in allocated objects
> > config: x86_64-randconfig-m001
> > compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
> >
> > If you fix the issue, kindly add following tag where applicable
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Reported-by: Dan Carpenter <error27@gmail.com>
> >
> > smatch warnings:
> > kernel/bpf/verifier.c:5623 process_spin_lock() error: uninitialized symbol 'rec'.
> >
> > vim +/rec +5623 kernel/bpf/verifier.c
> 
> Hi Dan,
> 
> Thanks for the report! I noticed it yesterday night.
> 
> How should I be crediting you in the respin? Not sure Reported-by: is ok if I
> incorporate the fix into the patch and resend it. Would Fixed-by: be better?
> 

I've always said there should be a Fixes-from: tag but it doesn't exist.

Just leave it off.  These emails are auto-generated by the kbuild-bot
because credit helps fund their work.  I just look the bug reports and
forward the reports.

regards,
dan carpenter
diff mbox series

Patch

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 7bc71995f17c..5bc0b9f0f306 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -336,6 +336,7 @@  const struct bpf_func_proto bpf_spin_lock_proto = {
 	.gpl_only	= false,
 	.ret_type	= RET_VOID,
 	.arg1_type	= ARG_PTR_TO_SPIN_LOCK,
+	.arg1_btf_id    = BPF_PTR_POISON,
 };
 
 static inline void __bpf_spin_unlock_irqrestore(struct bpf_spin_lock *lock)
@@ -358,6 +359,7 @@  const struct bpf_func_proto bpf_spin_unlock_proto = {
 	.gpl_only	= false,
 	.ret_type	= RET_VOID,
 	.arg1_type	= ARG_PTR_TO_SPIN_LOCK,
+	.arg1_btf_id    = BPF_PTR_POISON,
 };
 
 void copy_map_value_locked(struct bpf_map *map, void *dst, void *src,
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5cf14c1391a5..3831364af1ce 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -453,8 +453,16 @@  static bool reg_type_not_null(enum bpf_reg_type type)
 
 static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg)
 {
-	return reg->type == PTR_TO_MAP_VALUE &&
-	       btf_record_has_field(reg->map_ptr->record, BPF_SPIN_LOCK);
+	struct btf_record *rec = NULL;
+
+	if (reg->type == PTR_TO_MAP_VALUE) {
+		rec = reg->map_ptr->record;
+	} else if (reg->type == (PTR_TO_BTF_ID | MEM_ALLOC)) {
+		struct btf_struct_meta *meta = btf_find_struct_meta(reg->btf, reg->btf_id);
+		if (meta)
+			rec = meta->record;
+	}
+	return btf_record_has_field(rec, BPF_SPIN_LOCK);
 }
 
 static bool type_is_rdonly_mem(u32 type)
@@ -5583,8 +5591,10 @@  static int process_spin_lock(struct bpf_verifier_env *env, int regno,
 	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
 	struct bpf_verifier_state *cur = env->cur_state;
 	bool is_const = tnum_is_const(reg->var_off);
-	struct bpf_map *map = reg->map_ptr;
 	u64 val = reg->var_off.value;
+	struct bpf_map *map = NULL;
+	struct btf_record *rec;
+	struct btf *btf = NULL;
 
 	if (!is_const) {
 		verbose(env,
@@ -5592,19 +5602,32 @@  static int process_spin_lock(struct bpf_verifier_env *env, int regno,
 			regno);
 		return -EINVAL;
 	}
-	if (!map->btf) {
-		verbose(env,
-			"map '%s' has to have BTF in order to use bpf_spin_lock\n",
-			map->name);
-		return -EINVAL;
+	if (reg->type == PTR_TO_MAP_VALUE) {
+		map = reg->map_ptr;
+		if (!map->btf) {
+			verbose(env,
+				"map '%s' has to have BTF in order to use bpf_spin_lock\n",
+				map->name);
+			return -EINVAL;
+		}
+		rec = map->record;
+	} else {
+		struct btf_struct_meta *meta;
+
+		btf = reg->btf;
+		meta = btf_find_struct_meta(reg->btf, reg->btf_id);
+		if (meta)
+			rec = meta->record;
 	}
-	if (!btf_record_has_field(map->record, BPF_SPIN_LOCK)) {
-		verbose(env, "map '%s' has no valid bpf_spin_lock\n", map->name);
+
+	if (!btf_record_has_field(rec, BPF_SPIN_LOCK)) {
+		verbose(env, "%s '%s' has no valid bpf_spin_lock\n", map ? "map" : "local",
+			map ? map->name : "kptr");
 		return -EINVAL;
 	}
-	if (map->record->spin_lock_off != val + reg->off) {
+	if (rec->spin_lock_off != val + reg->off) {
 		verbose(env, "off %lld doesn't point to 'struct bpf_spin_lock' that is at %d\n",
-			val + reg->off, map->record->spin_lock_off);
+			val + reg->off, rec->spin_lock_off);
 		return -EINVAL;
 	}
 	if (is_lock) {
@@ -5810,13 +5833,19 @@  static const struct bpf_reg_types int_ptr_types = {
 	},
 };
 
+static const struct bpf_reg_types spin_lock_types = {
+	.types = {
+		PTR_TO_MAP_VALUE,
+		PTR_TO_BTF_ID | MEM_ALLOC,
+	}
+};
+
 static const struct bpf_reg_types fullsock_types = { .types = { PTR_TO_SOCKET } };
 static const struct bpf_reg_types scalar_types = { .types = { SCALAR_VALUE } };
 static const struct bpf_reg_types context_types = { .types = { PTR_TO_CTX } };
 static const struct bpf_reg_types ringbuf_mem_types = { .types = { PTR_TO_MEM | MEM_RINGBUF } };
 static const struct bpf_reg_types const_map_ptr_types = { .types = { CONST_PTR_TO_MAP } };
 static const struct bpf_reg_types btf_ptr_types = { .types = { PTR_TO_BTF_ID } };
-static const struct bpf_reg_types spin_lock_types = { .types = { PTR_TO_MAP_VALUE } };
 static const struct bpf_reg_types percpu_btf_ptr_types = { .types = { PTR_TO_BTF_ID | MEM_PERCPU } };
 static const struct bpf_reg_types func_ptr_types = { .types = { PTR_TO_FUNC } };
 static const struct bpf_reg_types stack_ptr_types = { .types = { PTR_TO_STACK } };
@@ -5941,6 +5970,11 @@  static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
 				return -EACCES;
 			}
 		}
+	} else if (type_is_alloc(reg->type)) {
+		if (meta->func_id != BPF_FUNC_spin_lock && meta->func_id != BPF_FUNC_spin_unlock) {
+			verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n");
+			return -EFAULT;
+		}
 	}
 
 	return 0;
@@ -6057,7 +6091,8 @@  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		goto skip_type_check;
 
 	/* arg_btf_id and arg_size are in a union. */
-	if (base_type(arg_type) == ARG_PTR_TO_BTF_ID)
+	if (base_type(arg_type) == ARG_PTR_TO_BTF_ID ||
+	    base_type(arg_type) == ARG_PTR_TO_SPIN_LOCK)
 		arg_btf_id = fn->arg_btf_id[arg];
 
 	err = check_reg_type(env, regno, arg_type, arg_btf_id, meta);
@@ -6675,9 +6710,10 @@  static bool check_btf_id_ok(const struct bpf_func_proto *fn)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++) {
-		if (base_type(fn->arg_type[i]) == ARG_PTR_TO_BTF_ID && !fn->arg_btf_id[i])
-			return false;
-
+		if (base_type(fn->arg_type[i]) == ARG_PTR_TO_BTF_ID)
+			return !!fn->arg_btf_id[i];
+		if (base_type(fn->arg_type[i]) == ARG_PTR_TO_SPIN_LOCK)
+			return fn->arg_btf_id[i] == BPF_PTR_POISON;
 		if (base_type(fn->arg_type[i]) != ARG_PTR_TO_BTF_ID && fn->arg_btf_id[i] &&
 		    /* arg_btf_id and arg_size are in a union. */
 		    (base_type(fn->arg_type[i]) != ARG_PTR_TO_MEM ||