Skip to contentSkip to conversation

Introduce bpf_ct_set_nat_info kfunc helper

PATCH Superseded Lorenzo Bianconi 4 participants 17 messages Sep 5, 2022 – Sep 7, 2022

Subsystems (7)
Networking Network drivers Documentation Kselftest +3 more subsystems

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Lorenzo Bianconi

author
#0 lore raw

v2 cover Introduce bpf_ct_set_nat_info kfunc helper

Introduce bpf_ct_set_nat_info kfunc helper in order to set source and destination nat addresses/ports in a new allocated ct entry not inserted in the connection tracking table yet.
Introduce support for per-parameter trusted args.

Changes since v1:
- enable CONFIG_NF_NAT in tools/testing/selftests/bpf/config

Kumar Kartikeya Dwivedi (2):
bpf: Add support for per-parameter trusted args selftests/bpf: Extend KF_TRUSTED_ARGS test for __ref annotation

Lorenzo Bianconi (2):
net: netfilter: add bpf_ct_set_nat_info kfunc helper selftests/bpf: add tests for bpf_ct_set_nat_info kfunc

 Documentation/bpf/kfuncs.rst                  | 18 +++++++
 kernel/bpf/btf.c                              | 39 ++++++++++-----
 net/bpf/test_run.c                            |  9 +++-
 net/netfilter/nf_conntrack_bpf.c              | 49 ++++++++++++++++++-
 tools/testing/selftests/bpf/config            |  1 +
 .../testing/selftests/bpf/prog_tests/bpf_nf.c |  2 +
 .../testing/selftests/bpf/progs/test_bpf_nf.c | 26 +++++++++-
 tools/testing/selftests/bpf/verifier/calls.c  | 38 +++++++++++---
 8 files changed, 157 insertions(+), 25 deletions(-)
Signature · 1 line
2.37.3
author
#1 lore raw

PATCH v2 1/4 bpf: Add support for per-parameter trusted args

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 trusted arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag but limited to the specific parameter. 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.

Signed-off-by Kumar Kartikeya Dwivedi <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++
 kernel/bpf/btf.c             | 39 ++++++++++++++++++++++++------------
 net/bpf/test_run.c           |  9 +++++++--
 3 files changed, 51 insertions(+), 15 deletions(-)
Documentation/bpf/kfuncs.rst +18 -0
index 781731749e55..a9d77d12fd0c 100644
@@ -72,6 +72,24 @@ argument as its size. By default, without __sz annotation, the size of the type
 of the pointer is used. Without __sz annotation, a kfunc cannot accept a void
 pointer.
 
+2.2.2 __ref Annotation
+----------------------
+
+This annotation is used to indicate that the argument is trusted, i.e. it will
+be a pointer from an acquire function (defined later), and its offset will be
+zero. This annotation has the same effect as the KF_TRUSTED_ARGS kfunc flag but
+only on the parameter it is applied to. An example is shown below::
+
+        void bpf_task_send_signal(struct task_struct *task__ref, int signal)
+        {
+        ...
+        }
+
+Here, bpf_task_send_signal will only act on trusted task_struct pointers, and
+cannot be used on pointers obtained using pointer walking. This ensures that
+caller always calls this kfunc on a task whose lifetime is guaranteed for the
+duration of the call.
+
 .. _BPF_kfunc_nodef:
 
 2.3 Using an existing kernel function
kernel/bpf/btf.c +26 -13
index 903719b89238..7e273f949ee8 100644
@@ -6140,18 +6140,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))

@@ -6160,10 +6155,26 @@ 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))
+	return !strncmp(param_name, suffix, sfx_len);
+}
+
+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 true;
+	return btf_param_match_suffix(btf, arg, "__sz");
 }
 
 static int btf_check_func_arg_match(struct bpf_verifier_env *env,

@@ -6173,7 +6184,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				    u32 kfunc_flags)
 {
 	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
-	bool rel = false, kptr_get = false, trusted_arg = false;
+	bool rel = false, kptr_get = false, kf_trusted_args = false;
 	bool sleepable = false;
 	struct bpf_verifier_log *log = &env->log;
 	u32 i, nargs, ref_id, ref_obj_id = 0;

@@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		/* Only kfunc can be release func */
 		rel = kfunc_flags & KF_RELEASE;
 		kptr_get = kfunc_flags & KF_KPTR_GET;
-		trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
+		kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS;
 		sleepable = kfunc_flags & KF_SLEEPABLE;
 	}
 

@@ -6222,6 +6233,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 = &regs[regno];
+		bool trusted_arg = false;
 
 		t = btf_type_skip_modifiers(btf, args[i].type, NULL);
 		if (btf_type_is_scalar(t)) {

@@ -6240,6 +6252,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		/* Check if argument must be a referenced pointer, args + i has
 		 * been verified to be a pointer (after skipping modifiers).
 		 */
+		trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i);
 		if (is_kfunc && trusted_arg && !reg->ref_obj_id) {
 			bpf_log(log, "R%d must be referenced\n", regno);
 			return -EINVAL;
net/bpf/test_run.c +7 -2
index 25d8ecf105aa..b735accf8750 100644
@@ -691,7 +691,11 @@ 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)
+noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p)
+{
+}
+
+noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref)
 {
 }
 

@@ -722,7 +726,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
 BTF_SET8_END(test_sk_check_kfunc_ids)
Signature · 1 line
2.37.3
reviewer Acked-by on v2 1/4
#2 lore raw

Re: [PATCH v2 bpf-next 1/4] bpf: Add support for per-parameter trusted args

Quoted 13 lines · Lorenzo Bianconi

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 trusted arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag but limited to the specific parameter. 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.

Quoted 2 lines
Signed-off-by Kumar Kartikeya Dwivedi <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>

LGTM:

Acked-by Song Liu <[email protected]>

PS: IIUC, we use "refcounted" and "trusted" for the same concept. Shall we just use __trusted? Pardon me if this has been discussed before.

Thanks,
Song

Quoted 155 lines
---
 Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++
 kernel/bpf/btf.c             | 39 ++++++++++++++++++++++++------------
 net/bpf/test_run.c           |  9 +++++++--
 3 files changed, 51 insertions(+), 15 deletions(-)
Documentation/bpf/kfuncs.rst +18 -0
index 781731749e55..a9d77d12fd0c 100644
@@ -72,6 +72,24 @@ argument as its size. By default, without __sz annotation, the size of the type
 of the pointer is used. Without __sz annotation, a kfunc cannot accept a void
 pointer.
 
