diff mbox series

[bpf-next,v2] bpf, x64: Check imm32 first at BPF_CALL in do_jit()

Message ID 20230914123527.34624-1-hffilwlqm@gmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next,v2] bpf, x64: Check imm32 first at BPF_CALL in do_jit() | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/cc_maintainers warning 16 maintainers not CCed: bp@alien8.de martin.lau@linux.dev jolsa@kernel.org haoluo@google.com davem@davemloft.net sdf@google.com mingo@redhat.com dsahern@kernel.org yonghong.song@linux.dev netdev@vger.kernel.org x86@kernel.org john.fastabend@gmail.com hpa@zytor.com kpsingh@kernel.org song@kernel.org dave.hansen@linux.intel.com
netdev/build_clang success Errors and warnings before: 1364 this patch: 1364
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1365 this patch: 1365
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 32 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-28 success Logs for veristat
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-0 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-5 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-1 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for x86_64 with llvm-16

Commit Message

Leon Hwang Sept. 14, 2023, 12:35 p.m. UTC 
It's unnecessary to check 'imm32' in both 'if' and 'else'.

It's better to check it first.

Meanwhile, refactor the code for 'offs' calculation.

v1 -> v2:
 * Add '#define RESTORE_TAIL_CALL_CNT_INSN_SIZE 7'.

Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
---
 arch/x86/net/bpf_jit_comp.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)


base-commit: cbb1dbcd99b0ae74c45c4c83c6d213c12c31785c

Comments

