[bpf-next,1/3] bpf: Support inlining bpf_kptr_xchg() helper

Commit Message

Hou Tao Dec. 19, 2023, 1:56 p.m. UTC

From: Hou Tao <[e-mail]>

Considering most 64-bit JIT backends support atomic_xchg() and the
implementation of xchg() is the same as atomic_xchg() on these 64-bits
architectures, inline bpf_kptr_xchg() by converting the calling of
bpf_kptr_xchg() into an atomic_xchg() instruction.

As a precaution, defining a weak function bpf_jit_supports_ptr_xchg()
to state whether such conversion is safe and only inlining under 64-bit
host.

Signed-off-by: Hou Tao <[e-mail]>
---
 include/linux/filter.h |  1 +
 kernel/bpf/core.c      | 10 ++++++++++
 kernel/bpf/verifier.c  | 17 +++++++++++++++++
 3 files changed, 28 insertions(+)

Comments

Daniel Borkmann Dec. 20, 2023, 3:02 p.m. UTC | #1

On 12/19/23 2:56 PM, Hou Tao wrote:
> From: Hou Tao <[e-mail]>
> 
> Considering most 64-bit JIT backends support atomic_xchg() and the
> implementation of xchg() is the same as atomic_xchg() on these 64-bits
> architectures, inline bpf_kptr_xchg() by converting the calling of
> bpf_kptr_xchg() into an atomic_xchg() instruction.
> 
> As a precaution, defining a weak function bpf_jit_supports_ptr_xchg()
> to state whether such conversion is safe and only inlining under 64-bit
> host.
> 
> Signed-off-by: Hou Tao <[e-mail]>

You can probably send patch 2/3 as stand-alone, and squash patch 3/3 with
this one in here.