+2.2.2 __ref Annotation
+----------------------
+
+This annotation is used to indicate that the argument is trusted, i.e. it will
+be a pointer from an acquire function (defined later), and its offset will be
+zero. This annotation has the same effect as the KF_TRUSTED_ARGS kfunc flag but
+only on the parameter it is applied to. An example is shown below::
+
+        void bpf_task_send_signal(struct task_struct *task__ref, int signal)
+        {
+        ...
+        }
+
+Here, bpf_task_send_signal will only act on trusted task_struct pointers, and
+cannot be used on pointers obtained using pointer walking. This ensures that
+caller always calls this kfunc on a task whose lifetime is guaranteed for the
+duration of the call.
+
 .. _BPF_kfunc_nodef:
 
 2.3 Using an existing kernel function
kernel/bpf/btf.c +26 -13
index 903719b89238..7e273f949ee8 100644
@@ -6140,18 +6140,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))

@@ -6160,10 +6155,26 @@ 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))
+       return !strncmp(param_name, suffix, sfx_len);
+}
+
+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 true;
+       return btf_param_match_suffix(btf, arg, "__sz");
 }
 
 static int btf_check_func_arg_match(struct bpf_verifier_env *env,

@@ -6173,7 +6184,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
                                    u32 kfunc_flags)
 {
        enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
-       bool rel = false, kptr_get = false, trusted_arg = false;
+       bool rel = false, kptr_get = false, kf_trusted_args = false;
        bool sleepable = false;
        struct bpf_verifier_log *log = &env->log;
        u32 i, nargs, ref_id, ref_obj_id = 0;

@@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
                /* Only kfunc can be release func */
                rel = kfunc_flags & KF_RELEASE;
                kptr_get = kfunc_flags & KF_KPTR_GET;
-               trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
+               kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS;
                sleepable = kfunc_flags & KF_SLEEPABLE;
        }
 

@@ -6222,6 +6233,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 = &regs[regno];
+               bool trusted_arg = false;
 
                t = btf_type_skip_modifiers(btf, args[i].type, NULL);
                if (btf_type_is_scalar(t)) {

@@ -6240,6 +6252,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
                /* Check if argument must be a referenced pointer, args + i has
                 * been verified to be a pointer (after skipping modifiers).
                 */
+               trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i);
                if (is_kfunc && trusted_arg && !reg->ref_obj_id) {
                        bpf_log(log, "R%d must be referenced\n", regno);
                        return -EINVAL;
net/bpf/test_run.c +7 -3
index 25d8ecf105aa..b735accf8750 100644
@@ -691,7 +691,11 @@ 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)
+noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p)
+{
+}
+
+noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref)
 {
 }
 

@@ -722,7 +726,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
 BTF_SET8_END(test_sk_check_kfunc_ids)
 
--

2.37.3

author
#3 lore raw

PATCH v2 2/4 selftests/bpf: Extend KF_TRUSTED_ARGS test for __ref annotation

In reply to Lorenzo Bianconi

From: Kumar Kartikeya Dwivedi <[email protected]>

Extend the existing test for KF_TRUSTED_ARGS by also checking whether the same happens when a __ref suffix is present in argument name of a kfunc.

Signed-off-by Kumar Kartikeya Dwivedi <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 tools/testing/selftests/bpf/verifier/calls.c | 38 +++++++++++++++-----
 1 file changed, 30 insertions(+), 8 deletions(-)
tools/testing/selftests/bpf/verifier/calls.c +30 -8
index 3fb4f69b1962..891fcda50d9d 100644
@@ -219,7 +219,7 @@
 	.errstr = "variable ptr_ access var_off=(0x0; 0x7) disallowed",
 },
 {
-	"calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID",
+	"calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID (KF_TRUSTED_ARGS)",
 	.insns = {
 	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
 	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),

@@ -227,10 +227,30 @@
 	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_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 16),
 	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_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_trusted", 7 },
+	},
+	.result_unpriv = REJECT,
+	.result = REJECT,
+	.errstr = "R1 must be referenced",
+},
+{
+	"calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID (__ref)",
+	.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_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 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(),

@@ -238,8 +258,7 @@
 	.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 },
+		{ "bpf_kfunc_call_test_ref", 7 },
 	},
 	.result_unpriv = REJECT,
 	.result = REJECT,

@@ -259,14 +278,17 @@
 	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_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 },
+		{ "bpf_kfunc_call_test_trusted", 8 },
+		{ "bpf_kfunc_call_test_ref", 10 },
+		{ "bpf_kfunc_call_test_release", 12 },
 	},
 	.result_unpriv = REJECT,
 	.result = ACCEPT,
Signature · 1 line
2.37.3
reviewer Acked-by on v2 2/4
#4 lore raw

Re: [PATCH v2 bpf-next 2/4] selftests/bpf: Extend KF_TRUSTED_ARGS test for __ref annotation

Quoted 9 lines · Lorenzo Bianconi

From: Kumar Kartikeya Dwivedi <[email protected]>

Extend the existing test for KF_TRUSTED_ARGS by also checking whether the same happens when a __ref suffix is present in argument name of a kfunc.

Signed-off-by Kumar Kartikeya Dwivedi <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
Acked-by Song Liu <[email protected]>
Quoted 84 lines
---
 tools/testing/selftests/bpf/verifier/calls.c | 38 +++++++++++++++-----
 1 file changed, 30 insertions(+), 8 deletions(-)
tools/testing/selftests/bpf/verifier/calls.c +30 -9
index 3fb4f69b1962..891fcda50d9d 100644
@@ -219,7 +219,7 @@
        .errstr = "variable ptr_ access var_off=(0x0; 0x7) disallowed",
 },
 {
-       "calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID",
+       "calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID (KF_TRUSTED_ARGS)",
        .insns = {
        BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
        BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),

@@ -227,10 +227,30 @@
        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_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 16),
        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_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_trusted", 7 },
+       },
+       .result_unpriv = REJECT,
+       .result = REJECT,
+       .errstr = "R1 must be referenced",
+},
+{
+       "calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID (__ref)",
+       .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_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 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(),

@@ -238,8 +258,7 @@
        .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 },
+               { "bpf_kfunc_call_test_ref", 7 },
        },
        .result_unpriv = REJECT,
        .result = REJECT,

@@ -259,14 +278,17 @@
        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_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 },
+               { "bpf_kfunc_call_test_trusted", 8 },
+               { "bpf_kfunc_call_test_ref", 10 },
+               { "bpf_kfunc_call_test_release", 12 },
        },
        .result_unpriv = REJECT,
        .result = ACCEPT,
--

2.37.3

author
#5 lore raw

PATCH v2 3/4 net: netfilter: add bpf_ct_set_nat_info kfunc helper

In reply to Lorenzo Bianconi

Introduce bpf_ct_set_nat_info kfunc helper in order to set source and destination nat addresses/ports in a new allocated ct entry not inserted in the connection tracking table yet.

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 net/netfilter/nf_conntrack_bpf.c | 49 +++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)
net/netfilter/nf_conntrack_bpf.c +48 -1
index 1cd87b28c9b0..85b8c7ee00af 100644
@@ -14,6 +14,7 @@
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_bpf.h>
 #include <net/netfilter/nf_conntrack_core.h>
