Skip to contentSkip to conversation

bpf: arm64: fix uninitialized variable

PATCH In review Alexei Starovoitov 3 participants 4 messages Dec 18, 2017 – Dec 19, 2017

Subsystems (5)
Networking Network drivers BPF arm64 +1 more subsystems
lore
Share & tools
lore

Alexei Starovoitov

author maintainer
#0 lore raw

PATCH 1/1 bpf: arm64: fix uninitialized variable

From: Alexei Starovoitov <[email protected]>

fix the following issue:
arch/arm64/net/bpf_jit_comp.c: In function 'bpf_int_jit_compile':
arch/arm64/net/bpf_jit_comp.c:982:18: error: 'image_size' may be used uninitialized in this function [-Werror=maybe-uninitialized]

Fixes db496944fdaa ("bpf: arm64: add JIT support for multi-function programs")
---
 arch/arm64/net/bpf_jit_comp.c | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/net/bpf_jit_comp.c +1 -0
index 396490cf7316..acaa935ed977 100644
@@ -897,6 +897,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 		image_ptr = jit_data->image;
 		header = jit_data->header;
 		extra_pass = true;
+		image_size = sizeof(u32) * ctx.idx;
 		goto skip_init_ctx;
 	}
 	memset(&ctx, 0, sizeof(ctx));
Signature · 1 line
2.9.5
maintainer
#1 lore raw
Quoted 20 lines · Alexei Starovoitov

From: Alexei Starovoitov <[email protected]>

fix the following issue:
arch/arm64/net/bpf_jit_comp.c: In function 'bpf_int_jit_compile':
arch/arm64/net/bpf_jit_comp.c:982:18: error: 'image_size' may be used uninitialized in this function [-Werror=maybe-uninitialized]

Fixes db496944fdaa ("bpf: arm64: add JIT support for multi-function programs")
---
 arch/arm64/net/bpf_jit_comp.c | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/net/bpf_jit_comp.c +0 -0
index 396490cf7316..acaa935ed977 100644

arch/arm64/net/bpf_jit_comp.c struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) · line 903

Quoted 7 lines
arch/arm64/net/bpf_jit_comp.c +1 -0
@@ -897,6 +897,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 		image_ptr = jit_data->image;
 		header = jit_data->header;
 		extra_pass = true;
+		image_size = sizeof(u32) * ctx.idx;
 		goto skip_init_ctx;
 	}
 	memset(&ctx, 0, sizeof(ctx));

I don't really mind, but it feels more complex than it needs to be imho, since in the initial pass you fetch 'image_size' in fake pass from ctx.idx, then we set ctx.idx to 0 again, do another pass and use the cached ctx.idx from that second pass instead of the first one where we set 'image_size' originally, so we definitely need to take that into consideration in future reviews at least.

Thanks,
Daniel

#2 lore raw
Quoted 27 lines · Daniel Borkmann
Quoted 26 lines · Alexei Starovoitov

From: Alexei Starovoitov <[email protected]>

fix the following issue:
arch/arm64/net/bpf_jit_comp.c: In function 'bpf_int_jit_compile':
arch/arm64/net/bpf_jit_comp.c:982:18: error: 'image_size' may be used uninitialized in this function [-Werror=maybe-uninitialized]

Fixes db496944fdaa ("bpf: arm64: add JIT support for multi-function programs")
---
 arch/arm64/net/bpf_jit_comp.c | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/net/bpf_jit_comp.c +1 -0
index 396490cf7316..acaa935ed977 100644
@@ -897,6 +897,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 		image_ptr = jit_data->image;
 		header = jit_data->header;
 		extra_pass = true;
+		image_size = sizeof(u32) * ctx.idx;
 		goto skip_init_ctx;
 	}
 	memset(&ctx, 0, sizeof(ctx));
Quoted 8 lines
Quoted 1 line

I don't really mind, but it feels more complex than it needs to be imho, since in the initial pass you fetch 'image_size' in fake pass from ctx.idx, then we set ctx.idx to 0 again, do another pass and use the cached ctx.idx from that second pass instead of the first one where we set 'image_size' originally, so we definitely need to take that into consideration in future reviews at least.

not sure what you mean.
This check: ctx.idx != jit_data->ctx.idx matters the most. After first alloc the 'image_size' variable used for dumping only. That's why the JITing itself worked fine. We could have removed it since it's computable from idx, but imo it's fine this way.

maintainer
#3 lore raw
Quoted 36 lines · Alexei Starovoitov
Quoted 26 lines · Daniel Borkmann
Quoted 25 lines · Alexei Starovoitov

From: Alexei Starovoitov <[email protected]>

fix the following issue:
arch/arm64/net/bpf_jit_comp.c: In function 'bpf_int_jit_compile':
arch/arm64/net/bpf_jit_comp.c:982:18: error: 'image_size' may be used uninitialized in this function [-Werror=maybe-uninitialized]

Fixes db496944fdaa ("bpf: arm64: add JIT support for multi-function programs")

---
 arch/arm64/net/bpf_jit_comp.c | 1 +

 1 file changed, 1 insertion(+)
arch/arm64/net/bpf_jit_comp.c +1 -0
index 396490cf7316..acaa935ed977 100644
@@ -897,6 +897,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
          image_ptr = jit_data->image;
          header = jit_data->header;
          extra_pass = true;
+        image_size = sizeof(u32) * ctx.idx;
          goto skip_init_ctx;
      }
Quoted 8 lines

arch/arm64/net/bpf_jit_comp.c struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) · line 903

Quoted 1 line
arch/arm64/net/bpf_jit_comp.c +0 -0
      memset(&ctx, 0, sizeof(ctx));

I don't really mind, but it feels more complex than it needs to be imho, since in the initial pass you fetch 'image_size' in fake pass from ctx.idx, then we set ctx.idx to 0 again, do another pass and use the cached ctx.idx from that second pass instead of the first one where we set 'image_size' originally, so we definitely need to take that into consideration in future reviews at least.

Quoted 5 lines

not sure what you mean.
This check: ctx.idx != jit_data->ctx.idx matters the most. After first alloc the 'image_size' variable used for dumping only. That's why the JITing itself worked fine. We could have removed it since it's computable from idx, but imo it's fine this way.

Fair enough, given final ctx.idx value must be guaranteed to never change in future between pass#1 and pass#2 from the first bpf_int_jit_compile() run, then lets go with this smaller version; applied to bpf-next, thanks Alexei!