diff mbox series

[v4,bpf-next,1/9] bpf: Move insn_buf[16] to bpf_verifier_env

Message ID 20240827194834.1423815-2-martin.lau@linux.dev (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf: Add gen_epilogue to bpf_verifier_ops | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test
bpf/vmtest-bpf-next-VM_Test-9 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-10 success Logs for s390x-gcc / test
bpf/vmtest-bpf-next-VM_Test-8 fail Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-4 fail Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-14 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-13 fail Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for x86_64-gcc / test
bpf/vmtest-bpf-next-VM_Test-16 success Logs for x86_64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-17 fail Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-18 fail Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-llvm-17 / test
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-23 fail Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-24 fail Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-25 fail Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-26 fail Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
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: 16 this patch: 16
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 6 maintainers not CCed: song@kernel.org sdf@fomichev.me haoluo@google.com jolsa@kernel.org kpsingh@kernel.org john.fastabend@gmail.com
netdev/build_clang success Errors and warnings before: 16 this patch: 16
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: 78 this patch: 78
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 72 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Martin KaFai Lau Aug. 27, 2024, 7:48 p.m. UTC 
From: Martin KaFai Lau <martin.lau@kernel.org>

This patch moves the 'struct bpf_insn insn_buf[16]' stack usage
to the bpf_verifier_env. A '#define INSN_BUF_SIZE 16' is also added
to replace the ARRAY_SIZE(insn_buf) usages.

Both convert_ctx_accesses() and do_misc_fixup() are changed
to use the env->insn_buf.

It is a prep work for adding the epilogue_buf[16] in a later patch.

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 include/linux/bpf_verifier.h |  3 +++
 kernel/bpf/verifier.c        | 15 ++++++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

Comments

Eduard Zingerman Aug. 29, 2024, 12:41 a.m. UTC | #1 
On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> This patch moves the 'struct bpf_insn insn_buf[16]' stack usage
> to the bpf_verifier_env. A '#define INSN_BUF_SIZE 16' is also added
> to replace the ARRAY_SIZE(insn_buf) usages.
> 
> Both convert_ctx_accesses() and do_misc_fixup() are changed
> to use the env->insn_buf.
> 
> It is a prep work for adding the epilogue_buf[16] in a later patch.
> 
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---

Not sure if this refactoring is worth it but code looks correct.
Note that there is also inline_bpf_loop()
(it needs a slightly bigger buffer).

Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>

[...]
Alexei Starovoitov Aug. 29, 2024, 1:46 a.m. UTC | #2 
On Wed, Aug 28, 2024 at 5:41 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
> > From: Martin KaFai Lau <martin.lau@kernel.org>
> >
> > This patch moves the 'struct bpf_insn insn_buf[16]' stack usage
> > to the bpf_verifier_env. A '#define INSN_BUF_SIZE 16' is also added
> > to replace the ARRAY_SIZE(insn_buf) usages.
> >
> > Both convert_ctx_accesses() and do_misc_fixup() are changed
> > to use the env->insn_buf.
> >
> > It is a prep work for adding the epilogue_buf[16] in a later patch.
> >
> > Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> > ---
>
> Not sure if this refactoring is worth it but code looks correct.
> Note that there is also inline_bpf_loop()
> (it needs a slightly bigger buffer).

Probably worth it in the follow up, since people complain that
this or that function in verifier.c reaches stack size limit
when compiled with sanitizers.
These buffers on stack are the biggest consumers.
Martin KaFai Lau Aug. 29, 2024, 3:20 p.m. UTC | #3 
On 8/28/24 6:46 PM, Alexei Starovoitov wrote:
> On Wed, Aug 28, 2024 at 5:41 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>>
>> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
>>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>>
>>> This patch moves the 'struct bpf_insn insn_buf[16]' stack usage
>>> to the bpf_verifier_env. A '#define INSN_BUF_SIZE 16' is also added
>>> to replace the ARRAY_SIZE(insn_buf) usages.
>>>
>>> Both convert_ctx_accesses() and do_misc_fixup() are changed
>>> to use the env->insn_buf.
>>>
>>> It is a prep work for adding the epilogue_buf[16] in a later patch.
>>>
>>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>>> ---
>>
>> Not sure if this refactoring is worth it but code looks correct.
>> Note that there is also inline_bpf_loop()
>> (it needs a slightly bigger buffer).
> 
> Probably worth it in the follow up, since people complain that
> this or that function in verifier.c reaches stack size limit
> when compiled with sanitizers.
> These buffers on stack are the biggest consumers.

ok. I will drop this patch for now. Redo it again as a followup and will 
consider inline_bpf_loop() together at that time.

Regarding the stack size, I did notice the compilation warning difference on the 
stack size which I should have put in the commit message.

Before:
./kernel/bpf/verifier.c:22133:5: warning: stack frame size (2584) exceeds limit 
(2048) in 'bpf_check' [-Wframe-larger-than]

After:
./kernel/bpf/verifier.c:22184:5: warning: stack frame size (2264) exceeds limit 
(2048) in 'bpf_check' [-Wframe-larger-than]
Alexei Starovoitov Aug. 29, 2024, 3:26 p.m. UTC | #4 
On Thu, Aug 29, 2024 at 8:20 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 8/28/24 6:46 PM, Alexei Starovoitov wrote:
> > On Wed, Aug 28, 2024 at 5:41 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> >>
> >> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
> >>> From: Martin KaFai Lau <martin.lau@kernel.org>
> >>>
> >>> This patch moves the 'struct bpf_insn insn_buf[16]' stack usage
> >>> to the bpf_verifier_env. A '#define INSN_BUF_SIZE 16' is also added
> >>> to replace the ARRAY_SIZE(insn_buf) usages.
> >>>
> >>> Both convert_ctx_accesses() and do_misc_fixup() are changed
> >>> to use the env->insn_buf.
> >>>
> >>> It is a prep work for adding the epilogue_buf[16] in a later patch.
> >>>
> >>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> >>> ---
> >>
> >> Not sure if this refactoring is worth it but code looks correct.
> >> Note that there is also inline_bpf_loop()
> >> (it needs a slightly bigger buffer).
> >
> > Probably worth it in the follow up, since people complain that
> > this or that function in verifier.c reaches stack size limit
> > when compiled with sanitizers.
> > These buffers on stack are the biggest consumers.
>
> ok. I will drop this patch for now. Redo it again as a followup and will
> consider inline_bpf_loop() together at that time.

why? Keep it. It's an improvement already.

> Regarding the stack size, I did notice the compilation warning difference on the
> stack size which I should have put in the commit message.
>
> Before:
> ./kernel/bpf/verifier.c:22133:5: warning: stack frame size (2584) exceeds limit
> (2048) in 'bpf_check' [-Wframe-larger-than]
>
> After:
> ./kernel/bpf/verifier.c:22184:5: warning: stack frame size (2264) exceeds limit
> (2048) in 'bpf_check' [-Wframe-larger-than]

Exactly. It's a step forward.
Martin KaFai Lau Aug. 29, 2024, 3:33 p.m. UTC | #5 
On 8/29/24 8:26 AM, Alexei Starovoitov wrote:
> On Thu, Aug 29, 2024 at 8:20 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 8/28/24 6:46 PM, Alexei Starovoitov wrote:
>>> On Wed, Aug 28, 2024 at 5:41 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>>>>
>>>> On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
>>>>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>>>>
>>>>> This patch moves the 'struct bpf_insn insn_buf[16]' stack usage
>>>>> to the bpf_verifier_env. A '#define INSN_BUF_SIZE 16' is also added
>>>>> to replace the ARRAY_SIZE(insn_buf) usages.
>>>>>
>>>>> Both convert_ctx_accesses() and do_misc_fixup() are changed
>>>>> to use the env->insn_buf.
>>>>>
>>>>> It is a prep work for adding the epilogue_buf[16] in a later patch.
>>>>>
>>>>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>>>>> ---
>>>>
>>>> Not sure if this refactoring is worth it but code looks correct.
>>>> Note that there is also inline_bpf_loop()
>>>> (it needs a slightly bigger buffer).
>>>
>>> Probably worth it in the follow up, since people complain that

I read it and Eduard's earlier comment together as the whole patch 1 as a follow 
up. :)

