diff mbox

[arm64-next] net: bpf: arm64: fix module memory leak when JIT image build fails

Message ID 1410428208-2446-1-git-send-email-dborkman@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Borkmann Sept. 11, 2014, 9:36 a.m. UTC
On ARM64, when the BPF JIT compiler fills the JIT image body with
opcodes during translation of eBPF into ARM64 opcodes, we may fail
for several reasons during that phase: one being that we jump to
the notyet label for not yet supported eBPF instructions such as
BPF_ST. In that case we only free offsets, but not the actual
allocated target image where opcodes are being stored. Fix it by
calling module_free() on dismantle time in case of errors.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Cc: Zi Shen Lim <zlim.lnx@gmail.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 [ Compile-tested only. ]

 arch/arm64/net/bpf_jit_comp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Will Deacon Sept. 11, 2014, 10:45 a.m. UTC | #1
On Thu, Sep 11, 2014 at 10:36:48AM +0100, Daniel Borkmann wrote:
> On ARM64, when the BPF JIT compiler fills the JIT image body with
> opcodes during translation of eBPF into ARM64 opcodes, we may fail
> for several reasons during that phase: one being that we jump to
> the notyet label for not yet supported eBPF instructions such as
> BPF_ST. In that case we only free offsets, but not the actual
> allocated target image where opcodes are being stored. Fix it by
> calling module_free() on dismantle time in case of errors.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Cc: Zi Shen Lim <zlim.lnx@gmail.com>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  [ Compile-tested only. ]
> 
>  arch/arm64/net/bpf_jit_comp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 38c4296..7ae3354 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -651,8 +651,10 @@ void bpf_int_jit_compile(struct bpf_prog *prog)
>  	build_prologue(&ctx);
>  
>  	ctx.body_offset = ctx.idx;
> -	if (build_body(&ctx))
> +	if (build_body(&ctx)) {
> +		module_free(NULL, ctx.image);
>  		goto out;
> +	}
>  

Looks good to me:

  Acked-by: Will Deacon <will.deacon@arm.com>

Catalin, can you apply this on the for-next/core branch, please?

Will
Zi Shen Lim Sept. 11, 2014, 2:59 p.m. UTC | #2
On Thu, Sep 11, 2014 at 3:45 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Sep 11, 2014 at 10:36:48AM +0100, Daniel Borkmann wrote:
>> On ARM64, when the BPF JIT compiler fills the JIT image body with
>> opcodes during translation of eBPF into ARM64 opcodes, we may fail
>> for several reasons during that phase: one being that we jump to
>> the notyet label for not yet supported eBPF instructions such as
>> BPF_ST. In that case we only free offsets, but not the actual
>> allocated target image where opcodes are being stored. Fix it by
>> calling module_free() on dismantle time in case of errors.
>>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> Cc: Zi Shen Lim <zlim.lnx@gmail.com>
>> Cc: Alexei Starovoitov <ast@plumgrid.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> ---
>>  [ Compile-tested only. ]
>>
>>  arch/arm64/net/bpf_jit_comp.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
>> index 38c4296..7ae3354 100644
>> --- a/arch/arm64/net/bpf_jit_comp.c
>> +++ b/arch/arm64/net/bpf_jit_comp.c
>> @@ -651,8 +651,10 @@ void bpf_int_jit_compile(struct bpf_prog *prog)
>>       build_prologue(&ctx);
>>
>>       ctx.body_offset = ctx.idx;
>> -     if (build_body(&ctx))
>> +     if (build_body(&ctx)) {
>> +             module_free(NULL, ctx.image);
>>               goto out;
>> +     }
>>
>
> Looks good to me:
>
>   Acked-by: Will Deacon <will.deacon@arm.com>

Same here:

    Acked-by: Zi Shen Lim <zlim.lnx@gmail.com>

Thank you Daniel.

>
> Catalin, can you apply this on the for-next/core branch, please?
>
> Will
Catalin Marinas Sept. 12, 2014, 3:33 p.m. UTC | #3
On Thu, Sep 11, 2014 at 11:45:13AM +0100, Will Deacon wrote:
> On Thu, Sep 11, 2014 at 10:36:48AM +0100, Daniel Borkmann wrote:
> > On ARM64, when the BPF JIT compiler fills the JIT image body with
> > opcodes during translation of eBPF into ARM64 opcodes, we may fail
> > for several reasons during that phase: one being that we jump to
> > the notyet label for not yet supported eBPF instructions such as
> > BPF_ST. In that case we only free offsets, but not the actual
> > allocated target image where opcodes are being stored. Fix it by
> > calling module_free() on dismantle time in case of errors.
> > 
> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> > Cc: Zi Shen Lim <zlim.lnx@gmail.com>
> > Cc: Alexei Starovoitov <ast@plumgrid.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > ---
> >  [ Compile-tested only. ]
> > 
> >  arch/arm64/net/bpf_jit_comp.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > index 38c4296..7ae3354 100644
> > --- a/arch/arm64/net/bpf_jit_comp.c
> > +++ b/arch/arm64/net/bpf_jit_comp.c
> > @@ -651,8 +651,10 @@ void bpf_int_jit_compile(struct bpf_prog *prog)
> >  	build_prologue(&ctx);
> >  
> >  	ctx.body_offset = ctx.idx;
> > -	if (build_body(&ctx))
> > +	if (build_body(&ctx)) {
> > +		module_free(NULL, ctx.image);
> >  		goto out;
> > +	}
> >  
> 
> Looks good to me:
> 
>   Acked-by: Will Deacon <will.deacon@arm.com>
> 
> Catalin, can you apply this on the for-next/core branch, please?

Applied, thanks.
diff mbox

Patch

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 38c4296..7ae3354 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -651,8 +651,10 @@  void bpf_int_jit_compile(struct bpf_prog *prog)
 	build_prologue(&ctx);
 
 	ctx.body_offset = ctx.idx;
-	if (build_body(&ctx))
+	if (build_body(&ctx)) {
+		module_free(NULL, ctx.image);
 		goto out;
+	}
 
 	build_epilogue(&ctx);