| Message ID | 20230814172830.1363918-1-yonghong.song@linux.dev (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Add support for local percpu kptr | expand |
On Mon, Aug 14, 2023 at 10:28:30AM -0700, Yonghong Song wrote: > The bpf helpers bpf_this_cpu_ptr() and bpf_per_cpu_ptr() are re-purposed > for allocated percpu objects. For an allocated percpu obj, > the reg type is 'PTR_TO_BTF_ID | MEM_PERCPU | MEM_RCU'. > > The return type for these two re-purposed helpera is > 'PTR_TO_MEM | MEM_RCU | MEM_ALLOC'. > The MEM_ALLOC allows that the per-cpu data can be read and written. > > Since the memory allocator bpf_mem_alloc() returns > a ptr to a percpu ptr for percpu data, the first argument > of bpf_this_cpu_ptr() and bpf_per_cpu_ptr() is patched > with a dereference before passing to the helper func. > > Signed-off-by: Yonghong Song <yonghong.song@linux.dev> > --- > include/linux/bpf_verifier.h | 1 + > kernel/bpf/verifier.c | 51 ++++++++++++++++++++++++++++++------ > 2 files changed, 44 insertions(+), 8 deletions(-) > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index f70f9ac884d2..e23480db37ec 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h > @@ -480,6 +480,7 @@ struct bpf_insn_aux_data { > bool zext_dst; /* this insn zero extends dst reg */ > bool storage_get_func_atomic; /* bpf_*_storage_get() with atomic memory alloc */ > bool is_iter_next; /* bpf_iter_<type>_next() kfunc call */ > + bool percpu_ptr_prog_alloc; /* {this,per}_cpu_ptr() with prog alloc */ > u8 alu_state; /* used in combination with alu_limit */ > > /* below fields are initialized once */ > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index a985fbf18a11..6fc200cb68b6 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -6221,7 +6221,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > } > > if (type_is_alloc(reg->type) && !type_is_non_owning_ref(reg->type) && > - !reg->ref_obj_id) { > + !(reg->type & MEM_RCU) && !reg->ref_obj_id) { > verbose(env, "verifier internal error: ref_obj_id for allocated object must be non-zero\n"); > return -EFAULT; > } > @@ -7765,6 +7765,7 @@ static const struct bpf_reg_types btf_ptr_types = { > static const struct bpf_reg_types percpu_btf_ptr_types = { > .types = { > PTR_TO_BTF_ID | MEM_PERCPU, > + PTR_TO_BTF_ID | MEM_PERCPU | MEM_RCU, > PTR_TO_BTF_ID | MEM_PERCPU | PTR_TRUSTED, > } > }; > @@ -7945,6 +7946,7 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > return -EACCES; > break; > case PTR_TO_BTF_ID | MEM_PERCPU: > + case PTR_TO_BTF_ID | MEM_PERCPU | MEM_RCU: > case PTR_TO_BTF_ID | MEM_PERCPU | PTR_TRUSTED: > /* Handled by helper specific checks */ > break; > @@ -8287,6 +8289,16 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > verbose(env, "Helper has invalid btf_id in R%d\n", regno); > return -EACCES; > } > + if (reg->type & MEM_RCU) { > + const struct btf_type *type = btf_type_by_id(reg->btf, reg->btf_id); > + > + if (!type || !btf_type_is_struct(type)) { > + verbose(env, "Helper has invalid btf/btf_id in R%d\n", regno); > + return -EFAULT; > + } > + env->insn_aux_data[insn_idx].percpu_ptr_prog_alloc = true; > + } Let's move this check out of check_func_arg() and do it in check_helper_call() after the loop. meta has what we need. Also I would do it only for specific helpers like: case BPF_FUNC_per_cpu_ptr: case BPF_FUNC_this_cpu_ptr: if (reg[R1]->type & MEM_RCU) { const struct btf_type *type = btf_type_by_id(meta->ret_btf, ...) ... returns_cpu_specific_alloc_ptr = true; insn_aux_datap[].call_with_percpu_alloc_ptr = ture; } > + > meta->ret_btf = reg->btf; > meta->ret_btf_id = reg->btf_id; > break; > @@ -9888,14 +9900,18 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > regs[BPF_REG_0].type = PTR_TO_MEM | ret_flag; > regs[BPF_REG_0].mem_size = tsize; > } else { > - /* MEM_RDONLY may be carried from ret_flag, but it > - * doesn't apply on PTR_TO_BTF_ID. Fold it, otherwise > - * it will confuse the check of PTR_TO_BTF_ID in > - * check_mem_access(). > - */ > - ret_flag &= ~MEM_RDONLY; > + if (env->insn_aux_data[insn_idx].percpu_ptr_prog_alloc) { and here I would only check the local bool returns_percpu_alloc_ptr. Because returns_cpu_specific_alloc_ptr is not the same as call_with_percpu_alloc_ptr. Like this_cpu_read() is a call_with_percpu_alloc_ptr, but it doesn't return cpu specific pointer. It derefs cpu specific pointer and returns the value. Of course, we don't have such helper today, but worth thinking ahead. > + regs[BPF_REG_0].type = PTR_TO_BTF_ID | MEM_ALLOC | MEM_RCU; > + } else { > + /* MEM_RDONLY may be carried from ret_flag, but it > + * doesn't apply on PTR_TO_BTF_ID. Fold it, otherwise > + * it will confuse the check of PTR_TO_BTF_ID in > + * check_mem_access(). > + */ > + ret_flag &= ~MEM_RDONLY; > + regs[BPF_REG_0].type = PTR_TO_BTF_ID | ret_flag; > + } > > - regs[BPF_REG_0].type = PTR_TO_BTF_ID | ret_flag; > regs[BPF_REG_0].btf = meta.ret_btf; > regs[BPF_REG_0].btf_id = meta.ret_btf_id; > } > @@ -18646,6 +18662,25 @@ static int do_misc_fixups(struct bpf_verifier_env *env) > goto patch_call_imm; > } > > + /* bpf_per_cpu_ptr() and bpf_this_cpu_ptr() */ > + if (env->insn_aux_data[i + delta].percpu_ptr_prog_alloc) { call_with_percpu_alloc_ptr > + /* patch with 'r1 = *(u64 *)(r1 + 0)' since for percpu data, > + * bpf_mem_alloc() returns a ptr to the percpu data ptr. > + */ > + insn_buf[0] = BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, 0); here R1 is no longer a concern, since we check R1 only in check_helper_call. > + insn_buf[1] = *insn; > + cnt = 2; > + > + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt); > + if (!new_prog) > + return -ENOMEM; > + > + delta += cnt - 1; > + env->prog = prog = new_prog; > + insn = new_prog->insnsi + i + delta; > + goto patch_call_imm; > + } > + > /* BPF_EMIT_CALL() assumptions in some of the map_gen_lookup > * and other inlining handlers are currently limited to 64 bit > * only. > -- > 2.34.1 >
On 8/18/23 6:01 PM, Alexei Starovoitov wrote: > On Mon, Aug 14, 2023 at 10:28:30AM -0700, Yonghong Song wrote: >> The bpf helpers bpf_this_cpu_ptr() and bpf_per_cpu_ptr() are re-purposed >> for allocated percpu objects. For an allocated percpu obj, >> the reg type is 'PTR_TO_BTF_ID | MEM_PERCPU | MEM_RCU'. >> >> The return type for these two re-purposed helpera is >> 'PTR_TO_MEM | MEM_RCU | MEM_ALLOC'. >> The MEM_ALLOC allows that the per-cpu data can be read and written. >> >> Since the memory allocator bpf_mem_alloc() returns >> a ptr to a percpu ptr for percpu data, the first argument >> of bpf_this_cpu_ptr() and bpf_per_cpu_ptr() is patched >> with a dereference before passing to the helper func. >> >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev> >> --- >> include/linux/bpf_verifier.h | 1 + >> kernel/bpf/verifier.c | 51 ++++++++++++++++++++++++++++++------ >> 2 files changed, 44 insertions(+), 8 deletions(-) >> >> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h >> index f70f9ac884d2..e23480db37ec 100644 >> --- a/include/linux/bpf_verifier.h >> +++ b/include/linux/bpf_verifier.h >> @@ -480,6 +480,7 @@ struct bpf_insn_aux_data { >> bool zext_dst; /* this insn zero extends dst reg */ >> bool storage_get_func_atomic; /* bpf_*_storage_get() with atomic memory alloc */ >> bool is_iter_next; /* bpf_iter_<type>_next() kfunc call */ >> + bool percpu_ptr_prog_alloc; /* {this,per}_cpu_ptr() with prog alloc */ >> u8 alu_state; /* used in combination with alu_limit */ >> >> /* below fields are initialized once */ >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index a985fbf18a11..6fc200cb68b6 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -6221,7 +6221,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, >> } >> >> if (type_is_alloc(reg->type) && !type_is_non_owning_ref(reg->type) && >> - !reg->ref_obj_id) { >> + !(reg->type & MEM_RCU) && !reg->ref_obj_id) { >> verbose(env, "verifier internal error: ref_obj_id for allocated object must be non-zero\n"); >> return -EFAULT; >> } >> @@ -7765,6 +7765,7 @@ static const struct bpf_reg_types btf_ptr_types = { >> static const struct bpf_reg_types percpu_btf_ptr_types = { >> .types = { >> PTR_TO_BTF_ID | MEM_PERCPU, >> + PTR_TO_BTF_ID | MEM_PERCPU | MEM_RCU, >> PTR_TO_BTF_ID | MEM_PERCPU | PTR_TRUSTED, >> } >> }; >> @@ -7945,6 +7946,7 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, >> return -EACCES; >> break; >> case PTR_TO_BTF_ID | MEM_PERCPU: >> + case PTR_TO_BTF_ID | MEM_PERCPU | MEM_RCU: >> case PTR_TO_BTF_ID | MEM_PERCPU | PTR_TRUSTED: >> /* Handled by helper specific checks */ >> break; >> @@ -8287,6 +8289,16 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, >> verbose(env, "Helper has invalid btf_id in R%d\n", regno); >> return -EACCES; >> } >> + if (reg->type & MEM_RCU) { >> + const struct btf_type *type = btf_type_by_id(reg->btf, reg->btf_id); >> + >> + if (!type || !btf_type_is_struct(type)) { >> + verbose(env, "Helper has invalid btf/btf_id in R%d\n", regno); >> + return -EFAULT; >> + } >> + env->insn_aux_data[insn_idx].percpu_ptr_prog_alloc = true; >> + } > > Let's move this check out of check_func_arg() and do it in check_helper_call() after the loop. > meta has what we need. > Also I would do it only for specific helpers like: > case BPF_FUNC_per_cpu_ptr: > case BPF_FUNC_this_cpu_ptr: > if (reg[R1]->type & MEM_RCU) { > const struct btf_type *type = btf_type_by_id(meta->ret_btf, ...) > ... > returns_cpu_specific_alloc_ptr = true; > insn_aux_datap[].call_with_percpu_alloc_ptr = ture; > } This is more explicit and easier to understand. Will use the above in v2. >> + >> meta->ret_btf = reg->btf; >> meta->ret_btf_id = reg->btf_id; >> break; >> @@ -9888,14 +9900,18 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn >> regs[BPF_REG_0].type = PTR_TO_MEM | ret_flag; >> regs[BPF_REG_0].mem_size = tsize; >> } else { >> - /* MEM_RDONLY may be carried from ret_flag, but it >> - * doesn't apply on PTR_TO_BTF_ID. Fold it, otherwise >> - * it will confuse the check of PTR_TO_BTF_ID in >> - * check_mem_access(). >> - */ >> - ret_flag &= ~MEM_RDONLY; >> + if (env->insn_aux_data[insn_idx].percpu_ptr_prog_alloc) { > > and here I would only check the local bool returns_percpu_alloc_ptr. > > Because returns_cpu_specific_alloc_ptr is not the same as call_with_percpu_alloc_ptr. > > Like this_cpu_read() is a call_with_percpu_alloc_ptr, but it doesn't return cpu specific pointer. > It derefs cpu specific pointer and returns the value. > Of course, we don't have such helper today, but worth thinking ahead. Agree. percpu_ptr_prog_alloc is a bad name. I actually spent some time on how to name it but did not come with a good one. Will use returns_cpu_specific_alloc_ptr as you suggested. > >> + regs[BPF_REG_0].type = PTR_TO_BTF_ID | MEM_ALLOC | MEM_RCU; >> + } else { >> + /* MEM_RDONLY may be carried from ret_flag, but it >> + * doesn't apply on PTR_TO_BTF_ID. Fold it, otherwise >> + * it will confuse the check of PTR_TO_BTF_ID in >> + * check_mem_access(). >> + */ >> + ret_flag &= ~MEM_RDONLY; >> + regs[BPF_REG_0].type = PTR_TO_BTF_ID | ret_flag; >> + } >> >> - regs[BPF_REG_0].type = PTR_TO_BTF_ID | ret_flag; >> regs[BPF_REG_0].btf = meta.ret_btf; >> regs[BPF_REG_0].btf_id = meta.ret_btf_id; >> } >> @@ -18646,6 +18662,25 @@ static int do_misc_fixups(struct bpf_verifier_env *env) >> goto patch_call_imm; >> } >> >> + /* bpf_per_cpu_ptr() and bpf_this_cpu_ptr() */ >> + if (env->insn_aux_data[i + delta].percpu_ptr_prog_alloc) { > > call_with_percpu_alloc_ptr > >> + /* patch with 'r1 = *(u64 *)(r1 + 0)' since for percpu data, >> + * bpf_mem_alloc() returns a ptr to the percpu data ptr. >> + */ >> + insn_buf[0] = BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, 0); > > here R1 is no longer a concern, since we check R1 only in check_helper_call. > >> + insn_buf[1] = *insn; >> + cnt = 2; >> + >> + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt); >> + if (!new_prog) >> + return -ENOMEM; >> + >> + delta += cnt - 1; >> + env->prog = prog = new_prog; >> + insn = new_prog->insnsi + i + delta; >> + goto patch_call_imm; >> + } >> + >> /* BPF_EMIT_CALL() assumptions in some of the map_gen_lookup >> * and other inlining handlers are currently limited to 64 bit >> * only. >> -- >> 2.34.1 >> >
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index f70f9ac884d2..e23480db37ec 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -480,6 +480,7 @@ struct bpf_insn_aux_data { bool zext_dst; /* this insn zero extends dst reg */ bool storage_get_func_atomic; /* bpf_*_storage_get() with atomic memory alloc */ bool is_iter_next; /* bpf_iter_<type>_next() kfunc call */ + bool percpu_ptr_prog_alloc; /* {this,per}_cpu_ptr() with prog alloc */ u8 alu_state; /* used in combination with alu_limit */ /* below fields are initialized once */ diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index a985fbf18a11..6fc200cb68b6 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -6221,7 +6221,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, } if (type_is_alloc(reg->type) && !type_is_non_owning_ref(reg->type) && - !reg->ref_obj_id) { + !(reg->type & MEM_RCU) && !reg->ref_obj_id) { verbose(env, "verifier internal error: ref_obj_id for allocated object must be non-zero\n"); return -EFAULT; } @@ -7765,6 +7765,7 @@ static const struct bpf_reg_types btf_ptr_types = { static const struct bpf_reg_types percpu_btf_ptr_types = { .types = { PTR_TO_BTF_ID | MEM_PERCPU, + PTR_TO_BTF_ID | MEM_PERCPU | MEM_RCU, PTR_TO_BTF_ID | MEM_PERCPU | PTR_TRUSTED, } }; @@ -7945,6 +7946,7 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, return -EACCES; break; case PTR_TO_BTF_ID | MEM_PERCPU: + case PTR_TO_BTF_ID | MEM_PERCPU | MEM_RCU: case PTR_TO_BTF_ID | MEM_PERCPU | PTR_TRUSTED: /* Handled by helper specific checks */ break; @@ -8287,6 +8289,16 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, verbose(env, "Helper has invalid btf_id in R%d\n", regno); return -EACCES; } + if (reg->type & MEM_RCU) { + const struct btf_type *type = btf_type_by_id(reg->btf, reg->btf_id); + + if (!type || !btf_type_is_struct(type)) { + verbose(env, "Helper has invalid btf/btf_id in R%d\n", regno); + return -EFAULT; + } + env->insn_aux_data[insn_idx].percpu_ptr_prog_alloc = true; + } + meta->ret_btf = reg->btf; meta->ret_btf_id = reg->btf_id; break; @@ -9888,14 +9900,18 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn regs[BPF_REG_0].type = PTR_TO_MEM | ret_flag; regs[BPF_REG_0].mem_size = tsize; } else { - /* MEM_RDONLY may be carried from ret_flag, but it - * doesn't apply on PTR_TO_BTF_ID. Fold it, otherwise - * it will confuse the check of PTR_TO_BTF_ID in - * check_mem_access(). - */ - ret_flag &= ~MEM_RDONLY; + if (env->insn_aux_data[insn_idx].percpu_ptr_prog_alloc) { + regs[BPF_REG_0].type = PTR_TO_BTF_ID | MEM_ALLOC | MEM_RCU; + } else { + /* MEM_RDONLY may be carried from ret_flag, but it + * doesn't apply on PTR_TO_BTF_ID. Fold it, otherwise + * it will confuse the check of PTR_TO_BTF_ID in + * check_mem_access(). + */ + ret_flag &= ~MEM_RDONLY; + regs[BPF_REG_0].type = PTR_TO_BTF_ID | ret_flag; + } - regs[BPF_REG_0].type = PTR_TO_BTF_ID | ret_flag; regs[BPF_REG_0].btf = meta.ret_btf; regs[BPF_REG_0].btf_id = meta.ret_btf_id; } @@ -18646,6 +18662,25 @@ static int do_misc_fixups(struct bpf_verifier_env *env) goto patch_call_imm; } + /* bpf_per_cpu_ptr() and bpf_this_cpu_ptr() */ + if (env->insn_aux_data[i + delta].percpu_ptr_prog_alloc) { + /* patch with 'r1 = *(u64 *)(r1 + 0)' since for percpu data, + * bpf_mem_alloc() returns a ptr to the percpu data ptr. + */ + insn_buf[0] = BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, 0); + insn_buf[1] = *insn; + cnt = 2; + + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt); + if (!new_prog) + return -ENOMEM; + + delta += cnt - 1; + env->prog = prog = new_prog; + insn = new_prog->insnsi + i + delta; + goto patch_call_imm; + } + /* BPF_EMIT_CALL() assumptions in some of the map_gen_lookup * and other inlining handlers are currently limited to 64 bit * only.
The bpf helpers bpf_this_cpu_ptr() and bpf_per_cpu_ptr() are re-purposed for allocated percpu objects. For an allocated percpu obj, the reg type is 'PTR_TO_BTF_ID | MEM_PERCPU | MEM_RCU'. The return type for these two re-purposed helpera is 'PTR_TO_MEM | MEM_RCU | MEM_ALLOC'. The MEM_ALLOC allows that the per-cpu data can be read and written. Since the memory allocator bpf_mem_alloc() returns a ptr to a percpu ptr for percpu data, the first argument of bpf_this_cpu_ptr() and bpf_per_cpu_ptr() is patched with a dereference before passing to the helper func. Signed-off-by: Yonghong Song <yonghong.song@linux.dev> --- include/linux/bpf_verifier.h | 1 + kernel/bpf/verifier.c | 51 ++++++++++++++++++++++++++++++------ 2 files changed, 44 insertions(+), 8 deletions(-)