yep. I will keep this one and follow up with the insn_buf in the inline_bpf_loop().

I will update the commit message with the stack usage in the next respin.

>>> this or that function in verifier.c reaches stack size limit
>>> when compiled with sanitizers.
>>> These buffers on stack are the biggest consumers.
>>
>> ok. I will drop this patch for now. Redo it again as a followup and will
>> consider inline_bpf_loop() together at that time.
> 
> why? Keep it. It's an improvement already.
diff mbox series

Patch

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 279b4a640644..0ad2d189c546 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -23,6 +23,8 @@ 
  * (in the "-8,-16,...,-512" form)
  */
 #define TMP_STR_BUF_LEN 320
+/* Patch buffer size */
+#define INSN_BUF_SIZE 16
 
 /* Liveness marks, used for registers and spilled-regs (in stack slots).
  * Read marks propagate upwards until they find a write mark; they record that
@@ -780,6 +782,7 @@  struct bpf_verifier_env {
 	 * e.g., in reg_type_str() to generate reg_type string
 	 */
 	char tmp_str_buf[TMP_STR_BUF_LEN];
+	struct bpf_insn insn_buf[INSN_BUF_SIZE];
 };
 
 static inline struct bpf_func_info_aux *subprog_aux(struct bpf_verifier_env *env, int subprog)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 33270b363689..b408692a12d7 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19678,7 +19678,8 @@  static int convert_ctx_accesses(struct bpf_verifier_env *env)
 	const struct bpf_verifier_ops *ops = env->ops;
 	int i, cnt, size, ctx_field_size, delta = 0;
 	const int insn_cnt = env->prog->len;
