diff mbox series

[bpf-next,v4,3/6] bpf: Move is_valid_bpf_tramp_flags() to the public trampoline code

Message ID 20220517071838.3366093-4-xukuohai@huawei.com (mailing list archive)
State New, archived
Headers show
Series bpf trampoline for arm64 | expand

Commit Message

Xu Kuohai May 17, 2022, 7:18 a.m. UTC 
is_valid_bpf_tramp_flags() is not relevant to architecture, so move it
to the public trampoline code.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
Acked-by: Song Liu <songliubraving@fb.com>
---
 arch/x86/net/bpf_jit_comp.c | 20 --------------------
 include/linux/bpf.h         |  6 ++++++
 kernel/bpf/bpf_struct_ops.c |  4 ++--
 kernel/bpf/trampoline.c     | 34 +++++++++++++++++++++++++++++++---
 4 files changed, 39 insertions(+), 25 deletions(-)

Comments

Alexei Starovoitov May 17, 2022, 3:53 p.m. UTC | #1 
On Tue, May 17, 2022 at 03:18:35AM -0400, Xu Kuohai wrote:
>  
> +static bool is_valid_bpf_tramp_flags(unsigned int flags)
> +{
> +	if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
> +	    (flags & BPF_TRAMP_F_SKIP_FRAME))
> +		return false;
> +
> +	/* BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
> +	 * and it must be used alone.
> +	 */
> +	if ((flags & BPF_TRAMP_F_RET_FENTRY_RET) &&
> +	    (flags & ~BPF_TRAMP_F_RET_FENTRY_RET))
> +		return false;
> +
> +	return true;
> +}
> +
> +int bpf_prepare_trampoline(struct bpf_tramp_image *tr, void *image,
> +			   void *image_end, const struct btf_func_model *m,
> +			   u32 flags, struct bpf_tramp_links *tlinks,
> +			   void *orig_call)
> +{
> +	if (!is_valid_bpf_tramp_flags(flags))
> +		return -EINVAL;
> +
> +	return arch_prepare_bpf_trampoline(tr, image, image_end, m, flags,
> +					   tlinks, orig_call);
> +}

It's an overkill to introduce a new helper function just to validate
flags that almost compile time constants.
The flags are not user supplied.
Please move /* BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops ... */
comment to bpf_struct_ops.c right before it calls arch_prepare_bpf_trampoline()
And add a comment to trampoline.c saying that BPF_TRAMP_F_RESTORE_REGS
and BPF_TRAMP_F_SKIP_FRAME should not be set together.
We could add a warn_on there or in arch code, but feels like overkill.
Xu Kuohai May 18, 2022, 3:35 a.m. UTC | #2 
On 5/17/2022 11:53 PM, Alexei Starovoitov wrote:
> On Tue, May 17, 2022 at 03:18:35AM -0400, Xu Kuohai wrote:
>>  
>> +static bool is_valid_bpf_tramp_flags(unsigned int flags)
>> +{
>> +	if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
>> +	    (flags & BPF_TRAMP_F_SKIP_FRAME))
>> +		return false;
>> +
>> +	/* BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
>> +	 * and it must be used alone.
>> +	 */
>> +	if ((flags & BPF_TRAMP_F_RET_FENTRY_RET) &&
>> +	    (flags & ~BPF_TRAMP_F_RET_FENTRY_RET))
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +int bpf_prepare_trampoline(struct bpf_tramp_image *tr, void *image,
>> +			   void *image_end, const struct btf_func_model *m,
>> +			   u32 flags, struct bpf_tramp_links *tlinks,
>> +			   void *orig_call)
>> +{
>> +	if (!is_valid_bpf_tramp_flags(flags))
>> +		return -EINVAL;
>> +
>> +	return arch_prepare_bpf_trampoline(tr, image, image_end, m, flags,
>> +					   tlinks, orig_call);
>> +}
> 
> It's an overkill to introduce a new helper function just to validate
> flags that almost compile time constants.
> The flags are not user supplied.
> Please move /* BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops ... */
> comment to bpf_struct_ops.c right before it calls arch_prepare_bpf_trampoline()
> And add a comment to trampoline.c saying that BPF_TRAMP_F_RESTORE_REGS
> and BPF_TRAMP_F_SKIP_FRAME should not be set together.
> We could add a warn_on there or in arch code, but feels like overkill.
> .

