* [PATCH bpf-next 0/5] Add a snprintf eBPF helper
@ 2021-03-10 22:02 Florent Revest
2021-03-10 22:02 ` [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type Florent Revest
` (4 more replies)
0 siblings, 5 replies; 27+ messages in thread
From: Florent Revest @ 2021-03-10 22:02 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, yhs, kpsingh, jackmanb, linux-kernel,
Florent Revest
We have a usecase where we want to audit symbol names (if available) in
callback registration hooks. (ex: fentry/nf_register_net_hook)
A few months back, I proposed a bpf_kallsyms_lookup series but it was
decided in the reviews that a more generic helper, bpf_snprintf, would
be more useful.
This series implements the helper according to the feedback received in
https://lore.kernel.org/bpf/20201126165748.1748417-1-revest@google.com/T/#u
- A new arg type guarantees the NULL-termination of string arguments and
lets us pass format strings in only one arg
- A new helper is implemented using that guarantee. Because the format
string is known at verification time, the format string validation is
done by the verifier
- To implement a series of tests for bpf_snprintf, the logic for
marshalling variadic args in a fixed-size array is reworked as per:
https://lore.kernel.org/bpf/20210310015455.1095207-1-revest@chromium.org/T/#u
Florent Revest (5):
bpf: Add a ARG_PTR_TO_CONST_STR argument type
bpf: Add a bpf_snprintf helper
libbpf: Initialize the bpf_seq_printf parameters array field by field
libbpf: Introduce a BPF_SNPRINTF helper macro
selftests/bpf: Add a series of tests for bpf_snprintf
include/linux/bpf.h | 5 +
include/uapi/linux/bpf.h | 28 +++
kernel/bpf/verifier.c | 178 ++++++++++++++++++
kernel/trace/bpf_trace.c | 110 +++++++++++
tools/include/uapi/linux/bpf.h | 28 +++
tools/lib/bpf/bpf_tracing.h | 45 ++++-
.../selftests/bpf/prog_tests/snprintf.c | 71 +++++++
.../selftests/bpf/progs/test_snprintf.c | 71 +++++++
8 files changed, 535 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf.c
create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf.c
--
2.30.1.766.gb4fecdf3b7-goog
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type 2021-03-10 22:02 [PATCH bpf-next 0/5] Add a snprintf eBPF helper Florent Revest @ 2021-03-10 22:02 ` Florent Revest 2021-03-11 0:04 ` kernel test robot ` (2 more replies) 2021-03-10 22:02 ` [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper Florent Revest ` (3 subsequent siblings) 4 siblings, 3 replies; 27+ messages in thread From: Florent Revest @ 2021-03-10 22:02 UTC (permalink / raw) To: bpf Cc: ast, daniel, andrii, yhs, kpsingh, jackmanb, linux-kernel, Florent Revest This type provides the guarantee that an argument is going to be a const pointer to somewhere in a read-only map value. It also checks that this pointer is followed by a NULL character before the end of the map value. Signed-off-by: Florent Revest <revest@chromium.org> --- include/linux/bpf.h | 1 + kernel/bpf/verifier.c | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index a25730eaa148..7b5319d75b3e 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -308,6 +308,7 @@ enum bpf_arg_type { ARG_PTR_TO_PERCPU_BTF_ID, /* pointer to in-kernel percpu type */ ARG_PTR_TO_FUNC, /* pointer to a bpf program function */ ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */ + ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */ __BPF_ARG_TYPE_MAX, }; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index f9096b049cd6..c99b2b67dc8d 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4601,6 +4601,7 @@ static const struct bpf_reg_types spin_lock_types = { .types = { PTR_TO_MAP_VALU static const struct bpf_reg_types percpu_btf_ptr_types = { .types = { PTR_TO_PERCPU_BTF_ID } }; 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 } }; +static const struct bpf_reg_types const_str_ptr_types = { .types = { PTR_TO_MAP_VALUE } }; static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, @@ -4631,6 +4632,7 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { [ARG_PTR_TO_PERCPU_BTF_ID] = &percpu_btf_ptr_types, [ARG_PTR_TO_FUNC] = &func_ptr_types, [ARG_PTR_TO_STACK_OR_NULL] = &stack_ptr_types, + [ARG_PTR_TO_CONST_STR] = &const_str_ptr_types, }; static int check_reg_type(struct bpf_verifier_env *env, u32 regno, @@ -4881,6 +4883,45 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, if (err) return err; err = check_ptr_alignment(env, reg, 0, size, true); + } else if (arg_type == ARG_PTR_TO_CONST_STR) { + struct bpf_map *map = reg->map_ptr; + int map_off, i; + u64 map_addr; + char *map_ptr; + + if (!map || !bpf_map_is_rdonly(map)) { + verbose(env, "R%d does not point to a readonly map'\n", regno); + return -EACCES; + } + + if (!tnum_is_const(reg->var_off)) { + verbose(env, "R%d is not a constant address'\n", regno); + return -EACCES; + } + + if (!map->ops->map_direct_value_addr) { + verbose(env, "no direct value access support for this map type\n"); + return -EACCES; + } + + err = check_helper_mem_access(env, regno, + map->value_size - reg->off, + false, meta); + if (err) + return err; + + map_off = reg->off + reg->var_off.value; + err = map->ops->map_direct_value_addr(map, &map_addr, map_off); + if (err) + return err; + + map_ptr = (char *)(map_addr); + for (i = map_off; map_ptr[i] != '\0'; i++) { + if (i == map->value_size - 1) { + verbose(env, "map does not contain a NULL-terminated string\n"); + return -EACCES; + } + } } return err; -- 2.30.1.766.gb4fecdf3b7-goog ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type 2021-03-10 22:02 ` [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type Florent Revest @ 2021-03-11 0:04 ` kernel test robot 2021-03-11 1:00 ` kernel test robot 2021-03-16 1:03 ` Andrii Nakryiko 2 siblings, 0 replies; 27+ messages in thread From: kernel test robot @ 2021-03-11 0:04 UTC (permalink / raw) To: Florent Revest, bpf Cc: kbuild-all, ast, daniel, andrii, yhs, kpsingh, jackmanb, linux-kernel, Florent Revest [-- Attachment #1: Type: text/plain, Size: 15541 bytes --] Hi Florent, I love your patch! Perhaps something to improve: [auto build test WARNING on bpf-next/master] url: https://github.com/0day-ci/linux/commits/Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master config: openrisc-randconfig-r023-20210308 (attached as .config) compiler: or1k-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/cbb95ec99fafe0955aeada270c9be3d1477c3866 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306 git checkout cbb95ec99fafe0955aeada270c9be3d1477c3866 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): kernel/bpf/verifier.c: In function 'check_func_arg': >> kernel/bpf/verifier.c:4918:13: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] 4918 | map_ptr = (char *)(map_addr); | ^ In file included from include/linux/bpf_verifier.h:9, from kernel/bpf/verifier.c:12: kernel/bpf/verifier.c: In function 'jit_subprogs': include/linux/filter.h:363:4: warning: cast between incompatible function types from 'unsigned int (*)(const void *, const struct bpf_insn *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:11728:16: note: in expansion of macro 'BPF_CAST_CALL' 11728 | insn->imm = BPF_CAST_CALL(func[subprog]->bpf_func) - | ^~~~~~~~~~~~~ kernel/bpf/verifier.c: In function 'do_misc_fixups': include/linux/filter.h:363:4: warning: cast between incompatible function types from 'void * (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12136:17: note: in expansion of macro 'BPF_CAST_CALL' 12136 | insn->imm = BPF_CAST_CALL(ops->map_lookup_elem) - | ^~~~~~~~~~~~~ include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *, void *, u64)' {aka 'int (* const)(struct bpf_map *, void *, void *, long long unsigned int)'} to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12140:17: note: in expansion of macro 'BPF_CAST_CALL' 12140 | insn->imm = BPF_CAST_CALL(ops->map_update_elem) - | ^~~~~~~~~~~~~ include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12144:17: note: in expansion of macro 'BPF_CAST_CALL' 12144 | insn->imm = BPF_CAST_CALL(ops->map_delete_elem) - | ^~~~~~~~~~~~~ include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *, u64)' {aka 'int (* const)(struct bpf_map *, void *, long long unsigned int)'} to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12148:17: note: in expansion of macro 'BPF_CAST_CALL' 12148 | insn->imm = BPF_CAST_CALL(ops->map_push_elem) - | ^~~~~~~~~~~~~ include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12152:17: note: in expansion of macro 'BPF_CAST_CALL' 12152 | insn->imm = BPF_CAST_CALL(ops->map_pop_elem) - | ^~~~~~~~~~~~~ include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12156:17: note: in expansion of macro 'BPF_CAST_CALL' 12156 | insn->imm = BPF_CAST_CALL(ops->map_peek_elem) - | ^~~~~~~~~~~~~ include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, u32, u64)' {aka 'int (* const)(struct bpf_map *, unsigned int, long long unsigned int)'} to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12160:17: note: in expansion of macro 'BPF_CAST_CALL' 12160 | insn->imm = BPF_CAST_CALL(ops->map_redirect) - | ^~~~~~~~~~~~~ vim +4918 kernel/bpf/verifier.c 4695 4696 static int check_func_arg(struct bpf_verifier_env *env, u32 arg, 4697 struct bpf_call_arg_meta *meta, 4698 const struct bpf_func_proto *fn) 4699 { 4700 u32 regno = BPF_REG_1 + arg; 4701 struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; 4702 enum bpf_arg_type arg_type = fn->arg_type[arg]; 4703 enum bpf_reg_type type = reg->type; 4704 int err = 0; 4705 4706 if (arg_type == ARG_DONTCARE) 4707 return 0; 4708 4709 err = check_reg_arg(env, regno, SRC_OP); 4710 if (err) 4711 return err; 4712 4713 if (arg_type == ARG_ANYTHING) { 4714 if (is_pointer_value(env, regno)) { 4715 verbose(env, "R%d leaks addr into helper function\n", 4716 regno); 4717 return -EACCES; 4718 } 4719 return 0; 4720 } 4721 4722 if (type_is_pkt_pointer(type) && 4723 !may_access_direct_pkt_data(env, meta, BPF_READ)) { 4724 verbose(env, "helper access to the packet is not allowed\n"); 4725 return -EACCES; 4726 } 4727 4728 if (arg_type == ARG_PTR_TO_MAP_VALUE || 4729 arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE || 4730 arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) { 4731 err = resolve_map_arg_type(env, meta, &arg_type); 4732 if (err) 4733 return err; 4734 } 4735 4736 if (register_is_null(reg) && arg_type_may_be_null(arg_type)) 4737 /* A NULL register has a SCALAR_VALUE type, so skip 4738 * type checking. 4739 */ 4740 goto skip_type_check; 4741 4742 err = check_reg_type(env, regno, arg_type, fn->arg_btf_id[arg]); 4743 if (err) 4744 return err; 4745 4746 if (type == PTR_TO_CTX) { 4747 err = check_ctx_reg(env, reg, regno); 4748 if (err < 0) 4749 return err; 4750 } 4751 4752 skip_type_check: 4753 if (reg->ref_obj_id) { 4754 if (meta->ref_obj_id) { 4755 verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n", 4756 regno, reg->ref_obj_id, 4757 meta->ref_obj_id); 4758 return -EFAULT; 4759 } 4760 meta->ref_obj_id = reg->ref_obj_id; 4761 } 4762 4763 if (arg_type == ARG_CONST_MAP_PTR) { 4764 /* bpf_map_xxx(map_ptr) call: remember that map_ptr */ 4765 meta->map_ptr = reg->map_ptr; 4766 } else if (arg_type == ARG_PTR_TO_MAP_KEY) { 4767 /* bpf_map_xxx(..., map_ptr, ..., key) call: 4768 * check that [key, key + map->key_size) are within 4769 * stack limits and initialized 4770 */ 4771 if (!meta->map_ptr) { 4772 /* in function declaration map_ptr must come before 4773 * map_key, so that it's verified and known before 4774 * we have to check map_key here. Otherwise it means 4775 * that kernel subsystem misconfigured verifier 4776 */ 4777 verbose(env, "invalid map_ptr to access map->key\n"); 4778 return -EACCES; 4779 } 4780 err = check_helper_mem_access(env, regno, 4781 meta->map_ptr->key_size, false, 4782 NULL); 4783 } else if (arg_type == ARG_PTR_TO_MAP_VALUE || 4784 (arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL && 4785 !register_is_null(reg)) || 4786 arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) { 4787 /* bpf_map_xxx(..., map_ptr, ..., value) call: 4788 * check [value, value + map->value_size) validity 4789 */ 4790 if (!meta->map_ptr) { 4791 /* kernel subsystem misconfigured verifier */ 4792 verbose(env, "invalid map_ptr to access map->value\n"); 4793 return -EACCES; 4794 } 4795 meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE); 4796 err = check_helper_mem_access(env, regno, 4797 meta->map_ptr->value_size, false, 4798 meta); 4799 } else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) { 4800 if (!reg->btf_id) { 4801 verbose(env, "Helper has invalid btf_id in R%d\n", regno); 4802 return -EACCES; 4803 } 4804 meta->ret_btf = reg->btf; 4805 meta->ret_btf_id = reg->btf_id; 4806 } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) { 4807 if (meta->func_id == BPF_FUNC_spin_lock) { 4808 if (process_spin_lock(env, regno, true)) 4809 return -EACCES; 4810 } else if (meta->func_id == BPF_FUNC_spin_unlock) { 4811 if (process_spin_lock(env, regno, false)) 4812 return -EACCES; 4813 } else { 4814 verbose(env, "verifier internal error\n"); 4815 return -EFAULT; 4816 } 4817 } else if (arg_type == ARG_PTR_TO_FUNC) { 4818 meta->subprogno = reg->subprogno; 4819 } else if (arg_type_is_mem_ptr(arg_type)) { 4820 /* The access to this pointer is only checked when we hit the 4821 * next is_mem_size argument below. 4822 */ 4823 meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MEM); 4824 } else if (arg_type_is_mem_size(arg_type)) { 4825 bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO); 4826 4827 /* This is used to refine r0 return value bounds for helpers 4828 * that enforce this value as an upper bound on return values. 4829 * See do_refine_retval_range() for helpers that can refine 4830 * the return value. C type of helper is u32 so we pull register 4831 * bound from umax_value however, if negative verifier errors 4832 * out. Only upper bounds can be learned because retval is an 4833 * int type and negative retvals are allowed. 4834 */ 4835 meta->msize_max_value = reg->umax_value; 4836 4837 /* The register is SCALAR_VALUE; the access check 4838 * happens using its boundaries. 4839 */ 4840 if (!tnum_is_const(reg->var_off)) 4841 /* For unprivileged variable accesses, disable raw 4842 * mode so that the program is required to 4843 * initialize all the memory that the helper could 4844 * just partially fill up. 4845 */ 4846 meta = NULL; 4847 4848 if (reg->smin_value < 0) { 4849 verbose(env, "R%d min value is negative, either use unsigned or 'var &= const'\n", 4850 regno); 4851 return -EACCES; 4852 } 4853 4854 if (reg->umin_value == 0) { 4855 err = check_helper_mem_access(env, regno - 1, 0, 4856 zero_size_allowed, 4857 meta); 4858 if (err) 4859 return err; 4860 } 4861 4862 if (reg->umax_value >= BPF_MAX_VAR_SIZ) { 4863 verbose(env, "R%d unbounded memory access, use 'var &= const' or 'if (var < const)'\n", 4864 regno); 4865 return -EACCES; 4866 } 4867 err = check_helper_mem_access(env, regno - 1, 4868 reg->umax_value, 4869 zero_size_allowed, meta); 4870 if (!err) 4871 err = mark_chain_precision(env, regno); 4872 } else if (arg_type_is_alloc_size(arg_type)) { 4873 if (!tnum_is_const(reg->var_off)) { 4874 verbose(env, "R%d is not a known constant'\n", 4875 regno); 4876 return -EACCES; 4877 } 4878 meta->mem_size = reg->var_off.value; 4879 } else if (arg_type_is_int_ptr(arg_type)) { 4880 int size = int_ptr_type_to_size(arg_type); 4881 4882 err = check_helper_mem_access(env, regno, size, false, meta); 4883 if (err) 4884 return err; 4885 err = check_ptr_alignment(env, reg, 0, size, true); 4886 } else if (arg_type == ARG_PTR_TO_CONST_STR) { 4887 struct bpf_map *map = reg->map_ptr; 4888 int map_off, i; 4889 u64 map_addr; 4890 char *map_ptr; 4891 4892 if (!map || !bpf_map_is_rdonly(map)) { 4893 verbose(env, "R%d does not point to a readonly map'\n", regno); 4894 return -EACCES; 4895 } 4896 4897 if (!tnum_is_const(reg->var_off)) { 4898 verbose(env, "R%d is not a constant address'\n", regno); 4899 return -EACCES; 4900 } 4901 4902 if (!map->ops->map_direct_value_addr) { 4903 verbose(env, "no direct value access support for this map type\n"); 4904 return -EACCES; 4905 } 4906 4907 err = check_helper_mem_access(env, regno, 4908 map->value_size - reg->off, 4909 false, meta); 4910 if (err) 4911 return err; 4912 4913 map_off = reg->off + reg->var_off.value; 4914 err = map->ops->map_direct_value_addr(map, &map_addr, map_off); 4915 if (err) 4916 return err; 4917 > 4918 map_ptr = (char *)(map_addr); 4919 for (i = map_off; map_ptr[i] != '\0'; i++) { 4920 if (i == map->value_size - 1) { 4921 verbose(env, "map does not contain a NULL-terminated string\n"); 4922 return -EACCES; 4923 } 4924 } 4925 } 4926 4927 return err; 4928 } 4929 --- 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: 29308 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type 2021-03-10 22:02 ` [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type Florent Revest 2021-03-11 0:04 ` kernel test robot @ 2021-03-11 1:00 ` kernel test robot 2021-03-16 1:03 ` Andrii Nakryiko 2 siblings, 0 replies; 27+ messages in thread From: kernel test robot @ 2021-03-11 1:00 UTC (permalink / raw) To: Florent Revest, bpf Cc: kbuild-all, ast, daniel, andrii, yhs, kpsingh, jackmanb, linux-kernel, Florent Revest [-- Attachment #1: Type: text/plain, Size: 10816 bytes --] Hi Florent, I love your patch! Perhaps something to improve: [auto build test WARNING on bpf-next/master] url: https://github.com/0day-ci/linux/commits/Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master config: i386-randconfig-s001-20210308 (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.3-262-g5e674421-dirty # https://github.com/0day-ci/linux/commit/cbb95ec99fafe0955aeada270c9be3d1477c3866 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306 git checkout cbb95ec99fafe0955aeada270c9be3d1477c3866 # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> "sparse warnings: (new ones prefixed by >>)" kernel/bpf/verifier.c:11728:76: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12136:81: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12140:81: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12144:81: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12148:79: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12152:78: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12156:79: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12160:78: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12204:38: sparse: sparse: subtraction of functions? Share your drugs >> kernel/bpf/verifier.c:4918:36: sparse: sparse: non size-preserving integer to pointer cast vim +4918 kernel/bpf/verifier.c 4695 4696 static int check_func_arg(struct bpf_verifier_env *env, u32 arg, 4697 struct bpf_call_arg_meta *meta, 4698 const struct bpf_func_proto *fn) 4699 { 4700 u32 regno = BPF_REG_1 + arg; 4701 struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; 4702 enum bpf_arg_type arg_type = fn->arg_type[arg]; 4703 enum bpf_reg_type type = reg->type; 4704 int err = 0; 4705 4706 if (arg_type == ARG_DONTCARE) 4707 return 0; 4708 4709 err = check_reg_arg(env, regno, SRC_OP); 4710 if (err) 4711 return err; 4712 4713 if (arg_type == ARG_ANYTHING) { 4714 if (is_pointer_value(env, regno)) { 4715 verbose(env, "R%d leaks addr into helper function\n", 4716 regno); 4717 return -EACCES; 4718 } 4719 return 0; 4720 } 4721 4722 if (type_is_pkt_pointer(type) && 4723 !may_access_direct_pkt_data(env, meta, BPF_READ)) { 4724 verbose(env, "helper access to the packet is not allowed\n"); 4725 return -EACCES; 4726 } 4727 4728 if (arg_type == ARG_PTR_TO_MAP_VALUE || 4729 arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE || 4730 arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) { 4731 err = resolve_map_arg_type(env, meta, &arg_type); 4732 if (err) 4733 return err; 4734 } 4735 4736 if (register_is_null(reg) && arg_type_may_be_null(arg_type)) 4737 /* A NULL register has a SCALAR_VALUE type, so skip 4738 * type checking. 4739 */ 4740 goto skip_type_check; 4741 4742 err = check_reg_type(env, regno, arg_type, fn->arg_btf_id[arg]); 4743 if (err) 4744 return err; 4745 4746 if (type == PTR_TO_CTX) { 4747 err = check_ctx_reg(env, reg, regno); 4748 if (err < 0) 4749 return err; 4750 } 4751 4752 skip_type_check: 4753 if (reg->ref_obj_id) { 4754 if (meta->ref_obj_id) { 4755 verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n", 4756 regno, reg->ref_obj_id, 4757 meta->ref_obj_id); 4758 return -EFAULT; 4759 } 4760 meta->ref_obj_id = reg->ref_obj_id; 4761 } 4762 4763 if (arg_type == ARG_CONST_MAP_PTR) { 4764 /* bpf_map_xxx(map_ptr) call: remember that map_ptr */ 4765 meta->map_ptr = reg->map_ptr; 4766 } else if (arg_type == ARG_PTR_TO_MAP_KEY) { 4767 /* bpf_map_xxx(..., map_ptr, ..., key) call: 4768 * check that [key, key + map->key_size) are within 4769 * stack limits and initialized 4770 */ 4771 if (!meta->map_ptr) { 4772 /* in function declaration map_ptr must come before 4773 * map_key, so that it's verified and known before 4774 * we have to check map_key here. Otherwise it means 4775 * that kernel subsystem misconfigured verifier 4776 */ 4777 verbose(env, "invalid map_ptr to access map->key\n"); 4778 return -EACCES; 4779 } 4780 err = check_helper_mem_access(env, regno, 4781 meta->map_ptr->key_size, false, 4782 NULL); 4783 } else if (arg_type == ARG_PTR_TO_MAP_VALUE || 4784 (arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL && 4785 !register_is_null(reg)) || 4786 arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) { 4787 /* bpf_map_xxx(..., map_ptr, ..., value) call: 4788 * check [value, value + map->value_size) validity 4789 */ 4790 if (!meta->map_ptr) { 4791 /* kernel subsystem misconfigured verifier */ 4792 verbose(env, "invalid map_ptr to access map->value\n"); 4793 return -EACCES; 4794 } 4795 meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE); 4796 err = check_helper_mem_access(env, regno, 4797 meta->map_ptr->value_size, false, 4798 meta); 4799 } else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) { 4800 if (!reg->btf_id) { 4801 verbose(env, "Helper has invalid btf_id in R%d\n", regno); 4802 return -EACCES; 4803 } 4804 meta->ret_btf = reg->btf; 4805 meta->ret_btf_id = reg->btf_id; 4806 } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) { 4807 if (meta->func_id == BPF_FUNC_spin_lock) { 4808 if (process_spin_lock(env, regno, true)) 4809 return -EACCES; 4810 } else if (meta->func_id == BPF_FUNC_spin_unlock) { 4811 if (process_spin_lock(env, regno, false)) 4812 return -EACCES; 4813 } else { 4814 verbose(env, "verifier internal error\n"); 4815 return -EFAULT; 4816 } 4817 } else if (arg_type == ARG_PTR_TO_FUNC) { 4818 meta->subprogno = reg->subprogno; 4819 } else if (arg_type_is_mem_ptr(arg_type)) { 4820 /* The access to this pointer is only checked when we hit the 4821 * next is_mem_size argument below. 4822 */ 4823 meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MEM); 4824 } else if (arg_type_is_mem_size(arg_type)) { 4825 bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO); 4826 4827 /* This is used to refine r0 return value bounds for helpers 4828 * that enforce this value as an upper bound on return values. 4829 * See do_refine_retval_range() for helpers that can refine 4830 * the return value. C type of helper is u32 so we pull register 4831 * bound from umax_value however, if negative verifier errors 4832 * out. Only upper bounds can be learned because retval is an 4833 * int type and negative retvals are allowed. 4834 */ 4835 meta->msize_max_value = reg->umax_value; 4836 4837 /* The register is SCALAR_VALUE; the access check 4838 * happens using its boundaries. 4839 */ 4840 if (!tnum_is_const(reg->var_off)) 4841 /* For unprivileged variable accesses, disable raw 4842 * mode so that the program is required to 4843 * initialize all the memory that the helper could 4844 * just partially fill up. 4845 */ 4846 meta = NULL; 4847 4848 if (reg->smin_value < 0) { 4849 verbose(env, "R%d min value is negative, either use unsigned or 'var &= const'\n", 4850 regno); 4851 return -EACCES; 4852 } 4853 4854 if (reg->umin_value == 0) { 4855 err = check_helper_mem_access(env, regno - 1, 0, 4856 zero_size_allowed, 4857 meta); 4858 if (err) 4859 return err; 4860 } 4861 4862 if (reg->umax_value >= BPF_MAX_VAR_SIZ) { 4863 verbose(env, "R%d unbounded memory access, use 'var &= const' or 'if (var < const)'\n", 4864 regno); 4865 return -EACCES; 4866 } 4867 err = check_helper_mem_access(env, regno - 1, 4868 reg->umax_value, 4869 zero_size_allowed, meta); 4870 if (!err) 4871 err = mark_chain_precision(env, regno); 4872 } else if (arg_type_is_alloc_size(arg_type)) { 4873 if (!tnum_is_const(reg->var_off)) { 4874 verbose(env, "R%d is not a known constant'\n", 4875 regno); 4876 return -EACCES; 4877 } 4878 meta->mem_size = reg->var_off.value; 4879 } else if (arg_type_is_int_ptr(arg_type)) { 4880 int size = int_ptr_type_to_size(arg_type); 4881 4882 err = check_helper_mem_access(env, regno, size, false, meta); 4883 if (err) 4884 return err; 4885 err = check_ptr_alignment(env, reg, 0, size, true); 4886 } else if (arg_type == ARG_PTR_TO_CONST_STR) { 4887 struct bpf_map *map = reg->map_ptr; 4888 int map_off, i; 4889 u64 map_addr; 4890 char *map_ptr; 4891 4892 if (!map || !bpf_map_is_rdonly(map)) { 4893 verbose(env, "R%d does not point to a readonly map'\n", regno); 4894 return -EACCES; 4895 } 4896 4897 if (!tnum_is_const(reg->var_off)) { 4898 verbose(env, "R%d is not a constant address'\n", regno); 4899 return -EACCES; 4900 } 4901 4902 if (!map->ops->map_direct_value_addr) { 4903 verbose(env, "no direct value access support for this map type\n"); 4904 return -EACCES; 4905 } 4906 4907 err = check_helper_mem_access(env, regno, 4908 map->value_size - reg->off, 4909 false, meta); 4910 if (err) 4911 return err; 4912 4913 map_off = reg->off + reg->var_off.value; 4914 err = map->ops->map_direct_value_addr(map, &map_addr, map_off); 4915 if (err) 4916 return err; 4917 > 4918 map_ptr = (char *)(map_addr); 4919 for (i = map_off; map_ptr[i] != '\0'; i++) { 4920 if (i == map->value_size - 1) { 4921 verbose(env, "map does not contain a NULL-terminated string\n"); 4922 return -EACCES; 4923 } 4924 } 4925 } 4926 4927 return err; 4928 } 4929 --- 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: 40991 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type 2021-03-10 22:02 ` [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type Florent Revest 2021-03-11 0:04 ` kernel test robot 2021-03-11 1:00 ` kernel test robot @ 2021-03-16 1:03 ` Andrii Nakryiko 2021-03-16 23:58 ` Florent Revest 2 siblings, 1 reply; 27+ messages in thread From: Andrii Nakryiko @ 2021-03-16 1:03 UTC (permalink / raw) To: Florent Revest Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > This type provides the guarantee that an argument is going to be a const > pointer to somewhere in a read-only map value. It also checks that this > pointer is followed by a NULL character before the end of the map value. > > Signed-off-by: Florent Revest <revest@chromium.org> > --- > include/linux/bpf.h | 1 + > kernel/bpf/verifier.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 42 insertions(+) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index a25730eaa148..7b5319d75b3e 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -308,6 +308,7 @@ enum bpf_arg_type { > ARG_PTR_TO_PERCPU_BTF_ID, /* pointer to in-kernel percpu type */ > ARG_PTR_TO_FUNC, /* pointer to a bpf program function */ > ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */ > + ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */ > __BPF_ARG_TYPE_MAX, > }; > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index f9096b049cd6..c99b2b67dc8d 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -4601,6 +4601,7 @@ static const struct bpf_reg_types spin_lock_types = { .types = { PTR_TO_MAP_VALU > static const struct bpf_reg_types percpu_btf_ptr_types = { .types = { PTR_TO_PERCPU_BTF_ID } }; > 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 } }; > +static const struct bpf_reg_types const_str_ptr_types = { .types = { PTR_TO_MAP_VALUE } }; > > static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, > @@ -4631,6 +4632,7 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > [ARG_PTR_TO_PERCPU_BTF_ID] = &percpu_btf_ptr_types, > [ARG_PTR_TO_FUNC] = &func_ptr_types, > [ARG_PTR_TO_STACK_OR_NULL] = &stack_ptr_types, > + [ARG_PTR_TO_CONST_STR] = &const_str_ptr_types, > }; > > static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > @@ -4881,6 +4883,45 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > if (err) > return err; > err = check_ptr_alignment(env, reg, 0, size, true); > + } else if (arg_type == ARG_PTR_TO_CONST_STR) { > + struct bpf_map *map = reg->map_ptr; > + int map_off, i; > + u64 map_addr; > + char *map_ptr; > + > + if (!map || !bpf_map_is_rdonly(map)) { > + verbose(env, "R%d does not point to a readonly map'\n", regno); > + return -EACCES; > + } > + > + if (!tnum_is_const(reg->var_off)) { > + verbose(env, "R%d is not a constant address'\n", regno); > + return -EACCES; > + } > + > + if (!map->ops->map_direct_value_addr) { > + verbose(env, "no direct value access support for this map type\n"); > + return -EACCES; > + } > + > + err = check_helper_mem_access(env, regno, > + map->value_size - reg->off, > + false, meta); you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly use check_map_access(). And double-check that register is of expected type. just the presence of ref->map_ptr might not be sufficient? > + if (err) > + return err; > + > + map_off = reg->off + reg->var_off.value; > + err = map->ops->map_direct_value_addr(map, &map_addr, map_off); > + if (err) > + return err; > + > + map_ptr = (char *)(map_addr); map_ptr is a very confusing name. str_ptr or value ptr? > + for (i = map_off; map_ptr[i] != '\0'; i++) { > + if (i == map->value_size - 1) { use strnchr()? > + verbose(env, "map does not contain a NULL-terminated string\n"); map in the user-visible message is quite confusing, given that users will probably use this through static variables, so maybe just "string is not zero-terminated?" And it's not really a NULL, it's zero character. > + return -EACCES; > + } > + } > } > > return err; > -- > 2.30.1.766.gb4fecdf3b7-goog > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type 2021-03-16 1:03 ` Andrii Nakryiko @ 2021-03-16 23:58 ` Florent Revest 2021-03-17 0:35 ` Andrii Nakryiko 0 siblings, 1 reply; 27+ messages in thread From: Florent Revest @ 2021-03-16 23:58 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > + } else if (arg_type == ARG_PTR_TO_CONST_STR) { > > + struct bpf_map *map = reg->map_ptr; > > + int map_off, i; > > + u64 map_addr; > > + char *map_ptr; > > + > > + if (!map || !bpf_map_is_rdonly(map)) { > > + verbose(env, "R%d does not point to a readonly map'\n", regno); > > + return -EACCES; > > + } > > + > > + if (!tnum_is_const(reg->var_off)) { > > + verbose(env, "R%d is not a constant address'\n", regno); > > + return -EACCES; > > + } > > + > > + if (!map->ops->map_direct_value_addr) { > > + verbose(env, "no direct value access support for this map type\n"); > > + return -EACCES; > > + } > > + > > + err = check_helper_mem_access(env, regno, > > + map->value_size - reg->off, > > + false, meta); > > you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly > use check_map_access(). And double-check that register is of expected > type. just the presence of ref->map_ptr might not be sufficient? Sorry, just making sure I understand your comment correctly, are you suggesting that we: 1- skip the check_map_access_type() currently done by check_helper_mem_access()? or did you implicitly mean that we should call it as well next to check_map_access() ? 2- enforce (reg->type == PTR_TO_MAP_VALUE) even if currently guaranteed by compatible_reg_types, just to stay on the safe side ? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type 2021-03-16 23:58 ` Florent Revest @ 2021-03-17 0:35 ` Andrii Nakryiko 2021-03-17 0:45 ` Florent Revest 0 siblings, 1 reply; 27+ messages in thread From: Andrii Nakryiko @ 2021-03-17 0:35 UTC (permalink / raw) To: Florent Revest Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <revest@chromium.org> wrote: > > On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > > + } else if (arg_type == ARG_PTR_TO_CONST_STR) { > > > + struct bpf_map *map = reg->map_ptr; > > > + int map_off, i; > > > + u64 map_addr; > > > + char *map_ptr; > > > + > > > + if (!map || !bpf_map_is_rdonly(map)) { > > > + verbose(env, "R%d does not point to a readonly map'\n", regno); > > > + return -EACCES; > > > + } > > > + > > > + if (!tnum_is_const(reg->var_off)) { > > > + verbose(env, "R%d is not a constant address'\n", regno); > > > + return -EACCES; > > > + } > > > + > > > + if (!map->ops->map_direct_value_addr) { > > > + verbose(env, "no direct value access support for this map type\n"); > > > + return -EACCES; > > > + } > > > + > > > + err = check_helper_mem_access(env, regno, > > > + map->value_size - reg->off, > > > + false, meta); > > > > you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly > > use check_map_access(). And double-check that register is of expected > > type. just the presence of ref->map_ptr might not be sufficient? > > Sorry, just making sure I understand your comment correctly, are you > suggesting that we: > 1- skip the check_map_access_type() currently done by > check_helper_mem_access()? or did you implicitly mean that we should > call it as well next to check_map_access() ? check_helper_mem_access() will call check_map_access() for PTR_TO_MAP_VALUE and we expect only PTR_TO_MAP_VALUE, right? So why go through check_helper_mem_access() if we know we need check_map_access()? Less indirection, more explicit. So I meant "replace check_helper_mem_access() with check_map_access()". > 2- enforce (reg->type == PTR_TO_MAP_VALUE) even if currently > guaranteed by compatible_reg_types, just to stay on the safe side ? I can't follow compatible_reg_types :( If it does, then I guess it's fine without this check. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type 2021-03-17 0:35 ` Andrii Nakryiko @ 2021-03-17 0:45 ` Florent Revest 2021-03-17 1:02 ` Andrii Nakryiko 0 siblings, 1 reply; 27+ messages in thread From: Florent Revest @ 2021-03-17 0:45 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Wed, Mar 17, 2021 at 1:35 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <revest@chromium.org> wrote: > > On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > > > + } else if (arg_type == ARG_PTR_TO_CONST_STR) { > > > > + struct bpf_map *map = reg->map_ptr; > > > > + int map_off, i; > > > > + u64 map_addr; > > > > + char *map_ptr; > > > > + > > > > + if (!map || !bpf_map_is_rdonly(map)) { > > > > + verbose(env, "R%d does not point to a readonly map'\n", regno); > > > > + return -EACCES; > > > > + } > > > > + > > > > + if (!tnum_is_const(reg->var_off)) { > > > > + verbose(env, "R%d is not a constant address'\n", regno); > > > > + return -EACCES; > > > > + } > > > > + > > > > + if (!map->ops->map_direct_value_addr) { > > > > + verbose(env, "no direct value access support for this map type\n"); > > > > + return -EACCES; > > > > + } > > > > + > > > > + err = check_helper_mem_access(env, regno, > > > > + map->value_size - reg->off, > > > > + false, meta); > > > > > > you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly > > > use check_map_access(). And double-check that register is of expected > > > type. just the presence of ref->map_ptr might not be sufficient? > > > > Sorry, just making sure I understand your comment correctly, are you > > suggesting that we: > > 1- skip the check_map_access_type() currently done by > > check_helper_mem_access()? or did you implicitly mean that we should > > call it as well next to check_map_access() ? > > check_helper_mem_access() will call check_map_access() for > PTR_TO_MAP_VALUE and we expect only PTR_TO_MAP_VALUE, right? So why go > through check_helper_mem_access() if we know we need > check_map_access()? Less indirection, more explicit. So I meant > "replace check_helper_mem_access() with check_map_access()". Mhh I suspect there's still a misunderstanding, these function names are really confusing ahah. What about check_map_access*_type*. which is also called by check_helper_mem_access (before check_map_access): https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/verifier.c#n4329 Your message sounds like we should skip it so I was asking if that's what you also implicitly meant or if you missed it? > > 2- enforce (reg->type == PTR_TO_MAP_VALUE) even if currently > > guaranteed by compatible_reg_types, just to stay on the safe side ? > > I can't follow compatible_reg_types :( If it does, then I guess it's > fine without this check. It's alright, I can keep an extra check just for safety. :) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type 2021-03-17 0:45 ` Florent Revest @ 2021-03-17 1:02 ` Andrii Nakryiko 2021-03-17 10:32 ` Florent Revest 0 siblings, 1 reply; 27+ messages in thread From: Andrii Nakryiko @ 2021-03-17 1:02 UTC (permalink / raw) To: Florent Revest Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Tue, Mar 16, 2021 at 5:46 PM Florent Revest <revest@chromium.org> wrote: > > On Wed, Mar 17, 2021 at 1:35 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <revest@chromium.org> wrote: > > > On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko > > > <andrii.nakryiko@gmail.com> wrote: > > > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > > > > + } else if (arg_type == ARG_PTR_TO_CONST_STR) { > > > > > + struct bpf_map *map = reg->map_ptr; > > > > > + int map_off, i; > > > > > + u64 map_addr; > > > > > + char *map_ptr; > > > > > + > > > > > + if (!map || !bpf_map_is_rdonly(map)) { > > > > > + verbose(env, "R%d does not point to a readonly map'\n", regno); > > > > > + return -EACCES; > > > > > + } > > > > > + > > > > > + if (!tnum_is_const(reg->var_off)) { > > > > > + verbose(env, "R%d is not a constant address'\n", regno); > > > > > + return -EACCES; > > > > > + } > > > > > + > > > > > + if (!map->ops->map_direct_value_addr) { > > > > > + verbose(env, "no direct value access support for this map type\n"); > > > > > + return -EACCES; > > > > > + } > > > > > + > > > > > + err = check_helper_mem_access(env, regno, > > > > > + map->value_size - reg->off, > > > > > + false, meta); > > > > > > > > you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly > > > > use check_map_access(). And double-check that register is of expected > > > > type. just the presence of ref->map_ptr might not be sufficient? > > > > > > Sorry, just making sure I understand your comment correctly, are you > > > suggesting that we: > > > 1- skip the check_map_access_type() currently done by > > > check_helper_mem_access()? or did you implicitly mean that we should > > > call it as well next to check_map_access() ? > > > > check_helper_mem_access() will call check_map_access() for > > PTR_TO_MAP_VALUE and we expect only PTR_TO_MAP_VALUE, right? So why go > > through check_helper_mem_access() if we know we need > > check_map_access()? Less indirection, more explicit. So I meant > > "replace check_helper_mem_access() with check_map_access()". > > Mhh I suspect there's still a misunderstanding, these function names > are really confusing ahah. > What about check_map_access*_type*. which is also called by > check_helper_mem_access (before check_map_access): > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/verifier.c#n4329 > > Your message sounds like we should skip it so I was asking if that's > what you also implicitly meant or if you missed it? ah, you meant READ/WRITE access? ok, let's keep check_helper_mem_access() then, never mind me > > > > 2- enforce (reg->type == PTR_TO_MAP_VALUE) even if currently > > > guaranteed by compatible_reg_types, just to stay on the safe side ? > > > > I can't follow compatible_reg_types :( If it does, then I guess it's > > fine without this check. > > It's alright, I can keep an extra check just for safety. :) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type 2021-03-17 1:02 ` Andrii Nakryiko @ 2021-03-17 10:32 ` Florent Revest 0 siblings, 0 replies; 27+ messages in thread From: Florent Revest @ 2021-03-17 10:32 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Wed, Mar 17, 2021 at 2:02 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > On Tue, Mar 16, 2021 at 5:46 PM Florent Revest <revest@chromium.org> wrote: > > On Wed, Mar 17, 2021 at 1:35 AM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <revest@chromium.org> wrote: > > > > On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko > > > > <andrii.nakryiko@gmail.com> wrote: > > > > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > > > > > + } else if (arg_type == ARG_PTR_TO_CONST_STR) { > > > > > > + struct bpf_map *map = reg->map_ptr; > > > > > > + int map_off, i; > > > > > > + u64 map_addr; > > > > > > + char *map_ptr; > > > > > > + > > > > > > + if (!map || !bpf_map_is_rdonly(map)) { > > > > > > + verbose(env, "R%d does not point to a readonly map'\n", regno); > > > > > > + return -EACCES; > > > > > > + } > > > > > > + > > > > > > + if (!tnum_is_const(reg->var_off)) { > > > > > > + verbose(env, "R%d is not a constant address'\n", regno); > > > > > > + return -EACCES; > > > > > > + } > > > > > > + > > > > > > + if (!map->ops->map_direct_value_addr) { > > > > > > + verbose(env, "no direct value access support for this map type\n"); > > > > > > + return -EACCES; > > > > > > + } > > > > > > + > > > > > > + err = check_helper_mem_access(env, regno, > > > > > > + map->value_size - reg->off, > > > > > > + false, meta); > > > > > > > > > > you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly > > > > > use check_map_access(). And double-check that register is of expected > > > > > type. just the presence of ref->map_ptr might not be sufficient? > > > > > > > > Sorry, just making sure I understand your comment correctly, are you > > > > suggesting that we: > > > > 1- skip the check_map_access_type() currently done by > > > > check_helper_mem_access()? or did you implicitly mean that we should > > > > call it as well next to check_map_access() ? > > > > > > check_helper_mem_access() will call check_map_access() for > > > PTR_TO_MAP_VALUE and we expect only PTR_TO_MAP_VALUE, right? So why go > > > through check_helper_mem_access() if we know we need > > > check_map_access()? Less indirection, more explicit. So I meant > > > "replace check_helper_mem_access() with check_map_access()". > > > > Mhh I suspect there's still a misunderstanding, these function names > > are really confusing ahah. > > What about check_map_access*_type*. which is also called by > > check_helper_mem_access (before check_map_access): > > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/verifier.c#n4329 > > > > Your message sounds like we should skip it so I was asking if that's > > what you also implicitly meant or if you missed it? > > ah, you meant READ/WRITE access? ok, let's keep > check_helper_mem_access() then, never mind me Ah cool, then we are on the same page :) ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper 2021-03-10 22:02 [PATCH bpf-next 0/5] Add a snprintf eBPF helper Florent Revest 2021-03-10 22:02 ` [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type Florent Revest @ 2021-03-10 22:02 ` Florent Revest 2021-03-11 0:14 ` kernel test robot ` (4 more replies) 2021-03-10 22:02 ` [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field Florent Revest ` (2 subsequent siblings) 4 siblings, 5 replies; 27+ messages in thread From: Florent Revest @ 2021-03-10 22:02 UTC (permalink / raw) To: bpf Cc: ast, daniel, andrii, yhs, kpsingh, jackmanb, linux-kernel, Florent Revest The implementation takes inspiration from the existing bpf_trace_printk helper but there are a few differences: To allow for a large number of format-specifiers, parameters are provided in an array, like in bpf_seq_printf. Because the output string takes two arguments and the array of parameters also takes two arguments, the format string needs to fit in one argument. But because ARG_PTR_TO_CONST_STR guarantees to point to a NULL-terminated read-only map, we don't need a format string length arg. Because the format-string is known at verification time, we also move most of the format string validation, currently done in formatting helper calls, into the verifier logic. This makes debugging easier and also slightly improves the runtime performance. Signed-off-by: Florent Revest <revest@chromium.org> --- include/linux/bpf.h | 4 + include/uapi/linux/bpf.h | 28 +++++++ kernel/bpf/verifier.c | 137 +++++++++++++++++++++++++++++++++ kernel/trace/bpf_trace.c | 110 ++++++++++++++++++++++++++ tools/include/uapi/linux/bpf.h | 28 +++++++ 5 files changed, 307 insertions(+) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 7b5319d75b3e..d78175c9a887 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1902,6 +1902,10 @@ extern const struct bpf_func_proto bpf_task_storage_get_proto; extern const struct bpf_func_proto bpf_task_storage_delete_proto; extern const struct bpf_func_proto bpf_for_each_map_elem_proto; +#define MAX_SNPRINTF_VARARGS 12 +#define MAX_SNPRINTF_MEMCPY 6 +#define MAX_SNPRINTF_STR_LEN 128 + const struct bpf_func_proto *bpf_tracing_func_proto( enum bpf_func_id func_id, const struct bpf_prog *prog); diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 2d3036e292a9..3cbdc8ae00e7 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -4660,6 +4660,33 @@ union bpf_attr { * Return * The number of traversed map elements for success, **-EINVAL** for * invalid **flags**. + * + * long bpf_snprintf(char *out, u32 out_size, const char *fmt, u64 *data, u32 data_len) + * Description + * Outputs a string into the **out** buffer of size **out_size** + * based on a format string stored in a read-only map pointed by + * **fmt**. + * + * Each format specifier in **fmt** corresponds to one u64 element + * in the **data** array. For strings and pointers where pointees + * are accessed, only the pointer values are stored in the *data* + * array. The *data_len* is the size of *data* in bytes. + * + * Formats **%s** and **%p{i,I}{4,6}** require to read kernel + * memory. Reading kernel memory may fail due to either invalid + * address or valid address but requiring a major memory fault. If + * reading kernel memory fails, the string for **%s** will be an + * empty string, and the ip address for **%p{i,I}{4,6}** will be 0. + * Not returning error to bpf program is consistent with what + * **bpf_trace_printk**\ () does for now. + * + * Return + * The strictly positive length of the printed string, including + * the trailing NUL character. If the return value is greater than + * **out_size**, **out** contains a truncated string, without a + * trailing NULL character. + * + * Or **-EBUSY** if the per-CPU memory copy buffer is busy. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -4827,6 +4854,7 @@ union bpf_attr { FN(sock_from_file), \ FN(check_mtu), \ FN(for_each_map_elem), \ + FN(snprintf), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index c99b2b67dc8d..3ab549df817b 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5732,6 +5732,137 @@ static int check_reference_leak(struct bpf_verifier_env *env) return state->acquired_refs ? -EINVAL : 0; } +int check_bpf_snprintf_call(struct bpf_verifier_env *env, + struct bpf_reg_state *regs) +{ + struct bpf_reg_state *fmt_reg = ®s[BPF_REG_3]; + struct bpf_reg_state *data_len_reg = ®s[BPF_REG_5]; + struct bpf_map *fmt_map = fmt_reg->map_ptr; + int err, fmt_map_off, i, fmt_cnt = 0, memcpy_cnt = 0, num_args; + u64 fmt_addr; + char *fmt; + + /* data must be an array of u64 so data_len must be a multiple of 8 */ + if (data_len_reg->var_off.value & 7) + return -EINVAL; + num_args = data_len_reg->var_off.value / 8; + + /* fmt being ARG_PTR_TO_CONST_STR guarantees that var_off is const + * and map_direct_value_addr is set. + */ + fmt_map_off = fmt_reg->off + fmt_reg->var_off.value; + err = fmt_map->ops->map_direct_value_addr(fmt_map, &fmt_addr, + fmt_map_off); + if (err) + return err; + fmt = (char *)fmt_addr; + + /* We are also guaranteed that fmt+fmt_map_off is NULL terminated, we + * can focus on validating the format specifiers. + */ + for (i = fmt_map_off; fmt[i] != '\0'; i++) { + if ((!isprint(fmt[i]) && !isspace(fmt[i])) || + !isascii(fmt[i])) { + verbose(env, "only printable ascii for now\n"); + return -EINVAL; + } + + if (fmt[i] != '%') + continue; + + if (fmt[i + 1] == '%') { + i++; + continue; + } + + if (fmt_cnt >= MAX_SNPRINTF_VARARGS) { + verbose(env, "too many format specifiers\n"); + return -E2BIG; + } + + if (fmt_cnt >= num_args) { + verbose(env, "not enough parameters to print\n"); + return -EINVAL; + } + + /* fmt[i] != 0 && fmt[last] == 0, so we can access fmt[i + 1] */ + i++; + + /* skip optional "[0 +-][num]" width formating field */ + while (fmt[i] == '0' || fmt[i] == '+' || fmt[i] == '-' || + fmt[i] == ' ') + i++; + if (fmt[i] >= '1' && fmt[i] <= '9') { + i++; + while (fmt[i] >= '0' && fmt[i] <= '9') + i++; + } + + if (fmt[i] == 's') { + if (memcpy_cnt >= MAX_SNPRINTF_MEMCPY) { + verbose(env, "too many buffer copies\n"); + return -E2BIG; + } + + fmt_cnt++; + memcpy_cnt++; + continue; + } + + if (fmt[i] == 'p') { + if (fmt[i + 1] == 0 || fmt[i + 1] == 'K' || + fmt[i + 1] == 'x' || fmt[i + 1] == 'B' || + fmt[i + 1] == 's' || fmt[i + 1] == 'S') { + fmt_cnt++; + continue; + } + + /* only support "%pI4", "%pi4", "%pI6" and "%pi6". */ + if (fmt[i + 1] != 'i' && fmt[i + 1] != 'I') { + verbose(env, "invalid specifier %%p%c\n", + fmt[i+1]); + return -EINVAL; + } + if (fmt[i + 2] != '4' && fmt[i + 2] != '6') { + verbose(env, "invalid specifier %%p%c%c\n", + fmt[i+1], fmt[i+2]); + return -EINVAL; + } + + if (memcpy_cnt >= MAX_SNPRINTF_MEMCPY) { + verbose(env, "too many buffer copies\n"); + return -E2BIG; + } + + i += 2; + fmt_cnt++; + memcpy_cnt++; + continue; + } + + if (fmt[i] == 'l') { + i++; + if (fmt[i] == 'l') + i++; + } + + if (fmt[i] != 'i' && fmt[i] != 'd' && fmt[i] != 'u' && + fmt[i] != 'x' && fmt[i] != 'X') { + verbose(env, "invalid format specifier %%%c\n", fmt[i]); + return -EINVAL; + } + + fmt_cnt++; + } + + if (fmt_cnt != num_args) { + verbose(env, "too many parameters to print\n"); + return -EINVAL; + } + + return 0; +} + static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn, int *insn_idx_p) { @@ -5846,6 +5977,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn return -EINVAL; } + if (func_id == BPF_FUNC_snprintf) { + err = check_bpf_snprintf_call(env, regs); + if (err < 0) + return err; + } + /* reset caller saved regs */ for (i = 0; i < CALLER_SAVED_REGS; i++) { mark_reg_not_init(env, regs, caller_saved[i]); diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 0d23755c2747..7b80759c10a9 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -1271,6 +1271,114 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = { .arg5_type = ARG_ANYTHING, }; +struct bpf_snprintf_buf { + char buf[MAX_SNPRINTF_MEMCPY][MAX_SNPRINTF_STR_LEN]; +}; +static DEFINE_PER_CPU(struct bpf_snprintf_buf, bpf_snprintf_buf); +static DEFINE_PER_CPU(int, bpf_snprintf_buf_used); + +BPF_CALL_5(bpf_snprintf, char *, out, u32, out_size, char *, fmt, u64 *, args, + u32, args_len) +{ + int err, i, buf_used, copy_size, fmt_cnt = 0, memcpy_cnt = 0; + u64 params[MAX_SNPRINTF_VARARGS]; + struct bpf_snprintf_buf *bufs; + + buf_used = this_cpu_inc_return(bpf_snprintf_buf_used); + if (WARN_ON_ONCE(buf_used > 1)) { + err = -EBUSY; + goto out; + } + + bufs = this_cpu_ptr(&bpf_snprintf_buf); + + /* + * The verifier has already done most of the heavy-work for us in + * check_bpf_snprintf_call. We know that fmt is well formatted and that + * args_len is valid. The only task left is to convert some of the + * arguments. For the %s and %pi* specifiers, we need to read buffers + * from a kernel address during the helper call. + */ + for (i = 0; fmt[i] != '\0'; i++) { + if (fmt[i] != '%') + continue; + + if (fmt[i + 1] == '%') { + i++; + continue; + } + + /* fmt[i] != 0 && fmt[last] == 0, so we can access fmt[i + 1] */ + i++; + + /* skip optional "[0 +-][num]" width formating field */ + while (fmt[i] == '0' || fmt[i] == '+' || fmt[i] == '-' || + fmt[i] == ' ') + i++; + if (fmt[i] >= '1' && fmt[i] <= '9') { + i++; + while (fmt[i] >= '0' && fmt[i] <= '9') + i++; + } + + if (fmt[i] == 's') { + void *unsafe_ptr = (void *)(long)args[fmt_cnt]; + + err = strncpy_from_kernel_nofault(bufs->buf[memcpy_cnt], + unsafe_ptr, + MAX_SNPRINTF_STR_LEN); + if (err < 0) + bufs->buf[memcpy_cnt][0] = '\0'; + params[fmt_cnt] = (u64)(long)bufs->buf[memcpy_cnt]; + + fmt_cnt++; + memcpy_cnt++; + continue; + } + + if (fmt[i] == 'p' && (fmt[i + 1] == 'i' || fmt[i + 1] == 'I')) { + copy_size = (fmt[i + 2] == '4') ? 4 : 16; + + err = copy_from_kernel_nofault(bufs->buf[memcpy_cnt], + (void *) (long) args[fmt_cnt], + copy_size); + if (err < 0) + memset(bufs->buf[memcpy_cnt], 0, copy_size); + params[fmt_cnt] = (u64)(long)bufs->buf[memcpy_cnt]; + + i += 2; + fmt_cnt++; + memcpy_cnt++; + continue; + } + + params[fmt_cnt] = args[fmt_cnt]; + fmt_cnt++; + } + + /* Maximumly we can have MAX_SNPRINTF_VARARGS parameters, just give + * all of them to snprintf(). + */ + err = snprintf(out, out_size, fmt, params[0], params[1], params[2], + params[3], params[4], params[5], params[6], params[7], + params[8], params[9], params[10], params[11]) + 1; + +out: + this_cpu_dec(bpf_snprintf_buf_used); + return err; +} + +static const struct bpf_func_proto bpf_snprintf_proto = { + .func = bpf_snprintf, + .gpl_only = true, + .ret_type = RET_INTEGER, + .arg1_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_CONST_SIZE, + .arg3_type = ARG_PTR_TO_CONST_STR, + .arg4_type = ARG_PTR_TO_MEM, + .arg5_type = ARG_CONST_SIZE_OR_ZERO, +}; + const struct bpf_func_proto * bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) { @@ -1373,6 +1481,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_task_storage_delete_proto; case BPF_FUNC_for_each_map_elem: return &bpf_for_each_map_elem_proto; + case BPF_FUNC_snprintf: + return &bpf_snprintf_proto; default: return NULL; } diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 2d3036e292a9..3cbdc8ae00e7 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -4660,6 +4660,33 @@ union bpf_attr { * Return * The number of traversed map elements for success, **-EINVAL** for * invalid **flags**. + * + * long bpf_snprintf(char *out, u32 out_size, const char *fmt, u64 *data, u32 data_len) + * Description + * Outputs a string into the **out** buffer of size **out_size** + * based on a format string stored in a read-only map pointed by + * **fmt**. + * + * Each format specifier in **fmt** corresponds to one u64 element + * in the **data** array. For strings and pointers where pointees + * are accessed, only the pointer values are stored in the *data* + * array. The *data_len* is the size of *data* in bytes. + * + * Formats **%s** and **%p{i,I}{4,6}** require to read kernel + * memory. Reading kernel memory may fail due to either invalid + * address or valid address but requiring a major memory fault. If + * reading kernel memory fails, the string for **%s** will be an + * empty string, and the ip address for **%p{i,I}{4,6}** will be 0. + * Not returning error to bpf program is consistent with what + * **bpf_trace_printk**\ () does for now. + * + * Return + * The strictly positive length of the printed string, including + * the trailing NUL character. If the return value is greater than + * **out_size**, **out** contains a truncated string, without a + * trailing NULL character. + * + * Or **-EBUSY** if the per-CPU memory copy buffer is busy. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -4827,6 +4854,7 @@ union bpf_attr { FN(sock_from_file), \ FN(check_mtu), \ FN(for_each_map_elem), \ + FN(snprintf), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper -- 2.30.1.766.gb4fecdf3b7-goog ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper 2021-03-10 22:02 ` [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper Florent Revest @ 2021-03-11 0:14 ` kernel test robot 2021-03-11 3:12 ` kernel test robot ` (3 subsequent siblings) 4 siblings, 0 replies; 27+ messages in thread From: kernel test robot @ 2021-03-11 0:14 UTC (permalink / raw) To: Florent Revest, bpf Cc: kbuild-all, ast, daniel, andrii, yhs, kpsingh, jackmanb, linux-kernel, Florent Revest [-- Attachment #1: Type: text/plain, Size: 11462 bytes --] Hi Florent, I love your patch! Perhaps something to improve: [auto build test WARNING on bpf-next/master] url: https://github.com/0day-ci/linux/commits/Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master config: openrisc-randconfig-r023-20210308 (attached as .config) compiler: or1k-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/c06d419a45370b897b7383625f0435873a7458fa git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306 git checkout c06d419a45370b897b7383625f0435873a7458fa # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): kernel/bpf/verifier.c: In function 'check_func_arg': kernel/bpf/verifier.c:4918:13: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] 4918 | map_ptr = (char *)(map_addr); | ^ kernel/bpf/verifier.c: At top level: >> kernel/bpf/verifier.c:5735:5: warning: no previous prototype for 'check_bpf_snprintf_call' [-Wmissing-prototypes] 5735 | int check_bpf_snprintf_call(struct bpf_verifier_env *env, | ^~~~~~~~~~~~~~~~~~~~~~~ kernel/bpf/verifier.c: In function 'check_bpf_snprintf_call': kernel/bpf/verifier.c:5758:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] 5758 | fmt = (char *)fmt_addr; | ^ In file included from include/linux/bpf_verifier.h:9, from kernel/bpf/verifier.c:12: kernel/bpf/verifier.c: In function 'jit_subprogs': include/linux/filter.h:363:4: warning: cast between incompatible function types from 'unsigned int (*)(const void *, const struct bpf_insn *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:11865:16: note: in expansion of macro 'BPF_CAST_CALL' 11865 | insn->imm = BPF_CAST_CALL(func[subprog]->bpf_func) - | ^~~~~~~~~~~~~ kernel/bpf/verifier.c: In function 'do_misc_fixups': include/linux/filter.h:363:4: warning: cast between incompatible function types from 'void * (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12273:17: note: in expansion of macro 'BPF_CAST_CALL' 12273 | insn->imm = BPF_CAST_CALL(ops->map_lookup_elem) - | ^~~~~~~~~~~~~ include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *, void *, u64)' {aka 'int (* const)(struct bpf_map *, void *, void *, long long unsigned int)'} to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12277:17: note: in expansion of macro 'BPF_CAST_CALL' 12277 | insn->imm = BPF_CAST_CALL(ops->map_update_elem) - | ^~~~~~~~~~~~~ include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12281:17: note: in expansion of macro 'BPF_CAST_CALL' 12281 | insn->imm = BPF_CAST_CALL(ops->map_delete_elem) - | ^~~~~~~~~~~~~ include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *, u64)' {aka 'int (* const)(struct bpf_map *, void *, long long unsigned int)'} to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12285:17: note: in expansion of macro 'BPF_CAST_CALL' 12285 | insn->imm = BPF_CAST_CALL(ops->map_push_elem) - | ^~~~~~~~~~~~~ include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12289:17: note: in expansion of macro 'BPF_CAST_CALL' 12289 | insn->imm = BPF_CAST_CALL(ops->map_pop_elem) - | ^~~~~~~~~~~~~ include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12293:17: note: in expansion of macro 'BPF_CAST_CALL' 12293 | insn->imm = BPF_CAST_CALL(ops->map_peek_elem) - | ^~~~~~~~~~~~~ include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, u32, u64)' {aka 'int (* const)(struct bpf_map *, unsigned int, long long unsigned int)'} to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type] 363 | ((u64 (*)(u64, u64, u64, u64, u64))(x)) | ^ kernel/bpf/verifier.c:12297:17: note: in expansion of macro 'BPF_CAST_CALL' 12297 | insn->imm = BPF_CAST_CALL(ops->map_redirect) - | ^~~~~~~~~~~~~ vim +/check_bpf_snprintf_call +5735 kernel/bpf/verifier.c 5734 > 5735 int check_bpf_snprintf_call(struct bpf_verifier_env *env, 5736 struct bpf_reg_state *regs) 5737 { 5738 struct bpf_reg_state *fmt_reg = ®s[BPF_REG_3]; 5739 struct bpf_reg_state *data_len_reg = ®s[BPF_REG_5]; 5740 struct bpf_map *fmt_map = fmt_reg->map_ptr; 5741 int err, fmt_map_off, i, fmt_cnt = 0, memcpy_cnt = 0, num_args; 5742 u64 fmt_addr; 5743 char *fmt; 5744 5745 /* data must be an array of u64 so data_len must be a multiple of 8 */ 5746 if (data_len_reg->var_off.value & 7) 5747 return -EINVAL; 5748 num_args = data_len_reg->var_off.value / 8; 5749 5750 /* fmt being ARG_PTR_TO_CONST_STR guarantees that var_off is const 5751 * and map_direct_value_addr is set. 5752 */ 5753 fmt_map_off = fmt_reg->off + fmt_reg->var_off.value; 5754 err = fmt_map->ops->map_direct_value_addr(fmt_map, &fmt_addr, 5755 fmt_map_off); 5756 if (err) 5757 return err; 5758 fmt = (char *)fmt_addr; 5759 5760 /* We are also guaranteed that fmt+fmt_map_off is NULL terminated, we 5761 * can focus on validating the format specifiers. 5762 */ 5763 for (i = fmt_map_off; fmt[i] != '\0'; i++) { 5764 if ((!isprint(fmt[i]) && !isspace(fmt[i])) || 5765 !isascii(fmt[i])) { 5766 verbose(env, "only printable ascii for now\n"); 5767 return -EINVAL; 5768 } 5769 5770 if (fmt[i] != '%') 5771 continue; 5772 5773 if (fmt[i + 1] == '%') { 5774 i++; 5775 continue; 5776 } 5777 5778 if (fmt_cnt >= MAX_SNPRINTF_VARARGS) { 5779 verbose(env, "too many format specifiers\n"); 5780 return -E2BIG; 5781 } 5782 5783 if (fmt_cnt >= num_args) { 5784 verbose(env, "not enough parameters to print\n"); 5785 return -EINVAL; 5786 } 5787 5788 /* fmt[i] != 0 && fmt[last] == 0, so we can access fmt[i + 1] */ 5789 i++; 5790 5791 /* skip optional "[0 +-][num]" width formating field */ 5792 while (fmt[i] == '0' || fmt[i] == '+' || fmt[i] == '-' || 5793 fmt[i] == ' ') 5794 i++; 5795 if (fmt[i] >= '1' && fmt[i] <= '9') { 5796 i++; 5797 while (fmt[i] >= '0' && fmt[i] <= '9') 5798 i++; 5799 } 5800 5801 if (fmt[i] == 's') { 5802 if (memcpy_cnt >= MAX_SNPRINTF_MEMCPY) { 5803 verbose(env, "too many buffer copies\n"); 5804 return -E2BIG; 5805 } 5806 5807 fmt_cnt++; 5808 memcpy_cnt++; 5809 continue; 5810 } 5811 5812 if (fmt[i] == 'p') { 5813 if (fmt[i + 1] == 0 || fmt[i + 1] == 'K' || 5814 fmt[i + 1] == 'x' || fmt[i + 1] == 'B' || 5815 fmt[i + 1] == 's' || fmt[i + 1] == 'S') { 5816 fmt_cnt++; 5817 continue; 5818 } 5819 5820 /* only support "%pI4", "%pi4", "%pI6" and "%pi6". */ 5821 if (fmt[i + 1] != 'i' && fmt[i + 1] != 'I') { 5822 verbose(env, "invalid specifier %%p%c\n", 5823 fmt[i+1]); 5824 return -EINVAL; 5825 } 5826 if (fmt[i + 2] != '4' && fmt[i + 2] != '6') { 5827 verbose(env, "invalid specifier %%p%c%c\n", 5828 fmt[i+1], fmt[i+2]); 5829 return -EINVAL; 5830 } 5831 5832 if (memcpy_cnt >= MAX_SNPRINTF_MEMCPY) { 5833 verbose(env, "too many buffer copies\n"); 5834 return -E2BIG; 5835 } 5836 5837 i += 2; 5838 fmt_cnt++; 5839 memcpy_cnt++; 5840 continue; 5841 } 5842 5843 if (fmt[i] == 'l') { 5844 i++; 5845 if (fmt[i] == 'l') 5846 i++; 5847 } 5848 5849 if (fmt[i] != 'i' && fmt[i] != 'd' && fmt[i] != 'u' && 5850 fmt[i] != 'x' && fmt[i] != 'X') { 5851 verbose(env, "invalid format specifier %%%c\n", fmt[i]); 5852 return -EINVAL; 5853 } 5854 5855 fmt_cnt++; 5856 } 5857 5858 if (fmt_cnt != num_args) { 5859 verbose(env, "too many parameters to print\n"); 5860 return -EINVAL; 5861 } 5862 5863 return 0; 5864 } 5865 --- 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: 29308 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper 2021-03-10 22:02 ` [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper Florent Revest 2021-03-11 0:14 ` kernel test robot @ 2021-03-11 3:12 ` kernel test robot 2021-03-11 3:12 ` [RFC PATCH] bpf: check_bpf_snprintf_call() can be static kernel test robot ` (2 subsequent siblings) 4 siblings, 0 replies; 27+ messages in thread From: kernel test robot @ 2021-03-11 3:12 UTC (permalink / raw) To: Florent Revest, bpf Cc: kbuild-all, ast, daniel, andrii, yhs, kpsingh, jackmanb, linux-kernel, Florent Revest [-- Attachment #1: Type: text/plain, Size: 2277 bytes --] Hi Florent, I love your patch! Perhaps something to improve: [auto build test WARNING on bpf-next/master] url: https://github.com/0day-ci/linux/commits/Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master config: x86_64-randconfig-s022-20210310 (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.3-262-g5e674421-dirty # https://github.com/0day-ci/linux/commit/c06d419a45370b897b7383625f0435873a7458fa git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306 git checkout c06d419a45370b897b7383625f0435873a7458fa # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> "sparse warnings: (new ones prefixed by >>)" >> kernel/bpf/verifier.c:5735:5: sparse: sparse: symbol 'check_bpf_snprintf_call' was not declared. Should it be static? kernel/bpf/verifier.c:11865:76: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12273:81: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12277:81: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12281:81: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12285:79: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12289:78: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12293:79: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12297:78: sparse: sparse: subtraction of functions? Share your drugs kernel/bpf/verifier.c:12341:38: sparse: sparse: subtraction of functions? Share your drugs Please review and possibly fold the followup patch. --- 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: 34370 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* [RFC PATCH] bpf: check_bpf_snprintf_call() can be static 2021-03-10 22:02 ` [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper Florent Revest 2021-03-11 0:14 ` kernel test robot 2021-03-11 3:12 ` kernel test robot @ 2021-03-11 3:12 ` kernel test robot 2021-03-16 1:25 ` [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper Andrii Nakryiko 2021-03-23 3:21 ` Alexei Starovoitov 4 siblings, 0 replies; 27+ messages in thread From: kernel test robot @ 2021-03-11 3:12 UTC (permalink / raw) To: Florent Revest, bpf Cc: kbuild-all, ast, daniel, andrii, yhs, kpsingh, jackmanb, linux-kernel, Florent Revest Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: kernel test robot <lkp@intel.com> --- verifier.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 3ab549df817b6..06c868989852d 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5732,8 +5732,8 @@ static int check_reference_leak(struct bpf_verifier_env *env) return state->acquired_refs ? -EINVAL : 0; } -int check_bpf_snprintf_call(struct bpf_verifier_env *env, - struct bpf_reg_state *regs) +static int check_bpf_snprintf_call(struct bpf_verifier_env *env, + struct bpf_reg_state *regs) { struct bpf_reg_state *fmt_reg = ®s[BPF_REG_3]; struct bpf_reg_state *data_len_reg = ®s[BPF_REG_5]; ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper 2021-03-10 22:02 ` [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper Florent Revest ` (2 preceding siblings ...) 2021-03-11 3:12 ` [RFC PATCH] bpf: check_bpf_snprintf_call() can be static kernel test robot @ 2021-03-16 1:25 ` Andrii Nakryiko 2021-03-16 13:18 ` Florent Revest 2021-03-23 3:21 ` Alexei Starovoitov 4 siblings, 1 reply; 27+ messages in thread From: Andrii Nakryiko @ 2021-03-16 1:25 UTC (permalink / raw) To: Florent Revest Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > The implementation takes inspiration from the existing bpf_trace_printk > helper but there are a few differences: > > To allow for a large number of format-specifiers, parameters are > provided in an array, like in bpf_seq_printf. > > Because the output string takes two arguments and the array of > parameters also takes two arguments, the format string needs to fit in > one argument. But because ARG_PTR_TO_CONST_STR guarantees to point to a > NULL-terminated read-only map, we don't need a format string length arg. > > Because the format-string is known at verification time, we also move > most of the format string validation, currently done in formatting > helper calls, into the verifier logic. This makes debugging easier and > also slightly improves the runtime performance. > > Signed-off-by: Florent Revest <revest@chromium.org> > --- > include/linux/bpf.h | 4 + > include/uapi/linux/bpf.h | 28 +++++++ > kernel/bpf/verifier.c | 137 +++++++++++++++++++++++++++++++++ > kernel/trace/bpf_trace.c | 110 ++++++++++++++++++++++++++ > tools/include/uapi/linux/bpf.h | 28 +++++++ > 5 files changed, 307 insertions(+) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 7b5319d75b3e..d78175c9a887 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1902,6 +1902,10 @@ extern const struct bpf_func_proto bpf_task_storage_get_proto; > extern const struct bpf_func_proto bpf_task_storage_delete_proto; > extern const struct bpf_func_proto bpf_for_each_map_elem_proto; > > +#define MAX_SNPRINTF_VARARGS 12 > +#define MAX_SNPRINTF_MEMCPY 6 > +#define MAX_SNPRINTF_STR_LEN 128 > + > const struct bpf_func_proto *bpf_tracing_func_proto( > enum bpf_func_id func_id, const struct bpf_prog *prog); > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 2d3036e292a9..3cbdc8ae00e7 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -4660,6 +4660,33 @@ union bpf_attr { > * Return > * The number of traversed map elements for success, **-EINVAL** for > * invalid **flags**. > + * > + * long bpf_snprintf(char *out, u32 out_size, const char *fmt, u64 *data, u32 data_len) bpf_snprintf_btf calls out and out_size str and str_size, let's be consistent? > + * Description > + * Outputs a string into the **out** buffer of size **out_size** > + * based on a format string stored in a read-only map pointed by > + * **fmt**. > + * > + * Each format specifier in **fmt** corresponds to one u64 element > + * in the **data** array. For strings and pointers where pointees > + * are accessed, only the pointer values are stored in the *data* > + * array. The *data_len* is the size of *data* in bytes. > + * > + * Formats **%s** and **%p{i,I}{4,6}** require to read kernel > + * memory. Reading kernel memory may fail due to either invalid > + * address or valid address but requiring a major memory fault. If > + * reading kernel memory fails, the string for **%s** will be an > + * empty string, and the ip address for **%p{i,I}{4,6}** will be 0. > + * Not returning error to bpf program is consistent with what > + * **bpf_trace_printk**\ () does for now. > + * > + * Return > + * The strictly positive length of the printed string, including > + * the trailing NUL character. If the return value is greater than > + * **out_size**, **out** contains a truncated string, without a > + * trailing NULL character. this deviates from the behavior in other BPF helpers dealing with strings. and it's extremely inconvenient for users to get non-zero-terminated string. I think we should always zero-terminate. > + * > + * Or **-EBUSY** if the per-CPU memory copy buffer is busy. > */ > #define __BPF_FUNC_MAPPER(FN) \ > FN(unspec), \ > @@ -4827,6 +4854,7 @@ union bpf_attr { > FN(sock_from_file), \ > FN(check_mtu), \ > FN(for_each_map_elem), \ > + FN(snprintf), \ > /* */ > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index c99b2b67dc8d..3ab549df817b 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -5732,6 +5732,137 @@ static int check_reference_leak(struct bpf_verifier_env *env) > return state->acquired_refs ? -EINVAL : 0; > } > > +int check_bpf_snprintf_call(struct bpf_verifier_env *env, > + struct bpf_reg_state *regs) > +{ can we please extra the printf format string parsing/checking logic and re-use them across all functions? We now have at least 4 variants of it, it's not great to say the least. I hope it's possible to generalize it in such a way that the same function will parse the string, and will record each expected argument and it's type, with whatever extra flags we need to. That should make the printing part simpler as well, as it will just follow "directions" from the parsing part? Devil is in the details, of course :) But it's worthwhile to try at least. > + struct bpf_reg_state *fmt_reg = ®s[BPF_REG_3]; > + struct bpf_reg_state *data_len_reg = ®s[BPF_REG_5]; > + struct bpf_map *fmt_map = fmt_reg->map_ptr; > + int err, fmt_map_off, i, fmt_cnt = 0, memcpy_cnt = 0, num_args; > + u64 fmt_addr; > + char *fmt; > + > + /* data must be an array of u64 so data_len must be a multiple of 8 */ > + if (data_len_reg->var_off.value & 7) > + return -EINVAL; > + num_args = data_len_reg->var_off.value / 8; > + > + /* fmt being ARG_PTR_TO_CONST_STR guarantees that var_off is const > + * and map_direct_value_addr is set. > + */ > + fmt_map_off = fmt_reg->off + fmt_reg->var_off.value; > + err = fmt_map->ops->map_direct_value_addr(fmt_map, &fmt_addr, > + fmt_map_off); > + if (err) > + return err; > + fmt = (char *)fmt_addr; > + [...] not fun to read this part over and over :) > + } > + > + return 0; > +} > + > static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > int *insn_idx_p) > { > @@ -5846,6 +5977,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > return -EINVAL; > } > > + if (func_id == BPF_FUNC_snprintf) { > + err = check_bpf_snprintf_call(env, regs); > + if (err < 0) > + return err; > + } > + > /* reset caller saved regs */ > for (i = 0; i < CALLER_SAVED_REGS; i++) { > mark_reg_not_init(env, regs, caller_saved[i]); > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 0d23755c2747..7b80759c10a9 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -1271,6 +1271,114 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = { > .arg5_type = ARG_ANYTHING, > }; > > +struct bpf_snprintf_buf { > + char buf[MAX_SNPRINTF_MEMCPY][MAX_SNPRINTF_STR_LEN]; > +}; > +static DEFINE_PER_CPU(struct bpf_snprintf_buf, bpf_snprintf_buf); > +static DEFINE_PER_CPU(int, bpf_snprintf_buf_used); > + > +BPF_CALL_5(bpf_snprintf, char *, out, u32, out_size, char *, fmt, u64 *, args, > + u32, args_len) > +{ > + int err, i, buf_used, copy_size, fmt_cnt = 0, memcpy_cnt = 0; > + u64 params[MAX_SNPRINTF_VARARGS]; > + struct bpf_snprintf_buf *bufs; > + > + buf_used = this_cpu_inc_return(bpf_snprintf_buf_used); > + if (WARN_ON_ONCE(buf_used > 1)) { > + err = -EBUSY; > + goto out; > + } > + > + bufs = this_cpu_ptr(&bpf_snprintf_buf); > + > + /* > + * The verifier has already done most of the heavy-work for us in > + * check_bpf_snprintf_call. We know that fmt is well formatted and that > + * args_len is valid. The only task left is to convert some of the > + * arguments. For the %s and %pi* specifiers, we need to read buffers > + * from a kernel address during the helper call. > + */ > + for (i = 0; fmt[i] != '\0'; i++) { same function should hopefully be reused here > + } > + > + /* Maximumly we can have MAX_SNPRINTF_VARARGS parameters, just give > + * all of them to snprintf(). > + */ > + err = snprintf(out, out_size, fmt, params[0], params[1], params[2], > + params[3], params[4], params[5], params[6], params[7], > + params[8], params[9], params[10], params[11]) + 1; > + > +out: > + this_cpu_dec(bpf_snprintf_buf_used); > + return err; > +} > + > +static const struct bpf_func_proto bpf_snprintf_proto = { > + .func = bpf_snprintf, > + .gpl_only = true, > + .ret_type = RET_INTEGER, > + .arg1_type = ARG_PTR_TO_MEM, > + .arg2_type = ARG_CONST_SIZE, can we mark is CONST_SIZE_OR_ZERO and just do nothing on zero at runtime? I still have scars from having to deal (prove, actually) with ARG_CONST_SIZE (> 0) limitations in perf_event_output. No need to make anyone's life harder, if it's easy to just do something sensible on zero (i.e., do nothing, but emit desired amount of bytes). > + .arg3_type = ARG_PTR_TO_CONST_STR, > + .arg4_type = ARG_PTR_TO_MEM, > + .arg5_type = ARG_CONST_SIZE_OR_ZERO, > +}; > + > const struct bpf_func_proto * > bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > { > @@ -1373,6 +1481,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > return &bpf_task_storage_delete_proto; > case BPF_FUNC_for_each_map_elem: > return &bpf_for_each_map_elem_proto; > + case BPF_FUNC_snprintf: > + return &bpf_snprintf_proto; why just tracing? can't all BPF programs use this functionality? > default: > return NULL; > } > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > index 2d3036e292a9..3cbdc8ae00e7 100644 > --- a/tools/include/uapi/linux/bpf.h > +++ b/tools/include/uapi/linux/bpf.h > @@ -4660,6 +4660,33 @@ union bpf_attr { > * Return > * The number of traversed map elements for success, **-EINVAL** for > * invalid **flags**. > + * > + * long bpf_snprintf(char *out, u32 out_size, const char *fmt, u64 *data, u32 data_len) > + * Description > + * Outputs a string into the **out** buffer of size **out_size** > + * based on a format string stored in a read-only map pointed by > + * **fmt**. > + * > + * Each format specifier in **fmt** corresponds to one u64 element > + * in the **data** array. For strings and pointers where pointees > + * are accessed, only the pointer values are stored in the *data* > + * array. The *data_len* is the size of *data* in bytes. > + * > + * Formats **%s** and **%p{i,I}{4,6}** require to read kernel > + * memory. Reading kernel memory may fail due to either invalid > + * address or valid address but requiring a major memory fault. If > + * reading kernel memory fails, the string for **%s** will be an > + * empty string, and the ip address for **%p{i,I}{4,6}** will be 0. > + * Not returning error to bpf program is consistent with what > + * **bpf_trace_printk**\ () does for now. > + * > + * Return > + * The strictly positive length of the printed string, including > + * the trailing NUL character. If the return value is greater than > + * **out_size**, **out** contains a truncated string, without a > + * trailing NULL character. > + * > + * Or **-EBUSY** if the per-CPU memory copy buffer is busy. > */ > #define __BPF_FUNC_MAPPER(FN) \ > FN(unspec), \ > @@ -4827,6 +4854,7 @@ union bpf_attr { > FN(sock_from_file), \ > FN(check_mtu), \ > FN(for_each_map_elem), \ > + FN(snprintf), \ > /* */ > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper > -- > 2.30.1.766.gb4fecdf3b7-goog > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper 2021-03-16 1:25 ` [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper Andrii Nakryiko @ 2021-03-16 13:18 ` Florent Revest 0 siblings, 0 replies; 27+ messages in thread From: Florent Revest @ 2021-03-16 13:18 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Tue, Mar 16, 2021 at 2:25 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > > > The implementation takes inspiration from the existing bpf_trace_printk > > helper but there are a few differences: > > > > To allow for a large number of format-specifiers, parameters are > > provided in an array, like in bpf_seq_printf. > > > > Because the output string takes two arguments and the array of > > parameters also takes two arguments, the format string needs to fit in > > one argument. But because ARG_PTR_TO_CONST_STR guarantees to point to a > > NULL-terminated read-only map, we don't need a format string length arg. > > > > Because the format-string is known at verification time, we also move > > most of the format string validation, currently done in formatting > > helper calls, into the verifier logic. This makes debugging easier and > > also slightly improves the runtime performance. > > > > Signed-off-by: Florent Revest <revest@chromium.org> > > --- > > include/linux/bpf.h | 4 + > > include/uapi/linux/bpf.h | 28 +++++++ > > kernel/bpf/verifier.c | 137 +++++++++++++++++++++++++++++++++ > > kernel/trace/bpf_trace.c | 110 ++++++++++++++++++++++++++ > > tools/include/uapi/linux/bpf.h | 28 +++++++ > > 5 files changed, 307 insertions(+) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 7b5319d75b3e..d78175c9a887 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -1902,6 +1902,10 @@ extern const struct bpf_func_proto bpf_task_storage_get_proto; > > extern const struct bpf_func_proto bpf_task_storage_delete_proto; > > extern const struct bpf_func_proto bpf_for_each_map_elem_proto; > > > > +#define MAX_SNPRINTF_VARARGS 12 > > +#define MAX_SNPRINTF_MEMCPY 6 > > +#define MAX_SNPRINTF_STR_LEN 128 > > + > > const struct bpf_func_proto *bpf_tracing_func_proto( > > enum bpf_func_id func_id, const struct bpf_prog *prog); > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index 2d3036e292a9..3cbdc8ae00e7 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -4660,6 +4660,33 @@ union bpf_attr { > > * Return > > * The number of traversed map elements for success, **-EINVAL** for > > * invalid **flags**. > > + * > > + * long bpf_snprintf(char *out, u32 out_size, const char *fmt, u64 *data, u32 data_len) > > bpf_snprintf_btf calls out and out_size str and str_size, let's be consistent? > > > + * Description > > + * Outputs a string into the **out** buffer of size **out_size** > > + * based on a format string stored in a read-only map pointed by > > + * **fmt**. > > + * > > + * Each format specifier in **fmt** corresponds to one u64 element > > + * in the **data** array. For strings and pointers where pointees > > + * are accessed, only the pointer values are stored in the *data* > > + * array. The *data_len* is the size of *data* in bytes. > > + * > > + * Formats **%s** and **%p{i,I}{4,6}** require to read kernel > > + * memory. Reading kernel memory may fail due to either invalid > > + * address or valid address but requiring a major memory fault. If > > + * reading kernel memory fails, the string for **%s** will be an > > + * empty string, and the ip address for **%p{i,I}{4,6}** will be 0. > > + * Not returning error to bpf program is consistent with what > > + * **bpf_trace_printk**\ () does for now. > > + * > > + * Return > > + * The strictly positive length of the printed string, including > > + * the trailing NUL character. If the return value is greater than > > + * **out_size**, **out** contains a truncated string, without a > > + * trailing NULL character. > > this deviates from the behavior in other BPF helpers dealing with > strings. and it's extremely inconvenient for users to get > non-zero-terminated string. I think we should always zero-terminate. > > > + * > > + * Or **-EBUSY** if the per-CPU memory copy buffer is busy. > > */ > > #define __BPF_FUNC_MAPPER(FN) \ > > FN(unspec), \ > > @@ -4827,6 +4854,7 @@ union bpf_attr { > > FN(sock_from_file), \ > > FN(check_mtu), \ > > FN(for_each_map_elem), \ > > + FN(snprintf), \ > > /* */ > > > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index c99b2b67dc8d..3ab549df817b 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -5732,6 +5732,137 @@ static int check_reference_leak(struct bpf_verifier_env *env) > > return state->acquired_refs ? -EINVAL : 0; > > } > > > > +int check_bpf_snprintf_call(struct bpf_verifier_env *env, > > + struct bpf_reg_state *regs) > > +{ > > can we please extra the printf format string parsing/checking logic > and re-use them across all functions? We now have at least 4 variants > of it, it's not great to say the least. I hope it's possible to > generalize it in such a way that the same function will parse the > string, and will record each expected argument and it's type, with > whatever extra flags we need to. That should make the printing part > simpler as well, as it will just follow "directions" from the parsing > part? Devil is in the details, of course :) But it's worthwhile to try > at least. Eheh this is gonna be fun, I'll try it out and see if I can come up with something ~decent. :) Thanks for the thorough review! I agree with all your points and will address them in v2. > > + struct bpf_reg_state *fmt_reg = ®s[BPF_REG_3]; > > + struct bpf_reg_state *data_len_reg = ®s[BPF_REG_5]; > > + struct bpf_map *fmt_map = fmt_reg->map_ptr; > > + int err, fmt_map_off, i, fmt_cnt = 0, memcpy_cnt = 0, num_args; > > + u64 fmt_addr; > > + char *fmt; > > + > > + /* data must be an array of u64 so data_len must be a multiple of 8 */ > > + if (data_len_reg->var_off.value & 7) > > + return -EINVAL; > > + num_args = data_len_reg->var_off.value / 8; > > + > > + /* fmt being ARG_PTR_TO_CONST_STR guarantees that var_off is const > > + * and map_direct_value_addr is set. > > + */ > > + fmt_map_off = fmt_reg->off + fmt_reg->var_off.value; > > + err = fmt_map->ops->map_direct_value_addr(fmt_map, &fmt_addr, > > + fmt_map_off); > > + if (err) > > + return err; > > + fmt = (char *)fmt_addr; > > + > > [...] not fun to read this part over and over :) > > > + } > > + > > + return 0; > > +} > > + > > static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > > int *insn_idx_p) > > { > > @@ -5846,6 +5977,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > > return -EINVAL; > > } > > > > + if (func_id == BPF_FUNC_snprintf) { > > + err = check_bpf_snprintf_call(env, regs); > > + if (err < 0) > > + return err; > > + } > > + > > /* reset caller saved regs */ > > for (i = 0; i < CALLER_SAVED_REGS; i++) { > > mark_reg_not_init(env, regs, caller_saved[i]); > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index 0d23755c2747..7b80759c10a9 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -1271,6 +1271,114 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = { > > .arg5_type = ARG_ANYTHING, > > }; > > > > +struct bpf_snprintf_buf { > > + char buf[MAX_SNPRINTF_MEMCPY][MAX_SNPRINTF_STR_LEN]; > > +}; > > +static DEFINE_PER_CPU(struct bpf_snprintf_buf, bpf_snprintf_buf); > > +static DEFINE_PER_CPU(int, bpf_snprintf_buf_used); > > + > > +BPF_CALL_5(bpf_snprintf, char *, out, u32, out_size, char *, fmt, u64 *, args, > > + u32, args_len) > > +{ > > + int err, i, buf_used, copy_size, fmt_cnt = 0, memcpy_cnt = 0; > > + u64 params[MAX_SNPRINTF_VARARGS]; > > + struct bpf_snprintf_buf *bufs; > > + > > + buf_used = this_cpu_inc_return(bpf_snprintf_buf_used); > > + if (WARN_ON_ONCE(buf_used > 1)) { > > + err = -EBUSY; > > + goto out; > > + } > > + > > + bufs = this_cpu_ptr(&bpf_snprintf_buf); > > + > > + /* > > + * The verifier has already done most of the heavy-work for us in > > + * check_bpf_snprintf_call. We know that fmt is well formatted and that > > + * args_len is valid. The only task left is to convert some of the > > + * arguments. For the %s and %pi* specifiers, we need to read buffers > > + * from a kernel address during the helper call. > > + */ > > + for (i = 0; fmt[i] != '\0'; i++) { > > same function should hopefully be reused here > > > + } > > + > > + /* Maximumly we can have MAX_SNPRINTF_VARARGS parameters, just give > > + * all of them to snprintf(). > > + */ > > + err = snprintf(out, out_size, fmt, params[0], params[1], params[2], > > + params[3], params[4], params[5], params[6], params[7], > > + params[8], params[9], params[10], params[11]) + 1; > > + > > +out: > > + this_cpu_dec(bpf_snprintf_buf_used); > > + return err; > > +} > > + > > +static const struct bpf_func_proto bpf_snprintf_proto = { > > + .func = bpf_snprintf, > > + .gpl_only = true, > > + .ret_type = RET_INTEGER, > > + .arg1_type = ARG_PTR_TO_MEM, > > + .arg2_type = ARG_CONST_SIZE, > > can we mark is CONST_SIZE_OR_ZERO and just do nothing on zero at > runtime? I still have scars from having to deal (prove, actually) with > ARG_CONST_SIZE (> 0) limitations in perf_event_output. No need to make > anyone's life harder, if it's easy to just do something sensible on > zero (i.e., do nothing, but emit desired amount of bytes). > > > + .arg3_type = ARG_PTR_TO_CONST_STR, > > + .arg4_type = ARG_PTR_TO_MEM, > > + .arg5_type = ARG_CONST_SIZE_OR_ZERO, > > +}; > > + > > const struct bpf_func_proto * > > bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > > { > > @@ -1373,6 +1481,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > > return &bpf_task_storage_delete_proto; > > case BPF_FUNC_for_each_map_elem: > > return &bpf_for_each_map_elem_proto; > > + case BPF_FUNC_snprintf: > > + return &bpf_snprintf_proto; > > why just tracing? can't all BPF programs use this functionality? > > > default: > > return NULL; > > } > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > > index 2d3036e292a9..3cbdc8ae00e7 100644 > > --- a/tools/include/uapi/linux/bpf.h > > +++ b/tools/include/uapi/linux/bpf.h > > @@ -4660,6 +4660,33 @@ union bpf_attr { > > * Return > > * The number of traversed map elements for success, **-EINVAL** for > > * invalid **flags**. > > + * > > + * long bpf_snprintf(char *out, u32 out_size, const char *fmt, u64 *data, u32 data_len) > > + * Description > > + * Outputs a string into the **out** buffer of size **out_size** > > + * based on a format string stored in a read-only map pointed by > > + * **fmt**. > > + * > > + * Each format specifier in **fmt** corresponds to one u64 element > > + * in the **data** array. For strings and pointers where pointees > > + * are accessed, only the pointer values are stored in the *data* > > + * array. The *data_len* is the size of *data* in bytes. > > + * > > + * Formats **%s** and **%p{i,I}{4,6}** require to read kernel > > + * memory. Reading kernel memory may fail due to either invalid > > + * address or valid address but requiring a major memory fault. If > > + * reading kernel memory fails, the string for **%s** will be an > > + * empty string, and the ip address for **%p{i,I}{4,6}** will be 0. > > + * Not returning error to bpf program is consistent with what > > + * **bpf_trace_printk**\ () does for now. > > + * > > + * Return > > + * The strictly positive length of the printed string, including > > + * the trailing NUL character. If the return value is greater than > > + * **out_size**, **out** contains a truncated string, without a > > + * trailing NULL character. > > + * > > + * Or **-EBUSY** if the per-CPU memory copy buffer is busy. > > */ > > #define __BPF_FUNC_MAPPER(FN) \ > > FN(unspec), \ > > @@ -4827,6 +4854,7 @@ union bpf_attr { > > FN(sock_from_file), \ > > FN(check_mtu), \ > > FN(for_each_map_elem), \ > > + FN(snprintf), \ > > /* */ > > > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper > > -- > > 2.30.1.766.gb4fecdf3b7-goog > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper 2021-03-10 22:02 ` [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper Florent Revest ` (3 preceding siblings ...) 2021-03-16 1:25 ` [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper Andrii Nakryiko @ 2021-03-23 3:21 ` Alexei Starovoitov 2021-03-23 14:04 ` Florent Revest 4 siblings, 1 reply; 27+ messages in thread From: Alexei Starovoitov @ 2021-03-23 3:21 UTC (permalink / raw) To: Florent Revest Cc: bpf, ast, daniel, andrii, yhs, kpsingh, jackmanb, linux-kernel On Wed, Mar 10, 2021 at 11:02:08PM +0100, Florent Revest wrote: > > +struct bpf_snprintf_buf { > + char buf[MAX_SNPRINTF_MEMCPY][MAX_SNPRINTF_STR_LEN]; > +}; > +static DEFINE_PER_CPU(struct bpf_snprintf_buf, bpf_snprintf_buf); > +static DEFINE_PER_CPU(int, bpf_snprintf_buf_used); > + > +BPF_CALL_5(bpf_snprintf, char *, out, u32, out_size, char *, fmt, u64 *, args, > + u32, args_len) > +{ > + int err, i, buf_used, copy_size, fmt_cnt = 0, memcpy_cnt = 0; > + u64 params[MAX_SNPRINTF_VARARGS]; > + struct bpf_snprintf_buf *bufs; > + > + buf_used = this_cpu_inc_return(bpf_snprintf_buf_used); > + if (WARN_ON_ONCE(buf_used > 1)) { this can trigger only if the helper itself gets preempted and another bpf prog will run on the same cpu and will call into this helper again, right? If so, how about adding preempt_disable here to avoid this case? It won't prevent the case where kprobe is inside snprintf core, so the counter is still needed, but it wouldn't trigger by accident. Also since bufs are not used always, how about grabbing the buffers only when %p or %s are seen in fmt? After snprintf() is done it would conditionally do: if (bufs_were_used) { this_cpu_dec(bpf_snprintf_buf_used); preempt_enable(); } This way simple bpf_snprintf won't ever hit EBUSY. > + err = -EBUSY; > + goto out; > + } > + > + bufs = this_cpu_ptr(&bpf_snprintf_buf); > + > + /* > + * The verifier has already done most of the heavy-work for us in > + * check_bpf_snprintf_call. We know that fmt is well formatted and that > + * args_len is valid. The only task left is to convert some of the > + * arguments. For the %s and %pi* specifiers, we need to read buffers > + * from a kernel address during the helper call. > + */ > + for (i = 0; fmt[i] != '\0'; i++) { > + if (fmt[i] != '%') > + continue; > + > + if (fmt[i + 1] == '%') { > + i++; > + continue; > + } > + > + /* fmt[i] != 0 && fmt[last] == 0, so we can access fmt[i + 1] */ > + i++; > + > + /* skip optional "[0 +-][num]" width formating field */ > + while (fmt[i] == '0' || fmt[i] == '+' || fmt[i] == '-' || > + fmt[i] == ' ') > + i++; > + if (fmt[i] >= '1' && fmt[i] <= '9') { > + i++; > + while (fmt[i] >= '0' && fmt[i] <= '9') > + i++; > + } > + > + if (fmt[i] == 's') { > + void *unsafe_ptr = (void *)(long)args[fmt_cnt]; > + > + err = strncpy_from_kernel_nofault(bufs->buf[memcpy_cnt], > + unsafe_ptr, > + MAX_SNPRINTF_STR_LEN); > + if (err < 0) > + bufs->buf[memcpy_cnt][0] = '\0'; > + params[fmt_cnt] = (u64)(long)bufs->buf[memcpy_cnt]; how about: char buf[512]; instead? instead of memcpy_cnt++ remember how many bytes of the buf were used and copy next arg after that. The scratch space would be used more efficiently. The helper would potentially return ENOSPC if the first string printed via %s consumed most of the 512 space and the second string doesn't fit. But the verifier-time if (memcpy_cnt >= MAX_SNPRINTF_MEMCPY) can be removed. Ten small %s will work fine. We can allocate a page per-cpu when this helper is used by prog and free that page when all progs with bpf_snprintf are unloaded. But extra complexity is probably not worth it. I would start with 512 per-cpu. It's going to be enough for most users. Overall looks great. Cannot wait for v2 :) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper 2021-03-23 3:21 ` Alexei Starovoitov @ 2021-03-23 14:04 ` Florent Revest 0 siblings, 0 replies; 27+ messages in thread From: Florent Revest @ 2021-03-23 14:04 UTC (permalink / raw) To: Alexei Starovoitov Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Tue, Mar 23, 2021 at 4:21 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Mar 10, 2021 at 11:02:08PM +0100, Florent Revest wrote: > > > > +struct bpf_snprintf_buf { > > + char buf[MAX_SNPRINTF_MEMCPY][MAX_SNPRINTF_STR_LEN]; > > +}; > > +static DEFINE_PER_CPU(struct bpf_snprintf_buf, bpf_snprintf_buf); > > +static DEFINE_PER_CPU(int, bpf_snprintf_buf_used); > > + > > +BPF_CALL_5(bpf_snprintf, char *, out, u32, out_size, char *, fmt, u64 *, args, > > + u32, args_len) > > +{ > > + int err, i, buf_used, copy_size, fmt_cnt = 0, memcpy_cnt = 0; > > + u64 params[MAX_SNPRINTF_VARARGS]; > > + struct bpf_snprintf_buf *bufs; > > + > > + buf_used = this_cpu_inc_return(bpf_snprintf_buf_used); > > + if (WARN_ON_ONCE(buf_used > 1)) { > > this can trigger only if the helper itself gets preempted and > another bpf prog will run on the same cpu and will call into this helper > again, right? > If so, how about adding preempt_disable here to avoid this case? Ah, neat, that sounds like a good idea indeed. This was really just cargo-culted from bpf_seq_printf but as part of my grand unification attempt for the various printf-like helpers, I can try to make it use preempt_disable as well yes. > It won't prevent the case where kprobe is inside snprintf core, > so the counter is still needed, but it wouldn't trigger by accident. Good point, I will keep it around then. > Also since bufs are not used always, how about grabbing the > buffers only when %p or %s are seen in fmt? > After snprintf() is done it would conditionally do: > if (bufs_were_used) { > this_cpu_dec(bpf_snprintf_buf_used); > preempt_enable(); > } > This way simple bpf_snprintf won't ever hit EBUSY. Absolutely, it would be nice. :) > > + err = -EBUSY; > > + goto out; > > + } > > + > > + bufs = this_cpu_ptr(&bpf_snprintf_buf); > > + > > + /* > > + * The verifier has already done most of the heavy-work for us in > > + * check_bpf_snprintf_call. We know that fmt is well formatted and that > > + * args_len is valid. The only task left is to convert some of the > > + * arguments. For the %s and %pi* specifiers, we need to read buffers > > + * from a kernel address during the helper call. > > + */ > > + for (i = 0; fmt[i] != '\0'; i++) { > > + if (fmt[i] != '%') > > + continue; > > + > > + if (fmt[i + 1] == '%') { > > + i++; > > + continue; > > + } > > + > > + /* fmt[i] != 0 && fmt[last] == 0, so we can access fmt[i + 1] */ > > + i++; > > + > > + /* skip optional "[0 +-][num]" width formating field */ > > + while (fmt[i] == '0' || fmt[i] == '+' || fmt[i] == '-' || > > + fmt[i] == ' ') > > + i++; > > + if (fmt[i] >= '1' && fmt[i] <= '9') { > > + i++; > > + while (fmt[i] >= '0' && fmt[i] <= '9') > > + i++; > > + } > > + > > + if (fmt[i] == 's') { > > + void *unsafe_ptr = (void *)(long)args[fmt_cnt]; > > + > > + err = strncpy_from_kernel_nofault(bufs->buf[memcpy_cnt], > > + unsafe_ptr, > > + MAX_SNPRINTF_STR_LEN); > > + if (err < 0) > > + bufs->buf[memcpy_cnt][0] = '\0'; > > + params[fmt_cnt] = (u64)(long)bufs->buf[memcpy_cnt]; > > how about: > char buf[512]; instead? > instead of memcpy_cnt++ remember how many bytes of the buf were used and > copy next arg after that. > The scratch space would be used more efficiently. > The helper would potentially return ENOSPC if the first string printed via %s > consumed most of the 512 space and the second string doesn't fit. > But the verifier-time if (memcpy_cnt >= MAX_SNPRINTF_MEMCPY) can be removed. > Ten small %s will work fine. Cool! That is also a good idea :) > We can allocate a page per-cpu when this helper is used by prog and free > that page when all progs with bpf_snprintf are unloaded. > But extra complexity is probably not worth it. I would start with 512 per-cpu. > It's going to be enough for most users. Yes, let's maybe keep that for later. I think there is already enough complexity going into the printf-like helpers unification patch. > Overall looks great. Cannot wait for v2 :) Ahah wait until you see that patch! :D ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field 2021-03-10 22:02 [PATCH bpf-next 0/5] Add a snprintf eBPF helper Florent Revest 2021-03-10 22:02 ` [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type Florent Revest 2021-03-10 22:02 ` [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper Florent Revest @ 2021-03-10 22:02 ` Florent Revest 2021-03-16 4:36 ` Andrii Nakryiko 2021-03-10 22:02 ` [PATCH bpf-next 4/5] libbpf: Introduce a BPF_SNPRINTF helper macro Florent Revest 2021-03-10 22:02 ` [PATCH bpf-next 5/5] selftests/bpf: Add a series of tests for bpf_snprintf Florent Revest 4 siblings, 1 reply; 27+ messages in thread From: Florent Revest @ 2021-03-10 22:02 UTC (permalink / raw) To: bpf Cc: ast, daniel, andrii, yhs, kpsingh, jackmanb, linux-kernel, Florent Revest When initializing the __param array with a one liner, if all args are const, the initial array value will be placed in the rodata section but because libbpf does not support relocation in the rodata section, any pointer in this array will stay NULL. This is a workaround, ideally the rodata relocation should be supported by libbpf but this would require a disproportionate amount of work given the actual usecases. (it is very unlikely that one uses a const array of relocated addresses) Signed-off-by: Florent Revest <revest@chromium.org> --- tools/lib/bpf/bpf_tracing.h | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h index f9ef37707888..f6a2deb3cd5b 100644 --- a/tools/lib/bpf/bpf_tracing.h +++ b/tools/lib/bpf/bpf_tracing.h @@ -413,6 +413,34 @@ typeof(name(0)) name(struct pt_regs *ctx) \ } \ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) +#define ___bpf_build_param0(narg, x) +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \ + ___bpf_build_param1(narg, args) +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \ + ___bpf_build_param2(narg, args) +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \ + ___bpf_build_param3(narg, args) +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \ + ___bpf_build_param4(narg, args) +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \ + ___bpf_build_param5(narg, args) +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \ + ___bpf_build_param6(narg, args) +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \ + ___bpf_build_param7(narg, args) +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \ + ___bpf_build_param8(narg, args) +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \ + ___bpf_build_param9(narg, args) +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \ + ___bpf_build_param10(narg, args) +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \ + ___bpf_build_param11(narg, args) +#define ___bpf_build_param(args...) \ + unsigned long long ___param[___bpf_narg(args)]; \ + ___bpf_apply(___bpf_build_param, ___bpf_narg(args))(___bpf_narg(args), args) + /* * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values * in a structure. @@ -422,7 +450,7 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) _Pragma("GCC diagnostic push") \ _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ static const char ___fmt[] = fmt; \ - unsigned long long ___param[] = { args }; \ + ___bpf_build_param(args); \ _Pragma("GCC diagnostic pop") \ int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \ ___param, sizeof(___param)); \ -- 2.30.1.766.gb4fecdf3b7-goog ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field 2021-03-10 22:02 ` [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field Florent Revest @ 2021-03-16 4:36 ` Andrii Nakryiko 2021-03-16 4:41 ` Andrii Nakryiko 2021-03-16 22:43 ` Florent Revest 0 siblings, 2 replies; 27+ messages in thread From: Andrii Nakryiko @ 2021-03-16 4:36 UTC (permalink / raw) To: Florent Revest Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > When initializing the __param array with a one liner, if all args are > const, the initial array value will be placed in the rodata section but > because libbpf does not support relocation in the rodata section, any > pointer in this array will stay NULL. > > This is a workaround, ideally the rodata relocation should be supported > by libbpf but this would require a disproportionate amount of work given > the actual usecases. (it is very unlikely that one uses a const array of > relocated addresses) > > Signed-off-by: Florent Revest <revest@chromium.org> > --- > tools/lib/bpf/bpf_tracing.h | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h > index f9ef37707888..f6a2deb3cd5b 100644 > --- a/tools/lib/bpf/bpf_tracing.h > +++ b/tools/lib/bpf/bpf_tracing.h > @@ -413,6 +413,34 @@ typeof(name(0)) name(struct pt_regs *ctx) \ > } \ > static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) > > +#define ___bpf_build_param0(narg, x) > +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x > +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \ > + ___bpf_build_param1(narg, args) > +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \ > + ___bpf_build_param2(narg, args) > +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \ > + ___bpf_build_param3(narg, args) > +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \ > + ___bpf_build_param4(narg, args) > +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \ > + ___bpf_build_param5(narg, args) > +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \ > + ___bpf_build_param6(narg, args) > +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \ > + ___bpf_build_param7(narg, args) > +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \ > + ___bpf_build_param8(narg, args) > +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \ > + ___bpf_build_param9(narg, args) > +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \ > + ___bpf_build_param10(narg, args) > +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \ > + ___bpf_build_param11(narg, args) took me some time to get why the [narg - 12] :) it makes sense, but then I started wondering why not #define ___bpf_build_param12(narg, x, args...) ___bpf_build_param11(narg, args); ___param[11] = x ? seems more straightforward, no? also please keep all of them on single line. And to make lines shorter, let's call it ___bpf_fillX? I also don't like hard-coded ___param, which is both inflexible and is obscure at the point of use of this macro. So let's pass it as the first argument? > +#define ___bpf_build_param(args...) \ > + unsigned long long ___param[___bpf_narg(args)]; \ > + ___bpf_apply(___bpf_build_param, ___bpf_narg(args))(___bpf_narg(args), args) > + And here I'd pass array as a parameter and let caller define it, so macro is literally just filling the array elements, not defining the array itself and what's the type of elements > /* > * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values > * in a structure. > @@ -422,7 +450,7 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) > _Pragma("GCC diagnostic push") \ > _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ > static const char ___fmt[] = fmt; \ > - unsigned long long ___param[] = { args }; \ > + ___bpf_build_param(args); \ > _Pragma("GCC diagnostic pop") \ > int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \ > ___param, sizeof(___param)); \ here you are violating separation of variables and code, ___bpf_build_param is defining a variable, then has code statements, then you are declaring ___ret after the code. So please split ___ret definition, > -- > 2.30.1.766.gb4fecdf3b7-goog > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field 2021-03-16 4:36 ` Andrii Nakryiko @ 2021-03-16 4:41 ` Andrii Nakryiko 2021-03-16 22:43 ` Florent Revest 1 sibling, 0 replies; 27+ messages in thread From: Andrii Nakryiko @ 2021-03-16 4:41 UTC (permalink / raw) To: Florent Revest Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Mon, Mar 15, 2021 at 9:36 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > > > When initializing the __param array with a one liner, if all args are > > const, the initial array value will be placed in the rodata section but > > because libbpf does not support relocation in the rodata section, any > > pointer in this array will stay NULL. > > > > This is a workaround, ideally the rodata relocation should be supported > > by libbpf but this would require a disproportionate amount of work given > > the actual usecases. (it is very unlikely that one uses a const array of > > relocated addresses) Can you please drop this paragraph? This is not a workaround, it's a completely working code that should continue working. And this is not something that libbpf doesn't support, there is no kernel interface to make it work at all. Please add Fixes: tag as well. > > > > Signed-off-by: Florent Revest <revest@chromium.org> > > --- > > tools/lib/bpf/bpf_tracing.h | 30 +++++++++++++++++++++++++++++- > > 1 file changed, 29 insertions(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h > > index f9ef37707888..f6a2deb3cd5b 100644 > > --- a/tools/lib/bpf/bpf_tracing.h > > +++ b/tools/lib/bpf/bpf_tracing.h > > @@ -413,6 +413,34 @@ typeof(name(0)) name(struct pt_regs *ctx) \ > > } \ > > static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) > > > > +#define ___bpf_build_param0(narg, x) > > +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x > > +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \ > > + ___bpf_build_param1(narg, args) > > +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \ > > + ___bpf_build_param2(narg, args) > > +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \ > > + ___bpf_build_param3(narg, args) > > +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \ > > + ___bpf_build_param4(narg, args) > > +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \ > > + ___bpf_build_param5(narg, args) > > +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \ > > + ___bpf_build_param6(narg, args) > > +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \ > > + ___bpf_build_param7(narg, args) > > +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \ > > + ___bpf_build_param8(narg, args) > > +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \ > > + ___bpf_build_param9(narg, args) > > +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \ > > + ___bpf_build_param10(narg, args) > > +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \ > > + ___bpf_build_param11(narg, args) > > took me some time to get why the [narg - 12] :) it makes sense, but > then I started wondering why not > > #define ___bpf_build_param12(narg, x, args...) > ___bpf_build_param11(narg, args); ___param[11] = x > > ? seems more straightforward, no? > > also please keep all of them on single line. And to make lines > shorter, let's call it ___bpf_fillX? I also don't like hard-coded > ___param, which is both inflexible and is obscure at the point of use > of this macro. So let's pass it as the first argument? > > > +#define ___bpf_build_param(args...) \ > > + unsigned long long ___param[___bpf_narg(args)]; \ > > + ___bpf_apply(___bpf_build_param, ___bpf_narg(args))(___bpf_narg(args), args) > > + > > And here I'd pass array as a parameter and let caller define it, so > macro is literally just filling the array elements, not defining the > array itself and what's the type of elements > > > /* > > * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values > > * in a structure. > > @@ -422,7 +450,7 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) > > _Pragma("GCC diagnostic push") \ > > _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ > > static const char ___fmt[] = fmt; \ > > - unsigned long long ___param[] = { args }; \ > > + ___bpf_build_param(args); \ > > _Pragma("GCC diagnostic pop") \ > > int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \ > > ___param, sizeof(___param)); \ > > here you are violating separation of variables and code, > ___bpf_build_param is defining a variable, then has code statements, > then you are declaring ___ret after the code. So please split ___ret > definition, > > > -- > > 2.30.1.766.gb4fecdf3b7-goog > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field 2021-03-16 4:36 ` Andrii Nakryiko 2021-03-16 4:41 ` Andrii Nakryiko @ 2021-03-16 22:43 ` Florent Revest 2021-03-16 23:06 ` Andrii Nakryiko 1 sibling, 1 reply; 27+ messages in thread From: Florent Revest @ 2021-03-16 22:43 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Tue, Mar 16, 2021 at 5:36 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > +#define ___bpf_build_param0(narg, x) > > +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x > > +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \ > > + ___bpf_build_param1(narg, args) > > +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \ > > + ___bpf_build_param2(narg, args) > > +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \ > > + ___bpf_build_param3(narg, args) > > +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \ > > + ___bpf_build_param4(narg, args) > > +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \ > > + ___bpf_build_param5(narg, args) > > +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \ > > + ___bpf_build_param6(narg, args) > > +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \ > > + ___bpf_build_param7(narg, args) > > +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \ > > + ___bpf_build_param8(narg, args) > > +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \ > > + ___bpf_build_param9(narg, args) > > +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \ > > + ___bpf_build_param10(narg, args) > > +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \ > > + ___bpf_build_param11(narg, args) > > took me some time to get why the [narg - 12] :) it makes sense, but > then I started wondering why not > > #define ___bpf_build_param12(narg, x, args...) > ___bpf_build_param11(narg, args); ___param[11] = x > > ? seems more straightforward, no? Unless I'm misunderstanding something, I don't think this would work. The awkward "narg - 12" comes from the fact that these variadic macros work by taking the first argument out of the variadic arguments (x followed by args) and calling another macro with what's left (args). So if you do __bpf_build_param(arg1, arg2) you will have __bpf_build_param2() called with arg1 and __bpf_build_param1() called with arg2. And if you do __bpf_build_param(arg1, arg2, arg3) you will have __bpf_build_param3() called with arg1, __bpf_build_param2() called with arg2, and __bpf_build_param1() called with arg3. Basically, things are inverted, the position at which you need to insert in ___param evolves in the opposite direction of the X after ___bpf_build_param which is the number of arguments left. No matter in which order __bpf_build_paramX calls __bpf_build_param(X-1) (before or after setting ___param[n]) you will be unable to know just from the macro name at which cell in __param you need to write the argument. (except for __bpf_build_param12 which is an exception, because the max number of arg is 12, if this macro gets called, then we know that narg=12 and we will always write at __param[0]) That being said, I share your concern that this code is hard to read. So instead of giving narg to each macro, I tried to give a pos argument which indicates in which cell the macro should write. pos is basically a counter that goes from 0 to narg as macros go from narg to 0. #define ___bpf_fill0(array, pos, x) #define ___bpf_fill1(array, pos, x) array[pos] = x #define ___bpf_fill2(array, pos, x, args...) array[pos] = x; ___bpf_fill1(array, pos + 1, args) #define ___bpf_fill3(array, pos, x, args...) array[pos] = x; ___bpf_fill2(array, pos + 1, args) #define ___bpf_fill4(array, pos, x, args...) array[pos] = x; ___bpf_fill3(array, pos + 1, args) #define ___bpf_fill5(array, pos, x, args...) array[pos] = x; ___bpf_fill4(array, pos + 1, args) #define ___bpf_fill6(array, pos, x, args...) array[pos] = x; ___bpf_fill5(array, pos + 1, args) #define ___bpf_fill7(array, pos, x, args...) array[pos] = x; ___bpf_fill6(array, pos + 1, args) #define ___bpf_fill8(array, pos, x, args...) array[pos] = x; ___bpf_fill7(array, pos + 1, args) #define ___bpf_fill9(array, pos, x, args...) array[pos] = x; ___bpf_fill8(array, pos + 1, args) #define ___bpf_fill10(array, pos, x, args...) array[pos] = x; ___bpf_fill9(array, pos + 1, args) #define ___bpf_fill11(array, pos, x, args...) array[pos] = x; ___bpf_fill10(array, pos + 1, args) #define ___bpf_fill12(array, pos, x, args...) array[pos] = x; ___bpf_fill11(array, pos + 1, args) #define ___bpf_fill(array, args...) \ ___bpf_apply(___bpf_fill, ___bpf_narg(args))(array, 0, args) I hope this makes things a bit clearer ? (I often joke that BPF is written in preprocessor... :p) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field 2021-03-16 22:43 ` Florent Revest @ 2021-03-16 23:06 ` Andrii Nakryiko 0 siblings, 0 replies; 27+ messages in thread From: Andrii Nakryiko @ 2021-03-16 23:06 UTC (permalink / raw) To: Florent Revest Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Tue, Mar 16, 2021 at 3:43 PM Florent Revest <revest@chromium.org> wrote: > > On Tue, Mar 16, 2021 at 5:36 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > > +#define ___bpf_build_param0(narg, x) > > > +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x > > > +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \ > > > + ___bpf_build_param1(narg, args) > > > +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \ > > > + ___bpf_build_param2(narg, args) > > > +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \ > > > + ___bpf_build_param3(narg, args) > > > +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \ > > > + ___bpf_build_param4(narg, args) > > > +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \ > > > + ___bpf_build_param5(narg, args) > > > +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \ > > > + ___bpf_build_param6(narg, args) > > > +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \ > > > + ___bpf_build_param7(narg, args) > > > +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \ > > > + ___bpf_build_param8(narg, args) > > > +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \ > > > + ___bpf_build_param9(narg, args) > > > +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \ > > > + ___bpf_build_param10(narg, args) > > > +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \ > > > + ___bpf_build_param11(narg, args) > > > > took me some time to get why the [narg - 12] :) it makes sense, but > > then I started wondering why not > > > > #define ___bpf_build_param12(narg, x, args...) > > ___bpf_build_param11(narg, args); ___param[11] = x > > > > ? seems more straightforward, no? > > Unless I'm misunderstanding something, I don't think this would work. > The awkward "narg - 12" comes from the fact that these variadic macros > work by taking the first argument out of the variadic arguments (x > followed by args) and calling another macro with what's left (args). You are right, of course, silly me. > > So if you do __bpf_build_param(arg1, arg2) you will have > __bpf_build_param2() called with arg1 and __bpf_build_param1() called > with arg2. And if you do __bpf_build_param(arg1, arg2, arg3) you will > have __bpf_build_param3() called with arg1, __bpf_build_param2() > called with arg2, and __bpf_build_param1() called with arg3. > Basically, things are inverted, the position at which you need to > insert in ___param evolves in the opposite direction of the X after > ___bpf_build_param which is the number of arguments left. > > No matter in which order __bpf_build_paramX calls > __bpf_build_param(X-1) (before or after setting ___param[n]) you will > be unable to know just from the macro name at which cell in __param > you need to write the argument. (except for __bpf_build_param12 which > is an exception, because the max number of arg is 12, if this macro > gets called, then we know that narg=12 and we will always write at > __param[0]) > > That being said, I share your concern that this code is hard to read. > So instead of giving narg to each macro, I tried to give a pos > argument which indicates in which cell the macro should write. pos is > basically a counter that goes from 0 to narg as macros go from narg to > 0. > > #define ___bpf_fill0(array, pos, x) > #define ___bpf_fill1(array, pos, x) array[pos] = x > #define ___bpf_fill2(array, pos, x, args...) array[pos] = x; > ___bpf_fill1(array, pos + 1, args) > #define ___bpf_fill3(array, pos, x, args...) array[pos] = x; > ___bpf_fill2(array, pos + 1, args) > #define ___bpf_fill4(array, pos, x, args...) array[pos] = x; > ___bpf_fill3(array, pos + 1, args) > #define ___bpf_fill5(array, pos, x, args...) array[pos] = x; > ___bpf_fill4(array, pos + 1, args) > #define ___bpf_fill6(array, pos, x, args...) array[pos] = x; > ___bpf_fill5(array, pos + 1, args) > #define ___bpf_fill7(array, pos, x, args...) array[pos] = x; > ___bpf_fill6(array, pos + 1, args) > #define ___bpf_fill8(array, pos, x, args...) array[pos] = x; > ___bpf_fill7(array, pos + 1, args) > #define ___bpf_fill9(array, pos, x, args...) array[pos] = x; > ___bpf_fill8(array, pos + 1, args) > #define ___bpf_fill10(array, pos, x, args...) array[pos] = x; > ___bpf_fill9(array, pos + 1, args) > #define ___bpf_fill11(array, pos, x, args...) array[pos] = x; > ___bpf_fill10(array, pos + 1, args) > #define ___bpf_fill12(array, pos, x, args...) array[pos] = x; > ___bpf_fill11(array, pos + 1, args) > #define ___bpf_fill(array, args...) \ > ___bpf_apply(___bpf_fill, ___bpf_narg(args))(array, 0, args) Yeah, this is still more straightforward, I think. Please use shorter names to keep it a bit more succinct: arr and p seems clear enough. > > I hope this makes things a bit clearer ? (I often joke that BPF is > written in preprocessor... :p) Definitely true for BPF_CORE_READ macros :) ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH bpf-next 4/5] libbpf: Introduce a BPF_SNPRINTF helper macro 2021-03-10 22:02 [PATCH bpf-next 0/5] Add a snprintf eBPF helper Florent Revest ` (2 preceding siblings ...) 2021-03-10 22:02 ` [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field Florent Revest @ 2021-03-10 22:02 ` Florent Revest 2021-03-16 4:39 ` Andrii Nakryiko 2021-03-10 22:02 ` [PATCH bpf-next 5/5] selftests/bpf: Add a series of tests for bpf_snprintf Florent Revest 4 siblings, 1 reply; 27+ messages in thread From: Florent Revest @ 2021-03-10 22:02 UTC (permalink / raw) To: bpf Cc: ast, daniel, andrii, yhs, kpsingh, jackmanb, linux-kernel, Florent Revest Similarly to BPF_SEQ_PRINTF, this macro turns variadic arguments into an array of u64, making it more natural to call the bpf_snprintf helper. Signed-off-by: Florent Revest <revest@chromium.org> --- tools/lib/bpf/bpf_tracing.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h index f6a2deb3cd5b..89e82da9b8a0 100644 --- a/tools/lib/bpf/bpf_tracing.h +++ b/tools/lib/bpf/bpf_tracing.h @@ -457,4 +457,19 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) ___ret; \ }) +/* + * BPF_SNPRINTF wraps the bpf_snprintf helper with variadic arguments instead of + * an array of u64. + */ +#define BPF_SNPRINTF(out, out_size, fmt, args...) \ + ({ \ + _Pragma("GCC diagnostic push") \ + _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ + ___bpf_build_param(args); \ + _Pragma("GCC diagnostic pop") \ + int ___ret = bpf_snprintf(out, out_size, fmt, \ + ___param, sizeof(___param)); \ + ___ret; \ + }) + #endif -- 2.30.1.766.gb4fecdf3b7-goog ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 4/5] libbpf: Introduce a BPF_SNPRINTF helper macro 2021-03-10 22:02 ` [PATCH bpf-next 4/5] libbpf: Introduce a BPF_SNPRINTF helper macro Florent Revest @ 2021-03-16 4:39 ` Andrii Nakryiko 0 siblings, 0 replies; 27+ messages in thread From: Andrii Nakryiko @ 2021-03-16 4:39 UTC (permalink / raw) To: Florent Revest Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > Similarly to BPF_SEQ_PRINTF, this macro turns variadic arguments into an > array of u64, making it more natural to call the bpf_snprintf helper. > > Signed-off-by: Florent Revest <revest@chromium.org> > --- > tools/lib/bpf/bpf_tracing.h | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h > index f6a2deb3cd5b..89e82da9b8a0 100644 > --- a/tools/lib/bpf/bpf_tracing.h > +++ b/tools/lib/bpf/bpf_tracing.h > @@ -457,4 +457,19 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args) > ___ret; \ > }) > > +/* > + * BPF_SNPRINTF wraps the bpf_snprintf helper with variadic arguments instead of > + * an array of u64. > + */ > +#define BPF_SNPRINTF(out, out_size, fmt, args...) \ > + ({ \ > + _Pragma("GCC diagnostic push") \ > + _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \ > + ___bpf_build_param(args); \ > + _Pragma("GCC diagnostic pop") \ > + int ___ret = bpf_snprintf(out, out_size, fmt, \ > + ___param, sizeof(___param)); \ same problem, mixing variable declarations and code > + ___ret; \ > + }) > + > #endif > -- > 2.30.1.766.gb4fecdf3b7-goog > ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH bpf-next 5/5] selftests/bpf: Add a series of tests for bpf_snprintf 2021-03-10 22:02 [PATCH bpf-next 0/5] Add a snprintf eBPF helper Florent Revest ` (3 preceding siblings ...) 2021-03-10 22:02 ` [PATCH bpf-next 4/5] libbpf: Introduce a BPF_SNPRINTF helper macro Florent Revest @ 2021-03-10 22:02 ` Florent Revest 2021-03-16 4:49 ` Andrii Nakryiko 4 siblings, 1 reply; 27+ messages in thread From: Florent Revest @ 2021-03-10 22:02 UTC (permalink / raw) To: bpf Cc: ast, daniel, andrii, yhs, kpsingh, jackmanb, linux-kernel, Florent Revest This exercices most of the format specifiers when things go well. Signed-off-by: Florent Revest <revest@chromium.org> --- .../selftests/bpf/prog_tests/snprintf.c | 71 +++++++++++++++++++ .../selftests/bpf/progs/test_snprintf.c | 71 +++++++++++++++++++ 2 files changed, 142 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf.c create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf.c diff --git a/tools/testing/selftests/bpf/prog_tests/snprintf.c b/tools/testing/selftests/bpf/prog_tests/snprintf.c new file mode 100644 index 000000000000..23af1dbd1eeb --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/snprintf.c @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2021 Google LLC. */ + +#include <test_progs.h> +#include "test_snprintf.skel.h" + +static int duration; + +#define EXP_NUM_OUT "-8 9 96 -424242 1337 DABBAD00" +#define EXP_NUM_RET sizeof(EXP_NUM_OUT) + +#define EXP_IP_OUT "127.000.000.001 0000:0000:0000:0000:0000:0000:0000:0001" +#define EXP_IP_RET sizeof(EXP_IP_OUT) + +/* The third specifier, %pB, depends on compiler inlining so don't check it */ +#define EXP_SYM_OUT "schedule schedule+0x0/" +#define MIN_SYM_RET sizeof(EXP_SYM_OUT) + +/* The third specifier, %p, is a hashed pointer which changes on every reboot */ +#define EXP_ADDR_OUT "0000000000000000 ffff00000add4e55 " +#define EXP_ADDR_RET sizeof(EXP_ADDR_OUT "unknownhashedptr") + +#define EXP_STR_OUT "str1 longstr" +#define EXP_STR_RET sizeof(EXP_STR_OUT) + +#define EXP_OVER_OUT {'%', 'o', 'v', 'e', 'r'} +#define EXP_OVER_RET 10 + +void test_snprintf(void) +{ + char exp_addr_out[] = EXP_ADDR_OUT; + char exp_over_out[] = EXP_OVER_OUT; + char exp_sym_out[] = EXP_SYM_OUT; + struct test_snprintf *skel; + int err; + + skel = test_snprintf__open_and_load(); + if (CHECK(!skel, "skel_open", "failed to open and load skeleton\n")) + return; + + err = test_snprintf__attach(skel); + if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err)) + goto cleanup; + + /* trigger tracepoint */ + usleep(1); + + ASSERT_STREQ(skel->bss->num_out, EXP_NUM_OUT, "num_out"); + ASSERT_EQ(skel->bss->num_ret, EXP_NUM_RET, "num_ret"); + + ASSERT_STREQ(skel->bss->ip_out, EXP_IP_OUT, "ip_out"); + ASSERT_EQ(skel->bss->ip_ret, EXP_IP_RET, "ip_ret"); + + ASSERT_OK(memcmp(skel->bss->sym_out, exp_sym_out, + sizeof(exp_sym_out) - 1), "sym_out"); + ASSERT_LT(MIN_SYM_RET, skel->bss->sym_ret, "sym_ret"); + + ASSERT_OK(memcmp(skel->bss->addr_out, exp_addr_out, + sizeof(exp_addr_out) - 1), "addr_out"); + ASSERT_EQ(skel->bss->addr_ret, EXP_ADDR_RET, "addr_ret"); + + ASSERT_STREQ(skel->bss->str_out, EXP_STR_OUT, "str_out"); + ASSERT_EQ(skel->bss->str_ret, EXP_STR_RET, "str_ret"); + + ASSERT_OK(memcmp(skel->bss->over_out, exp_over_out, + sizeof(exp_over_out)), "over_out"); + ASSERT_EQ(skel->bss->over_ret, EXP_OVER_RET, "over_ret"); + +cleanup: + test_snprintf__destroy(skel); +} diff --git a/tools/testing/selftests/bpf/progs/test_snprintf.c b/tools/testing/selftests/bpf/progs/test_snprintf.c new file mode 100644 index 000000000000..6c8aa4988e69 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_snprintf.c @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2021 Google LLC. */ + +#include <linux/bpf.h> +#include <bpf/bpf_endian.h> +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h> + +#define OUT_LEN 64 + +/* Integer types */ +static const char num_fmt[] = "%d %u %x %li %llu %lX"; +#define NUMBERS -8, 9, 150, -424242, 1337, 0xDABBAD00 + +char num_out[OUT_LEN] = {}; +long num_ret = 0; + +/* IP addresses */ +static const char ip_fmt[] = "%pi4 %pI6"; +static const __u8 dummy_ipv4[] = {127, 0, 0, 1}; /* 127.0.0.1 */ +static const __u32 dummy_ipv6[] = {0, 0, 0, bpf_htonl(1)}; /* ::1/128 */ +#define IPS &dummy_ipv4, &dummy_ipv6 + +char ip_out[OUT_LEN] = {}; +long ip_ret = 0; + +/* Symbol lookup formatting */ +static const char sym_fmt[] = "%ps %pS %pB"; +extern const void schedule __ksym; +#define SYMBOLS &schedule, &schedule, &schedule + +char sym_out[OUT_LEN] = {}; +long sym_ret = 0; + +/* Kernel pointers */ +static const char addr_fmt[] = "%pK %px %p"; +#define ADDRESSES 0, 0xFFFF00000ADD4E55, 0xFFFF00000ADD4E55 + +char addr_out[OUT_LEN] = {}; +long addr_ret = 0; + +/* Strings embedding */ +static const char str_fmt[] = "%s %+05s"; +static const char str1[] = "str1"; +static const char longstr[] = "longstr"; +#define STRINGS str1, longstr + +char str_out[OUT_LEN] = {}; +long str_ret = 0; + +/* Overflow */ +static const char over_fmt[] = "%%overflow"; + +#define OVER_OUT_LEN 6 +char over_out[OVER_OUT_LEN] = {}; +long over_ret = 0; + +SEC("raw_tp/sys_enter") +int handler(const void *ctx) +{ + num_ret = BPF_SNPRINTF(num_out, OUT_LEN, num_fmt, NUMBERS); + ip_ret = BPF_SNPRINTF(ip_out, OUT_LEN, ip_fmt, IPS); + sym_ret = BPF_SNPRINTF(sym_out, OUT_LEN, sym_fmt, SYMBOLS); + addr_ret = BPF_SNPRINTF(addr_out, OUT_LEN, addr_fmt, ADDRESSES); + str_ret = BPF_SNPRINTF(str_out, OUT_LEN, str_fmt, STRINGS); + over_ret = BPF_SNPRINTF(over_out, OVER_OUT_LEN, over_fmt); + + return 0; +} + +char _license[] SEC("license") = "GPL"; -- 2.30.1.766.gb4fecdf3b7-goog ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 5/5] selftests/bpf: Add a series of tests for bpf_snprintf 2021-03-10 22:02 ` [PATCH bpf-next 5/5] selftests/bpf: Add a series of tests for bpf_snprintf Florent Revest @ 2021-03-16 4:49 ` Andrii Nakryiko 0 siblings, 0 replies; 27+ messages in thread From: Andrii Nakryiko @ 2021-03-16 4:49 UTC (permalink / raw) To: Florent Revest Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, KP Singh, Brendan Jackman, open list On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote: > > This exercices most of the format specifiers when things go well. typo: exercises > > Signed-off-by: Florent Revest <revest@chromium.org> > --- > .../selftests/bpf/prog_tests/snprintf.c | 71 +++++++++++++++++++ > .../selftests/bpf/progs/test_snprintf.c | 71 +++++++++++++++++++ > 2 files changed, 142 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf.c > create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/snprintf.c b/tools/testing/selftests/bpf/prog_tests/snprintf.c > new file mode 100644 > index 000000000000..23af1dbd1eeb > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/snprintf.c > @@ -0,0 +1,71 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2021 Google LLC. */ > + > +#include <test_progs.h> > +#include "test_snprintf.skel.h" > + > +static int duration; if you drop CHECK() below, you won't need duration here at all > + > +#define EXP_NUM_OUT "-8 9 96 -424242 1337 DABBAD00" > +#define EXP_NUM_RET sizeof(EXP_NUM_OUT) > + > +#define EXP_IP_OUT "127.000.000.001 0000:0000:0000:0000:0000:0000:0000:0001" > +#define EXP_IP_RET sizeof(EXP_IP_OUT) > + > +/* The third specifier, %pB, depends on compiler inlining so don't check it */ > +#define EXP_SYM_OUT "schedule schedule+0x0/" > +#define MIN_SYM_RET sizeof(EXP_SYM_OUT) > + > +/* The third specifier, %p, is a hashed pointer which changes on every reboot */ > +#define EXP_ADDR_OUT "0000000000000000 ffff00000add4e55 " > +#define EXP_ADDR_RET sizeof(EXP_ADDR_OUT "unknownhashedptr") > + > +#define EXP_STR_OUT "str1 longstr" > +#define EXP_STR_RET sizeof(EXP_STR_OUT) > + > +#define EXP_OVER_OUT {'%', 'o', 'v', 'e', 'r'} > +#define EXP_OVER_RET 10 > + > +void test_snprintf(void) > +{ > + char exp_addr_out[] = EXP_ADDR_OUT; > + char exp_over_out[] = EXP_OVER_OUT; > + char exp_sym_out[] = EXP_SYM_OUT; > + struct test_snprintf *skel; > + int err; > + > + skel = test_snprintf__open_and_load(); > + if (CHECK(!skel, "skel_open", "failed to open and load skeleton\n")) ASSERT_OK_PTR > + return; > + > + err = test_snprintf__attach(skel); > + if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err)) ASSERT_OK > + goto cleanup; > + > + /* trigger tracepoint */ > + usleep(1); > + > + ASSERT_STREQ(skel->bss->num_out, EXP_NUM_OUT, "num_out"); > + ASSERT_EQ(skel->bss->num_ret, EXP_NUM_RET, "num_ret"); > + > + ASSERT_STREQ(skel->bss->ip_out, EXP_IP_OUT, "ip_out"); > + ASSERT_EQ(skel->bss->ip_ret, EXP_IP_RET, "ip_ret"); > + > + ASSERT_OK(memcmp(skel->bss->sym_out, exp_sym_out, > + sizeof(exp_sym_out) - 1), "sym_out"); > + ASSERT_LT(MIN_SYM_RET, skel->bss->sym_ret, "sym_ret"); > + > + ASSERT_OK(memcmp(skel->bss->addr_out, exp_addr_out, > + sizeof(exp_addr_out) - 1), "addr_out"); > + ASSERT_EQ(skel->bss->addr_ret, EXP_ADDR_RET, "addr_ret"); > + > + ASSERT_STREQ(skel->bss->str_out, EXP_STR_OUT, "str_out"); > + ASSERT_EQ(skel->bss->str_ret, EXP_STR_RET, "str_ret"); > + > + ASSERT_OK(memcmp(skel->bss->over_out, exp_over_out, > + sizeof(exp_over_out)), "over_out"); > + ASSERT_EQ(skel->bss->over_ret, EXP_OVER_RET, "over_ret"); > + > +cleanup: > + test_snprintf__destroy(skel); > +} > diff --git a/tools/testing/selftests/bpf/progs/test_snprintf.c b/tools/testing/selftests/bpf/progs/test_snprintf.c > new file mode 100644 > index 000000000000..6c8aa4988e69 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/test_snprintf.c > @@ -0,0 +1,71 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2021 Google LLC. */ > + > +#include <linux/bpf.h> > +#include <bpf/bpf_endian.h> > +#include <bpf/bpf_helpers.h> > +#include <bpf/bpf_tracing.h> > + > +#define OUT_LEN 64 > + > +/* Integer types */ > +static const char num_fmt[] = "%d %u %x %li %llu %lX"; > +#define NUMBERS -8, 9, 150, -424242, 1337, 0xDABBAD00 here I actually don't get the point of #define, can you please just inline them at the invocation place? I think that will be nicer and simpler (and will match common usage pattern) > + > +char num_out[OUT_LEN] = {}; > +long num_ret = 0; > + > +/* IP addresses */ > +static const char ip_fmt[] = "%pi4 %pI6"; > +static const __u8 dummy_ipv4[] = {127, 0, 0, 1}; /* 127.0.0.1 */ > +static const __u32 dummy_ipv6[] = {0, 0, 0, bpf_htonl(1)}; /* ::1/128 */ > +#define IPS &dummy_ipv4, &dummy_ipv6 > + > +char ip_out[OUT_LEN] = {}; > +long ip_ret = 0; > + > +/* Symbol lookup formatting */ > +static const char sym_fmt[] = "%ps %pS %pB"; > +extern const void schedule __ksym; > +#define SYMBOLS &schedule, &schedule, &schedule > + > +char sym_out[OUT_LEN] = {}; > +long sym_ret = 0; > + > +/* Kernel pointers */ > +static const char addr_fmt[] = "%pK %px %p"; > +#define ADDRESSES 0, 0xFFFF00000ADD4E55, 0xFFFF00000ADD4E55 > + > +char addr_out[OUT_LEN] = {}; > +long addr_ret = 0; > + > +/* Strings embedding */ > +static const char str_fmt[] = "%s %+05s"; > +static const char str1[] = "str1"; > +static const char longstr[] = "longstr"; > +#define STRINGS str1, longstr > + > +char str_out[OUT_LEN] = {}; > +long str_ret = 0; > + > +/* Overflow */ > +static const char over_fmt[] = "%%overflow"; > + > +#define OVER_OUT_LEN 6 > +char over_out[OVER_OUT_LEN] = {}; > +long over_ret = 0; > + same for all the above #defines, tests will be easier to follow if you just use value in BPF_SNPRINTF below > +SEC("raw_tp/sys_enter") > +int handler(const void *ctx) > +{ > + num_ret = BPF_SNPRINTF(num_out, OUT_LEN, num_fmt, NUMBERS); > + ip_ret = BPF_SNPRINTF(ip_out, OUT_LEN, ip_fmt, IPS); > + sym_ret = BPF_SNPRINTF(sym_out, OUT_LEN, sym_fmt, SYMBOLS); > + addr_ret = BPF_SNPRINTF(addr_out, OUT_LEN, addr_fmt, ADDRESSES); > + str_ret = BPF_SNPRINTF(str_out, OUT_LEN, str_fmt, STRINGS); > + over_ret = BPF_SNPRINTF(over_out, OVER_OUT_LEN, over_fmt); in practice you'd do BPF_SNPRINTF(num_out, sizeof(num_out), ...). So use that in test code as well please. > + > + return 0; > +} > + > +char _license[] SEC("license") = "GPL"; > -- > 2.30.1.766.gb4fecdf3b7-goog > ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2021-03-23 14:05 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-10 22:02 [PATCH bpf-next 0/5] Add a snprintf eBPF helper Florent Revest 2021-03-10 22:02 ` [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type Florent Revest 2021-03-11 0:04 ` kernel test robot 2021-03-11 1:00 ` kernel test robot 2021-03-16 1:03 ` Andrii Nakryiko 2021-03-16 23:58 ` Florent Revest 2021-03-17 0:35 ` Andrii Nakryiko 2021-03-17 0:45 ` Florent Revest 2021-03-17 1:02 ` Andrii Nakryiko 2021-03-17 10:32 ` Florent Revest 2021-03-10 22:02 ` [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper Florent Revest 2021-03-11 0:14 ` kernel test robot 2021-03-11 3:12 ` kernel test robot 2021-03-11 3:12 ` [RFC PATCH] bpf: check_bpf_snprintf_call() can be static kernel test robot 2021-03-16 1:25 ` [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper Andrii Nakryiko 2021-03-16 13:18 ` Florent Revest 2021-03-23 3:21 ` Alexei Starovoitov 2021-03-23 14:04 ` Florent Revest 2021-03-10 22:02 ` [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field Florent Revest 2021-03-16 4:36 ` Andrii Nakryiko 2021-03-16 4:41 ` Andrii Nakryiko 2021-03-16 22:43 ` Florent Revest 2021-03-16 23:06 ` Andrii Nakryiko 2021-03-10 22:02 ` [PATCH bpf-next 4/5] libbpf: Introduce a BPF_SNPRINTF helper macro Florent Revest 2021-03-16 4:39 ` Andrii Nakryiko 2021-03-10 22:02 ` [PATCH bpf-next 5/5] selftests/bpf: Add a series of tests for bpf_snprintf Florent Revest 2021-03-16 4:49 ` Andrii Nakryiko
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).