+#include <net/netfilter/nf_nat.h>
 
 /* bpf_ct_opts - Options for CT lookup helpers
  *

@@ -134,7 +135,6 @@ __bpf_nf_ct_alloc_entry(struct net *net, struct bpf_sock_tuple *bpf_tuple,
 
 	memset(&ct->proto, 0, sizeof(ct->proto));
 	__nf_ct_set_timeout(ct, timeout * HZ);
-	ct->status |= IPS_CONFIRMED;
 
 out:
 	if (opts->netns_id >= 0)

@@ -339,6 +339,7 @@ struct nf_conn *bpf_ct_insert_entry(struct nf_conn___init *nfct_i)
 	struct nf_conn *nfct = (struct nf_conn *)nfct_i;
 	int err;
 
+	nfct->status |= IPS_CONFIRMED;
 	err = nf_conntrack_hash_check_insert(nfct);
 	if (err < 0) {
 		nf_conntrack_free(nfct);

@@ -424,6 +425,51 @@ int bpf_ct_change_status(struct nf_conn *nfct, u32 status)
 	return nf_ct_change_status_common(nfct, status);
 }
 
+/* bpf_ct_set_nat_info - Set source or destination nat address
+ *
+ * Set source or destination nat address of the newly allocated
+ * nf_conn before insertion. This must be invoked for referenced
+ * PTR_TO_BTF_ID to nf_conn___init.
+ *
+ * Parameters:
+ * @nfct	- Pointer to referenced nf_conn object, obtained using
+ *		  bpf_xdp_ct_alloc or bpf_skb_ct_alloc.
+ * @addr	- Nat source/destination address
+ * @port	- Nat source/destination port
+ * @manip	- NF_NAT_MANIP_SRC or NF_NAT_MANIP_DST
+ */
+int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
+			union nf_inet_addr *addr, __be16 *port,
+			enum nf_nat_manip_type manip)
+{
+#if ((IS_MODULE(CONFIG_NF_NAT) && IS_MODULE(CONFIG_NF_CONNTRACK)) || \
+     IS_BUILTIN(CONFIG_NF_NAT))
+	struct nf_conn *ct = (struct nf_conn *)nfct__ref;
+	u16 proto = nf_ct_l3num(ct);
+	struct nf_nat_range2 range;
+
+	if (proto != NFPROTO_IPV4 && proto != NFPROTO_IPV6)
+		return -EINVAL;
+
+	if (!addr)
+		return -EINVAL;
+
+	memset(&range, 0, sizeof(struct nf_nat_range2));
+	range.flags = NF_NAT_RANGE_MAP_IPS;
+	range.min_addr = *addr;
+	range.max_addr = *addr;
+	if (port) {
+		range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
+		range.min_proto.all = *port;
+		range.max_proto.all = *port;
+	}
+
+	return nf_nat_setup_info(ct, &range, manip) == NF_DROP ? -ENOMEM : 0;
+#else
+	return -EOPNOTSUPP;
+#endif
+}
+
 __diag_pop()
 
 BTF_SET8_START(nf_ct_kfunc_set)

@@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
 BTF_SET8_END(nf_ct_kfunc_set)
 
 static const struct btf_kfunc_id_set nf_conntrack_kfunc_set = {
Signature · 1 line
2.37.3
reviewer
#6 lore raw

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

Quoted 39 lines · Lorenzo Bianconi

Introduce bpf_ct_set_nat_info kfunc helper in order to set source and destination nat addresses/ports in a new allocated ct entry not inserted in the connection tracking table yet.

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 net/netfilter/nf_conntrack_bpf.c | 49 +++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)
net/netfilter/nf_conntrack_bpf.c +2 -1
index 1cd87b28c9b0..85b8c7ee00af 100644
@@ -14,6 +14,7 @@
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_bpf.h>
 #include <net/netfilter/nf_conntrack_core.h>