OK, will fix in next version.
diff mbox series

Patch

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index a2b6d197c226..7698ef3b4821 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1922,23 +1922,6 @@  static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
 	return 0;
 }
 
-static bool is_valid_bpf_tramp_flags(unsigned int flags)
-{
-	if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
-	    (flags & BPF_TRAMP_F_SKIP_FRAME))
-		return false;
-
-	/*
-	 * BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
-	 * and it must be used alone.
-	 */
-	if ((flags & BPF_TRAMP_F_RET_FENTRY_RET) &&
-	    (flags & ~BPF_TRAMP_F_RET_FENTRY_RET))
-		return false;
-
-	return true;
-}
-
 /* Example:
  * __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
  * its 'struct btf_func_model' will be nr_args=2
@@ -2017,9 +2000,6 @@  int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	if (nr_args > 6)
 		return -ENOTSUPP;
 
-	if (!is_valid_bpf_tramp_flags(flags))
-		return -EINVAL;
-
 	/* Generated trampoline stack layout:
 	 *
 	 * RBP + 8         [ return address  ]
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c107392b0ba7..5615f31b2a30 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -760,6 +760,12 @@  int arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *i
 				const struct btf_func_model *m, u32 flags,
 				struct bpf_tramp_links *tlinks,
 				void *orig_call);
+int bpf_prepare_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end,
+			   const struct btf_func_model *m, u32 flags,
+			   struct bpf_tramp_links *tlinks,
+			   void *orig_call);
+
+
 /* these two functions are called from generated trampoline */
 u64 notrace __bpf_prog_enter(struct bpf_prog *prog, struct bpf_tramp_run_ctx *run_ctx);
 void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start, struct bpf_tramp_run_ctx *run_ctx);
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index d9a3c9207240..4d81675e0dd5 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -342,8 +342,8 @@  int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
 	tlinks[BPF_TRAMP_FENTRY].links[0] = link;
 	tlinks[BPF_TRAMP_FENTRY].nr_links = 1;
 	flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0;
-	return arch_prepare_bpf_trampoline(NULL, image, image_end,
-					   model, flags, tlinks, NULL);
+	return bpf_prepare_trampoline(NULL, image, image_end, model, flags,
+				      tlinks, NULL);
 }
 
 static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 93c7675f0c9e..bef786fe307d 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -363,9 +363,9 @@  static int bpf_trampoline_update(struct bpf_trampoline *tr)
 	if (ip_arg)
 		flags |= BPF_TRAMP_F_IP_ARG;
 
-	err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE,
-					  &tr->func.model, flags, tlinks,
-					  tr->func.addr);
+	err = bpf_prepare_trampoline(im, im->image, im->image + PAGE_SIZE,
+				     &tr->func.model, flags, tlinks,
+				     tr->func.addr);
 	if (err < 0)
 		goto out;
 
@@ -671,6 +671,34 @@  arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image
 	return -ENOTSUPP;
 }
 
+static bool is_valid_bpf_tramp_flags(unsigned int flags)
+{
+	if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
+	    (flags & BPF_TRAMP_F_SKIP_FRAME))
+		return false;
+
+	/* BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
+	 * and it must be used alone.
+	 */
+	if ((flags & BPF_TRAMP_F_RET_FENTRY_RET) &&
+	    (flags & ~BPF_TRAMP_F_RET_FENTRY_RET))
+		return false;
+
+	return true;
+}
+
+int bpf_prepare_trampoline(struct bpf_tramp_image *tr, void *image,
+			   void *image_end, const struct btf_func_model *m,
+			   u32 flags, struct bpf_tramp_links *tlinks,
+			   void *orig_call)
+{
+	if (!is_valid_bpf_tramp_flags(flags))
+		return -EINVAL;
+
+	return arch_prepare_bpf_trampoline(tr, image, image_end, m, flags,
+					   tlinks, orig_call);
+}
+
 static int __init init_trampolines(void)
 {
 	int i;