> ---
>   include/linux/filter.h |  1 +
>   kernel/bpf/core.c      | 10 ++++++++++
>   kernel/bpf/verifier.c  | 17 +++++++++++++++++
>   3 files changed, 28 insertions(+)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 12d907f17d36..fee070b9826e 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -955,6 +955,7 @@ bool bpf_jit_supports_subprog_tailcalls(void);
>   bool bpf_jit_supports_kfunc_call(void);
>   bool bpf_jit_supports_far_kfunc_call(void);
>   bool bpf_jit_supports_exceptions(void);
> +bool bpf_jit_supports_ptr_xchg(void);
>   void arch_bpf_stack_walk(bool (*consume_fn)(void *cookie, u64 ip, u64 sp, u64 bp), void *cookie);
>   bool bpf_helper_changes_pkt_data(void *func);
>   
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 5aa6863ac33b..b64885827f90 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2922,6 +2922,16 @@ bool __weak bpf_jit_supports_far_kfunc_call(void)
>   	return false;
>   }
>   
> +/* Return TRUE if the JIT backend satisfies the following two conditions:
> + * 1) JIT backend supports atomic_xchg() on pointer-sized words.
> + * 2) Under the specific arch, the implementation of xchg() is the same
> + *    as atomic_xchg() on pointer-sized words.
> + */
> +bool __weak bpf_jit_supports_ptr_xchg(void)
> +{
> +	return false;
> +}
> +
>   /* To execute LD_ABS/LD_IND instructions __bpf_prog_run() may call
>    * skb_copy_bits(), so provide a weak definition of it for NET-less config.
>    */
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 9456ee0ad129..7814c4f7576e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19668,6 +19668,23 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
>   			continue;
>   		}
>   
> +		/* Implement bpf_kptr_xchg inline */
> +		if (prog->jit_requested && BITS_PER_LONG == 64 &&
> +		    insn->imm == BPF_FUNC_kptr_xchg &&
> +		    bpf_jit_supports_ptr_xchg()) {
> +			insn_buf[0] = BPF_MOV64_REG(BPF_REG_0, BPF_REG_2);
> +			insn_buf[1] = BPF_ATOMIC_OP(BPF_DW, BPF_XCHG, BPF_REG_1, BPF_REG_0, 0);
> +			cnt = 2;
> +
> +			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> +			if (!new_prog)
> +				return -ENOMEM;
> +
> +			delta    += cnt - 1;
> +			env->prog = prog = new_prog;
> +			insn      = new_prog->insnsi + i + delta;
> +			continue;
> +		}
>   patch_call_imm:
>   		fn = env->ops->get_func_proto(insn->imm, env->prog);
>   		/* all functions that have prototype and verifier allowed
>

Eduard Zingerman Dec. 20, 2023, 5:07 p.m. UTC | #2

On Tue, 2023-12-19 at 21:56 +0800, Hou Tao wrote:
> From: Hou Tao <[e-mail]>
[...]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 9456ee0ad129..7814c4f7576e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19668,6 +19668,23 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
>  			continue;
>  		}
>  
> +		/* Implement bpf_kptr_xchg inline */
> +		if (prog->jit_requested && BITS_PER_LONG == 64 &&
> +		    insn->imm == BPF_FUNC_kptr_xchg &&
> +		    bpf_jit_supports_ptr_xchg()) {
> +			insn_buf[0] = BPF_MOV64_REG(BPF_REG_0, BPF_REG_2);
> +			insn_buf[1] = BPF_ATOMIC_OP(BPF_DW, BPF_XCHG, BPF_REG_1, BPF_REG_0, 0);
> +			cnt = 2;
> +
> +			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> +			if (!new_prog)
> +				return -ENOMEM;
> +
> +			delta    += cnt - 1;
> +			env->prog = prog = new_prog;
> +			insn      = new_prog->insnsi + i + delta;
> +			continue;
> +		}
>  patch_call_imm:
>  		fn = env->ops->get_func_proto(insn->imm, env->prog);
>  		/* all functions that have prototype and verifier allowed

Hi Hou,

I have a suggestion about testing this rewrite.
It is possible to use function get_xlated_program() from
tools/testing/selftests/bpf/prog_tests/ctx_rewrite.c,
to obtain a BPF disassembly for the program after
do_misc_fixups() are applied.

So, it shouldn't be difficult to:
- prepare a dummy program in progs/ that uses bpf_kptr_xchg();
- prepare a new test_* function in prog_tests/ that:
  - loads that dummy program;
  - queries it's disassembly using get_xlated_program();
  - compares it with expected template.

I know that do_misc_fixups() are usually not tested this way,
but that does not mean they shouldn't, wdyt?

Thanks,
Eduard

Hou Tao Dec. 21, 2023, 11:40 a.m. UTC | #3

Hi Eduard,

On 12/21/2023 1:07 AM, Eduard Zingerman wrote:
> On Tue, 2023-12-19 at 21:56 +0800, Hou Tao wrote:
>> From: Hou Tao <[e-mail]>
> [...]
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 9456ee0ad129..7814c4f7576e 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -19668,6 +19668,23 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
>>  			continue;
>>  		}
>>  
>> +		/* Implement bpf_kptr_xchg inline */
>> +		if (prog->jit_requested && BITS_PER_LONG == 64 &&
>> +		    insn->imm == BPF_FUNC_kptr_xchg &&
>> +		    bpf_jit_supports_ptr_xchg()) {
>> +			insn_buf[0] = BPF_MOV64_REG(BPF_REG_0, BPF_REG_2);
>> +			insn_buf[1] = BPF_ATOMIC_OP(BPF_DW, BPF_XCHG, BPF_REG_1, BPF_REG_0, 0);
>> +			cnt = 2;
>> +
>> +			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
>> +			if (!new_prog)
>> +				return -ENOMEM;
>> +
>> +			delta    += cnt - 1;
>> +			env->prog = prog = new_prog;
>> +			insn      = new_prog->insnsi + i + delta;
>> +			continue;
>> +		}
>>  patch_call_imm:
>>  		fn = env->ops->get_func_proto(insn->imm, env->prog);
>>  		/* all functions that have prototype and verifier allowed
> Hi Hou,
>
> I have a suggestion about testing this rewrite.
> It is possible to use function get_xlated_program() from
> tools/testing/selftests/bpf/prog_tests/ctx_rewrite.c,
> to obtain a BPF disassembly for the program after
> do_misc_fixups() are applied.
>
> So, it shouldn't be difficult to:
> - prepare a dummy program in progs/ that uses bpf_kptr_xchg();
> - prepare a new test_* function in prog_tests/ that:
>   - loads that dummy program;
>   - queries it's disassembly using get_xlated_program();
>   - compares it with expected template.
>
> I know that do_misc_fixups() are usually not tested this way,
> but that does not mean they shouldn't, wdyt?

Good idea and thanks for the detailed guidance. Will try it in v2.
>
> Thanks,
> Eduard
> .

Patch

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 12d907f17d36..fee070b9826e 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -955,6 +955,7 @@  bool bpf_jit_supports_subprog_tailcalls(void);
 bool bpf_jit_supports_kfunc_call(void);
 bool bpf_jit_supports_far_kfunc_call(void);
 bool bpf_jit_supports_exceptions(void);
+bool bpf_jit_supports_ptr_xchg(void);
 void arch_bpf_stack_walk(bool (*consume_fn)(void *cookie, u64 ip, u64 sp, u64 bp), void *cookie);
 bool bpf_helper_changes_pkt_data(void *func);
 
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 5aa6863ac33b..b64885827f90 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2922,6 +2922,16 @@  bool __weak bpf_jit_supports_far_kfunc_call(void)
 	return false;
 }
 
+/* Return TRUE if the JIT backend satisfies the following two conditions:
+ * 1) JIT backend supports atomic_xchg() on pointer-sized words.
+ * 2) Under the specific arch, the implementation of xchg() is the same
+ *    as atomic_xchg() on pointer-sized words.
+ */
+bool __weak bpf_jit_supports_ptr_xchg(void)
+{
+	return false;
+}
+
 /* To execute LD_ABS/LD_IND instructions __bpf_prog_run() may call
  * skb_copy_bits(), so provide a weak definition of it for NET-less config.
  */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9456ee0ad129..7814c4f7576e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19668,6 +19668,23 @@  static int do_misc_fixups(struct bpf_verifier_env *env)
 			continue;
 		}
 
+		/* Implement bpf_kptr_xchg inline */
+		if (prog->jit_requested && BITS_PER_LONG == 64 &&
+		    insn->imm == BPF_FUNC_kptr_xchg &&
+		    bpf_jit_supports_ptr_xchg()) {
+			insn_buf[0] = BPF_MOV64_REG(BPF_REG_0, BPF_REG_2);
+			insn_buf[1] = BPF_ATOMIC_OP(BPF_DW, BPF_XCHG, BPF_REG_1, BPF_REG_0, 0);
+			cnt = 2;
+
+			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
+			if (!new_prog)
+				return -ENOMEM;
+
+			delta    += cnt - 1;
+			env->prog = prog = new_prog;
+			insn      = new_prog->insnsi + i + delta;
+			continue;
+		}
 patch_call_imm:
 		fn = env->ops->get_func_proto(insn->imm, env->prog);
 		/* all functions that have prototype and verifier allowed