From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexei Starovoitov Subject: Re: [PATCH bpf-next 12/13] bpf: arm64: add JIT support for multi-function programs Date: Mon, 18 Dec 2017 09:55:28 -0800 Message-ID: <78484175-eba0-0847-5f2f-44d1c06b1951@fb.com> References: <20171215015517.409513-1-ast@kernel.org> <20171215015517.409513-13-ast@kernel.org> <90da62d9-830e-4ea7-b542-ac7a2eb0a811@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: "David S . Miller" , John Fastabend , Edward Cree , Jakub Kicinski , Networking , To: Daniel Borkmann , Arnd Bergmann , Alexei Starovoitov Return-path: Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:33876 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934378AbdLRR4A (ORCPT ); Mon, 18 Dec 2017 12:56:00 -0500 In-Reply-To: <90da62d9-830e-4ea7-b542-ac7a2eb0a811@iogearbox.net> Sender: netdev-owner@vger.kernel.org List-ID: On 12/18/17 7:51 AM, Daniel Borkmann wrote: > On 12/18/2017 04:29 PM, Arnd Bergmann wrote: >> On Fri, Dec 15, 2017 at 2:55 AM, Alexei Starovoitov wrote: >> >> >>> + if (jit_data->ctx.offset) { >>> + ctx = jit_data->ctx; >>> + image_ptr = jit_data->image; >>> + header = jit_data->header; >>> + extra_pass = true; >>> + goto skip_init_ctx; >>> + } >>> memset(&ctx, 0, sizeof(ctx)); >>> ctx.prog = prog; >> >> The 'goto' jumps over the 'image_size' initialization >> >>> prog->bpf_func = (void *)ctx.image; >>> prog->jited = 1; >>> prog->jited_len = image_size; >> >> so we now get a warning here, starting with linux-next-20171218: >> >> 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] >> >> I could not figure out what the code should be doing instead, or if it is >> indeed safe and the warning is a false-positive. > > Good catch, it's buggy indeed. Fix like below is needed; I can submit > it properly a bit later today: good catch. My arm gcc 4.8.5 didn't warn about it. the following would be better fix: diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c index 396490cf7316..acaa935ed977 100644 --- a/arch/arm64/net/bpf_jit_comp.c +++ b/arch/arm64/net/bpf_jit_comp.c @@ -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)); > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c > index 396490c..a6fd585 100644 > --- a/arch/arm64/net/bpf_jit_comp.c > +++ b/arch/arm64/net/bpf_jit_comp.c > @@ -855,6 +855,7 @@ static inline void bpf_flush_icache(void *start, void *end) > struct arm64_jit_data { > struct bpf_binary_header *header; > u8 *image; > + int image_size; > struct jit_ctx ctx; > }; > > @@ -895,6 +896,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) > if (jit_data->ctx.offset) { > ctx = jit_data->ctx; > image_ptr = jit_data->image; > + image_size = jit_data->image_size; > header = jit_data->header; > extra_pass = true; > goto skip_init_ctx; > @@ -975,6 +977,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) > } else { > jit_data->ctx = ctx; > jit_data->image = image_ptr; > + jit_data->image_size = image_size; > jit_data->header = header; > } > prog->bpf_func = (void *)ctx.image; >