-	struct bpf_insn insn_buf[16], *insn;
+	struct bpf_insn *insn_buf = env->insn_buf;
+	struct bpf_insn *insn;
 	u32 target_size, size_default, off;
 	struct bpf_prog *new_prog;
 	enum bpf_access_type type;
@@ -19691,7 +19692,7 @@  static int convert_ctx_accesses(struct bpf_verifier_env *env)
 		}
 		cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
 					env->prog);
-		if (cnt >= ARRAY_SIZE(insn_buf)) {
+		if (cnt >= INSN_BUF_SIZE) {
 			verbose(env, "bpf verifier is misconfigured\n");
 			return -EINVAL;
 		} else if (cnt) {
@@ -19838,7 +19839,7 @@  static int convert_ctx_accesses(struct bpf_verifier_env *env)
 		target_size = 0;
 		cnt = convert_ctx_access(type, insn, insn_buf, env->prog,
 					 &target_size);
-		if (cnt == 0 || cnt >= ARRAY_SIZE(insn_buf) ||
+		if (cnt == 0 || cnt >= INSN_BUF_SIZE ||
 		    (ctx_field_size && !target_size)) {
 			verbose(env, "bpf verifier is misconfigured\n");
 			return -EINVAL;
@@ -19847,7 +19848,7 @@  static int convert_ctx_accesses(struct bpf_verifier_env *env)
 		if (is_narrower_load && size < target_size) {
 			u8 shift = bpf_ctx_narrow_access_offset(
 				off, size, size_default) * 8;
-			if (shift && cnt + 1 >= ARRAY_SIZE(insn_buf)) {
+			if (shift && cnt + 1 >= INSN_BUF_SIZE) {
 				verbose(env, "bpf verifier narrow ctx load misconfigured\n");
 				return -EINVAL;
 			}
@@ -20392,7 +20393,7 @@  static int do_misc_fixups(struct bpf_verifier_env *env)
 	const int insn_cnt = prog->len;
 	const struct bpf_map_ops *ops;
 	struct bpf_insn_aux_data *aux;
-	struct bpf_insn insn_buf[16];
+	struct bpf_insn *insn_buf = env->insn_buf;
 	struct bpf_prog *new_prog;
 	struct bpf_map *map_ptr;
 	int i, ret, cnt, delta = 0, cur_subprog = 0;
@@ -20511,7 +20512,7 @@  static int do_misc_fixups(struct bpf_verifier_env *env)
 		    (BPF_MODE(insn->code) == BPF_ABS ||
 		     BPF_MODE(insn->code) == BPF_IND)) {
 			cnt = env->ops->gen_ld_abs(insn, insn_buf);
-			if (cnt == 0 || cnt >= ARRAY_SIZE(insn_buf)) {
+			if (cnt == 0 || cnt >= INSN_BUF_SIZE) {
 				verbose(env, "bpf verifier is misconfigured\n");
 				return -EINVAL;
 			}
@@ -20804,7 +20805,7 @@  static int do_misc_fixups(struct bpf_verifier_env *env)
 				cnt = ops->map_gen_lookup(map_ptr, insn_buf);
 				if (cnt == -EOPNOTSUPP)
 					goto patch_map_ops_generic;
-				if (cnt <= 0 || cnt >= ARRAY_SIZE(insn_buf)) {
+				if (cnt <= 0 || cnt >= INSN_BUF_SIZE) {
 					verbose(env, "bpf verifier is misconfigured\n");
 					return -EINVAL;
 				}