[PATCH 0/2] More strict error checking in bpf_asm.

[PATCH 0/2] More strict error checking in bpf_asm.

[Ian Denhardt]

Hi,

Enclosed are two patches related to my earlier message, which make the
error checking in the bpf_asm tool more strict, the first by upgrading a
warning to an error, the second by using non-zero exit codes when
aborting.

These could be conceptually separated, but it seemed sensible to submit
them together.

-Ian

Ian Denhardt (2):
  tools, bpf_asm: Hard error on out of range jumps.
  tools, bpf_asm: exit non-zero on errors.

 tools/bpf/bpf_exp.y | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

--
2.30.1

[PATCH 1/2] tools, bpf_asm: Hard error on out of range jumps.

[Ian Denhardt]

Per discussion at:

https://lore.kernel.org/bpf/c964892195a6b91d20a67691448567ef528ffa6d.camel@linux.ibm.com/T/#t

...this was originally introduced as a warning due to concerns about
breaking existing code, but a hard error probably makes more sense,
especially given that concerns about breakage were only speculation.
---
 tools/bpf/bpf_exp.y | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpf_exp.y b/tools/bpf/bpf_exp.y
index 8d48e896be50..8d03e5245da5 100644
--- a/tools/bpf/bpf_exp.y
+++ b/tools/bpf/bpf_exp.y
@@ -549,9 +549,11 @@ static uint8_t bpf_encode_jt_jf_offset(int off, int i)
 {
 	int delta = off - i - 1;
 
-	if (delta < 0 || delta > 255)
-		fprintf(stderr, "warning: insn #%d jumps to insn #%d, "
+	if (delta < 0 || delta > 255) {
+		fprintf(stderr, "error: insn #%d jumps to insn #%d, "
 				"which is out of range\n", i, off);
+		exit(1);
+	}
 	return (uint8_t) delta;
 }
 
-- 
2.30.1

Re: [PATCH 1/2] tools, bpf_asm: Hard error on out of range jumps.

[Ilya Leoshkevich]

On Tue, 2021-02-23 at 21:15 -0500, Ian Denhardt wrote:
> Per discussion at:
> 
> https://lore.kernel.org/bpf/c964892195a6b91d20a67691448567ef528ffa6d.camel@linux.ibm.com/T/#t
> 
> ...this was originally introduced as a warning due to concerns about
> breaking existing code, but a hard error probably makes more sense,
> especially given that concerns about breakage were only speculation.
> ---
>  tools/bpf/bpf_exp.y | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/bpf/bpf_exp.y b/tools/bpf/bpf_exp.y
> index 8d48e896be50..8d03e5245da5 100644
> --- a/tools/bpf/bpf_exp.y
> +++ b/tools/bpf/bpf_exp.y
> @@ -549,9 +549,11 @@ static uint8_t bpf_encode_jt_jf_offset(int off,
> int i)
>  {
>         int delta = off - i - 1;
>  
> -       if (delta < 0 || delta > 255)
> -               fprintf(stderr, "warning: insn #%d jumps to insn #%d, "
> +       if (delta < 0 || delta > 255) {
> +               fprintf(stderr, "error: insn #%d jumps to insn #%d, "
>                                 "which is out of range\n", i, off);
> +               exit(1);
> +       }
>         return (uint8_t) delta;
>  }
>  

Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>

[PATCH 2/2] tools, bpf_asm: exit non-zero on errors.

[Ian Denhardt]

...so callers can correctly detect failure.
---
 tools/bpf/bpf_exp.y | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/bpf/bpf_exp.y b/tools/bpf/bpf_exp.y
index 8d03e5245da5..dfb7254a24e8 100644
--- a/tools/bpf/bpf_exp.y
+++ b/tools/bpf/bpf_exp.y
@@ -185,13 +185,13 @@ ldx
 	| OP_LDXB number '*' '(' '[' number ']' '&' number ')' {
 		if ($2 != 4 || $9 != 0xf) {
 			fprintf(stderr, "ldxb offset not supported!\n");
-			exit(0);
+			exit(1);
 		} else {
 			bpf_set_curr_instr(BPF_LDX | BPF_MSH | BPF_B, 0, 0, $6); } }
 	| OP_LDX number '*' '(' '[' number ']' '&' number ')' {
 		if ($2 != 4 || $9 != 0xf) {
 			fprintf(stderr, "ldxb offset not supported!\n");
-			exit(0);
+			exit(1);
 		} else {
 			bpf_set_curr_instr(BPF_LDX | BPF_MSH | BPF_B, 0, 0, $6); } }
 	;
@@ -472,7 +472,7 @@ static void bpf_assert_max(void)
 {
 	if (curr_instr >= BPF_MAXINSNS) {
 		fprintf(stderr, "only max %u insns allowed!\n", BPF_MAXINSNS);
-		exit(0);
+		exit(1);
 	}
 }
 
@@ -522,7 +522,7 @@ static int bpf_find_insns_offset(const char *label)
 
 	if (ret == -ENOENT) {
 		fprintf(stderr, "no such label \'%s\'!\n", label);
-		exit(0);
+		exit(1);
 	}
 
 	return ret;
-- 
2.30.1

Re: [PATCH 0/2] More strict error checking in bpf_asm.

[Daniel Borkmann]

Hi Ian,

On 2/24/21 3:36 AM, Ian Denhardt wrote:
> Hi,
> 
> Enclosed are two patches related to my earlier message, which make the
> error checking in the bpf_asm tool more strict, the first by upgrading a
> warning to an error, the second by using non-zero exit codes when
> aborting.
> 
> These could be conceptually separated, but it seemed sensible to submit
> them together.
> 
> -Ian
> 
> Ian Denhardt (2):
>    tools, bpf_asm: Hard error on out of range jumps.
>    tools, bpf_asm: exit non-zero on errors.

Both of the patches need to have your Signed-off-by [0] in order to be able
to apply them, for example see [1]. Please resubmit with them & feel free to
carry Ilya's ACK forward for the v2. Thanks!

   [0] https://www.kernel.org/doc/html/latest/process/submitting-patches.html
   [1] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=557c223b643a35effec9654958d8edc62fd2603a

>   tools/bpf/bpf_exp.y | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> --
> 2.30.1
>