net: netfilter: add kfunc helper to update ct timeout
Subsystems (8)
v4 cover net: netfilter: add kfunc helper to update ct timeout
Changes since v3:
- split bpf_xdp_ct_add in bpf_xdp_ct_alloc/bpf_skb_ct_alloc and bpf_ct_insert_entry
- add verifier code to properly populate/configure ct entry
- improve selftests
Changes since v2:
- add bpf_xdp_ct_add and bpf_ct_refresh_timeout kfunc helpers
- remove conntrack dependency from selftests
- add support for forcing kfunc args to be referenced and related selftests
Changes since v1:
- add bpf_ct_refresh_timeout kfunc selftest
Kumar Kartikeya Dwivedi (11):
bpf: Add support for forcing kfunc args to be referenced
bpf: Print multiple type flags in verifier log
bpf: Support rdonly PTR_TO_BTF_ID for pointer to const return value
bpf: Support storing rdonly PTR_TO_BTF_ID in BPF maps
bpf: Support passing rdonly PTR_TO_BTF_ID to kfunc
bpf: Whitelist some fields in nf_conn for BPF_WRITE
bpf: Define acquire-release pairs for kfuncs selftests/bpf: Add verifier tests for forced kfunc ref args selftests/bpf: Add C tests for rdonly PTR_TO_BTF_ID selftests/bpf: Add verifier tests for rdonly PTR_TO_BTF_ID selftests/bpf: Add negative tests for bpf_nf
Lorenzo Bianconi (3):
net: netfilter: add kfunc helper to update ct timeout
net: netfilter: add kfunc helpers to alloc and insert a new ct entry selftests/bpf: add selftest for bpf_xdp_ct_add and
bpf_ct_refresh_timeout kfunc
include/linux/bpf.h | 17 +- include/linux/bpf_verifier.h | 1 + include/linux/btf.h | 40 ++ include/linux/filter.h | 3 + include/net/netfilter/nf_conntrack.h | 1 + include/net/netfilter/nf_conntrack_bpf.h | 5 + include/uapi/linux/bpf.h | 2 +- kernel/bpf/btf.c | 206 ++++++++-- kernel/bpf/helpers.c | 4 +- kernel/bpf/verifier.c | 110 ++++-- net/bpf/test_run.c | 20 +- net/core/filter.c | 28 ++ net/netfilter/nf_conntrack_bpf.c | 367 ++++++++++++++++-- net/netfilter/nf_conntrack_core.c | 23 +- tools/include/uapi/linux/bpf.h | 2 +- .../testing/selftests/bpf/prog_tests/bpf_nf.c | 58 ++- .../selftests/bpf/prog_tests/map_kptr.c | 9 +- tools/testing/selftests/bpf/progs/map_kptr.c | 31 +- .../selftests/bpf/progs/map_kptr_fail.c | 114 ++++++ .../testing/selftests/bpf/progs/test_bpf_nf.c | 87 ++++- .../selftests/bpf/progs/test_bpf_nf_fail.c | 73 ++++ tools/testing/selftests/bpf/test_verifier.c | 17 +- tools/testing/selftests/bpf/verifier/calls.c | 53 +++ .../testing/selftests/bpf/verifier/map_kptr.c | 156 ++++++++ 24 files changed, 1276 insertions(+), 151 deletions(-) create mode 100644 tools/testing/selftests/bpf/progs/test_bpf_nf_fail.c
Signature · 1 line
2.35.3
PATCH v4 1/14 bpf: Add support for forcing kfunc args to be referenced
From: Kumar Kartikeya Dwivedi <[email protected]>
Similar to how we detect mem, size pairs in kfunc, teach verifier to treat __ref suffix on argument name to imply that it must be a referenced pointer when passed to kfunc. This is required to ensure that kfunc that operate on some object only work on acquired pointers and not normal PTR_TO_BTF_ID with same type which can be obtained by pointer walking. Release functions need not specify such suffix on release arguments as they are already expected to receive one referenced argument.
--- kernel/bpf/btf.c | 42 ++++++++++++++++++++++++++++++++---------- net/bpf/test_run.c | 5 +++++ 2 files changed, 37 insertions(+), 10 deletions(-)
kernel/bpf/btf.c +32 -10
@@ -6023,18 +6023,13 @@ static bool __btf_type_is_scalar_struct(struct bpf_verifier_log *log,return true; } -static bool is_kfunc_arg_mem_size (const struct btf *btf, -const struct btf_param *arg, -const struct bpf_reg_state *reg) +static bool btf_param_match_suffix (const struct btf *btf, +const struct btf_param *arg, +const char *suffix) { -int len, sfx_len =sizeof ("__sz" ) -1 ; -const struct btf_type *t; +int len, sfx_len =strlen (suffix);const char *param_name; - t =btf_type_skip_modifiers (btf, arg->type,NULL ); -if (!btf_type_is_scalar (t) || reg->type !=SCALAR_VALUE ) -return false; -/* In the future, this can be ported to use BTF tagging */ param_name =btf_name_by_offset (btf, arg->name_off);if (str_is_empty (param_name))
@@ -6043,12 +6038,31 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,if (len < sfx_len)return false; param_name += len - sfx_len; -if (strncmp (param_name,"__sz" , sfx_len)) +if (strncmp (param_name, suffix, sfx_len))return false;return true; } +static bool is_kfunc_arg_ref (const struct btf *btf, +const struct btf_param *arg) +{ +return btf_param_match_suffix (btf, arg,"__ref" ); +} + +static bool is_kfunc_arg_mem_size (const struct btf *btf, +const struct btf_param *arg, +const struct bpf_reg_state *reg) +{ +const struct btf_type *t; + + t =btf_type_skip_modifiers (btf, arg->type,NULL ); +if (!btf_type_is_scalar (t) || reg->type !=SCALAR_VALUE ) +return false; + +return btf_param_match_suffix (btf, arg,"__sz" ); +} + static int btf_check_func_arg_match(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id, struct bpf_reg_state *regs,
@@ -6117,6 +6131,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,return -EINVAL ; } +/* Check if argument must be a referenced pointer, args + i has +* been verified to be a pointer (after skipping modifiers). +*/ +if (is_kfunc &&is_kfunc_arg_ref (btf, args + i) && !reg->ref_obj_id) { +bpf_log (log,"R%d must be referenced\n" , regno); +return -EINVAL ; + } + ref_t =btf_type_skip_modifiers (btf, t->type, &ref_id); ref_tname =btf_name_by_offset (btf, ref_t->name_off);
net/bpf/test_run.c +5 -0
@@ -691,6 +691,10 @@ noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len) { } +noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref) +{ +} +__diag_pop ();ALLOW_ERROR_INJECTION (bpf_modify_return_test,ERRNO );
@@ -714,6 +718,7 @@ BTF_ID(func, bpf_kfunc_call_test_fail3) BTF_ID(func, bpf_kfunc_call_test_mem_len_pass1) BTF_ID(func, bpf_kfunc_call_test_mem_len_fail1) BTF_ID(func, bpf_kfunc_call_test_mem_len_fail2) +BTF_ID(func, bpf_kfunc_call_test_ref)BTF_SET_END (test_sk_check_kfunc_ids)BTF_SET_START (test_sk_acquire_kfunc_ids)
Signature · 1 line
2.35.3
PATCH v4 2/14 bpf: Print multiple type flags in verifier log
In reply to Lorenzo BianconiFrom: Kumar Kartikeya Dwivedi <[email protected]>
Since MEM_RDONLY and PTR_UNTRUSTED can be present together in register type now, try to print multiple tags using the prefix buffer. Since all 5 cannot be present together, 32 bytes is still enough room for any possible combination. Instead of tracking the current position into the buffer, simply rely on snprintf, which also ensures nul termination.
--- kernel/bpf/verifier.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
kernel/bpf/verifier.c +6 -10
@@ -564,16 +564,12 @@ static const char *reg_type_str(struct bpf_verifier_env *env,strncpy (postfix,"_or_null" ,16 ); } -if (type &MEM_RDONLY ) -strncpy (prefix,"rdonly_" ,32 ); -if (type &MEM_ALLOC ) -strncpy (prefix,"alloc_" ,32 ); -if (type &MEM_USER ) -strncpy (prefix,"user_" ,32 ); -if (type &MEM_PERCPU ) -strncpy (prefix,"percpu_" ,32 ); -if (type &PTR_UNTRUSTED ) -strncpy (prefix,"untrusted_" ,32 ); +snprintf (prefix,sizeof (prefix),"%s%s%s%s%s" , + (type &MEM_RDONLY ?"rdonly_" :"" ), + (type &MEM_ALLOC ?"alloc_" :"" ), + (type &MEM_USER ?"user_" :"" ), + (type &MEM_PERCPU ?"percpu_" :"" ), + (type &PTR_UNTRUSTED ?"untrusted_" :"" ));snprintf (env->type_str_buf,TYPE_STR_BUF_LEN ,"%s%s%s" , prefix, str[base_type (type)], postfix);
Signature · 1 line
2.35.3
PATCH v4 3/14 bpf: Support rdonly PTR_TO_BTF_ID for pointer to const return value
In reply to Lorenzo BianconiFrom: Kumar Kartikeya Dwivedi <[email protected]>
Allow specifying MEM_RDONLY flag with PTR_TO_BTF_ID, which blocks write access to object even for cases where it is permitted using a custom btf_struct_access callback. This is currently set for return values from kfunc where it points to const struct.
This is not to mean that helpers cannot modify such a 'pointer to const struct', it's just that any write access that is usually permitted for such pointers in a program won't be allowed for this instance.
For RET_PTR_TO_MEM_OR_BTF_ID, MEM_RDONLY was previously folded because it didn't apply to PTR_TO_BTF_ID. Now, we check if variable is const and mark the pointer to it as MEM_RDONLY.
--- include/linux/btf.h | 29 ++++++++++++++++++++++++ kernel/bpf/btf.c | 24 -------------------- kernel/bpf/verifier.c | 39 ++++++++++++++++++++++++-------- net/bpf/test_run.c | 15 ++++++++++-- net/netfilter/nf_conntrack_bpf.c | 4 ++-- 5 files changed, 74 insertions(+), 37 deletions(-)
include/linux/btf.h +29 -0
@@ -234,6 +234,11 @@ static inline bool btf_type_is_typedef(const struct btf_type *t)return BTF_INFO_KIND (t->info) ==BTF_KIND_TYPEDEF ; } +static inline bool btf_type_is_const (const struct btf_type *t) +{ +return BTF_INFO_KIND (t->info) ==BTF_KIND_CONST ; +} +static inline bool btf_type_is_func (const struct btf_type *t) {return BTF_INFO_KIND (t->info) ==BTF_KIND_FUNC ;
@@ -264,6 +269,30 @@ static inline bool btf_type_is_struct(const struct btf_type *t)return kind ==BTF_KIND_STRUCT || kind ==BTF_KIND_UNION ; } +static inline bool btf_type_is_modifier (const struct btf_type *t) +{ +/* Some of them is not strictly a C modifier +* but they are grouped into the same bucket +* for BTF concern: +* A type (t) that refers to another +* type through t->type AND its size cannot +* be determined without following the t->type. +* +* ptr does not fall into this bucket +* because its size is always sizeof(void *). +*/ +switch (BTF_INFO_KIND (t->info)) { +case BTF_KIND_TYPEDEF : +case BTF_KIND_VOLATILE : +case BTF_KIND_CONST : +case BTF_KIND_RESTRICT : +case BTF_KIND_TYPE_TAG : +return true; + } + +return false; +} +static inline u16 btf_type_vlen (const struct btf_type *t) {return BTF_INFO_VLEN (t->info);
kernel/bpf/btf.c +0 -24
@@ -431,30 +431,6 @@ static int btf_resolve(struct btf_verifier_env *env,static int btf_func_check (struct btf_verifier_env *env,const struct btf_type *t); -static bool btf_type_is_modifier (const struct btf_type *t) -{ -/* Some of them is not strictly a C modifier -* but they are grouped into the same bucket -* for BTF concern: -* A type (t) that refers to another -* type through t->type AND its size cannot -* be determined without following the t->type. -* -* ptr does not fall into this bucket -* because its size is always sizeof(void *). -*/ -switch (BTF_INFO_KIND (t->info)) { -case BTF_KIND_TYPEDEF : -case BTF_KIND_VOLATILE : -case BTF_KIND_CONST : -case BTF_KIND_RESTRICT : -case BTF_KIND_TYPE_TAG : -return true; - } - -return false; -} -bool btf_type_is_void (const struct btf_type *t) {return t == &btf_void;
kernel/bpf/verifier.c +30 -9
@@ -4507,6 +4507,11 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, }if (env->ops->btf_struct_access) { +if (atype !=BPF_READ && reg->type &MEM_RDONLY ) { +verbose (env,"pointer points to const object, only read is supported\n" ); +return -EACCES ; + } + ret = env->ops->btf_struct_access (&env->log, reg->btf, t, off, size, atype, &btf_id, &flag); }else {
@@ -7316,9 +7321,15 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn regs[BPF_REG_0 ].mem_size = meta.mem_size; }else if (base_type (ret_type) ==RET_PTR_TO_MEM_OR_BTF_ID ) {const struct btf_type *t; +bool is_const = false;mark_reg_known_zero (env, regs,BPF_REG_0 ); - t =btf_type_skip_modifiers (meta.ret_btf, meta.ret_btf_id,NULL ); + t =btf_type_by_id (meta.ret_btf, meta.ret_btf_id); +while (btf_type_is_modifier (t)) { +if (btf_type_is_const (t)) + is_const = true; + t =btf_type_by_id (meta.ret_btf, t->type); + }if (!btf_type_is_struct (t)) {u32 tsize;const struct btf_type *ret;
@@ -7335,12 +7346,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn regs[BPF_REG_0 ].type =PTR_TO_MEM | ret_flag; regs[BPF_REG_0 ].mem_size = tsize; }else { -/* MEM_RDONLY may be carried from ret_flag, but it -* doesn't apply on PTR_TO_BTF_ID. Fold it, otherwise -* it will confuse the check of PTR_TO_BTF_ID in -* check_mem_access(). +/* MEM_RDONLY is carried from ret_flag. Fold it if the +* variable whose pointer is being returned is not +* const. */ - ret_flag &= ~MEM_RDONLY ; +if (!is_const) + ret_flag &= ~MEM_RDONLY ; regs[BPF_REG_0 ].type =PTR_TO_BTF_ID | ret_flag; regs[BPF_REG_0 ].btf = meta.ret_btf;
@@ -7535,8 +7546,17 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,mark_reg_unknown (env, regs,BPF_REG_0 );mark_btf_func_reg_size (env,BPF_REG_0 , t->size); }else if (btf_type_is_ptr (t)) { - ptr_type =btf_type_skip_modifiers (desc_btf, t->type, - &ptr_type_id); +bool is_const = false; + + ptr_type_id = t->type; + ptr_type =btf_type_by_id (desc_btf, ptr_type_id); +while (btf_type_is_modifier (ptr_type)) { +if (btf_type_is_const (ptr_type)) + is_const = true; + ptr_type_id = ptr_type->type; + ptr_type =btf_type_by_id (desc_btf, ptr_type_id); + } +if (!btf_type_is_struct (ptr_type)) { ptr_type_name =btf_name_by_offset (desc_btf, ptr_type->name_off);
@@ -7547,7 +7567,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, }mark_reg_known_zero (env, regs,BPF_REG_0 ); regs[BPF_REG_0 ].btf = desc_btf; - regs[BPF_REG_0 ].type =PTR_TO_BTF_ID ; + regs[BPF_REG_0 ].type =PTR_TO_BTF_ID | (is_const ?MEM_RDONLY :0 ); regs[BPF_REG_0 ].btf_id = ptr_type_id;if (btf_kfunc_id_set_contains (desc_btf,resolve_prog_type (env->prog),BTF_KFUNC_TYPE_RET_NULL , func_id)) {
@@ -13374,6 +13394,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)break ;case PTR_TO_BTF_ID :case PTR_TO_BTF_ID |PTR_UNTRUSTED : +case PTR_TO_BTF_ID |MEM_RDONLY :if (type ==BPF_READ ) { insn->code =BPF_LDX |BPF_PROBE_MEM |BPF_SIZE ((insn)->code);
net/bpf/test_run.c +13 -2
@@ -582,6 +582,12 @@ bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr)return &prog_test_struct; } +noinline const struct prog_test_ref_kfunc * +bpf_kfunc_call_test_acquire_const (void ) +{ +return bpf_kfunc_call_test_acquire (NULL ); +} + noinline struct prog_test_member *bpf_kfunc_call_memb_acquire (void ) {
@@ -589,12 +595,14 @@ bpf_kfunc_call_memb_acquire(void)return NULL ; } -noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) +noinline void bpf_kfunc_call_test_release(const struct prog_test_ref_kfunc *p) { +struct prog_test_ref_kfunc *pp = (void *)p; +if (!p)return ; -refcount_dec (&p->cnt); +refcount_dec (&pp->cnt); } noinline void bpf_kfunc_call_memb_release(struct prog_test_member *p)
@@ -704,6 +712,7 @@ BTF_ID(func, bpf_kfunc_call_test1) BTF_ID(func, bpf_kfunc_call_test2) BTF_ID(func, bpf_kfunc_call_test3) BTF_ID(func, bpf_kfunc_call_test_acquire) +BTF_ID(func, bpf_kfunc_call_test_acquire_const) BTF_ID(func, bpf_kfunc_call_memb_acquire) BTF_ID(func, bpf_kfunc_call_test_release)BTF_ID (func, bpf_kfunc_call_memb_release)
@@ -723,6 +732,7 @@ BTF_SET_END(test_sk_check_kfunc_ids) BTF_SET_START(test_sk_acquire_kfunc_ids) BTF_ID(func, bpf_kfunc_call_test_acquire) +BTF_ID(func, bpf_kfunc_call_test_acquire_const) BTF_ID(func, bpf_kfunc_call_memb_acquire) BTF_ID(func, bpf_kfunc_call_test_kptr_get) BTF_SET_END(test_sk_acquire_kfunc_ids)
@@ -735,6 +745,7 @@ BTF_SET_END(test_sk_release_kfunc_ids) BTF_SET_START(test_sk_ret_null_kfunc_ids) BTF_ID(func, bpf_kfunc_call_test_acquire) +BTF_ID(func, bpf_kfunc_call_test_acquire_const) BTF_ID(func, bpf_kfunc_call_memb_acquire) BTF_ID(func, bpf_kfunc_call_test_kptr_get) BTF_SET_END(test_sk_ret_null_kfunc_ids)
net/netfilter/nf_conntrack_bpf.c +2 -2
@@ -130,7 +130,7 @@ __diag_ignore_all("-Wmissing-prototypes",* @opts__sz - Length of the bpf_ct_opts structure * Must be NF_BPF_CT_OPTS_SZ (12) */ -struct nf_conn * +const struct nf_conn *bpf_xdp_ct_lookup (struct xdp_md *xdp_ctx,struct bpf_sock_tuple *bpf_tuple,u32 tuple__sz,struct bpf_ct_opts *opts,u32 opts__sz) {
@@ -173,7 +173,7 @@ bpf_xdp_ct_lookup(struct xdp_md *xdp_ctx, struct bpf_sock_tuple *bpf_tuple,* @opts__sz - Length of the bpf_ct_opts structure * Must be NF_BPF_CT_OPTS_SZ (12) */ -struct nf_conn * +const struct nf_conn *bpf_skb_ct_lookup (struct __sk_buff *skb_ctx,struct bpf_sock_tuple *bpf_tuple,u32 tuple__sz,struct bpf_ct_opts *opts,u32 opts__sz) {
Signature · 1 line
2.35.3
PATCH v4 4/14 bpf: Support storing rdonly PTR_TO_BTF_ID in BPF maps
In reply to Lorenzo BianconiFrom: Kumar Kartikeya Dwivedi <[email protected]>
User needs to declare kptr as pointer to const struct in map to be able to permit store of a read only PTR_TO_BTF_ID, be it trusted or untrusted. It is ofcourse allowed to also store normal PTR_TO_BTF_ID to such fields, but the next load will mark the register as read only PTR_TO_BTF_ID.
bpf_kptr_xchg now accepts read only PTR_TO_BTF_ID in addition to normal ones, and changes its second parameter type from void * to const void * so that when it is a prototype in BPF C, users can pass const declared pointers into it.
On load, the destination registers now gains MEM_RDONLY in addition to flags that were set previously. This why kptr needs to point to const struct, since we need to carry the MEM_RDONLY flag in the prog to map to prog chain.
In addition to this, loads for members now also set MEM_RDONLY when it is for pointer to const struct.
--- include/linux/bpf.h | 5 +++++ include/uapi/linux/bpf.h | 2 +- kernel/bpf/btf.c | 21 +++++++++++++++++++-- kernel/bpf/helpers.c | 4 ++-- kernel/bpf/verifier.c | 32 +++++++++++++++++++++----------- tools/include/uapi/linux/bpf.h | 2 +- 6 files changed, 49 insertions(+), 17 deletions(-)
include/linux/bpf.h +5 -0
@@ -169,6 +169,10 @@ enum bpf_kptr_type { BPF_KPTR_REF, }; +enum bpf_kptr_flags { +BPF_KPTR_F_RDONLY = (1 <<0 ), +}; +struct bpf_map_value_off_desc {u32 offset;enum bpf_kptr_type type;
@@ -177,6 +181,7 @@ struct bpf_map_value_off_desc {struct module *module;btf_dtor_kfunc_t dtor;u32 btf_id; +int flags; } kptr; };
include/uapi/linux/bpf.h +1 -1
@@ -5154,7 +5154,7 @@ union bpf_attr {* **-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if * invalid arguments are passed. * -* void *bpf_kptr_xchg(void *map_value, void *ptr) +* void *bpf_kptr_xchg(void *map_value, const void *ptr) * Description * Exchange kptr at pointer *map_value* with *ptr*, and return the * old value. *ptr* can be NULL, otherwise it must be a referenced
kernel/bpf/btf.c +19 -2
@@ -3174,6 +3174,7 @@ struct btf_field_info {u32 type_id;u32 off;enum bpf_kptr_type type; +int flags; }; static int btf_find_struct(const struct btf *btf, const struct btf_type *t,
@@ -3191,6 +3192,7 @@ static int btf_find_kptr(const struct btf *btf, const struct btf_type *t, u32 off, int sz, struct btf_field_info *info) { enum bpf_kptr_type type; +bool is_const = false;u32 res_id;/* For PTR, sz is always == 8 */
@@ -3211,7 +3213,13 @@ static int btf_find_kptr(const struct btf *btf, const struct btf_type *t,return -EINVAL ;/* Get the base type */ - t =btf_type_skip_modifiers (btf, t->type, &res_id); +do { + res_id = t->type; + t =btf_type_by_id (btf, res_id); +if (btf_type_is_const (t)) + is_const = true; + }while (btf_type_is_modifier (t)); +/* Only pointer to struct is allowed */ if (!__btf_type_is_struct (t))return -EINVAL ;
@@ -3219,6 +3227,7 @@ static int btf_find_kptr(const struct btf *btf, const struct btf_type *t, info->type_id = res_id; info->off = off; info->type = type; + info->flags = is_const ?BPF_KPTR_F_RDONLY :0 ;return BTF_FIELD_FOUND ; }
@@ -3473,6 +3482,7 @@ struct bpf_map_value_off *btf_parse_kptrs(const struct btf *btf, tab->off[i].kptr.btf_id = id; tab->off[i].kptr.btf = kernel_btf; tab->off[i].kptr.module = mod; + tab->off[i].kptr.flags = info_arr[i].flags; } tab->nr_off = nr_off;return tab;
@@ -5597,7 +5607,14 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf, tmp_flag =MEM_PERCPU ; } - stype =btf_type_skip_modifiers (btf, mtype->type, &id); + stype = mtype; +do { + id = stype->type; + stype =btf_type_by_id (btf, id); +if (btf_type_is_const (stype)) + tmp_flag |=MEM_RDONLY ; + }while (btf_type_is_modifier (stype)); +if (btf_type_is_struct (stype)) { *next_btf_id = id; *flag = tmp_flag;
kernel/bpf/helpers.c +2 -2
@@ -1390,7 +1390,7 @@ void bpf_timer_cancel_and_free(void *val)kfree (t); } -BPF_CALL_2(bpf_kptr_xchg, void *, map_value, void *, ptr) +BPF_CALL_2(bpf_kptr_xchg, void *, map_value, const void *, ptr) {unsigned long *kptr = map_value;
@@ -1408,7 +1408,7 @@ const struct bpf_func_proto bpf_kptr_xchg_proto = { .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, .ret_btf_id = BPF_PTR_POISON, .arg1_type = ARG_PTR_TO_KPTR, - .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL | OBJ_RELEASE, + .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL | MEM_RDONLY | OBJ_RELEASE, .arg2_btf_id = BPF_PTR_POISON, };
kernel/bpf/verifier.c +21 -11
@@ -3658,6 +3658,8 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,/* Only unreferenced case accepts untrusted pointers */ if (off_desc->type ==BPF_KPTR_UNREF ) perm_flags |=PTR_UNTRUSTED ; +if (off_desc->kptr.flags &BPF_KPTR_F_RDONLY ) + perm_flags |=MEM_RDONLY ;if (base_type (reg->type) !=PTR_TO_BTF_ID || (type_flag (reg->type) & ~perm_flags)) goto bad_type;
@@ -3756,6 +3758,8 @@ static int check_map_kptr_access(struct bpf_verifier_env *env, u32 regno, off_desc->kptr.btf_id, PTR_MAYBE_NULL | PTR_UNTRUSTED);/* For mark_ptr_or_null_reg */ val_reg->id = ++env->id_gen; +if (off_desc->kptr.flags &BPF_KPTR_F_RDONLY ) + val_reg->type |=MEM_RDONLY ; }else if (class ==BPF_STX ) { val_reg =reg_state (env, value_regno); if (!register_is_null(val_reg) &&
@@ -5732,20 +5736,13 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,for (i =0 ; i <ARRAY_SIZE (compatible->types); i++) { expected = compatible->types[i];if (expected ==NOT_INIT ) -break ; + goto error;if (type == expected) - goto found; +break ; } -verbose (env,"R%d type=%s expected=" , regno,reg_type_str (env, reg->type)); -for (j =0 ; j +1 < i; j++) -verbose (env,"%s, " ,reg_type_str (env, compatible->types[j])); -verbose (env,"%s\n" ,reg_type_str (env, compatible->types[j])); -return -EACCES ; - -found: -if (reg->type ==PTR_TO_BTF_ID ) { +if (base_type (reg->type) ==PTR_TO_BTF_ID ) {/* For bpf_sk_release, it needs to match against first member * 'struct sock_common', hence make an exception for it. This * allows bpf_sk_release to work for multiple socket types.
@@ -5753,6 +5750,10 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,bool strict_type_match =arg_type_is_release (arg_type) && meta->func_id != BPF_FUNC_sk_release; +if (type_flag (reg->type) &MEM_PERCPU ) + goto done; +if (type_flag (reg->type) & ~(PTR_MAYBE_NULL |MEM_RDONLY )) + goto error;if (!arg_btf_id) {if (!compatible->btf_id) {verbose (env,"verifier internal error: missing arg compatible BTF ID\n" );
@@ -5773,8 +5774,14 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,return -EACCES ; } } - +done:return 0 ; +error: +verbose (env,"R%d type=%s expected=" , regno,reg_type_str (env, reg->type)); +for (j =0 ; j +1 < i; j++) +verbose (env,"%s, " ,reg_type_str (env, compatible->types[j])); +verbose (env,"%s\n" ,reg_type_str (env, compatible->types[j])); +return -EACCES ; } int check_func_arg_reg_off(struct bpf_verifier_env *env,
@@ -7364,6 +7371,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insnmark_reg_known_zero (env, regs,BPF_REG_0 ); regs[BPF_REG_0 ].type =PTR_TO_BTF_ID | ret_flag;if (func_id == BPF_FUNC_kptr_xchg) { +if (meta.kptr_off_desc->kptr.flags &BPF_KPTR_F_RDONLY ) + regs[BPF_REG_0 ].type |=MEM_RDONLY ; ret_btf = meta.kptr_off_desc->kptr.btf; ret_btf_id = meta.kptr_off_desc->kptr.btf_id; }else {
@@ -13395,6 +13404,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)case PTR_TO_BTF_ID :case PTR_TO_BTF_ID |PTR_UNTRUSTED :case PTR_TO_BTF_ID |MEM_RDONLY : +case PTR_TO_BTF_ID |PTR_UNTRUSTED |MEM_RDONLY :if (type ==BPF_READ ) { insn->code =BPF_LDX |BPF_PROBE_MEM |BPF_SIZE ((insn)->code);
tools/include/uapi/linux/bpf.h +1 -1
@@ -5154,7 +5154,7 @@ union bpf_attr {* **-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if * invalid arguments are passed. * -* void *bpf_kptr_xchg(void *map_value, void *ptr) +* void *bpf_kptr_xchg(void *map_value, const void *ptr) * Description * Exchange kptr at pointer *map_value* with *ptr*, and return the * old value. *ptr* can be NULL, otherwise it must be a referenced
Signature · 1 line
2.35.3
PATCH v4 5/14 bpf: Support passing rdonly PTR_TO_BTF_ID to kfunc
In reply to Lorenzo BianconiFrom: Kumar Kartikeya Dwivedi <[email protected]>
The kfunc must declare its argument as pointer to const struct, which will be used to allow read only PTR_TO_BTF_ID to be only passed to such arguments. Normal PTR_TO_BTF_ID can also be passed to functions expecting read only PTR_TO_BTF_ID.
The kptr_get function for now doesn't support kptr_get on such kptr to const, however it can be supported in the future (by indicating const in the prototype of kptr_get kfunc).
--- kernel/bpf/btf.c | 22 ++++++++++++++++++++-- net/netfilter/nf_conntrack_bpf.c | 4 ++-- 2 files changed, 22 insertions(+), 4 deletions(-)
kernel/bpf/btf.c +20 -2
@@ -6109,6 +6109,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,enum bpf_arg_type arg_type =ARG_DONTCARE ;u32 regno = i +1 ;struct bpf_reg_state *reg = ®s[regno]; +bool is_ref_t_const = false; t =btf_type_skip_modifiers (btf, args[i].type,NULL );if (btf_type_is_scalar (t)) {
@@ -6132,7 +6133,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,return -EINVAL ; } - ref_t =btf_type_skip_modifiers (btf, t->type, &ref_id); + ref_id = t->type; + ref_t =btf_type_by_id (btf, ref_id); +while (btf_type_is_modifier (ref_t)) { +if (btf_type_is_const (ref_t)) + is_ref_t_const = true; + ref_id = ref_t->type; + ref_t =btf_type_by_id (btf, ref_id); + } ref_tname =btf_name_by_offset (btf, ref_t->name_off);if (rel && reg->ref_obj_id)
@@ -6166,6 +6174,11 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,return -EINVAL ; } +if (off_desc->kptr.flags &BPF_KPTR_F_RDONLY ) { +bpf_log (log,"arg#0 cannot raise reference for pointer to const\n" ); +return -EINVAL ; + } +if (!btf_type_is_ptr (ref_t)) {bpf_log (log,"arg#0 BTF type must be a double pointer\n" );return -EINVAL ;
@@ -6197,6 +6210,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,return -EINVAL ; } }else if (is_kfunc && (reg->type ==PTR_TO_BTF_ID || + reg->type == (PTR_TO_BTF_ID |MEM_RDONLY ) || (reg2btf_ids[base_type (reg->type)] && !type_flag (reg->type)))) {const struct btf_type *reg_ref_t;const struct btf *reg_btf;
@@ -6210,7 +6224,11 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,return -EINVAL ; } -if (reg->type ==PTR_TO_BTF_ID ) { +if (base_type (reg->type) ==PTR_TO_BTF_ID ) { +if ((reg->type &MEM_RDONLY ) && !is_ref_t_const) { +bpf_log (log,"cannot pass read only pointer to arg#%d" , i); +return -EINVAL ; + } reg_btf = reg->btf; reg_ref_id = reg->btf_id;/* Ensure only one argument is referenced PTR_TO_BTF_ID */
net/netfilter/nf_conntrack_bpf.c +2 -2
@@ -210,11 +210,11 @@ bpf_skb_ct_lookup(struct __sk_buff *skb_ctx, struct bpf_sock_tuple *bpf_tuple,* @nf_conn - Pointer to referenced nf_conn object, obtained using * bpf_xdp_ct_lookup or bpf_skb_ct_lookup. */ -void bpf_ct_release (struct nf_conn *nfct) +void bpf_ct_release (const struct nf_conn *nfct) {if (!nfct)return ; -nf_ct_put (nfct); +nf_ct_put ((struct nf_conn *)nfct); }__diag_pop ()
Signature · 1 line
2.35.3
PATCH v4 6/14 bpf: Whitelist some fields in nf_conn for BPF_WRITE
In reply to Lorenzo BianconiFrom: Kumar Kartikeya Dwivedi <[email protected]>
Since we want to allow user to set some fields in nf_conn after it is allocated but before it is inserted, we can permit BPF_WRITE for normal nf_conn, and then mark return value as read only on insert, preventing further BPF_WRITE. This way, nf_conn can be written to using normal BPF instructions after allocation, but not after insertion.
Note that we special nf_conn a bit here, inside the btf_struct_access callback for XDP and TC programs. Since this is the only struct for these programs requiring such adjustments, making this mechanism more generic has been left as an exercise for a future patch adding custom callbacks for more structs.
--- include/linux/bpf.h | 12 ++--- include/linux/filter.h | 3 ++ include/net/netfilter/nf_conntrack_bpf.h | 5 +++ kernel/bpf/verifier.c | 6 ++- net/core/filter.c | 28 ++++++++++++ net/netfilter/nf_conntrack_bpf.c | 56 +++++++++++++++++++++++- net/netfilter/nf_conntrack_core.c | 2 + 7 files changed, 105 insertions(+), 7 deletions(-)
include/linux/bpf.h +7 -5
@@ -640,6 +640,12 @@ struct bpf_prog_ops { union bpf_attr __user *uattr); }; +typedef int (*btf_struct_access_t )(struct bpf_verifier_log *log, +const struct btf *btf, +const struct btf_type *t,int off,int size, +enum bpf_access_type atype,u32 *next_btf_id, +enum bpf_type_flag *flag); +struct bpf_verifier_ops {/* return eBPF function prototype for verification */ const struct bpf_func_proto *
@@ -660,11 +666,7 @@ struct bpf_verifier_ops {const struct bpf_insn *src, struct bpf_insn *dst, struct bpf_prog *prog, u32 *target_size); -int (*btf_struct_access)(struct bpf_verifier_log *log, -const struct btf *btf, -const struct btf_type *t,int off,int size, -enum bpf_access_type atype, -u32 *next_btf_id,enum bpf_type_flag *flag); +btf_struct_access_t btf_struct_access; };struct bpf_prog_offload_ops {
include/linux/filter.h +3 -0
@@ -1389,6 +1389,9 @@ struct bpf_sk_lookup_kern {extern struct static_key_false bpf_sk_lookup_enabled; +extern btf_struct_access_t nf_conn_btf_struct_access; +extern struct mutex nf_conn_btf_struct_access_mtx; +/* Runners for BPF_SK_LOOKUP programs to invoke on socket lookup. * * Allowed return values for a BPF SK_LOOKUP program are SK_PASS and
include/net/netfilter/nf_conntrack_bpf.h +5 -0
@@ -10,6 +10,7 @@ (IS_MODULE(CONFIG_NF_CONNTRACK) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))extern int register_nf_conntrack_bpf (void ); +extern void unregister_nf_conntrack_bpf (void );#else
@@ -18,6 +19,10 @@ static inline int register_nf_conntrack_bpf(void)return 0 ; } +static inline void unregister_nf_conntrack_bpf (void ) +{ +} +#endif #endif /* _NF_CONNTRACK_BPF_H */
kernel/bpf/verifier.c +5 -1
@@ -13342,6 +13342,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)for (i =0 ; i < insn_cnt; i++, insn++) {bpf_convert_ctx_access_t convert_ctx_access; +enum bpf_prog_type prog_type;bool ctx_access; if (insn->code == (BPF_LDX | BPF_MEM | BPF_B) ||
@@ -13385,6 +13386,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)if (!ctx_access)continue ; + prog_type =resolve_prog_type (env->prog);switch ((int )env->insn_aux_data[i + delta].ptr_type) {case PTR_TO_CTX :if (!ops->convert_ctx_access)
@@ -13409,7 +13411,9 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env) insn->code =BPF_LDX |BPF_PROBE_MEM |BPF_SIZE ((insn)->code); env->prog->aux->num_exentries++; - }else if (resolve_prog_type (env->prog) !=BPF_PROG_TYPE_STRUCT_OPS ) { + }else if (prog_type !=BPF_PROG_TYPE_STRUCT_OPS && + prog_type !=BPF_PROG_TYPE_XDP && + prog_type !=BPF_PROG_TYPE_SCHED_CLS ) {verbose (env,"Writes through BTF pointers are not allowed\n" );return -EINVAL ; }
net/core/filter.c +28 -0
@@ -10453,6 +10453,32 @@ static u32 sk_msg_convert_ctx_access(enum bpf_access_type type,return insn - insn_buf; } +btf_struct_access_t nf_conn_btf_struct_access; +/* Protects nf_conn_btf_struct_access */ +DEFINE_MUTEX (nf_conn_btf_struct_access_mtx); + +static int xdp_tc_btf_struct_access (struct bpf_verifier_log *log, +const struct btf *btf, +const struct btf_type *t,int off,int size, +enum bpf_access_type atype, +u32 *next_btf_id,enum bpf_type_flag *flag) +{ +int ret; + +if (atype ==BPF_READ || !READ_ONCE (nf_conn_btf_struct_access)) + goto end; +mutex_lock (&nf_conn_btf_struct_access_mtx); +if (!nf_conn_btf_struct_access) + goto end_unlock; + ret =nf_conn_btf_struct_access (log, btf, t, off, size, atype, next_btf_id, flag); +mutex_unlock (&nf_conn_btf_struct_access_mtx); +return ret; +end_unlock: +mutex_unlock (&nf_conn_btf_struct_access_mtx); +end: +return btf_struct_access (log, btf, t, off, size, atype, next_btf_id, flag); +} +const struct bpf_verifier_ops sk_filter_verifier_ops = { .get_func_proto = sk_filter_func_proto, .is_valid_access = sk_filter_is_valid_access,
@@ -10470,6 +10496,7 @@ const struct bpf_verifier_ops tc_cls_act_verifier_ops = { .convert_ctx_access = tc_cls_act_convert_ctx_access, .gen_prologue = tc_cls_act_prologue, .gen_ld_abs = bpf_gen_ld_abs, + .btf_struct_access = xdp_tc_btf_struct_access, };const struct bpf_prog_ops tc_cls_act_prog_ops = {
@@ -10481,6 +10508,7 @@ const struct bpf_verifier_ops xdp_verifier_ops = { .is_valid_access = xdp_is_valid_access, .convert_ctx_access = xdp_convert_ctx_access, .gen_prologue = bpf_noop_prologue, + .btf_struct_access = xdp_tc_btf_struct_access, };const struct bpf_prog_ops xdp_prog_ops = {
net/netfilter/nf_conntrack_bpf.c +55 -1
72 lines not on this page · raw message
net/netfilter/nf_conntrack_core.c +2 -0
@@ -2497,6 +2497,8 @@ void nf_conntrack_cleanup_start(void)void nf_conntrack_cleanup_end (void ) { +unregister_nf_conntrack_bpf (); +RCU_INIT_POINTER (nf_ct_hook,NULL );cancel_delayed_work_sync (&conntrack_gc_work.dwork);kvfree (nf_conntrack_hash);
Signature · 1 line
2.35.3
Florian Westphal
maintainerRe: [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE
Quoted 8 lines · Lorenzo Bianconi
From: Kumar Kartikeya Dwivedi <[email protected]>
Since we want to allow user to set some fields in
nf_connafter it is allocated but before it is inserted, we can permitBPF_WRITEfor normalnf_conn, and then mark return value as read only on insert, preventing furtherBPF_WRITE. This way,nf_conncan be written to using normal BPF instructions after allocation, but not after insertion.
Quoted 5 lines
Note that we special
nf_conna bit here, inside thebtf_struct_accesscallback for XDP and TC programs. Since this is the only struct for these programs requiring such adjustments, making this mechanism more generic has been left as an exercise for a future patch adding custom callbacks for more structs.
Are you sure this is safe?
As far as I can see this allows nf_conn->status = ~0ul.
I'm fairly sure this isn't a good idea, see nf_ct_delete() for example.
Kumar Kartikeya Dwivedi
maintainerRe: [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE
Quoted 15 lines · Florian Westphal
Quoted 8 lines · Lorenzo Bianconi
From: Kumar Kartikeya Dwivedi <[email protected]>
Since we want to allow user to set some fields in
nf_connafter it is allocated but before it is inserted, we can permitBPF_WRITEfor normalnf_conn, and then mark return value as read only on insert, preventing furtherBPF_WRITE. This way,nf_conncan be written to using normal BPF instructions after allocation, but not after insertion.Quoted 5 lines
Note that we special
nf_conna bit here, inside thebtf_struct_accesscallback for XDP and TC programs. Since this is the only struct for these programs requiring such adjustments, making this mechanism more generic has been left as an exercise for a future patch adding custom callbacks for more structs.
Quoted 3 lines
Are you sure this is safe?
As far as I can see this allowsnf_conn->status= ~0ul.
I'm fairly sure this isn't a good idea, seenf_ct_delete()for example.
This only allows writing to an allocated but not yet inserted nf_conn. The idea was that insert checks whether ct->status only has permitted bits set before making the entry visible, and then we make nf_conn pointer read only, however the runtime check seems to be missing right now in patch 12; something to fix in v5. With that sorted, would it be fine?
--
Kartikeya
Florian Westphal
maintainerRe: [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE
Quoted 20 lines · Kumar Kartikeya Dwivedi
Quoted 15 lines · Florian Westphal
Quoted 8 lines · Lorenzo Bianconi
From: Kumar Kartikeya Dwivedi <[email protected]>
Since we want to allow user to set some fields in
nf_connafter it is allocated but before it is inserted, we can permitBPF_WRITEfor normalnf_conn, and then mark return value as read only on insert, preventing furtherBPF_WRITE. This way,nf_conncan be written to using normal BPF instructions after allocation, but not after insertion.Quoted 5 lines
Note that we special
nf_conna bit here, inside thebtf_struct_accesscallback for XDP and TC programs. Since this is the only struct for these programs requiring such adjustments, making this mechanism more generic has been left as an exercise for a future patch adding custom callbacks for more structs.Quoted 3 lines
Are you sure this is safe?
As far as I can see this allowsnf_conn->status= ~0ul.
I'm fairly sure this isn't a good idea, seenf_ct_delete()for example.
Quoted 5 lines
This only allows writing to an allocated but not yet inserted
nf_conn. The idea was that insert checks whetherct->statusonly has permitted bits set before making the entry visible, and then we makenf_connpointer read only, however the runtime check seems to be missing right now in patch 12; something to fix in v5. With that sorted, would it be fine?
Its fragile, e.g. what if I set TEMPLATE bit? If refcount goes down to 0, object is released via kfree() instead of kmem_cache_free.
What if I clear SNAT_DONE bit? Would it leave the (freed) entry on the bysource hash list (see nf_nat_core.c)?
Or is there some magic that prevents this from happening? I have no idea how processing pipeline looks like...
kernel test robot
build botRe: [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE
In reply to Lorenzo BianconiThank you for the patch! Yet something to improve:
Hi Lorenzo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Bianconi/net-netfilter-add-kfunc-helper-to-update-ct-timeout/20220527-053913 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20220527/[email protected]/config) compiler: gcc-11 (Debian 11.3.0-1) 11.3.0 reproduce (this is a W=1 build): # https://github.com/intel-lab-lkp/linux/commit/c346565af9b023d9231ca8fca2e1b8c66a782f84 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Lorenzo-Bianconi/net-netfilter-add-kfunc-helper-to-update-ct-timeout/20220527-053913 git checkout c346565af9b023d9231ca8fca2e1b8c66a782f84 # save the config file mkdir build_dir && cp config build_dir/.config make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
All errors (new ones prefixed by >>):
net/core/filter.c: In function 'xdp_tc_btf_struct_access':
>> net/core/filter.c:10479:16: error: implicit declaration of function 'btf_struct_access'; did you mean 'xdp_tc_btf_struct_access'? [-Werror=implicit-function-declaration]
10479 | return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
| ^~~~~~~~~~~~~~~~~
| xdp_tc_btf_struct_accesscc1: some warnings being treated as errors
vim +10479 net/core/filter.c
10459
10460 static int xdp_tc_btf_struct_access(struct bpf_verifier_log *log,
10461 const struct btf *btf, 10462 const struct btf_type *t, int off, int size, 10463 enum bpf_access_type atype, 10464 u32 *next_btf_id, enum bpf_type_flag *flag)
10465 {
10466 int ret;
10467
10468 if (atype == BPF_READ || !READ_ONCE(nf_conn_btf_struct_access))
10469 goto end;
10470 mutex_lock(&nf_conn_btf_struct_access_mtx);
10471 if (!nf_conn_btf_struct_access)
10472 goto end_unlock;
10473 ret = nf_conn_btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
10474 mutex_unlock(&nf_conn_btf_struct_access_mtx);
10475 return ret;
10476 end_unlock:
10477 mutex_unlock(&nf_conn_btf_struct_access_mtx);
10478 end:
10479 return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
10480 }
10481
Signature · 2 lines
0-DAY CI Kernel Test Service https://01.org/lkp
kernel test robot
build botRe: [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE
In reply to Lorenzo BianconiThank you for the patch! Yet something to improve:
Hi Lorenzo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Bianconi/net-netfilter-add-kfunc-helper-to-update-ct-timeout/20220527-053913 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20220527/[email protected]/config) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 134d7f9a4b97e9035150d970bd9e376043c4577e)
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/intel-lab-lkp/linux/commit/c346565af9b023d9231ca8fca2e1b8c66a782f84
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Lorenzo-Bianconi/net-netfilter-add-kfunc-helper-to-update-ct-timeout/20220527-053913 git checkout c346565af9b023d9231ca8fca2e1b8c66a782f84
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash net/
If you fix the issue, kindly add following tag where applicable
All errors (new ones prefixed by >>):
>> net/core/filter.c:10479:9: error: call to undeclared function 'btf_struct_access'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
^1 error generated.
vim +/btf_struct_access +10479 net/core/filter.c
10459
10460 static int xdp_tc_btf_struct_access(struct bpf_verifier_log *log,
10461 const struct btf *btf, 10462 const struct btf_type *t, int off, int size, 10463 enum bpf_access_type atype, 10464 u32 *next_btf_id, enum bpf_type_flag *flag)
10465 {
10466 int ret;
10467
10468 if (atype == BPF_READ || !READ_ONCE(nf_conn_btf_struct_access))
10469 goto end;
10470 mutex_lock(&nf_conn_btf_struct_access_mtx);
10471 if (!nf_conn_btf_struct_access)
10472 goto end_unlock;
10473 ret = nf_conn_btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
10474 mutex_unlock(&nf_conn_btf_struct_access_mtx);
10475 return ret;
10476 end_unlock:
10477 mutex_unlock(&nf_conn_btf_struct_access_mtx);
10478 end:
10479 return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
10480 }
10481
Signature · 2 lines
0-DAY CI Kernel Test Service https://01.org/lkp
kernel test robot
build botRe: [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE
In reply to Lorenzo BianconiThank you for the patch! Yet something to improve:
Hi Lorenzo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Bianconi/net-netfilter-add-kfunc-helper-to-update-ct-timeout/20220527-053913 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master config: x86_64-rhel-8.3-kselftests (https://download.01.org/0day-ci/archive/20220527/[email protected]/config) compiler: gcc-11 (Debian 11.3.0-1) 11.3.0 reproduce (this is a W=1 build): # https://github.com/intel-lab-lkp/linux/commit/c346565af9b023d9231ca8fca2e1b8c66a782f84 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Lorenzo-Bianconi/net-netfilter-add-kfunc-helper-to-update-ct-timeout/20220527-053913 git checkout c346565af9b023d9231ca8fca2e1b8c66a782f84 # save the config file mkdir build_dir && cp config build_dir/.config make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "btf_type_by_id" [net/netfilter/nf_conntrack.ko] undefined! >> ERROR: modpost: "nf_conn_btf_struct_access_mtx" [net/netfilter/nf_conntrack.ko] undefined! >> ERROR: modpost: "bpf_log" [net/netfilter/nf_conntrack.ko] undefined! >> ERROR: modpost: "nf_conn_btf_struct_access" [net/netfilter/nf_conntrack.ko] undefined!
Signature · 2 lines
0-DAY CI Kernel Test Service https://01.org/lkp
PATCH v4 7/14 bpf: Define acquire-release pairs for kfuncs
In reply to Lorenzo BianconiFrom: Kumar Kartikeya Dwivedi <[email protected]>
Introduce support for specifying pairings of acquire release kfuncs, so that when there are multiple candidates that may produce the same type of referenced PTR_TO_BTF_ID, it is clear which ones are allowed and which ones are not. This is an additional check performed after the typical type checks kfuncs are subjected to.
This is needed because we want to prevent bpf_ct_release for pointer obtained from bpf_xdp_ct_alloc, and only allow it to be passed to insert kfunc. Since bpf_ct_release takes const parameter, it can take rdonly and non-rdonly PTR_TO_BTF_ID. To avoid this confusion in case of multiple candidate release kfuncs, we can define a strict pairing between acquire and release kfuncs that will be checked and enforced only if defined. The only condition is that an acquire kfunc either has no mappings, or defines all possible mappings.
--- include/linux/bpf_verifier.h | 1 + include/linux/btf.h | 11 ++++ kernel/bpf/btf.c | 99 +++++++++++++++++++++++++++++++- kernel/bpf/verifier.c | 17 +++++- net/netfilter/nf_conntrack_bpf.c | 31 ++++++---- 5 files changed, 147 insertions(+), 12 deletions(-)
include/linux/bpf_verifier.h +1 -0
@@ -68,6 +68,7 @@ struct bpf_reg_state {struct {struct btf *btf;u32 btf_id; +u32 acq_kfunc_btf_id; };u32 mem_size;/* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */
include/linux/btf.h +11 -0
@@ -40,6 +40,8 @@ struct btf_kfunc_id_set { };struct btf_id_set *sets[BTF_KFUNC_TYPE_MAX ]; }; +u32 *acq_rel_pairs; +u32 acq_rel_pairs_cnt; };struct btf_id_dtor_kfunc {
@@ -382,6 +384,9 @@ struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);bool btf_kfunc_id_set_contains (const struct btf *btf,enum bpf_prog_type prog_type,enum btf_kfunc_type type,u32 kfunc_btf_id); +int btf_kfunc_match_acq_rel_pair (const struct btf *btf, +enum bpf_prog_type prog_type, +u32 acq_kfunc_btf_id,u32 rel_kfunc_btf_id);int register_btf_kfunc_id_set (enum bpf_prog_type prog_type,const struct btf_kfunc_id_set *s);s32 btf_find_dtor_kfunc (struct btf *btf,u32 btf_id);
@@ -405,6 +410,12 @@ static inline bool btf_kfunc_id_set_contains(const struct btf *btf, {return false; } +static inline int btf_kfunc_match_acq_rel_pair (const struct btf *btf, +enum bpf_prog_type prog_type, +u32 acq_kfunc_btf_id,u32 rel_kfunc_btf_id) +{ +return 0 ; +}static inline int register_btf_kfunc_id_set (enum bpf_prog_type prog_type,const struct btf_kfunc_id_set *s) {
kernel/bpf/btf.c +98 -1
135 lines not on this page · raw message
kernel/bpf/verifier.c +16 -1
@@ -7532,7 +7532,21 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,* PTR_TO_BTF_ID back from btf_check_kfunc_arg_match, do the release now */ if (err) { - err =release_reference (env, regs[err].ref_obj_id); +int regno = err; + +if (regs[regno].acq_kfunc_btf_id && desc_btf != regs[regno].btf) { +verbose (env,"verifier internal error: acquire and release kfunc BTF must match" ); +return -EFAULT ; + } + err =btf_kfunc_match_acq_rel_pair (desc_btf,resolve_prog_type (env->prog), + regs[regno].acq_kfunc_btf_id, func_id); +if (err) { +if (err == -ENOENT ) +verbose (env,"kfunc %s#%d not permitted to release reference\n" , + func_name, func_id); +return err; + } + err =release_reference (env, regs[regno].ref_obj_id);if (err) {verbose (env,"kfunc %s#%d reference has not been acquired before\n" , func_name, func_id);
@@ -7592,6 +7606,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,return id; regs[BPF_REG_0 ].id = id; regs[BPF_REG_0 ].ref_obj_id = id; + regs[BPF_REG_0 ].acq_kfunc_btf_id = func_id; } }/* else { add_kfunc_call() ensures it is btf_type_is_void(t) } */
net/netfilter/nf_conntrack_bpf.c +21 -10
@@ -243,20 +243,31 @@ BTF_SET_END(nf_ct_release_kfunc_ids)/* Both sets are identical */ #define nf_ct_ret_null_kfunc_ids nf_ct_acquire_kfunc_ids +BTF_ID_LIST (nf_ct_acq_rel_pairs) +BTF_ID (func ,bpf_xdp_ct_lookup ) +BTF_ID (func, bpf_ct_release) + +BTF_ID (func, bpf_skb_ct_lookup) +BTF_ID (func, bpf_ct_release) + static const struct btf_kfunc_id_set nf_conntrack_xdp_kfunc_set = { - .owner =THIS_MODULE , - .check_set = &nf_ct_xdp_check_kfunc_ids, - .acquire_set = &nf_ct_acquire_kfunc_ids, - .release_set = &nf_ct_release_kfunc_ids, - .ret_null_set = &nf_ct_ret_null_kfunc_ids, + .owner =THIS_MODULE , + .check_set = &nf_ct_xdp_check_kfunc_ids, + .acquire_set = &nf_ct_acquire_kfunc_ids, + .release_set = &nf_ct_release_kfunc_ids, + .ret_null_set = &nf_ct_ret_null_kfunc_ids, + .acq_rel_pairs = nf_ct_acq_rel_pairs, + .acq_rel_pairs_cnt =10 , };static const struct btf_kfunc_id_set nf_conntrack_tc_kfunc_set = { - .owner =THIS_MODULE , - .check_set = &nf_ct_tc_check_kfunc_ids, - .acquire_set = &nf_ct_acquire_kfunc_ids, - .release_set = &nf_ct_release_kfunc_ids, - .ret_null_set = &nf_ct_ret_null_kfunc_ids, + .owner =THIS_MODULE , + .check_set = &nf_ct_tc_check_kfunc_ids, + .acquire_set = &nf_ct_acquire_kfunc_ids, + .release_set = &nf_ct_release_kfunc_ids, + .ret_null_set = &nf_ct_ret_null_kfunc_ids, + .acq_rel_pairs = nf_ct_acq_rel_pairs, + .acq_rel_pairs_cnt =10 , };BTF_ID_LIST_SINGLE (nf_conn_btf_id, struct, nf_conn)
Signature · 1 line
2.35.3
PATCH v4 8/14 selftests/bpf: Add verifier tests for forced kfunc ref args
In reply to Lorenzo BianconiFrom: Kumar Kartikeya Dwivedi <[email protected]>
Make sure verifier rejects the bad cases and ensure the good case keeps working. The selftests make use of the bpf_kfunc_call_test_ref kfunc added in the previous patch only for verification.
--- tools/testing/selftests/bpf/verifier/calls.c | 53 ++++++++++++++++++++ 1 file changed, 53 insertions(+)
tools/testing/selftests/bpf/verifier/calls.c +53 -0
@@ -218,6 +218,59 @@ .result = REJECT, .errstr = "variable ptr_ access var_off=(0x0; 0x7) disallowed", }, +{ + "calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID", + .insns = { +BPF_MOV64_REG (BPF_REG_1 ,BPF_REG_10 ), +BPF_ALU64_IMM (BPF_ADD ,BPF_REG_1 ,-8 ), +BPF_ST_MEM (BPF_DW ,BPF_REG_1 ,0 ,0 ), +BPF_RAW_INSN (BPF_JMP |BPF_CALL ,0 ,BPF_PSEUDO_KFUNC_CALL ,0 ,0 ), +BPF_JMP_IMM (BPF_JNE ,BPF_REG_0 ,0 ,1 ), +BPF_EXIT_INSN (), +BPF_MOV64_REG (BPF_REG_6 ,BPF_REG_0 ), +BPF_MOV64_REG (BPF_REG_1 ,BPF_REG_0 ), +BPF_RAW_INSN (BPF_JMP |BPF_CALL ,0 ,BPF_PSEUDO_KFUNC_CALL ,0 ,0 ), +BPF_LDX_MEM (BPF_DW ,BPF_REG_1 ,BPF_REG_6 ,16 ), +BPF_RAW_INSN (BPF_JMP |BPF_CALL ,0 ,BPF_PSEUDO_KFUNC_CALL ,0 ,0 ), +BPF_MOV64_IMM (BPF_REG_0 ,0 ), + BPF_EXIT_INSN(), + }, + .prog_type =BPF_PROG_TYPE_SCHED_CLS , + .fixup_kfunc_btf_id = { + { "bpf_kfunc_call_test_acquire", 3 }, + { "bpf_kfunc_call_test_ref", 8 }, + { "bpf_kfunc_call_test_ref", 10 }, + }, + .result_unpriv = REJECT, + .result = REJECT, + .errstr = "R1 must be referenced", +}, +{ +"calls: valid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID" , + .insns = { +BPF_MOV64_REG (BPF_REG_1 ,BPF_REG_10 ), +BPF_ALU64_IMM (BPF_ADD ,BPF_REG_1 ,-8 ), +BPF_ST_MEM (BPF_DW ,BPF_REG_1 ,0 ,0 ), +BPF_RAW_INSN (BPF_JMP |BPF_CALL ,0 ,BPF_PSEUDO_KFUNC_CALL ,0 ,0 ), +BPF_JMP_IMM (BPF_JNE ,BPF_REG_0 ,0 ,1 ), +BPF_EXIT_INSN (), +BPF_MOV64_REG (BPF_REG_6 ,BPF_REG_0 ), +BPF_MOV64_REG (BPF_REG_1 ,BPF_REG_0 ), +BPF_RAW_INSN (BPF_JMP |BPF_CALL ,0 ,BPF_PSEUDO_KFUNC_CALL ,0 ,0 ), +BPF_MOV64_REG (BPF_REG_1 ,BPF_REG_6 ), +BPF_RAW_INSN (BPF_JMP |BPF_CALL ,0 ,BPF_PSEUDO_KFUNC_CALL ,0 ,0 ), +BPF_MOV64_IMM (BPF_REG_0 ,0 ), + BPF_EXIT_INSN(), + }, + .prog_type =BPF_PROG_TYPE_SCHED_CLS , + .fixup_kfunc_btf_id = { + { "bpf_kfunc_call_test_acquire", 3 }, + { "bpf_kfunc_call_test_ref", 8 }, + { "bpf_kfunc_call_test_release", 10 }, + }, + .result_unpriv = REJECT, + .result = ACCEPT, +}, {"calls: basic sanity" , .insns = {
Signature · 1 line
2.35.3
PATCH v4 9/14 selftests/bpf: Add C tests for rdonly PTR_TO_BTF_ID
In reply to Lorenzo BianconiFrom: Kumar Kartikeya Dwivedi <[email protected]>
Both the return value of bpf_kptr_xchg and load of read only kptr must be marked with MEM_RDONLY.
--- .../selftests/bpf/prog_tests/map_kptr.c | 9 +- tools/testing/selftests/bpf/progs/map_kptr.c | 31 ++++- .../selftests/bpf/progs/map_kptr_fail.c | 114 ++++++++++++++++++ 3 files changed, 152 insertions(+), 2 deletions(-)
tools/testing/selftests/bpf/prog_tests/map_kptr.c +8 -1
@@ -36,6 +36,13 @@ struct { { "reject_indirect_global_func_access", "kptr cannot be accessed indirectly by helper" }, { "kptr_xchg_ref_state", "Unreleased reference id=5 alloc_insn=" }, { "kptr_get_ref_state", "Unreleased reference id=3 alloc_insn=" }, + { "kptr_const_to_non_const", "invalid kptr access, R0 type=rdonly_ptr_" }, + { "kptr_const_to_non_const_xchg", "invalid kptr access, R2 type=rdonly_ptr_" }, + { "kptr_const_or_null_to_non_const_xchg", "invalid kptr access, R2 type=rdonly_ptr_or_null_" }, + { "mark_rdonly", "R1 type=rdonly_untrusted_ptr_or_null_ expected=percpu_ptr_" }, + { "mark_ref_rdonly", "R1 type=rdonly_untrusted_ptr_or_null_ expected=percpu_ptr_" }, + { "mark_xchg_rdonly", "R1 type=rdonly_ptr_or_null_ expected=percpu_ptr_" }, + { "kptr_get_no_const", "arg#0 cannot raise reference for pointer to const" }, };static void test_map_kptr_fail_prog (const char *prog_name,const char *err_msg)
@@ -91,7 +98,7 @@ static void test_map_kptr_success(bool test_run) );struct map_kptr *skel;int key =0 , ret; -char buf[16 ]; +char buf[32 ]; skel =map_kptr__open_and_load ();if (!ASSERT_OK_PTR (skel,"map_kptr__open_and_load" ))
tools/testing/selftests/bpf/progs/map_kptr.c +30 -1
@@ -6,6 +6,8 @@struct map_value { struct prog_test_ref_kfunc __kptr *unref_ptr; struct prog_test_ref_kfunc __kptr_ref *ref_ptr; + const struct prog_test_ref_kfunc __kptr *const_unref_ptr; + const struct prog_test_ref_kfunc __kptr_ref *const_ref_ptr; };struct array_map {
@@ -58,12 +60,14 @@ DEFINE_MAP_OF_MAP(BPF_MAP_TYPE_HASH_OF_MAPS, hash_malloc_map, hash_of_hash_malloDEFINE_MAP_OF_MAP (BPF_MAP_TYPE_HASH_OF_MAPS , lru_hash_map, hash_of_lru_hash_maps);extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire (unsigned long *sp) __ksym; +extern const struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire_const (void ) __ksym;extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_kptr_get (struct prog_test_ref_kfunc **p,int a,int b) __ksym; -extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym; +extern void bpf_kfunc_call_test_release(const struct prog_test_ref_kfunc *p) __ksym;static void test_kptr_unref (struct map_value *v) { +const struct prog_test_ref_kfunc *pc;struct prog_test_ref_kfunc *p; p = v->unref_ptr;
@@ -77,10 +81,21 @@ static void test_kptr_unref(struct map_value *v) v->unref_ptr = p;/* store NULL */ v->unref_ptr =NULL ; + + pc = v->const_ref_ptr; +/* store rdonly_untrusted_ptr_or_null_ */ + v->const_unref_ptr = pc; +if (!pc) +return ; +/* store rdonly_untrusted_ptr_ */ + v->const_unref_ptr = pc; +/* store NULL */ + v->const_unref_ptr =NULL ; }static void test_kptr_ref (struct map_value *v) { +const struct prog_test_ref_kfunc *pc;struct prog_test_ref_kfunc *p; p = v->ref_ptr;
@@ -114,6 +129,20 @@ static void test_kptr_ref(struct map_value *v)return ; }bpf_kfunc_call_test_release (p); + + pc =bpf_kptr_xchg (&v->const_ref_ptr,NULL ); +if (!pc) +return ; +/* store rdonly_ptr_ */ + v->const_unref_ptr = pc; +bpf_kfunc_call_test_release (pc); + + pc =bpf_kfunc_call_test_acquire_const (); +if (!pc) +return ; + v->const_unref_ptr = pc; +bpf_kfunc_call_test_release (pc); + v->const_unref_ptr = v->const_ref_ptr; }static void test_kptr_get (struct map_value *v)
tools/testing/selftests/bpf/progs/map_kptr_fail.c +114 -0
130 lines not on this page · raw message
Signature · 1 line
2.35.3
PATCH v4 10/14 selftests/bpf: Add verifier tests for rdonly PTR_TO_BTF_ID
In reply to Lorenzo BianconiFrom: Kumar Kartikeya Dwivedi <[email protected]>
Add matching verifier tests which ensure the read only flag is set and handled appropriately.
--- tools/testing/selftests/bpf/test_verifier.c | 17 +- .../testing/selftests/bpf/verifier/map_kptr.c | 156 ++++++++++++++++++ 2 files changed, 169 insertions(+), 4 deletions(-)
tools/testing/selftests/bpf/test_verifier.c +13 -4
@@ -626,6 +626,8 @@ static int create_cgroup_storage(bool percpu)* struct prog_test_ref_kfunc __kptr *ptr; * struct prog_test_ref_kfunc __kptr_ref *ptr; * struct prog_test_member __kptr_ref *ptr; +* const struct prog_test_ref_kfunc __kptr *ptr; +* const struct prog_test_ref_kfunc __kptr_ref *ptr; * } */ static const char btf_str_sec[] ="\0bpf_spin_lock\0val\0cnt\0l\0bpf_timer\0timer\0t"
@@ -657,11 +659,18 @@ static __u32 btf_raw_types[] = {BTF_PTR_ENC (8 ),/* [11] */ BTF_PTR_ENC (9 ),/* [12] */ BTF_PTR_ENC (10 ),/* [13] */ -/* struct btf_ptr */ /* [14] */ -BTF_STRUCT_ENC (43 ,3 ,24 ), +BTF_CONST_ENC (6 ),/* [14] */ +BTF_TYPE_TAG_ENC (75 ,14 ),/* [15] */ +BTF_TYPE_TAG_ENC (80 ,14 ),/* [16] */ +BTF_PTR_ENC (15 ),/* [17] */ +BTF_PTR_ENC (16 ),/* [18] */ +/* struct btf_ptr */ /* [19] */ +BTF_STRUCT_ENC (43 ,5 ,40 ),BTF_MEMBER_ENC (71 ,11 ,0 ),/* struct prog_test_ref_kfunc __kptr *ptr; */ BTF_MEMBER_ENC (71 ,12 ,64 ),/* struct prog_test_ref_kfunc __kptr_ref *ptr; */ BTF_MEMBER_ENC (71 ,13 ,128 ),/* struct prog_test_member __kptr_ref *ptr; */ +BTF_MEMBER_ENC (71 ,17 ,192 ),/* const struct prog_test_ref_kfunc __kptr *ptr; */ + BTF_MEMBER_ENC(71, 18, 256), /* const struct prog_test_ref_kfunc __kptr_ref *ptr; */ };static int load_btf (void )
@@ -755,7 +764,7 @@ static int create_map_kptr(void) {LIBBPF_OPTS (bpf_map_create_opts, opts, .btf_key_type_id = 1, - .btf_value_type_id = 14, + .btf_value_type_id = 19, );int fd, btf_fd;
@@ -764,7 +773,7 @@ static int create_map_kptr(void)return -1 ; opts.btf_fd = btf_fd; - fd =bpf_map_create (BPF_MAP_TYPE_ARRAY ,"test_map" ,4 ,24 ,1 , &opts); + fd =bpf_map_create (BPF_MAP_TYPE_ARRAY ,"test_map" ,4 ,40 ,1 , &opts);if (fd <0 )printf ("Failed to create map with btf_id pointer\n" );return fd;
tools/testing/selftests/bpf/verifier/map_kptr.c +156 -0
183 lines not on this page · raw message
Signature · 1 line
2.35.3
PATCH v4 11/14 net: netfilter: add kfunc helper to update ct timeout
In reply to Lorenzo BianconiIntroduce bpf_ct_refresh_timeout kfunc helper in order to update nf_conn lifetime. Move timeout update logic in nf_ct_refresh_timeout utility routine.
--- include/net/netfilter/nf_conntrack.h | 1 + net/netfilter/nf_conntrack_bpf.c | 23 +++++++++++++++++++++++ net/netfilter/nf_conntrack_core.c | 21 +++++++++++++-------- 3 files changed, 37 insertions(+), 8 deletions(-)
include/net/netfilter/nf_conntrack.h +1 -0
@@ -205,6 +205,7 @@ bool nf_ct_get_tuplepr(const struct sk_buff *skb, unsigned int nhoff, u_int16_t l3num, struct net *net, struct nf_conntrack_tuple *tuple); +void nf_ct_refresh_timeout (struct nf_conn *ct,u32 extra_jiffies);void __nf_ct_refresh_acct (struct nf_conn *ct,enum ip_conntrack_info ctinfo,const struct sk_buff *skb,u32 extra_jiffies,bool do_acct);
net/netfilter/nf_conntrack_bpf.c +23 -0
@@ -219,16 +219,39 @@ void bpf_ct_release(const struct nf_conn *nfct)nf_ct_put ((struct nf_conn *)nfct); } +/* bpf_ct_refresh_timeout - Refresh nf_conn object +* +* Refresh timeout associated to the provided connection tracking entry. +* This must be invoked for referenced PTR_TO_BTF_ID. +* +* Parameters: +* @nfct__ref - Pointer to referenced nf_conn object, obtained using +* bpf_xdp_ct_lookup or bpf_skb_ct_lookup. +* @timeout - delta time in msecs used to increase the ct entry lifetime. +*/ +void bpf_ct_refresh_timeout (const struct nf_conn *nfct__ref,u32 timeout) +{ +struct nf_conn *nfct; + +if (!nfct__ref) +return ; + + nfct = (struct nf_conn *)nfct__ref; +nf_ct_refresh_timeout (nfct,msecs_to_jiffies (timeout)); +} + __diag_pop() BTF_SET_START(nf_ct_xdp_check_kfunc_ids) BTF_ID(func, bpf_xdp_ct_lookup) BTF_ID(func, bpf_ct_release) +BTF_ID(func, bpf_ct_refresh_timeout); BTF_SET_END(nf_ct_xdp_check_kfunc_ids) BTF_SET_START(nf_ct_tc_check_kfunc_ids) BTF_ID(func, bpf_skb_ct_lookup) BTF_ID(func, bpf_ct_release) +BTF_ID(func, bpf_ct_refresh_timeout); BTF_SET_END(nf_ct_tc_check_kfunc_ids) BTF_SET_START(nf_ct_acquire_kfunc_ids)
net/netfilter/nf_conntrack_core.c +13 -8
@@ -2055,16 +2055,11 @@ void nf_conntrack_alter_reply(struct nf_conn *ct, }EXPORT_SYMBOL_GPL (nf_conntrack_alter_reply); -/* Refresh conntrack for this many jiffies and do accounting if do_acct is 1 */ -void __nf_ct_refresh_acct (struct nf_conn *ct, -enum ip_conntrack_info ctinfo, -const struct sk_buff *skb, -u32 extra_jiffies, -bool do_acct) +void nf_ct_refresh_timeout (struct nf_conn *ct,u32 extra_jiffies) {/* Only update if this is not a fixed timeout */ if (test_bit (IPS_FIXED_TIMEOUT_BIT , &ct->status)) - goto acct; +return ;/* If not in hash table, timer will not be active yet */ if (nf_ct_is_confirmed (ct))
@@ -2072,7 +2067,17 @@ void __nf_ct_refresh_acct(struct nf_conn *ct,if (READ_ONCE (ct->timeout) != extra_jiffies)WRITE_ONCE (ct->timeout, extra_jiffies); -acct: +} + +/* Refresh conntrack for this many jiffies and do accounting if do_acct is 1 */ +void __nf_ct_refresh_acct (struct nf_conn *ct, +enum ip_conntrack_info ctinfo, +const struct sk_buff *skb, +u32 extra_jiffies, +bool do_acct) +{ +nf_ct_refresh_timeout (ct, extra_jiffies); +if (do_acct)nf_ct_acct_update (ct,CTINFO2DIR (ctinfo), skb->len); }
Signature · 1 line
2.35.3
PATCH v4 12/14 net: netfilter: add kfunc helpers to alloc and insert a new ct entry
In reply to Lorenzo BianconiIntroduce bpf_xdp_ct_alloc, bpf_skb_ct_alloc and bpf_ct_insert_entry kfunc helpers in order to add a new entry to ct map from an ebpf program. Introduce bpf_nf_ct_tuple_parse utility routine.
--- net/netfilter/nf_conntrack_bpf.c | 249 ++++++++++++++++++++++++++++--- 1 file changed, 226 insertions(+), 23 deletions(-)
net/netfilter/nf_conntrack_bpf.c +226 -23
@@ -57,41 +57,106 @@ enum { NF_BPF_CT_OPTS_SZ = 12, }; -static struct nf_conn *__bpf_nf_ct_lookup (struct net *net, -struct bpf_sock_tuple *bpf_tuple, -u32 tuple_len,u8 protonum, -s32 netns_id,u8 *dir) +static int bpf_nf_ct_tuple_parse (struct bpf_sock_tuple *bpf_tuple, +u32 tuple_len,u8 protonum,u8 dir, +struct nf_conntrack_tuple *tuple) { -struct nf_conntrack_tuple_hash *hash; -struct nf_conntrack_tuple tuple; -struct nf_conn *ct; +union nf_inet_addr *src = dir ? &tuple->dst.u3 : &tuple->src.u3; +union nf_inet_addr *dst = dir ? &tuple->src.u3 : &tuple->dst.u3; +union nf_conntrack_man_proto *sport = dir ? (void *)&tuple->dst.u + : &tuple->src.u; +union nf_conntrack_man_proto *dport = dir ? &tuple->src.u + : (void *)&tuple->dst.u;if (unlikely (protonum !=IPPROTO_TCP && protonum !=IPPROTO_UDP )) -return ERR_PTR (-EPROTO ); -if (unlikely (netns_id <BPF_F_CURRENT_NETNS )) -return ERR_PTR (-EINVAL ); +return -EPROTO ; + +memset (tuple,0 ,sizeof (*tuple)); -memset (&tuple,0 ,sizeof (tuple));switch (tuple_len) {case sizeof (bpf_tuple->ipv4): - tuple.src.l3num =AF_INET ; - tuple.src.u3.ip = bpf_tuple->ipv4.saddr; - tuple.src.u.tcp.port = bpf_tuple->ipv4.sport; - tuple.dst.u3.ip = bpf_tuple->ipv4.daddr; - tuple.dst.u.tcp.port = bpf_tuple->ipv4.dport; + tuple->src.l3num =AF_INET ; + src->ip = bpf_tuple->ipv4.saddr; + sport->tcp.port = bpf_tuple->ipv4.sport; + dst->ip = bpf_tuple->ipv4.daddr; + dport->tcp.port = bpf_tuple->ipv4.dport;break ;case sizeof (bpf_tuple->ipv6): - tuple.src.l3num =AF_INET6 ; -memcpy (tuple.src.u3.ip6, bpf_tuple->ipv6.saddr,sizeof (bpf_tuple->ipv6.saddr)); - tuple.src.u.tcp.port = bpf_tuple->ipv6.sport; -memcpy (tuple.dst.u3.ip6, bpf_tuple->ipv6.daddr,sizeof (bpf_tuple->ipv6.daddr)); - tuple.dst.u.tcp.port = bpf_tuple->ipv6.dport; + tuple->src.l3num =AF_INET6 ; +memcpy (src->ip6, bpf_tuple->ipv6.saddr,sizeof (bpf_tuple->ipv6.saddr)); + sport->tcp.port = bpf_tuple->ipv6.sport; +memcpy (dst->ip6, bpf_tuple->ipv6.daddr,sizeof (bpf_tuple->ipv6.daddr)); + dport->tcp.port = bpf_tuple->ipv6.dport;break ;default : -return ERR_PTR (-EAFNOSUPPORT ); +return -EAFNOSUPPORT ; + } + tuple->dst.protonum = protonum; + tuple->dst.dir = dir; + +return 0 ; +} + +static struct nf_conn * +__bpf_nf_ct_alloc_entry (struct net *net,struct bpf_sock_tuple *bpf_tuple, +u32 tuple_len,u8 protonum,s32 netns_id,u32 timeout) +{ +struct nf_conntrack_tuple otuple, rtuple; +struct nf_conn *ct; +int err; + +if (unlikely (netns_id <BPF_F_CURRENT_NETNS )) +return ERR_PTR (-EINVAL ); + + err =bpf_nf_ct_tuple_parse (bpf_tuple, tuple_len, protonum, +IP_CT_DIR_ORIGINAL , &otuple); +if (err <0 ) +return ERR_PTR (err); + + err =bpf_nf_ct_tuple_parse (bpf_tuple, tuple_len, protonum, +IP_CT_DIR_REPLY , &rtuple); +if (err <0 ) +return ERR_PTR (err); + +if (netns_id >=0 ) { + net =get_net_ns_by_id (net, netns_id); +if (unlikely (!net)) +return ERR_PTR (-ENONET ); } - tuple.dst.protonum = protonum; + ct =nf_conntrack_alloc (net, &nf_ct_zone_dflt, &otuple, &rtuple, +GFP_ATOMIC ); +if (IS_ERR (ct)) + goto out; + +memset (&ct->proto,0 ,sizeof (ct->proto)); + ct->timeout = timeout *HZ + jiffies; + ct->status |=IPS_CONFIRMED ; + +out: +if (netns_id >=0 ) +put_net (net); + +return ct; +} + +static struct nf_conn *__bpf_nf_ct_lookup (struct net *net, +struct bpf_sock_tuple *bpf_tuple, +u32 tuple_len,u8 protonum, +s32 netns_id,u8 *dir) +{ +struct nf_conntrack_tuple_hash *hash; +struct nf_conntrack_tuple tuple; +struct nf_conn *ct; +int err; + +if (unlikely (netns_id <BPF_F_CURRENT_NETNS )) +return ERR_PTR (-EINVAL ); + + err =bpf_nf_ct_tuple_parse (bpf_tuple, tuple_len, protonum, +IP_CT_DIR_ORIGINAL , &tuple); +if (err <0 ) +return ERR_PTR (err);if (netns_id >=0 ) { net =get_net_ns_by_id (net, netns_id);
@@ -116,6 +181,49 @@ __diag_push();__diag_ignore_all ("-Wmissing-prototypes" ,"Global functions as their definitions will be in nf_conntrack BTF" ); +/* bpf_xdp_ct_alloc - Alloc a new CT entry for the given tuple +* +* Parameters: +* @xdp_ctx - Pointer to ctx (xdp_md) in XDP program +* Cannot be NULL +* @bpf_tuple - Pointer to memory representing the tuple to look up +* Cannot be NULL +* @tuple__sz - Length of the tuple structure +* Must be one of sizeof(bpf_tuple->ipv4) or +* sizeof(bpf_tuple->ipv6) +* @opts - Additional options for lookup (documented above) +* Cannot be NULL +* @opts__sz - Length of the bpf_ct_opts structure +* Must be NF_BPF_CT_OPTS_SZ (12) +*/ +struct nf_conn * +bpf_xdp_ct_alloc (struct xdp_md *xdp_ctx,struct bpf_sock_tuple *bpf_tuple, +u32 tuple__sz,struct bpf_ct_opts *opts,u32 opts__sz) +{ +struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx; +struct nf_conn *nfct; + +BUILD_BUG_ON (sizeof (struct bpf_ct_opts ) !=NF_BPF_CT_OPTS_SZ ); +if (!opts) +return NULL ; + +if (!bpf_tuple || opts->reserved[0 ] || opts->reserved[1 ] || + opts__sz !=NF_BPF_CT_OPTS_SZ ) { + opts->error = -EINVAL ; +return NULL ; + } + + nfct =__bpf_nf_ct_alloc_entry (dev_net (ctx->rxq->dev), bpf_tuple, + tuple__sz, opts->l4proto, + opts->netns_id,10 ); +if (IS_ERR_OR_NULL (nfct)) { + opts->error =PTR_ERR (nfct); +return NULL ; + } + +return nfct; +} +/* bpf_xdp_ct_lookup - Lookup CT entry for the given tuple, and acquire a * reference to it *
@@ -159,6 +267,50 @@ bpf_xdp_ct_lookup(struct xdp_md *xdp_ctx, struct bpf_sock_tuple *bpf_tuple,return nfct; } +/* bpf_skb_ct_alloc - Alloc a new CT entry for the given tuple +* +* Parameters: +* @skb_ctx - Pointer to ctx (__sk_buff) in TC program +* Cannot be NULL +* @bpf_tuple - Pointer to memory representing the tuple to look up +* Cannot be NULL +* @tuple__sz - Length of the tuple structure +* Must be one of sizeof(bpf_tuple->ipv4) or +* sizeof(bpf_tuple->ipv6) +* @opts - Additional options for lookup (documented above) +* Cannot be NULL +* @opts__sz - Length of the bpf_ct_opts structure +* Must be NF_BPF_CT_OPTS_SZ (12) +*/ +struct nf_conn * +bpf_skb_ct_alloc (struct __sk_buff *skb_ctx,struct bpf_sock_tuple *bpf_tuple, +u32 tuple__sz,struct bpf_ct_opts *opts,u32 opts__sz) +{ +struct sk_buff *skb = (struct sk_buff *)skb_ctx; +struct nf_conn *nfct; +struct net *net; + +BUILD_BUG_ON (sizeof (struct bpf_ct_opts ) !=NF_BPF_CT_OPTS_SZ ); +if (!opts) +return NULL ; + +if (!bpf_tuple || opts->reserved[0 ] || opts->reserved[1 ] || + opts__sz !=NF_BPF_CT_OPTS_SZ ) { + opts->error = -EINVAL ; +return NULL ; + } + + net = skb->dev ?dev_net (skb->dev) :sock_net (skb->sk); + nfct =__bpf_nf_ct_alloc_entry (net, bpf_tuple, tuple__sz, + opts->l4proto, opts->netns_id,10 ); +if (IS_ERR_OR_NULL (nfct)) { + opts->error =PTR_ERR (nfct); +return NULL ; + } + +return nfct; +} +/* bpf_skb_ct_lookup - Lookup CT entry for the given tuple, and acquire a * reference to it *
@@ -202,6 +354,40 @@ bpf_skb_ct_lookup(struct __sk_buff *skb_ctx, struct bpf_sock_tuple *bpf_tuple,return nfct; } +/* bpf_ct_insert_entry - Add the provided entry into a CT map +* +* This must be invoked for referenced PTR_TO_BTF_ID. +* +* @nfct__ref - Pointer to referenced nf_conn object +*/ +const struct nf_conn * +bpf_ct_insert_entry (struct nf_conn *nfct__ref,struct bpf_ct_opts *opts, +u32 opts__sz) +{ +int err; + +BUILD_BUG_ON (sizeof (struct bpf_ct_opts ) !=NF_BPF_CT_OPTS_SZ ); + +if (!nfct__ref) +return NULL ; + +if (!opts || opts->reserved[0 ] || opts->reserved[1 ] || + opts__sz !=NF_BPF_CT_OPTS_SZ ) { +nf_conntrack_free (nfct__ref); + opts->error = -EINVAL ; +return NULL ; + } + + err =nf_conntrack_hash_check_insert (nfct__ref); +if (err <0 ) { +nf_conntrack_free (nfct__ref); + opts->error = err; +return NULL ; + } + +return nfct__ref; +} +/* bpf_ct_release - Release acquired nf_conn object * * This must be invoked for referenced PTR_TO_BTF_ID, and the verifier rejects
@@ -243,23 +429,31 @@ void bpf_ct_refresh_timeout(const struct nf_conn *nfct__ref, u32 timeout) __diag_pop() BTF_SET_START(nf_ct_xdp_check_kfunc_ids) +BTF_ID(func, bpf_xdp_ct_alloc) BTF_ID(func, bpf_xdp_ct_lookup) +BTF_ID(func, bpf_ct_insert_entry) BTF_ID(func, bpf_ct_release) BTF_ID(func, bpf_ct_refresh_timeout); BTF_SET_END(nf_ct_xdp_check_kfunc_ids) BTF_SET_START(nf_ct_tc_check_kfunc_ids) +BTF_ID(func, bpf_skb_ct_alloc) BTF_ID(func, bpf_skb_ct_lookup) +BTF_ID(func, bpf_ct_insert_entry) BTF_ID(func, bpf_ct_release) BTF_ID(func, bpf_ct_refresh_timeout); BTF_SET_END(nf_ct_tc_check_kfunc_ids) BTF_SET_START(nf_ct_acquire_kfunc_ids) +BTF_ID(func, bpf_xdp_ct_alloc) BTF_ID(func, bpf_xdp_ct_lookup) +BTF_ID(func, bpf_skb_ct_alloc) BTF_ID(func, bpf_skb_ct_lookup) +BTF_ID(func, bpf_ct_insert_entry) BTF_SET_END(nf_ct_acquire_kfunc_ids) BTF_SET_START(nf_ct_release_kfunc_ids) +BTF_ID(func, bpf_ct_insert_entry) BTF_ID(func, bpf_ct_release) BTF_SET_END(nf_ct_release_kfunc_ids)
@@ -267,12 +461,21 @@ BTF_SET_END(nf_ct_release_kfunc_ids)#define nf_ct_ret_null_kfunc_ids nf_ct_acquire_kfunc_idsBTF_ID_LIST (nf_ct_acq_rel_pairs) +BTF_ID (func ,bpf_xdp_ct_alloc ) +BTF_ID (func, bpf_ct_insert_entry) + +BTF_ID (func, bpf_skb_ct_alloc) +BTF_ID (func, bpf_ct_insert_entry) +BTF_ID (func, bpf_xdp_ct_lookup)BTF_ID (func, bpf_ct_release)BTF_ID (func, bpf_skb_ct_lookup)BTF_ID (func, bpf_ct_release) +BTF_ID (func, bpf_ct_insert_entry) +BTF_ID (func, bpf_ct_release) + static const struct btf_kfunc_id_set nf_conntrack_xdp_kfunc_set = { .owner =THIS_MODULE , .check_set = &nf_ct_xdp_check_kfunc_ids,
Signature · 1 line
2.35.3
PATCH v4 13/14 selftests/bpf: add selftest for bpf_xdp_ct_add and bpf_ct_refresh_timeout kfunc
In reply to Lorenzo BianconiIntroduce selftests for the following kfunc helpers:
- bpf_xdp_ct_alloc
- bpf_skb_ct_alloc
- bpf_ct_insert_entry
- bpf_ct_refresh_timeout
--- .../testing/selftests/bpf/prog_tests/bpf_nf.c | 6 ++ .../testing/selftests/bpf/progs/test_bpf_nf.c | 87 +++++++++++++++---- 2 files changed, 76 insertions(+), 17 deletions(-)
tools/testing/selftests/bpf/prog_tests/bpf_nf.c +6 -0
@@ -39,6 +39,12 @@ void test_bpf_nf_ct(int mode)ASSERT_EQ (skel->bss->test_enonet_netns_id, -ENONET ,"Test ENONET for bad but valid netns_id" );ASSERT_EQ (skel->bss->test_enoent_lookup, -ENOENT ,"Test ENOENT for failed lookup" );ASSERT_EQ (skel->bss->test_eafnosupport, -EAFNOSUPPORT ,"Test EAFNOSUPPORT for invalid len__tuple" ); +ASSERT_EQ (skel->data->test_alloc_entry,0 ,"Test for alloc new entry" ); +ASSERT_EQ (skel->data->test_insert_entry,0 ,"Test for insert new entry" ); +ASSERT_EQ (skel->data->test_succ_lookup,0 ,"Test for successful lookup" ); +/* allow some tolerance for test_delta_timeout value to avoid races. */ +ASSERT_GT (skel->bss->test_delta_timeout,9 ,"Test for min ct timeout update" ); +ASSERT_LE (skel->bss->test_delta_timeout,10 ,"Test for max ct timeout update" ); end:test_bpf_nf__destroy (skel); }
tools/testing/selftests/bpf/progs/test_bpf_nf.c +70 -17
171 lines not on this page · raw message
Signature · 1 line
2.35.3
PATCH v4 14/14 selftests/bpf: Add negative tests for bpf_nf
In reply to Lorenzo BianconiFrom: Kumar Kartikeya Dwivedi <[email protected]>
Test cases we care about and ensure improper usage is caught and rejected by verifier. This also ends up exercising acquire release pair logic and MEM_RDONLY's effect on a PTR_TO_BTF_ID which usually permits BPF_WRITE.
--- .../testing/selftests/bpf/prog_tests/bpf_nf.c | 52 ++++++++++++- .../selftests/bpf/progs/test_bpf_nf_fail.c | 73 +++++++++++++++++++ 2 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/bpf/progs/test_bpf_nf_fail.c
tools/testing/selftests/bpf/prog_tests/bpf_nf.c +51 -1
@@ -2,13 +2,25 @@#include <test_progs.h> #include <network_helpers.h> #include "test_bpf_nf.skel.h" +#include "test_bpf_nf_fail.skel.h" + +static char log_buf[1024 *1024 ]; + +struct { +const char *prog_name; +const char *err_msg; +} test_bpf_nf_fail_tests[] = { + {"alloc_release" ,"not permitted to release reference" }, + {"write_after_insert" ,"pointer points to const object, only read is supported" }, + {"lookup_insert" ,"cannot pass read only pointer to arg#0" }, +};enum {TEST_XDP ,TEST_TC_BPF , }; -void test_bpf_nf_ct (int mode) +static void test_bpf_nf_ct (int mode) {struct test_bpf_nf *skel;int prog_fd, err;
@@ -49,10 +61,48 @@ void test_bpf_nf_ct(int mode)test_bpf_nf__destroy (skel); } +static void test_bpf_nf_ct_fail (const char *prog_name,const char *err_msg) +{ + LIBBPF_OPTS(bpf_object_open_opts, opts, .kernel_log_buf = log_buf, + .kernel_log_size = sizeof(log_buf), + .kernel_log_level = 1); +struct test_bpf_nf_fail *skel; +struct bpf_program *prog; +int ret; + + skel =test_bpf_nf_fail__open_opts (&opts); +if (!ASSERT_OK_PTR (skel,"test_bpf_nf_fail__open" )) +return ; + + prog =bpf_object__find_program_by_name (skel->obj, prog_name); +if (!ASSERT_OK_PTR (prog,"bpf_object__find_program_by_name" )) + goto end; + +bpf_program__set_autoload (prog, true); + + ret =test_bpf_nf_fail__load (skel); +if (!ASSERT_ERR (ret,"test_bpf_nf_fail__load must fail" )) + goto end; + +if (!ASSERT_OK_PTR (strstr (log_buf, err_msg),"expected error message" )) { +fprintf (stderr,"Expected: %s\n" , err_msg); +fprintf (stderr,"Verifier: %s\n" , log_buf); + } + +end: +test_bpf_nf_fail__destroy (skel); +} +void test_bpf_nf (void ) { +int i;if (test__start_subtest ("xdp-ct" ))test_bpf_nf_ct (TEST_XDP );if (test__start_subtest ("tc-bpf-ct" ))test_bpf_nf_ct (TEST_TC_BPF ); +for (i =0 ; i <ARRAY_SIZE (test_bpf_nf_fail_tests); i++) { +if (test__start_subtest (test_bpf_nf_fail_tests[i].prog_name)) +test_bpf_nf_ct_fail (test_bpf_nf_fail_tests[i].prog_name, + test_bpf_nf_fail_tests[i].err_msg); + } }
tools/testing/selftests/bpf/progs/test_bpf_nf_fail.c new file +73 -0
73 lines not on this page · raw message
Signature · 1 line
2.35.3
Quoted 5 lines · Lorenzo Bianconi
Changes since v3:
- splitbpf_xdp_ct_addin bpf_xdp_ct_alloc/bpf_skb_ct_alloc andbpf_ct_insert_entry
- add verifier code to properly populate/configure ct entry
- improve selftests
Kumar, Lorenzo,
are you planning on sending v5 ?
The patches 1-5 look good.
Patch 6 is questionable as Florian pointed out.
What is the motivation to allow writes into ct->status?
The selftest doesn't do that anyway.
Patch 7 (acquire-release pairs) is too narrow.
The concept of a pair will not work well. There could be two acq funcs and one release. Please think of some other mechanism. Maybe type based? BTF?
Or encode that info into type name? or some other way.
Kumar Kartikeya Dwivedi
maintainerQuoted 7 lines · Alexei Starovoitov
Quoted 5 lines · Lorenzo Bianconi
Changes since v3:
- splitbpf_xdp_ct_addin bpf_xdp_ct_alloc/bpf_skb_ct_alloc andbpf_ct_insert_entry
- add verifier code to properly populate/configure ct entry
- improve selftests
Quoted 5 lines
Kumar, Lorenzo,
are you planning on sending v5 ?
The patches 1-5 look good.
Patch 6 is questionable as Florian pointed out.
Yes, it is almost there.
Quoted 1 line
What is the motivation to allow writes into
ct->status?
It will only be allowed for ct from alloc function, after that ct = insert(ct) releases old one with new read only ct. I need to recheck once again with the code what other bits would cause problems on insert before I rework and reply.
Quoted 1 line
The selftest doesn't do that anyway.
Yes, it wasn't updated, we will do that in v5.
Quoted 2 lines
Patch 7 (acquire-release pairs) is too narrow.
The concept of a pair will not work well. There could be two acq funcs and one release.
That is already handled (you define two pairs: acq1, rel and acq2, rel). There is also an example: bpf_ct_insert_entry -> bpf_ct_release, bpf_xdp_ct_lookup -> ct_release.
Quoted 2 lines
Please think of some other mechanism. Maybe type based? BTF? Or encode that info into type name? or some other way.
Hmm, ok. I kinda dislike this solution too. The other idea that comes to mind is encapsulating nf_conn into another struct and returning pointer to that:
struct nf_conn_alloc {
struct nf_conn ct;
};
struct nf_conn_alloc *bpf_xdp_ct_alloc(...);
struct nf_conn *bpf_ct_insert_entry(struct nf_conn_alloc *act, ...);Then nf_conn_alloc gets a different BTF ID, and hence the type can be matched in the prototype. Any opinions?
--
Kartikeya
Quoted 15 lines · Kumar Kartikeya Dwivedi
Quoted 7 lines · Alexei Starovoitov
Quoted 5 lines · Lorenzo Bianconi
Changes since v3:
- splitbpf_xdp_ct_addin bpf_xdp_ct_alloc/bpf_skb_ct_alloc andbpf_ct_insert_entry
- add verifier code to properly populate/configure ct entry
- improve selftestsQuoted 5 lines
Kumar, Lorenzo,
are you planning on sending v5 ?
The patches 1-5 look good.
Patch 6 is questionable as Florian pointed out.
Quoted 7 lines
Yes, it is almost there.
Quoted 1 line
What is the motivation to allow writes into
ct->status?It will only be allowed for ct from alloc function, after that ct = insert(ct) releases old one with new read only ct. I need to recheck once again with the code what other bits would cause problems on insert before I rework and reply.
I still don't understand why you want to allow writing after alloc.
Quoted 10 lines
Quoted 1 line
The selftest doesn't do that anyway.
Yes, it wasn't updated, we will do that in v5.
Quoted 2 lines
Patch 7 (acquire-release pairs) is too narrow.
The concept of a pair will not work well. There could be two acq funcs and one release.That is already handled (you define two pairs: acq1, rel and acq2, rel). There is also an example:
bpf_ct_insert_entry->bpf_ct_release,bpf_xdp_ct_lookup->ct_release.
If we can represent that without all these additional btf_id sets it would be much better.
Quoted 13 lines
Quoted 2 lines
Please think of some other mechanism. Maybe type based? BTF? Or encode that info into type name? or some other way.
Hmm, ok. I kinda dislike this solution too. The other idea that comes to mind is encapsulating
nf_conninto another struct and returning pointer to that:struct nf_conn_alloc { struct nf_conn ct; }; struct nf_conn_alloc *bpf_xdp_ct_alloc(...); struct nf_conn *bpf_ct_insert_entry(struct nf_conn_alloc *act, ...);
Quoted 2 lines
Then
nf_conn_allocgets a different BTF ID, and hence the type can be matched in the prototype. Any opinions?
Yes. Or maybe typedef ?
typedef struct nf_conn nf_conn__alloc;
typedef struct nf_conn nf_conn__ro;
C will accept silent type casts from one type to another, but BTF type checking can be strict?
Not sure. wrapping a struct works too, but extra '.ct' accessor might be annoying? Unless you only use it with container_of(). I would prefer double or triple underscore to highlight a flavor. struct nf_conn___init {...}
The main benefit, of course, is no need for extra btf_id sets. Different types take care of correct arg passing.
In that sense typedef idea doesn't quite work,
since BTF checks with typedef would be unnecessarily strict compared to regular C type checking rules. That difference in behavior might bite us later.
So let's go with struct wrapping.
Kumar Kartikeya Dwivedi
maintainerQuoted 19 lines · Alexei Starovoitov
Quoted 17 lines · Kumar Kartikeya Dwivedi
Quoted 7 lines · Alexei Starovoitov
Quoted 5 lines · Lorenzo Bianconi
Changes since v3:
- splitbpf_xdp_ct_addin bpf_xdp_ct_alloc/bpf_skb_ct_alloc andbpf_ct_insert_entry
- add verifier code to properly populate/configure ct entry
- improve selftestsQuoted 5 lines
Kumar, Lorenzo,
are you planning on sending v5 ?
The patches 1-5 look good.
Patch 6 is questionable as Florian pointed out.Yes, it is almost there.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
What is the motivation to allow writes into
ct->status?It will only be allowed for ct from alloc function, after that ct = insert(ct) releases old one with new read only ct. I need to recheck once again with the code what other bits would cause problems on insert before I rework and reply.
I still don't understand why you want to allow writing after alloc.
It is just a way to set the status, instead of a helper to set it. Eventually before nf_conntrack_hash_check_insert it will still be checked and error returned for disallowed bits (e.g. anything in IPS_UNCHANGEABLE_MASK, etc.). The current series missed that check.
Florian is right in that it is a can of worms, but I think we can atleast permit things like confirmed, assured, etc. which can also be set when crafting a ct using netlink. Both of them can share the same check so it is consistent when done from kfunc or netlink side, and any changes internally wrt status bits are in sync.
Anyway, if you don't like the direct write into ct, I can drop it, for now just insert a confirmed entry (since this was just for testing). That also means patch 3-6 are not strictly needed anymore, so they can be dropped, but I can keep them if you want, since they might be useful.
Florian asked for the pipeline, it is like this:
ct = bpf_xdp_ct_alloc();ct->a = ...; // private ct, not yet visible to anyone but usct->b = ...;
or we would now set using helpersalloc_ct_set_status(ct, IPS_CONFIRMED);alloc_ct_set_timeout(ct, timeout);
...
ct = bpf_ct_insert_entry(ct); // old alloc_ct release, new inserted nf_conn returned if (!ct)
return -1;
/* Inserted successfully */
In the earlier approach this ct->a could now not be written to, as it was inserted, instead of allocated ct, which insert function took as arg and invalidated, so BPF program held a read only pointer now. If we drop that approach all pointers are read only anyway, so writing won't be allowed either.
Quoted 34 lines
Quoted 10 lines
Quoted 1 line
The selftest doesn't do that anyway.
Yes, it wasn't updated, we will do that in v5.
Quoted 2 lines
Patch 7 (acquire-release pairs) is too narrow.
The concept of a pair will not work well. There could be two acq funcs and one release.That is already handled (you define two pairs: acq1, rel and acq2, rel). There is also an example:
bpf_ct_insert_entry->bpf_ct_release,bpf_xdp_ct_lookup->ct_release.If we can represent that without all these additional
btf_idsets it would be much better.Quoted 13 lines
Quoted 2 lines
Please think of some other mechanism. Maybe type based? BTF? Or encode that info into type name? or some other way.
Hmm, ok. I kinda dislike this solution too. The other idea that comes to mind is encapsulating
nf_conninto another struct and returning pointer to that:struct nf_conn_alloc { struct nf_conn ct; }; struct nf_conn_alloc *bpf_xdp_ct_alloc(...); struct nf_conn *bpf_ct_insert_entry(struct nf_conn_alloc *act, ...);Quoted 2 lines
Then
nf_conn_allocgets a different BTF ID, and hence the type can be matched in the prototype. Any opinions?Yes. Or maybe typedef ?
typedef structnf_connnf_conn__alloc;
typedef structnf_connnf_conn__ro;
Quoted 13 lines
C will accept silent type casts from one type to another, but BTF type checking can be strict?
Not sure. wrapping a struct works too, but extra '.ct' accessor might be annoying? Unless you only use it withcontainer_of(). I would prefer double or triple underscore to highlight a flavor. structnf_conn___init{...}
The main benefit, of course, is no need for extrabtf_idsets. Different types take care of correct arg passing.
In that sense typedef idea doesn't quite work,
since BTF checks with typedef would be unnecessarily strict compared to regular C type checking rules. That difference in behavior might bite us later.
So let's go with struct wrapping.
Makes sense, I will go with this. But now if we are not even allowing write to such allocated ct (probably only helpers that set some field and check value), it can just be an empty opaque struct for the BPF program, while it is still a nf_conn in the kernel. There doesn't seem to be much point in wrapping around nf_conn when reading from allocated nf_conn isn't going to be of any use.
--
Kartikeya
Quoted 45 lines · Kumar Kartikeya Dwivedi
Quoted 19 lines · Alexei Starovoitov
Quoted 17 lines · Kumar Kartikeya Dwivedi
Quoted 7 lines · Alexei Starovoitov
Quoted 5 lines · Lorenzo Bianconi
Changes since v3:
- splitbpf_xdp_ct_addin bpf_xdp_ct_alloc/bpf_skb_ct_alloc andbpf_ct_insert_entry
- add verifier code to properly populate/configure ct entry
- improve selftestsQuoted 5 lines
Kumar, Lorenzo,
are you planning on sending v5 ?
The patches 1-5 look good.
Patch 6 is questionable as Florian pointed out.Yes, it is almost there.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
What is the motivation to allow writes into
ct->status?It will only be allowed for ct from alloc function, after that ct = insert(ct) releases old one with new read only ct. I need to recheck once again with the code what other bits would cause problems on insert before I rework and reply.
I still don't understand why you want to allow writing after alloc.
It is just a way to set the status, instead of a helper to set it. Eventually before
nf_conntrack_hash_check_insertit will still be checked and error returned for disallowed bits (e.g. anything inIPS_UNCHANGEABLE_MASK, etc.). The current series missed that check.Florian is right in that it is a can of worms, but I think we can atleast permit things like confirmed, assured, etc. which can also be set when crafting a ct using netlink. Both of them can share the same check so it is consistent when done from kfunc or netlink side, and any changes internally wrt status bits are in sync.
Anyway, if you don't like the direct write into ct, I can drop it, for now just insert a confirmed entry (since this was just for testing). That also means patch 3-6 are not strictly needed anymore, so they can be dropped, but I can keep them if you want, since they might be useful.
Quoted 8 lines
Florian asked for the pipeline, it is like this:
ct =
bpf_xdp_ct_alloc();ct->a= ...; // private ct, not yet visible to anyone but usct->b= ...;
or we would now set using helpersalloc_ct_set_status(ct, IPS_CONFIRMED);alloc_ct_set_timeout(ct, timeout);
In other cases it probably will be useful to write into allocated structs, but ct's timeout and status fields are a bit too special.
It's probably cleaner to generalize ctnetlink_change_status/ctnetlink_change_timeout as kfuncs and let progs modify the fields through these two helpers. Especially since timeout and status&IPS_DYING_BIT are related.
Let's indeed drop 3-6 for now.
Though recognizing 'const' modifier in BTF is useful it creates ambiguity. Unreferenced ptr_to_btf_id is readonly anyway.
This 'const' would make it readable with normal load vs probe_load, but that difference is too subtle for users. It looks like normal dereference in C in both cases. Let's keep existing probe_load only for now.
Quoted 58 lines
...
ct =bpf_ct_insert_entry(ct); // oldalloc_ctrelease, new insertednf_connreturned if (!ct)return -1;/* Inserted successfully */
In the earlier approach thisct->acould now not be written to, as it was inserted, instead of allocated ct, which insert function took as arg and invalidated, so BPF program held a read only pointer now. If we drop that approach all pointers are read only anyway, so writing won't be allowed either.Quoted 34 lines
Quoted 10 lines
Quoted 1 line
The selftest doesn't do that anyway.
Yes, it wasn't updated, we will do that in v5.
Quoted 2 lines
Patch 7 (acquire-release pairs) is too narrow.
The concept of a pair will not work well. There could be two acq funcs and one release.That is already handled (you define two pairs: acq1, rel and acq2, rel). There is also an example:
bpf_ct_insert_entry->bpf_ct_release,bpf_xdp_ct_lookup->ct_release.If we can represent that without all these additional
btf_idsets it would be much better.Quoted 13 lines
Quoted 2 lines
Please think of some other mechanism. Maybe type based? BTF? Or encode that info into type name? or some other way.
Hmm, ok. I kinda dislike this solution too. The other idea that comes to mind is encapsulating
nf_conninto another struct and returning pointer to that:struct nf_conn_alloc { struct nf_conn ct; }; struct nf_conn_alloc *bpf_xdp_ct_alloc(...); struct nf_conn *bpf_ct_insert_entry(struct nf_conn_alloc *act, ...);Quoted 2 lines
Then
nf_conn_allocgets a different BTF ID, and hence the type can be matched in the prototype. Any opinions?Yes. Or maybe typedef ?
typedef structnf_connnf_conn__alloc;
typedef structnf_connnf_conn__ro;Quoted 13 lines
C will accept silent type casts from one type to another, but BTF type checking can be strict?
Not sure. wrapping a struct works too, but extra '.ct' accessor might be annoying? Unless you only use it withcontainer_of(). I would prefer double or triple underscore to highlight a flavor. structnf_conn___init{...}
The main benefit, of course, is no need for extrabtf_idsets. Different types take care of correct arg passing.
In that sense typedef idea doesn't quite work,
since BTF checks with typedef would be unnecessarily strict compared to regular C type checking rules. That difference in behavior might bite us later.
So let's go with struct wrapping.
Quoted 5 lines
Makes sense, I will go with this. But now if we are not even allowing write to such allocated ct (probably only helpers that set some field and check value), it can just be an empty opaque struct for the BPF program, while it is still a
nf_connin the kernel. There doesn't seem to be much point in wrapping aroundnf_connwhen reading from allocatednf_connisn't going to be of any use.
Let's not make it opaque. struct nf_conn is readonly with probe_load. No need to disable that access either for allocated nf_conn or inserted.