Zvi Effron Sept. 15, 2023, 4:29 p.m. UTC | #1 
On Thu, Sep 14, 2023 at 7:36 AM Leon Hwang <hffilwlqm@gmail.com> wrote:
>
> It's unnecessary to check 'imm32' in both 'if' and 'else'.
>
> It's better to check it first.
>
> Meanwhile, refactor the code for 'offs' calculation.
>
> v1 -> v2:
>  * Add '#define RESTORE_TAIL_CALL_CNT_INSN_SIZE 7'.
>
> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
> ---
>  arch/x86/net/bpf_jit_comp.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 2846c21d75bfa..fe0393c7e7b68 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1025,6 +1025,7 @@ static void emit_shiftx(u8 **pprog, u32 dst_reg, u8 src_reg, bool is64, u8 op)
>  /* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
>  #define RESTORE_TAIL_CALL_CNT(stack)                           \
>         EMIT3_off32(0x48, 0x8B, 0x85, -round_up(stack, 8) - 8)
> +#define RESTORE_TAIL_CALL_CNT_INSN_SIZE 7
>
>  static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image,
>                   int oldproglen, struct jit_context *ctx, bool jmp_padding)
> @@ -1629,17 +1630,16 @@ st:                     if (is_imm8(insn->off))
>                 case BPF_JMP | BPF_CALL: {
>                         int offs;
>

<snip>

> +                       if (tail_call_reachable)
>                                 RESTORE_TAIL_CALL_CNT(bpf_prog->aux->stack_depth);

<snip>

> +                       offs = (tail_call_reachable ?
> +                               RESTORE_TAIL_CALL_CNT_INSN_SIZE : 0);

I'm not sure which is preferred style for the kernel, but this seems like it
could be replaced with

int offs = 0;
...
if (tail_call_reachable) {
    RESTORE_TAIL_CALL_CNT(bpf_prog->aux->stack_depth);
    offs = RESTORE_TAIL_CALL_CNT_INSN_SIZE;
}

which is easier for my brain to follow because it reduces the branches (in C,
I assume the compiler is smart enough to optimize). It does create an
unconditional write (of 0) that could otherwise be avoided (unless the compiler
is also smart enough to optimize that).

Also not sure if the performance difference matters here.

> +                       offs += x86_call_depth_emit_accounting(&prog, func);
>                         if (emit_call(&prog, func, image + addrs[i - 1] + offs))
>                                 return -EINVAL;
>                         break;
>
> base-commit: cbb1dbcd99b0ae74c45c4c83c6d213c12c31785c
> --
> 2.41.0
>
>
Leon Hwang Sept. 16, 2023, 5:32 a.m. UTC | #2 
On 2023/9/16 00:29, Zvi Effron wrote:
> On Thu, Sep 14, 2023 at 7:36 AM Leon Hwang <hffilwlqm@gmail.com> wrote:
>>
>> It's unnecessary to check 'imm32' in both 'if' and 'else'.
>>
>> It's better to check it first.
>>
>> Meanwhile, refactor the code for 'offs' calculation.
>>
>> v1 -> v2:
>>  * Add '#define RESTORE_TAIL_CALL_CNT_INSN_SIZE 7'.
>>
>> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
>> ---
>>  arch/x86/net/bpf_jit_comp.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index 2846c21d75bfa..fe0393c7e7b68 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -1025,6 +1025,7 @@ static void emit_shiftx(u8 **pprog, u32 dst_reg, u8 src_reg, bool is64, u8 op)
>>  /* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
>>  #define RESTORE_TAIL_CALL_CNT(stack)                           \
>>         EMIT3_off32(0x48, 0x8B, 0x85, -round_up(stack, 8) - 8)
>> +#define RESTORE_TAIL_CALL_CNT_INSN_SIZE 7
>>
>>  static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image,
>>                   int oldproglen, struct jit_context *ctx, bool jmp_padding)
>> @@ -1629,17 +1630,16 @@ st:                     if (is_imm8(insn->off))
>>                 case BPF_JMP | BPF_CALL: {
>>                         int offs;
>>
> 
> <snip>
> 
>> +                       if (tail_call_reachable)
>>                                 RESTORE_TAIL_CALL_CNT(bpf_prog->aux->stack_depth);
> 
> <snip>
> 
>> +                       offs = (tail_call_reachable ?
>> +                               RESTORE_TAIL_CALL_CNT_INSN_SIZE : 0);
> 
> I'm not sure which is preferred style for the kernel, but this seems like it
> could be replaced with

I'm not sure either.

> 
> int offs = 0;
> ...
> if (tail_call_reachable) {
>     RESTORE_TAIL_CALL_CNT(bpf_prog->aux->stack_depth);
>     offs = RESTORE_TAIL_CALL_CNT_INSN_SIZE;
> }

I considered this way once.

But, I think the code of the patch is more clean.

> 
> which is easier for my brain to follow because it reduces the branches (in C,
> I assume the compiler is smart enough to optimize). It does create an
> unconditional write (of 0) that could otherwise be avoided (unless the compiler
> is also smart enough to optimize that).

I think, as for gcc -O2, there's no diff between these two ways.

Thanks,
Leon

> 
> Also not sure if the performance difference matters here.
> 
>> +                       offs += x86_call_depth_emit_accounting(&prog, func);
>>                         if (emit_call(&prog, func, image + addrs[i - 1] + offs))
>>                                 return -EINVAL;
>>                         break;
>>
>> base-commit: cbb1dbcd99b0ae74c45c4c83c6d213c12c31785c
>> --
>> 2.41.0
>>
>>
Daniel Borkmann Sept. 19, 2023, 10:19 a.m. UTC | #3 
On 9/14/23 2:35 PM, Leon Hwang wrote:
> It's unnecessary to check 'imm32' in both 'if' and 'else'.
> 
> It's better to check it first.
> 
> Meanwhile, refactor the code for 'offs' calculation.
> 
> v1 -> v2:
>   * Add '#define RESTORE_TAIL_CALL_CNT_INSN_SIZE 7'.
> 
> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
> ---
>   arch/x86/net/bpf_jit_comp.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 2846c21d75bfa..fe0393c7e7b68 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1025,6 +1025,7 @@ static void emit_shiftx(u8 **pprog, u32 dst_reg, u8 src_reg, bool is64, u8 op)
>   /* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
>   #define RESTORE_TAIL_CALL_CNT(stack)				\
>   	EMIT3_off32(0x48, 0x8B, 0x85, -round_up(stack, 8) - 8)
> +#define RESTORE_TAIL_CALL_CNT_INSN_SIZE 7
>   
>   static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image,
>   		  int oldproglen, struct jit_context *ctx, bool jmp_padding)
> @@ -1629,17 +1630,16 @@ st:			if (is_imm8(insn->off))
>   		case BPF_JMP | BPF_CALL: {
>   			int offs;
>   
> +			if (!imm32)
> +				return -EINVAL;
> +
>   			func = (u8 *) __bpf_call_base + imm32;
> -			if (tail_call_reachable) {
> +			if (tail_call_reachable)
>   				RESTORE_TAIL_CALL_CNT(bpf_prog->aux->stack_depth);
> -				if (!imm32)
> -					return -EINVAL;
> -				offs = 7 + x86_call_depth_emit_accounting(&prog, func);
> -			} else {
> -				if (!imm32)
> -					return -EINVAL;
> -				offs = x86_call_depth_emit_accounting(&prog, func);
> -			}
> +
> +			offs = (tail_call_reachable ?
> +				RESTORE_TAIL_CALL_CNT_INSN_SIZE : 0);
> +			offs += x86_call_depth_emit_accounting(&prog, func);

I don't think this makes anything significantly better, I would rather prefer to keep
it as is.

>   			if (emit_call(&prog, func, image + addrs[i - 1] + offs))
>   				return -EINVAL;
>   			break;
> 
> base-commit: cbb1dbcd99b0ae74c45c4c83c6d213c12c31785c
>
diff mbox series

Patch

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 2846c21d75bfa..fe0393c7e7b68 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1025,6 +1025,7 @@  static void emit_shiftx(u8 **pprog, u32 dst_reg, u8 src_reg, bool is64, u8 op)
 /* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
 #define RESTORE_TAIL_CALL_CNT(stack)				\
 	EMIT3_off32(0x48, 0x8B, 0x85, -round_up(stack, 8) - 8)
+#define RESTORE_TAIL_CALL_CNT_INSN_SIZE 7
 
 static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image,
 		  int oldproglen, struct jit_context *ctx, bool jmp_padding)
@@ -1629,17 +1630,16 @@  st:			if (is_imm8(insn->off))
 		case BPF_JMP | BPF_CALL: {
 			int offs;
 
+			if (!imm32)
+				return -EINVAL;
+
 			func = (u8 *) __bpf_call_base + imm32;
-			if (tail_call_reachable) {
+			if (tail_call_reachable)
 				RESTORE_TAIL_CALL_CNT(bpf_prog->aux->stack_depth);
-				if (!imm32)
-					return -EINVAL;
-				offs = 7 + x86_call_depth_emit_accounting(&prog, func);
-			} else {
-				if (!imm32)
-					return -EINVAL;
-				offs = x86_call_depth_emit_accounting(&prog, func);
-			}
+
+			offs = (tail_call_reachable ?
+				RESTORE_TAIL_CALL_CNT_INSN_SIZE : 0);
+			offs += x86_call_depth_emit_accounting(&prog, func);
 			if (emit_call(&prog, func, image + addrs[i - 1] + offs))
 				return -EINVAL;
 			break;