+#include <net/netfilter/nf_nat.h>
 
 /* bpf_ct_opts - Options for CT lookup helpers
  *

@@ -134,7 +135,6 @@ __bpf_nf_ct_alloc_entry(struct net *net, struct bpf_sock_tuple *bpf_tuple,
 
        memset(&ct->proto, 0, sizeof(ct->proto));
        __nf_ct_set_timeout(ct, timeout * HZ);
-       ct->status |= IPS_CONFIRMED;
 
 out:
        if (opts->netns_id >= 0)

@@ -339,6 +339,7 @@ struct nf_conn *bpf_ct_insert_entry(struct nf_conn___init *nfct_i)
        struct nf_conn *nfct = (struct nf_conn *)nfct_i;
        int err;
 
+       nfct->status |= IPS_CONFIRMED;
        err = nf_conntrack_hash_check_insert(nfct);
        if (err < 0) {
                nf_conntrack_free(nfct);

net/netfilter/nf_conntrack_bpf.c int bpf_ct_change_status(struct nf_conn *nfct, u32 status) · line 426

Quoted 2 lines
net/netfilter/nf_conntrack_bpf.c +0 -0
@@ -424,6 +425,51 @@ int bpf_ct_change_status(struct nf_conn *nfct, u32 status)
        return nf_ct_change_status_common(nfct, status);
 }

Why do we need the above two changes in this patch?

Thanks,
Song

author
#7 lore raw

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

Quoted 35 lines · Lorenzo Bianconi
Quoted 34 lines

Introduce bpf_ct_set_nat_info kfunc helper in order to set source and destination nat addresses/ports in a new allocated ct entry not inserted in the connection tracking table yet.

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 net/netfilter/nf_conntrack_bpf.c | 49 +++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)
net/netfilter/nf_conntrack_bpf.c +1 -1
index 1cd87b28c9b0..85b8c7ee00af 100644
@@ -14,6 +14,7 @@
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_bpf.h>
 #include <net/netfilter/nf_conntrack_core.h>
+#include <net/netfilter/nf_nat.h>
 
 /* bpf_ct_opts - Options for CT lookup helpers
  *

@@ -134,7 +135,6 @@ __bpf_nf_ct_alloc_entry(struct net *net, struct bpf_sock_tuple *bpf_tuple,
 
        memset(&ct->proto, 0, sizeof(ct->proto));
        __nf_ct_set_timeout(ct, timeout * HZ);
-       ct->status |= IPS_CONFIRMED;
 
 out:
        if (opts->netns_id >= 0)

@@ -339,6 +339,7 @@ struct nf_conn *bpf_ct_insert_entry(struct nf_conn___init *nfct_i)
        struct nf_conn *nfct = (struct nf_conn *)nfct_i;
        int err;
Quoted 9 lines

net/netfilter/nf_conntrack_bpf.c int bpf_ct_change_status(struct nf_conn *nfct, u32 status) · line 426

Quoted 7 lines
net/netfilter/nf_conntrack_bpf.c +1 -0
+       nfct->status |= IPS_CONFIRMED;
        err = nf_conntrack_hash_check_insert(nfct);
        if (err < 0) {
                nf_conntrack_free(nfct);

@@ -424,6 +425,51 @@ int bpf_ct_change_status(struct nf_conn *nfct, u32 status)
        return nf_ct_change_status_common(nfct, status);
 }

Why do we need the above two changes in this patch?

nf_nat_setup_info() does not really add the nat info in the connection tracking entry if it is already confirmed (it just returns NF_ACCEPT). I moved IPS_CONFIRMED in bpf_ct_insert_entry() since we can run bpf_ct_set_nat_info() just if the entry has not inserted in the table yet.

Regards,
Lorenzo

Quoted 3 lines

Thanks,
Song

#8 lore raw

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

In reply to Lorenzo Bianconi
Quoted 4 lines · Lorenzo Bianconi
net/netfilter/nf_conntrack_bpf.c +4 -0
+int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
+                       union nf_inet_addr *addr, __be16 *port,
+                       enum nf_nat_manip_type manip)
+{

...

net/netfilter/nf_conntrack_bpf.c BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS) · line 487

Quoted 6 lines
net/netfilter/nf_conntrack_bpf.c +1 -0
@@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
 BTF_SET8_END(nf_ct_kfunc_set)

Instead of __ref and patch 1 and 2 it would be better to change the meaning of "trusted_args".
In this case "addr" and "port" are just as "trusted". They're not refcounted per verifier definition, but they need to be "trusted" by the helper.
At the end the "trusted_args" flags would mean "this helper can assume that all pointers can be safely accessed without worrying about lifetime".

maintainer
#9 lore raw

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

Quoted 7 lines · Alexei Starovoitov
Quoted 4 lines · Lorenzo Bianconi
net/netfilter/nf_conntrack_bpf.c +4 -0
+int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
+                       union nf_inet_addr *addr, __be16 *port,
+                       enum nf_nat_manip_type manip)
+{

...

Quoted 15 lines

net/netfilter/nf_conntrack_bpf.c BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS) · line 487

Quoted 6 lines
net/netfilter/nf_conntrack_bpf.c +1 -0
@@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
 BTF_SET8_END(nf_ct_kfunc_set)

Instead of __ref and patch 1 and 2 it would be better to change the meaning of "trusted_args".
In this case "addr" and "port" are just as "trusted". They're not refcounted per verifier definition, but they need to be "trusted" by the helper.
At the end the "trusted_args" flags would mean "this helper can assume that all pointers can be safely accessed without worrying about lifetime".

So you mean it only forces PTR_TO_BTF_ID to have reg->ref_obj_id > 0?

But suppose in the future you have a type that has scalars only.

struct foo { int a; int b; ... };
Just data, and this is acquired from a kfunc and released using another kfunc. Now with this new definition you are proposing, verifier ends up allowing PTR_TO_MEM to also be passed to such helpers for the struct foo *.

I guess even reg->ref_obj_id check is not enough, user may also pass PTR_TO_MEM | MEM_ALLOC which can be refcounted.

It would be easy to forget such subtle details later.

What we want to actually force here is 'please give me refcounted PTR_TO_BTF_ID to foo'.
So maybe KF_TRUSTED_ARGS should change meaning to what you described, and then a __btf tag should force PTR_TO_BTF_ID. KF_TRUSTED_ARGS then just forces reg->ref_obj_id for PTR_TO_BTF_ID.

#10 lore raw

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

Quoted 36 lines · Kumar Kartikeya Dwivedi
Quoted 7 lines · Alexei Starovoitov
Quoted 4 lines · Lorenzo Bianconi
net/netfilter/nf_conntrack_bpf.c +4 -0
+int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
+                       union nf_inet_addr *addr, __be16 *port,
+                       enum nf_nat_manip_type manip)
+{

...

Quoted 15 lines

net/netfilter/nf_conntrack_bpf.c BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS) · line 487

Quoted 6 lines
net/netfilter/nf_conntrack_bpf.c +1 -0
@@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
 BTF_SET8_END(nf_ct_kfunc_set)

Instead of __ref and patch 1 and 2 it would be better to change the meaning of "trusted_args".
In this case "addr" and "port" are just as "trusted". They're not refcounted per verifier definition, but they need to be "trusted" by the helper.
At the end the "trusted_args" flags would mean "this helper can assume that all pointers can be safely accessed without worrying about lifetime".

So you mean it only forces PTR_TO_BTF_ID to have reg->ref_obj_id > 0?

But suppose in the future you have a type that has scalars only.

struct foo { int a; int b; ... };
Just data, and this is acquired from a kfunc and released using another kfunc. Now with this new definition you are proposing, verifier ends up allowing PTR_TO_MEM to also be passed to such helpers for the struct foo *.

Quoted 4 lines

I guess even reg->ref_obj_id check is not enough, user may also pass PTR_TO_MEM | MEM_ALLOC which can be refcounted.

It would be easy to forget such subtle details later.

It may add headaches to the verifier side, but here we have to think from pov of other subsystems that add kfuncs.
They shouldn't need to know the verifier details.
The internals will change anyway.
Ideally KF_TRUSTED_ARGS will become the default flag that every kfunc will use to indicate that the function assumes valid pointers. How the verifier recognizes them is irrelevant from kfunc pov. People that write bpf progs are not that much different from people that write kfuncs that bpf progs use.
Both should be easy to write.

maintainer
#11 lore raw

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

Quoted 41 lines · Alexei Starovoitov
Quoted 39 lines · Kumar Kartikeya Dwivedi
Quoted 7 lines · Alexei Starovoitov
Quoted 4 lines · Lorenzo Bianconi
net/netfilter/nf_conntrack_bpf.c +4 -0
+int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
+                       union nf_inet_addr *addr, __be16 *port,
+                       enum nf_nat_manip_type manip)
+{

...

Quoted 15 lines

net/netfilter/nf_conntrack_bpf.c BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS) · line 487

Quoted 6 lines
net/netfilter/nf_conntrack_bpf.c +1 -0
@@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
 BTF_SET8_END(nf_ct_kfunc_set)

Instead of __ref and patch 1 and 2 it would be better to change the meaning of "trusted_args".
In this case "addr" and "port" are just as "trusted". They're not refcounted per verifier definition, but they need to be "trusted" by the helper.
At the end the "trusted_args" flags would mean "this helper can assume that all pointers can be safely accessed without worrying about lifetime".

So you mean it only forces PTR_TO_BTF_ID to have reg->ref_obj_id > 0?

But suppose in the future you have a type that has scalars only.

struct foo { int a; int b; ... };
Just data, and this is acquired from a kfunc and released using another kfunc. Now with this new definition you are proposing, verifier ends up allowing PTR_TO_MEM to also be passed to such helpers for the struct foo *.

I guess even reg->ref_obj_id check is not enough, user may also pass PTR_TO_MEM | MEM_ALLOC which can be refcounted.

Quoted 6 lines
Quoted 1 line

It would be easy to forget such subtle details later.

It may add headaches to the verifier side, but here we have to think from pov of other subsystems that add kfuncs. They shouldn't need to know the verifier details. The internals will change anyway.

Ok, I'll go with making it work for all args for this case.

Quoted 6 lines

Ideally KF_TRUSTED_ARGS will become the default flag that every kfunc will use to indicate that the function assumes valid pointers. How the verifier recognizes them is irrelevant from kfunc pov. People that write bpf progs are not that much different from people that write kfuncs that bpf progs use.
Both should be easy to write.

That is a worthy goal, but it can't become the default unless we somehow fix how normal PTR_TO_BTF_ID without ref_obj_id is allowed to be valid, valid-looking-but-uaf pointer, NULL all at the same time depending on how it was obtained. Currently all helpers, even stable ones, are broken in this regard. Similarly recently added cgroup_rstat_flush etc. kfuncs are equally unsafe.

All stable helpers taking PTR_TO_BTF_ID are not even checking for at least NULL, even though it's right there in bpf.h.
592 /* PTR_TO_BTF_ID points to a kernel struct that does not need
593 * to be null checked by the BPF program. This does not imply the
594 * pointer is _not_ null and in practice this can
easily be a null
595 * pointer when reading pointer chains. The assumption is program
which just proves how confusing it is right now. And "fixing" that by adding a NULL check doesn't fix it completely, since it can also be a seemingly valid looking but freed pointer.

My previous proposal still stands, to accommodate direct PTR_TO_BTF_ID pointers from loads from PTR_TO_CTX of tracing progs into this definition of 'trusted', but not those obtained from walking them. It works for iterator arguments also.

We could limit these restrictions only to kfuncs instead of stable helpers.

It might be possible to instead just whitelist the function BTF IDs as well, even saying pointers from walks are also safe in this context for the kfuncs allowed there, or we work on annotating the safe cases using BTF tags.

There are some problems currently (GCC not supporting BTF tags yet, is argument really trusted in fexit program in 'xyz_free' function), but overall it seems like a better state than status quo. It might also finally push GCC to begin supporting BTF tags.

Mapping of a set of btf_ids can be done to a specific kfunc hook (instead of current program type), so you are basically composing a kfunc hook out of a set of btf_ids instead of program type. It represents a safe context to call those kfuncs in.

It is impossible to know otherwise what case is safe to call a kfunc for and what is not statically - short of also allowing the unsafe cases.

Then the kfuncs work on refcounted pointers, and also unrefcounted ones for known safe cases (basically where the lifetime is guaranteed by bpf program caller). For arguments it works by default. The only extra work is annotating things inside structures.
Might not even need that extra annotation in many cases, since kernel already has __rcu etc. which we can start recognizing like __user to complain in non-sleepable programs (e.g. without explicit RCU section which may be added in the future).

Then just flip KF_TRUSTED_ARGS by default, and people have to opt into 'unsafe' instead to make it work for some edge cases, with a big fat warning for the user of that kfunc.

#12 lore raw

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

Quoted 113 lines · Kumar Kartikeya Dwivedi
Quoted 41 lines · Alexei Starovoitov
Quoted 39 lines · Kumar Kartikeya Dwivedi
Quoted 7 lines · Alexei Starovoitov
Quoted 4 lines · Lorenzo Bianconi
net/netfilter/nf_conntrack_bpf.c +4 -0
+int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
+                       union nf_inet_addr *addr, __be16 *port,
+                       enum nf_nat_manip_type manip)
+{

...

Quoted 15 lines

net/netfilter/nf_conntrack_bpf.c BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS) · line 487

Quoted 6 lines
net/netfilter/nf_conntrack_bpf.c +1 -0
@@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
 BTF_SET8_END(nf_ct_kfunc_set)

Instead of __ref and patch 1 and 2 it would be better to change the meaning of "trusted_args".
In this case "addr" and "port" are just as "trusted". They're not refcounted per verifier definition, but they need to be "trusted" by the helper.
At the end the "trusted_args" flags would mean "this helper can assume that all pointers can be safely accessed without worrying about lifetime".

So you mean it only forces PTR_TO_BTF_ID to have reg->ref_obj_id > 0?

But suppose in the future you have a type that has scalars only.

struct foo { int a; int b; ... };
Just data, and this is acquired from a kfunc and released using another kfunc. Now with this new definition you are proposing, verifier ends up allowing PTR_TO_MEM to also be passed to such helpers for the struct foo *.

I guess even reg->ref_obj_id check is not enough, user may also pass PTR_TO_MEM | MEM_ALLOC which can be refcounted.

Quoted 6 lines
Quoted 1 line

It would be easy to forget such subtle details later.

It may add headaches to the verifier side, but here we have to think from pov of other subsystems that add kfuncs. They shouldn't need to know the verifier details. The internals will change anyway.

Ok, I'll go with making it work for all args for this case.

Quoted 6 lines

Ideally KF_TRUSTED_ARGS will become the default flag that every kfunc will use to indicate that the function assumes valid pointers. How the verifier recognizes them is irrelevant from kfunc pov. People that write bpf progs are not that much different from people that write kfuncs that bpf progs use.
Both should be easy to write.

That is a worthy goal, but it can't become the default unless we somehow fix how normal PTR_TO_BTF_ID without ref_obj_id is allowed to be valid, valid-looking-but-uaf pointer, NULL all at the same time depending on how it was obtained. Currently all helpers, even stable ones, are broken in this regard. Similarly recently added cgroup_rstat_flush etc. kfuncs are equally unsafe.

All stable helpers taking PTR_TO_BTF_ID are not even checking for at least NULL, even though it's right there in bpf.h.
592 /* PTR_TO_BTF_ID points to a kernel struct that does not need
593 * to be null checked by the BPF program. This does not imply the
594 * pointer is _not_ null and in practice this can
easily be a null
595 * pointer when reading pointer chains. The assumption is program
which just proves how confusing it is right now. And "fixing" that by adding a NULL check doesn't fix it completely, since it can also be a seemingly valid looking but freed pointer.

My previous proposal still stands, to accommodate direct PTR_TO_BTF_ID pointers from loads from PTR_TO_CTX of tracing progs into this definition of 'trusted', but not those obtained from walking them. It works for iterator arguments also.

We could limit these restrictions only to kfuncs instead of stable helpers.

It might be possible to instead just whitelist the function BTF IDs as well, even saying pointers from walks are also safe in this context for the kfuncs allowed there, or we work on annotating the safe cases using BTF tags.

There are some problems currently (GCC not supporting BTF tags yet, is argument really trusted in fexit program in 'xyz_free' function), but overall it seems like a better state than status quo. It might also finally push GCC to begin supporting BTF tags.

Mapping of a set of btf_ids can be done to a specific kfunc hook (instead of current program type), so you are basically composing a kfunc hook out of a set of btf_ids instead of program type. It represents a safe context to call those kfuncs in.

It is impossible to know otherwise what case is safe to call a kfunc for and what is not statically - short of also allowing the unsafe cases.

Then the kfuncs work on refcounted pointers, and also unrefcounted ones for known safe cases (basically where the lifetime is guaranteed by bpf program caller). For arguments it works by default. The only extra work is annotating things inside structures.
Might not even need that extra annotation in many cases, since kernel already has __rcu etc. which we can start recognizing like __user to complain in non-sleepable programs (e.g. without explicit RCU section which may be added in the future).

Quoted 3 lines

Then just flip KF_TRUSTED_ARGS by default, and people have to opt into 'unsafe' instead to make it work for some edge cases, with a big fat warning for the user of that kfunc.

With few minor nits, that I don't want to get into right now, all of the above makes sense. It can be a plan of record. But all that will be done later.
The immediate first step I'm proposing is
to extend the definition of KF_TRUSTED_ARGS to include this particular use case of:
union nf_inet_addr *addr, __be16 *port,

Those won't be PTR_TO_BTF_ID so above plan doesn't affect this case. They're PTR_TO_MEM (if I'm reading the selftest in the next patch correctly) and we can relax:

                if (is_kfunc && trusted_arg && !reg->ref_obj_id) {

Just minimal amount of verifier work to enable this specific bpf_ct_set_nat_info kfunc.

I think that's user friendlier approach than __ref suffix which forces kfunc writers to understand all of the above verifier details (valid-looking-but-uaf, null-but-not-null, etc).

maintainer
#13 lore raw

Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

Quoted 131 lines · Alexei Starovoitov
Quoted 113 lines · Kumar Kartikeya Dwivedi
Quoted 41 lines · Alexei Starovoitov
Quoted 39 lines · Kumar Kartikeya Dwivedi
Quoted 7 lines · Alexei Starovoitov
Quoted 4 lines · Lorenzo Bianconi
net/netfilter/nf_conntrack_bpf.c +4 -0
+int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
+                       union nf_inet_addr *addr, __be16 *port,
+                       enum nf_nat_manip_type manip)
+{

...

Quoted 15 lines

net/netfilter/nf_conntrack_bpf.c BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS) · line 487

Quoted 6 lines
net/netfilter/nf_conntrack_bpf.c +1 -0
@@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
 BTF_SET8_END(nf_ct_kfunc_set)

Instead of __ref and patch 1 and 2 it would be better to change the meaning of "trusted_args".
In this case "addr" and "port" are just as "trusted". They're not refcounted per verifier definition, but they need to be "trusted" by the helper.
At the end the "trusted_args" flags would mean "this helper can assume that all pointers can be safely accessed without worrying about lifetime".

So you mean it only forces PTR_TO_BTF_ID to have reg->ref_obj_id > 0?

But suppose in the future you have a type that has scalars only.

struct foo { int a; int b; ... };
Just data, and this is acquired from a kfunc and released using another kfunc. Now with this new definition you are proposing, verifier ends up allowing PTR_TO_MEM to also be passed to such helpers for the struct foo *.

I guess even reg->ref_obj_id check is not enough, user may also pass PTR_TO_MEM | MEM_ALLOC which can be refcounted.

Quoted 6 lines
Quoted 1 line

It would be easy to forget such subtle details later.

It may add headaches to the verifier side, but here we have to think from pov of other subsystems that add kfuncs. They shouldn't need to know the verifier details. The internals will change anyway.

Ok, I'll go with making it work for all args for this case.

Quoted 6 lines

Ideally KF_TRUSTED_ARGS will become the default flag that every kfunc will use to indicate that the function assumes valid pointers. How the verifier recognizes them is irrelevant from kfunc pov. People that write bpf progs are not that much different from people that write kfuncs that bpf progs use.
Both should be easy to write.

That is a worthy goal, but it can't become the default unless we somehow fix how normal PTR_TO_BTF_ID without ref_obj_id is allowed to be valid, valid-looking-but-uaf pointer, NULL all at the same time depending on how it was obtained. Currently all helpers, even stable ones, are broken in this regard. Similarly recently added cgroup_rstat_flush etc. kfuncs are equally unsafe.

All stable helpers taking PTR_TO_BTF_ID are not even checking for at least NULL, even though it's right there in bpf.h.
592 /* PTR_TO_BTF_ID points to a kernel struct that does not need
593 * to be null checked by the BPF program. This does not imply the
594 * pointer is _not_ null and in practice this can
easily be a null
595 * pointer when reading pointer chains. The assumption is program
which just proves how confusing it is right now. And "fixing" that by adding a NULL check doesn't fix it completely, since it can also be a seemingly valid looking but freed pointer.

My previous proposal still stands, to accommodate direct PTR_TO_BTF_ID pointers from loads from PTR_TO_CTX of tracing progs into this definition of 'trusted', but not those obtained from walking them. It works for iterator arguments also.

We could limit these restrictions only to kfuncs instead of stable helpers.

It might be possible to instead just whitelist the function BTF IDs as well, even saying pointers from walks are also safe in this context for the kfuncs allowed there, or we work on annotating the safe cases using BTF tags.

There are some problems currently (GCC not supporting BTF tags yet, is argument really trusted in fexit program in 'xyz_free' function), but overall it seems like a better state than status quo. It might also finally push GCC to begin supporting BTF tags.

Mapping of a set of btf_ids can be done to a specific kfunc hook (instead of current program type), so you are basically composing a kfunc hook out of a set of btf_ids instead of program type. It represents a safe context to call those kfuncs in.

It is impossible to know otherwise what case is safe to call a kfunc for and what is not statically - short of also allowing the unsafe cases.

Then the kfuncs work on refcounted pointers, and also unrefcounted ones for known safe cases (basically where the lifetime is guaranteed by bpf program caller). For arguments it works by default. The only extra work is annotating things inside structures.
Might not even need that extra annotation in many cases, since kernel already has __rcu etc. which we can start recognizing like __user to complain in non-sleepable programs (e.g. without explicit RCU section which may be added in the future).

Quoted 3 lines

Then just flip KF_TRUSTED_ARGS by default, and people have to opt into 'unsafe' instead to make it work for some edge cases, with a big fat warning for the user of that kfunc.

With few minor nits, that I don't want to get into right now, all of the above makes sense. It can be a plan of record. But all that will be done later.
The immediate first step I'm proposing is
to extend the definition of KF_TRUSTED_ARGS to include this particular use case of:
union nf_inet_addr *addr, __be16 *port,

Those won't be PTR_TO_BTF_ID so above plan doesn't affect this case. They're PTR_TO_MEM (if I'm reading the selftest in the next patch correctly) and we can relax:

Quoted 8 lines
                if (is_kfunc && trusted_arg && !reg->ref_obj_id) {

Just minimal amount of verifier work to enable this specific bpf_ct_set_nat_info kfunc.

I think that's user friendlier approach than __ref suffix which forces kfunc writers to understand all of the above verifier details (valid-looking-but-uaf, null-but-not-null, etc).

I agree, it seems better from a UX standpoint. I'll relax the trusted_args check for non-PTR_TO_BTF_ID, and send the patch to flip it to default as an RFC later.

author
#14 lore raw

PATCH v2 4/4 selftests/bpf: add tests for bpf_ct_set_nat_info kfunc

In reply to Lorenzo Bianconi

Introduce self-tests for bpf_ct_set_nat_info kfunc used to set the source or destination nat addresses/ports.

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 tools/testing/selftests/bpf/config            |  1 +
 .../testing/selftests/bpf/prog_tests/bpf_nf.c |  2 ++
 .../testing/selftests/bpf/progs/test_bpf_nf.c | 26 ++++++++++++++++++-
 3 files changed, 28 insertions(+), 1 deletion(-)
tools/testing/selftests/bpf/config +1 -0
index 3fc46f9cfb22..8ce48f7213cb 100644
@@ -57,6 +57,7 @@ CONFIG_NF_CONNTRACK=y
 CONFIG_NF_CONNTRACK_MARK=y
 CONFIG_NF_DEFRAG_IPV4=y
 CONFIG_NF_DEFRAG_IPV6=y
+CONFIG_NF_NAT=y
 CONFIG_RC_CORE=y
 CONFIG_SECURITY=y
 CONFIG_SECURITYFS=y
tools/testing/selftests/bpf/prog_tests/bpf_nf.c +2 -0
index 544bf90ac2a7..f16913f8fca2 100644
@@ -115,6 +115,8 @@ static void test_bpf_nf_ct(int mode)
 	ASSERT_EQ(skel->bss->test_status, 2, "Test for ct status update ");
 	ASSERT_EQ(skel->data->test_exist_lookup, 0, "Test existing connection lookup");
 	ASSERT_EQ(skel->bss->test_exist_lookup_mark, 43, "Test existing connection lookup ctmark");
+	ASSERT_EQ(skel->data->test_snat_addr, 0, "Test for source natting");
+	ASSERT_EQ(skel->data->test_dnat_addr, 0, "Test for destination natting");
 end:
 	if (srv_client_fd != -1)
 		close(srv_client_fd);
tools/testing/selftests/bpf/progs/test_bpf_nf.c +25 -1
index 2722441850cc..3f441595098b 100644
@@ -23,6 +23,8 @@ int test_insert_entry = -EAFNOSUPPORT;
 int test_succ_lookup = -ENOENT;
 u32 test_delta_timeout = 0;
 u32 test_status = 0;
+int test_snat_addr = -EINVAL;
+int test_dnat_addr = -EINVAL;
 __be32 saddr = 0;
 __be16 sport = 0;
 __be32 daddr = 0;

@@ -53,6 +55,8 @@ void bpf_ct_set_timeout(struct nf_conn *, u32) __ksym;
 int bpf_ct_change_timeout(struct nf_conn *, u32) __ksym;
 int bpf_ct_set_status(struct nf_conn *, u32) __ksym;
 int bpf_ct_change_status(struct nf_conn *, u32) __ksym;
+int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *,
+			__be16 *port, enum nf_nat_manip_type) __ksym;
 
 static __always_inline void
 nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,

@@ -140,10 +144,19 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
 	ct = alloc_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
 		      sizeof(opts_def));
 	if (ct) {
+		__be16 sport = bpf_get_prandom_u32();
+		__be16 dport = bpf_get_prandom_u32();
+		union nf_inet_addr saddr = {};
+		union nf_inet_addr daddr = {};
 		struct nf_conn *ct_ins;
 
 		bpf_ct_set_timeout(ct, 10000);
-		bpf_ct_set_status(ct, IPS_CONFIRMED);
+		/* snat */
+		saddr.ip = bpf_get_prandom_u32();
+		bpf_ct_set_nat_info(ct, &saddr, &sport, NF_NAT_MANIP_SRC);
+		/* dnat */
+		daddr.ip = bpf_get_prandom_u32();
+		bpf_ct_set_nat_info(ct, &daddr, &dport, NF_NAT_MANIP_DST);
 
 		ct_ins = bpf_ct_insert_entry(ct);
 		if (ct_ins) {

@@ -152,6 +165,17 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
 			ct_lk = lookup_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4),
 					  &opts_def, sizeof(opts_def));
 			if (ct_lk) {
+				struct nf_conntrack_tuple *tuple;
+
+				/* check snat and dnat addresses */
+				tuple = &ct_lk->tuplehash[IP_CT_DIR_REPLY].tuple;
+				if (tuple->dst.u3.ip == saddr.ip &&
+				    tuple->dst.u.all == sport)
+					test_snat_addr = 0;
+				if (tuple->src.u3.ip == daddr.ip &&
+				    tuple->src.u.all == dport)
+					test_dnat_addr = 0;
+
 				/* update ct entry timeout */
 				bpf_ct_change_timeout(ct_lk, 10000);
 				test_delta_timeout = ct_lk->timeout - bpf_jiffies64();
Signature · 1 line
2.37.3
reviewer
#15 lore raw

Re: [PATCH v2 bpf-next 4/4] selftests/bpf: add tests for bpf_ct_set_nat_info kfunc

Quoted 60 lines · Lorenzo Bianconi

Introduce self-tests for bpf_ct_set_nat_info kfunc used to set the source or destination nat addresses/ports.

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 tools/testing/selftests/bpf/config            |  1 +
 .../testing/selftests/bpf/prog_tests/bpf_nf.c |  2 ++
 .../testing/selftests/bpf/progs/test_bpf_nf.c | 26 ++++++++++++++++++-
 3 files changed, 28 insertions(+), 1 deletion(-)
tools/testing/selftests/bpf/config +1 -0
index 3fc46f9cfb22..8ce48f7213cb 100644
@@ -57,6 +57,7 @@ CONFIG_NF_CONNTRACK=y
 CONFIG_NF_CONNTRACK_MARK=y
 CONFIG_NF_DEFRAG_IPV4=y
 CONFIG_NF_DEFRAG_IPV6=y
+CONFIG_NF_NAT=y
 CONFIG_RC_CORE=y
 CONFIG_SECURITY=y
 CONFIG_SECURITYFS=y
tools/testing/selftests/bpf/prog_tests/bpf_nf.c +2 -0
index 544bf90ac2a7..f16913f8fca2 100644
@@ -115,6 +115,8 @@ static void test_bpf_nf_ct(int mode)
        ASSERT_EQ(skel->bss->test_status, 2, "Test for ct status update ");
        ASSERT_EQ(skel->data->test_exist_lookup, 0, "Test existing connection lookup");
        ASSERT_EQ(skel->bss->test_exist_lookup_mark, 43, "Test existing connection lookup ctmark");
+       ASSERT_EQ(skel->data->test_snat_addr, 0, "Test for source natting");
+       ASSERT_EQ(skel->data->test_dnat_addr, 0, "Test for destination natting");
 end:
        if (srv_client_fd != -1)
                close(srv_client_fd);
tools/testing/selftests/bpf/progs/test_bpf_nf.c +4 -0
index 2722441850cc..3f441595098b 100644
@@ -23,6 +23,8 @@ int test_insert_entry = -EAFNOSUPPORT;
 int test_succ_lookup = -ENOENT;
 u32 test_delta_timeout = 0;
 u32 test_status = 0;
+int test_snat_addr = -EINVAL;
+int test_dnat_addr = -EINVAL;
 __be32 saddr = 0;
 __be16 sport = 0;
 __be32 daddr = 0;

@@ -53,6 +55,8 @@ void bpf_ct_set_timeout(struct nf_conn *, u32) __ksym;
 int bpf_ct_change_timeout(struct nf_conn *, u32) __ksym;
 int bpf_ct_set_status(struct nf_conn *, u32) __ksym;
 int bpf_ct_change_status(struct nf_conn *, u32) __ksym;
+int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *,
+                       __be16 *port, enum nf_nat_manip_type) __ksym;
 
 static __always_inline void
 nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,

