* [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
* [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 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
* 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.