* [PATCH bpf-next 0/2] fixes for bpf_prog_pack @ 2022-03-02 0:43 Song Liu 2022-03-02 0:43 ` [PATCH bpf-next 1/2] x86: disable HAVE_ARCH_HUGE_VMALLOC on 32-bit x86 Song Liu 2022-03-02 0:43 ` [PATCH bpf-next 2/2] bpf, x86: set header->size properly before freeing it Song Liu 0 siblings, 2 replies; 6+ messages in thread From: Song Liu @ 2022-03-02 0:43 UTC (permalink / raw) To: bpf, netdev; +Cc: ast, daniel, andrii, kernel-team, Song Liu Two fixes for bpf_prog_pack. Song Liu (2): x86: disable HAVE_ARCH_HUGE_VMALLOC on 32-bit x86 bpf, x86: set header->size properly before freeing it arch/x86/Kconfig | 2 +- arch/x86/net/bpf_jit_comp.c | 6 +++++- kernel/bpf/core.c | 7 ++++--- 3 files changed, 10 insertions(+), 5 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next 1/2] x86: disable HAVE_ARCH_HUGE_VMALLOC on 32-bit x86 2022-03-02 0:43 [PATCH bpf-next 0/2] fixes for bpf_prog_pack Song Liu @ 2022-03-02 0:43 ` Song Liu 2022-03-02 7:08 ` Yonghong Song 2022-03-02 0:43 ` [PATCH bpf-next 2/2] bpf, x86: set header->size properly before freeing it Song Liu 1 sibling, 1 reply; 6+ messages in thread From: Song Liu @ 2022-03-02 0:43 UTC (permalink / raw) To: bpf, netdev; +Cc: ast, daniel, andrii, kernel-team, Song Liu, kernel test robot kernel test robot reported kernel BUG like: [ 44.587744][ T1] kernel BUG at arch/x86/mm/physaddr.c:76! [ 44.590151][ T1] __vmalloc_area_node (mm/vmalloc.c:622 mm/vmalloc.c:2995) [ 44.590151][ T1] __vmalloc_node_range (mm/vmalloc.c:3108) [ 44.590151][ T1] __vmalloc_node (mm/vmalloc.c:3157) which is triggered with HAVE_ARCH_HUGE_VMALLOC on 32-bit x86. Since BPF only uses HAVE_ARCH_HUGE_VMALLOC for x86_64, turn it off for 32-bit x86. Reported-by: kernel test robot <oliver.sang@intel.com> Fixes: fac54e2bfb5b ("x86/Kconfig: Select HAVE_ARCH_HUGE_VMALLOC with HAVE_ARCH_HUGE_VMAP") Signed-off-by: Song Liu <song@kernel.org> --- arch/x86/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 995f2dc28631..9b356da6f46b 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -158,7 +158,7 @@ config X86 select HAVE_ALIGNED_STRUCT_PAGE if SLUB select HAVE_ARCH_AUDITSYSCALL select HAVE_ARCH_HUGE_VMAP if X86_64 || X86_PAE - select HAVE_ARCH_HUGE_VMALLOC if HAVE_ARCH_HUGE_VMAP + select HAVE_ARCH_HUGE_VMALLOC if X86_64 select HAVE_ARCH_JUMP_LABEL select HAVE_ARCH_JUMP_LABEL_RELATIVE select HAVE_ARCH_KASAN if X86_64 -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] x86: disable HAVE_ARCH_HUGE_VMALLOC on 32-bit x86 2022-03-02 0:43 ` [PATCH bpf-next 1/2] x86: disable HAVE_ARCH_HUGE_VMALLOC on 32-bit x86 Song Liu @ 2022-03-02 7:08 ` Yonghong Song 0 siblings, 0 replies; 6+ messages in thread From: Yonghong Song @ 2022-03-02 7:08 UTC (permalink / raw) To: Song Liu, bpf, netdev; +Cc: ast, daniel, andrii, kernel-team, kernel test robot On 3/1/22 4:43 PM, Song Liu wrote: > kernel test robot reported kernel BUG like: > > [ 44.587744][ T1] kernel BUG at arch/x86/mm/physaddr.c:76! > [ 44.590151][ T1] __vmalloc_area_node (mm/vmalloc.c:622 mm/vmalloc.c:2995) > [ 44.590151][ T1] __vmalloc_node_range (mm/vmalloc.c:3108) > [ 44.590151][ T1] __vmalloc_node (mm/vmalloc.c:3157) > > which is triggered with HAVE_ARCH_HUGE_VMALLOC on 32-bit x86. Since BPF > only uses HAVE_ARCH_HUGE_VMALLOC for x86_64, turn it off for 32-bit x86. > > Reported-by: kernel test robot <oliver.sang@intel.com> > Fixes: fac54e2bfb5b ("x86/Kconfig: Select HAVE_ARCH_HUGE_VMALLOC with HAVE_ARCH_HUGE_VMAP") > Signed-off-by: Song Liu <song@kernel.org> Acked-by: Yonghong Song <yhs@fb.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next 2/2] bpf, x86: set header->size properly before freeing it 2022-03-02 0:43 [PATCH bpf-next 0/2] fixes for bpf_prog_pack Song Liu 2022-03-02 0:43 ` [PATCH bpf-next 1/2] x86: disable HAVE_ARCH_HUGE_VMALLOC on 32-bit x86 Song Liu @ 2022-03-02 0:43 ` Song Liu 2022-03-02 7:12 ` Yonghong Song 1 sibling, 1 reply; 6+ messages in thread From: Song Liu @ 2022-03-02 0:43 UTC (permalink / raw) To: bpf, netdev; +Cc: ast, daniel, andrii, kernel-team, Song Liu, Kui-Feng Lee On do_jit failure path, the header is freed by bpf_jit_binary_pack_free. While bpf_jit_binary_pack_free doesn't require proper ro_header->size, bpf_prog_pack_free still uses it. Set header->size in bpf_int_jit_compile before calling bpf_jit_binary_pack_free. Fixes: 1022a5498f6f ("bpf, x86_64: Use bpf_jit_binary_pack_alloc") Fixes: 33c9805860e5 ("bpf: Introduce bpf_jit_binary_pack_[alloc|finalize|free]") Reported-by: Kui-Feng Lee <kuifeng@fb.com> Signed-off-by: Song Liu <song@kernel.org> --- arch/x86/net/bpf_jit_comp.c | 6 +++++- kernel/bpf/core.c | 7 ++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c index c7db0fe4de2f..b923d81ff6f9 100644 --- a/arch/x86/net/bpf_jit_comp.c +++ b/arch/x86/net/bpf_jit_comp.c @@ -2330,8 +2330,12 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) if (proglen <= 0) { out_image: image = NULL; - if (header) + if (header) { + /* set header->size for bpf_arch_text_copy */ + bpf_arch_text_copy(&header->size, &rw_header->size, + sizeof(rw_header->size)); bpf_jit_binary_pack_free(header, rw_header); + } prog = orig_prog; goto out_addrs; } diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index ebb0193d07f0..da587e4619e0 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -1112,13 +1112,14 @@ int bpf_jit_binary_pack_finalize(struct bpf_prog *prog, * 1) when the program is freed after; * 2) when the JIT engine fails (before bpf_jit_binary_pack_finalize). * For case 2), we need to free both the RO memory and the RW buffer. - * Also, ro_header->size in 2) is not properly set yet, so rw_header->size - * is used for uncharge. + * + * If bpf_jit_binary_pack_free is called before _finalize (jit error), + * it is necessary to set ro_header->size properly before freeing it. */ void bpf_jit_binary_pack_free(struct bpf_binary_header *ro_header, struct bpf_binary_header *rw_header) { - u32 size = rw_header ? rw_header->size : ro_header->size; + u32 size = ro_header->size; bpf_prog_pack_free(ro_header); kvfree(rw_header); -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 2/2] bpf, x86: set header->size properly before freeing it 2022-03-02 0:43 ` [PATCH bpf-next 2/2] bpf, x86: set header->size properly before freeing it Song Liu @ 2022-03-02 7:12 ` Yonghong Song 2022-03-02 17:24 ` Song Liu 0 siblings, 1 reply; 6+ messages in thread From: Yonghong Song @ 2022-03-02 7:12 UTC (permalink / raw) To: Song Liu, bpf, netdev; +Cc: ast, daniel, andrii, kernel-team, Kui-Feng Lee On 3/1/22 4:43 PM, Song Liu wrote: > On do_jit failure path, the header is freed by bpf_jit_binary_pack_free. > While bpf_jit_binary_pack_free doesn't require proper ro_header->size, > bpf_prog_pack_free still uses it. Set header->size in bpf_int_jit_compile > before calling bpf_jit_binary_pack_free. > > Fixes: 1022a5498f6f ("bpf, x86_64: Use bpf_jit_binary_pack_alloc") > Fixes: 33c9805860e5 ("bpf: Introduce bpf_jit_binary_pack_[alloc|finalize|free]") > Reported-by: Kui-Feng Lee <kuifeng@fb.com> > Signed-off-by: Song Liu <song@kernel.org> LGTM with a nit below related to comments. Acked-by: Yonghong Song <yhs@fb.com> > --- > arch/x86/net/bpf_jit_comp.c | 6 +++++- > kernel/bpf/core.c | 7 ++++--- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > index c7db0fe4de2f..b923d81ff6f9 100644 > --- a/arch/x86/net/bpf_jit_comp.c > +++ b/arch/x86/net/bpf_jit_comp.c > @@ -2330,8 +2330,12 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) > if (proglen <= 0) { > out_image: > image = NULL; > - if (header) > + if (header) { > + /* set header->size for bpf_arch_text_copy */ This comment is confusing. Setting header->size is not for bpf_arch_text_copy. Probably you mean 'by bpf_arch_text_copy? I think this comment is not necessary. > + bpf_arch_text_copy(&header->size, &rw_header->size, > + sizeof(rw_header->size)); > bpf_jit_binary_pack_free(header, rw_header); > + } > prog = orig_prog; > goto out_addrs; > } > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > index ebb0193d07f0..da587e4619e0 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -1112,13 +1112,14 @@ int bpf_jit_binary_pack_finalize(struct bpf_prog *prog, > * 1) when the program is freed after; > * 2) when the JIT engine fails (before bpf_jit_binary_pack_finalize). > * For case 2), we need to free both the RO memory and the RW buffer. > - * Also, ro_header->size in 2) is not properly set yet, so rw_header->size > - * is used for uncharge. > + * > + * If bpf_jit_binary_pack_free is called before _finalize (jit error), Do you mean bpf_jit_binary_pack_free() is called before calling bpf_jit_binary_pack_finalize()? This seems not the case. > + * it is necessary to set ro_header->size properly before freeing it. > */ > void bpf_jit_binary_pack_free(struct bpf_binary_header *ro_header, > struct bpf_binary_header *rw_header) > { > - u32 size = rw_header ? rw_header->size : ro_header->size; > + u32 size = ro_header->size; > > bpf_prog_pack_free(ro_header); > kvfree(rw_header); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 2/2] bpf, x86: set header->size properly before freeing it 2022-03-02 7:12 ` Yonghong Song @ 2022-03-02 17:24 ` Song Liu 0 siblings, 0 replies; 6+ messages in thread From: Song Liu @ 2022-03-02 17:24 UTC (permalink / raw) To: Yonghong Song Cc: Song Liu, bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kernel Team, Kui-Feng Lee > On Mar 1, 2022, at 11:12 PM, Yonghong Song <yhs@fb.com> wrote: > > > > On 3/1/22 4:43 PM, Song Liu wrote: >> On do_jit failure path, the header is freed by bpf_jit_binary_pack_free. >> While bpf_jit_binary_pack_free doesn't require proper ro_header->size, >> bpf_prog_pack_free still uses it. Set header->size in bpf_int_jit_compile >> before calling bpf_jit_binary_pack_free. >> Fixes: 1022a5498f6f ("bpf, x86_64: Use bpf_jit_binary_pack_alloc") >> Fixes: 33c9805860e5 ("bpf: Introduce bpf_jit_binary_pack_[alloc|finalize|free]") >> Reported-by: Kui-Feng Lee <kuifeng@fb.com> >> Signed-off-by: Song Liu <song@kernel.org> > > LGTM with a nit below related to comments. > > Acked-by: Yonghong Song <yhs@fb.com> > >> --- >> arch/x86/net/bpf_jit_comp.c | 6 +++++- >> kernel/bpf/core.c | 7 ++++--- >> 2 files changed, 9 insertions(+), 4 deletions(-) >> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c >> index c7db0fe4de2f..b923d81ff6f9 100644 >> --- a/arch/x86/net/bpf_jit_comp.c >> +++ b/arch/x86/net/bpf_jit_comp.c >> @@ -2330,8 +2330,12 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) >> if (proglen <= 0) { >> out_image: >> image = NULL; >> - if (header) >> + if (header) { >> + /* set header->size for bpf_arch_text_copy */ > > This comment is confusing. Setting header->size is not for bpf_arch_text_copy. Probably you mean 'by bpf_arch_text_copy? > I think this comment is not necessary. I meant to say set header->size for bpf_jit_binary_pack_free(). I guess it is not really necessary. Let me remove it. > >> + bpf_arch_text_copy(&header->size, &rw_header->size, >> + sizeof(rw_header->size)); >> bpf_jit_binary_pack_free(header, rw_header); >> + } >> prog = orig_prog; >> goto out_addrs; >> } >> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c >> index ebb0193d07f0..da587e4619e0 100644 >> --- a/kernel/bpf/core.c >> +++ b/kernel/bpf/core.c >> @@ -1112,13 +1112,14 @@ int bpf_jit_binary_pack_finalize(struct bpf_prog *prog, >> * 1) when the program is freed after; >> * 2) when the JIT engine fails (before bpf_jit_binary_pack_finalize). >> * For case 2), we need to free both the RO memory and the RW buffer. >> - * Also, ro_header->size in 2) is not properly set yet, so rw_header->size >> - * is used for uncharge. >> + * >> + * If bpf_jit_binary_pack_free is called before _finalize (jit error), > > Do you mean bpf_jit_binary_pack_free() is called before calling > bpf_jit_binary_pack_finalize()? This seems not the case. Now I see this is a little confusing. Let me rephrase it in v2. Thanks, Song > >> + * it is necessary to set ro_header->size properly before freeing it. >> */ >> void bpf_jit_binary_pack_free(struct bpf_binary_header *ro_header, >> struct bpf_binary_header *rw_header) >> { >> - u32 size = rw_header ? rw_header->size : ro_header->size; >> + u32 size = ro_header->size; >> bpf_prog_pack_free(ro_header); >> kvfree(rw_header); ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-02 17:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-03-02 0:43 [PATCH bpf-next 0/2] fixes for bpf_prog_pack Song Liu 2022-03-02 0:43 ` [PATCH bpf-next 1/2] x86: disable HAVE_ARCH_HUGE_VMALLOC on 32-bit x86 Song Liu 2022-03-02 7:08 ` Yonghong Song 2022-03-02 0:43 ` [PATCH bpf-next 2/2] bpf, x86: set header->size properly before freeing it Song Liu 2022-03-02 7:12 ` Yonghong Song 2022-03-02 17:24 ` Song Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).