@@ -140,10 +144,19 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
        ct = alloc_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,

tools/testing/selftests/bpf/progs/test_bpf_nf.c nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, · line 146

Quoted 10 lines
tools/testing/selftests/bpf/progs/test_bpf_nf.c +4 -1
                      sizeof(opts_def));
        if (ct) {
+               __be16 sport = bpf_get_prandom_u32();
+               __be16 dport = bpf_get_prandom_u32();
+               union nf_inet_addr saddr = {};
+               union nf_inet_addr daddr = {};
                struct nf_conn *ct_ins;
 
                bpf_ct_set_timeout(ct, 10000);
-               bpf_ct_set_status(ct, IPS_CONFIRMED);

So this is paired with the IPS_CONFIRMED change in 3/4?

Thanks,
Song

author
#16 lore raw

Re: [PATCH v2 bpf-next 4/4] selftests/bpf: add tests for bpf_ct_set_nat_info kfunc

Quoted 69 lines · Lorenzo Bianconi
Quoted 68 lines

Introduce self-tests for bpf_ct_set_nat_info kfunc used to set the source or destination nat addresses/ports.

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 tools/testing/selftests/bpf/config            |  1 +
 .../testing/selftests/bpf/prog_tests/bpf_nf.c |  2 ++
 .../testing/selftests/bpf/progs/test_bpf_nf.c | 26 ++++++++++++++++++-
 3 files changed, 28 insertions(+), 1 deletion(-)
tools/testing/selftests/bpf/config +1 -0
index 3fc46f9cfb22..8ce48f7213cb 100644
@@ -57,6 +57,7 @@ CONFIG_NF_CONNTRACK=y
 CONFIG_NF_CONNTRACK_MARK=y
 CONFIG_NF_DEFRAG_IPV4=y
 CONFIG_NF_DEFRAG_IPV6=y
+CONFIG_NF_NAT=y
 CONFIG_RC_CORE=y
 CONFIG_SECURITY=y
 CONFIG_SECURITYFS=y
tools/testing/selftests/bpf/prog_tests/bpf_nf.c +2 -0
index 544bf90ac2a7..f16913f8fca2 100644
@@ -115,6 +115,8 @@ static void test_bpf_nf_ct(int mode)
        ASSERT_EQ(skel->bss->test_status, 2, "Test for ct status update ");
        ASSERT_EQ(skel->data->test_exist_lookup, 0, "Test existing connection lookup");
        ASSERT_EQ(skel->bss->test_exist_lookup_mark, 43, "Test existing connection lookup ctmark");
+       ASSERT_EQ(skel->data->test_snat_addr, 0, "Test for source natting");
+       ASSERT_EQ(skel->data->test_dnat_addr, 0, "Test for destination natting");
 end:
        if (srv_client_fd != -1)
                close(srv_client_fd);
tools/testing/selftests/bpf/progs/test_bpf_nf.c +8 -0
index 2722441850cc..3f441595098b 100644
@@ -23,6 +23,8 @@ int test_insert_entry = -EAFNOSUPPORT;
 int test_succ_lookup = -ENOENT;
 u32 test_delta_timeout = 0;
 u32 test_status = 0;
+int test_snat_addr = -EINVAL;
+int test_dnat_addr = -EINVAL;
 __be32 saddr = 0;
 __be16 sport = 0;
 __be32 daddr = 0;

@@ -53,6 +55,8 @@ void bpf_ct_set_timeout(struct nf_conn *, u32) __ksym;
 int bpf_ct_change_timeout(struct nf_conn *, u32) __ksym;
 int bpf_ct_set_status(struct nf_conn *, u32) __ksym;
 int bpf_ct_change_status(struct nf_conn *, u32) __ksym;
+int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *,
+                       __be16 *port, enum nf_nat_manip_type) __ksym;
 
 static __always_inline void
 nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,

@@ -140,10 +144,19 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
        ct = alloc_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
                      sizeof(opts_def));
        if (ct) {
+               __be16 sport = bpf_get_prandom_u32();
+               __be16 dport = bpf_get_prandom_u32();
+               union nf_inet_addr saddr = {};
+               union nf_inet_addr daddr = {};
                struct nf_conn *ct_ins;
Quoted 4 lines

tools/testing/selftests/bpf/progs/test_bpf_nf.c nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, · line 146

Quoted 2 lines
tools/testing/selftests/bpf/progs/test_bpf_nf.c +0 -1
                bpf_ct_set_timeout(ct, 10000);
-               bpf_ct_set_status(ct, IPS_CONFIRMED);

So this is paired with the IPS_CONFIRMED change in 3/4?

we actually do not need it since it is already done during entry allocation (or insertion).
Looking again at the code I spotted a bug since we do not really check the value configured with bpf_ct_change_status(ct_lk, IPS_SEEN_REPLY). I will post a fix.

Regards,
Lorenzo

Quoted 3 lines

Thanks,
Song