[PATCH v2 0/2] bpf: permit JIT allocations to be served outside the module region

[PATCH v2 0/2] bpf: permit JIT allocations to be served outside the module region

[Ard Biesheuvel]

On arm64, modules are allocated from a 128 MB window which is close to
the core kernel, so that relative direct branches are guaranteed to be
in range (except in some KASLR configurations). Also, module_alloc()
is in charge of allocating KASAN shadow memory when running with KASAN
enabled.

This means that the way BPF reuses module_alloc()/module_memfree() is
undesirable on arm64 (and potentially other architectures as well),
and so this series refactors BPF's use of those functions to permit
architectures to change this behavior.

Patch #1 breaks out the module_alloc() and module_memfree() calls into
__weak functions so they can be overridden.

Patch #4 implements the new alloc/free overrides for arm64

Changes since v1:
- Drop misguided attempt to 'fix' and refactor the free path. Instead,
  just add another __weak wrapper for the invocation of module_memfree()

Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>

Cc: Jessica Yu <jeyu@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org

Ard Biesheuvel (2):
  bpf: add __weak hook for allocating executable memory
  arm64/bpf: don't allocate BPF JIT programs in module memory

 arch/arm64/net/bpf_jit_comp.c | 10 ++++++++++
 kernel/bpf/core.c             | 14 ++++++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH v2 0/2] bpf: permit JIT allocations to be served outside the module region

[Ard Biesheuvel]

On arm64, modules are allocated from a 128 MB window which is close to
the core kernel, so that relative direct branches are guaranteed to be
in range (except in some KASLR configurations). Also, module_alloc()
is in charge of allocating KASAN shadow memory when running with KASAN
enabled.

This means that the way BPF reuses module_alloc()/module_memfree() is
undesirable on arm64 (and potentially other architectures as well),
and so this series refactors BPF's use of those functions to permit
architectures to change this behavior.

Patch #1 breaks out the module_alloc() and module_memfree() calls into
__weak functions so they can be overridden.

Patch #4 implements the new alloc/free overrides for arm64

Changes since v1:
- Drop misguided attempt to 'fix' and refactor the free path. Instead,
  just add another __weak wrapper for the invocation of module_memfree()

Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>

Cc: Jessica Yu <jeyu@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
Cc: netdev at vger.kernel.org

Ard Biesheuvel (2):
  bpf: add __weak hook for allocating executable memory
  arm64/bpf: don't allocate BPF JIT programs in module memory

 arch/arm64/net/bpf_jit_comp.c | 10 ++++++++++
 kernel/bpf/core.c             | 14 ++++++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] bpf: add __weak hook for allocating executable memory

[Ard Biesheuvel]

By default, BPF uses module_alloc() to allocate executable memory,
but this is not necessary on all arches and potentially undesirable
on some of them.

So break out the module_alloc() and module_memfree() calls into __weak
functions to allow them to be overridden in arch code.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 kernel/bpf/core.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 1a796e0799ec..f28d8a5eb6b8 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -609,6 +609,16 @@ static void bpf_jit_uncharge_modmem(u32 pages)
 	atomic_long_sub(pages, &bpf_jit_current);
 }
 
+void *__weak bpf_jit_alloc_exec(unsigned long size)
+{
+	return module_alloc(size);
+}
+
+void __weak bpf_jit_free_exec(const void *addr)
+{
+	return module_memfree(size);
+}
+
 struct bpf_binary_header *
 bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
 		     unsigned int alignment,
@@ -626,7 +636,7 @@ bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
 
 	if (bpf_jit_charge_modmem(pages))
 		return NULL;
-	hdr = module_alloc(size);
+	hdr = bpf_jit_alloc_exec(size);
 	if (!hdr) {
 		bpf_jit_uncharge_modmem(pages);
 		return NULL;
@@ -650,7 +660,7 @@ void bpf_jit_binary_free(struct bpf_binary_header *hdr)
 {
 	u32 pages = hdr->pages;
 
-	module_memfree(hdr);
+	bpf_jit_free_exec(hdr);
 	bpf_jit_uncharge_modmem(pages);
 }
 
-- 
2.17.1

[PATCH v2 1/2] bpf: add __weak hook for allocating executable memory

[Ard Biesheuvel]

By default, BPF uses module_alloc() to allocate executable memory,
but this is not necessary on all arches and potentially undesirable
on some of them.

So break out the module_alloc() and module_memfree() calls into __weak
functions to allow them to be overridden in arch code.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 kernel/bpf/core.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 1a796e0799ec..f28d8a5eb6b8 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -609,6 +609,16 @@ static void bpf_jit_uncharge_modmem(u32 pages)
 	atomic_long_sub(pages, &bpf_jit_current);
 }
 
+void *__weak bpf_jit_alloc_exec(unsigned long size)
+{
+	return module_alloc(size);
+}
+
+void __weak bpf_jit_free_exec(const void *addr)
+{
+	return module_memfree(size);
+}
+
 struct bpf_binary_header *
 bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
 		     unsigned int alignment,
@@ -626,7 +636,7 @@ bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
 
 	if (bpf_jit_charge_modmem(pages))
 		return NULL;
-	hdr = module_alloc(size);
+	hdr = bpf_jit_alloc_exec(size);
 	if (!hdr) {
 		bpf_jit_uncharge_modmem(pages);
 		return NULL;
@@ -650,7 +660,7 @@ void bpf_jit_binary_free(struct bpf_binary_header *hdr)
 {
 	u32 pages = hdr->pages;
 
-	module_memfree(hdr);
+	bpf_jit_free_exec(hdr);
 	bpf_jit_uncharge_modmem(pages);
 }
 
-- 
2.17.1

[PATCH v2 2/2] arm64/bpf: don't allocate BPF JIT programs in module memory

[Ard Biesheuvel]

The arm64 module region is a 128 MB region that is kept close to
the core kernel, in order to ensure that relative branches are
always in range. So using the same region for programs that do
not have this restriction is wasteful, and preferably avoided.

Now that the core BPF JIT code permits the alloc/free routines to
be overridden, implement them by simple vmalloc_exec()/vfree()
calls, which can be served from anywere. This also solves an
issue under KASAN, where shadow memory is needlessly allocated for
all BPF programs (which don't require KASAN shadow pages since
they are not KASAN instrumented)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/net/bpf_jit_comp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index a6fdaea07c63..f91b7c157841 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -940,3 +940,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 					   tmp : orig_prog);
 	return prog;
 }
+
+void *bpf_jit_alloc_exec(unsigned long size)
+{
+	return vmalloc_exec(size);
+}
+
+void bpf_jit_free_exec(const void *addr)
+{
+	return vfree(size);
+}
-- 
2.17.1

[PATCH v2 2/2] arm64/bpf: don't allocate BPF JIT programs in module memory

[Ard Biesheuvel]

The arm64 module region is a 128 MB region that is kept close to
the core kernel, in order to ensure that relative branches are
always in range. So using the same region for programs that do
not have this restriction is wasteful, and preferably avoided.

Now that the core BPF JIT code permits the alloc/free routines to
be overridden, implement them by simple vmalloc_exec()/vfree()
calls, which can be served from anywere. This also solves an
issue under KASAN, where shadow memory is needlessly allocated for
all BPF programs (which don't require KASAN shadow pages since
they are not KASAN instrumented)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/net/bpf_jit_comp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index a6fdaea07c63..f91b7c157841 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -940,3 +940,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 					   tmp : orig_prog);
 	return prog;
 }
+
+void *bpf_jit_alloc_exec(unsigned long size)
+{
+	return vmalloc_exec(size);
+}
+
+void bpf_jit_free_exec(const void *addr)
+{
+	return vfree(size);
+}
-- 
2.17.1

Re: [PATCH v2 0/2] bpf: permit JIT allocations to be served outside the module region

[Edgecombe, Rick P]

On Wed, 2018-11-21 at 14:17 +0100, Ard Biesheuvel wrote:
> On arm64, modules are allocated from a 128 MB window which is close to
> the core kernel, so that relative direct branches are guaranteed to be
> in range (except in some KASLR configurations). Also, module_alloc()
> is in charge of allocating KASAN shadow memory when running with KASAN
> enabled.
> 
> This means that the way BPF reuses module_alloc()/module_memfree() is
> undesirable on arm64 (and potentially other architectures as well),
> and so this series refactors BPF's use of those functions to permit
> architectures to change this behavior.
> 
Hi Ard,

I am looking at adding optional BPF JIT in vmalloc functionality for x86 that
would use this refactor. In fact I have done the same thing with just different
names.

My implementation intends to use the module space until a usage limit is reached
and then overflow into vmalloc, so it would be an additional knob like
"bpf_jit_limit". Wondering if that should be a cross-arch concept that connects
to this. Does it fit in with what you are trying to do for arm64 here?

Thanks,

Rick

Re: [PATCH v2 0/2] bpf: permit JIT allocations to be served outside the module region

[Edgecombe, Rick P]

On Wed, 2018-11-21 at 14:17 +0100, Ard Biesheuvel wrote:
> On arm64, modules are allocated from a 128 MB window which is close to
> the core kernel, so that relative direct branches are guaranteed to be
> in range (except in some KASLR configurations). Also, module_alloc()
> is in charge of allocating KASAN shadow memory when running with KASAN
> enabled.
> 
> This means that the way BPF reuses module_alloc()/module_memfree() is
> undesirable on arm64 (and potentially other architectures as well),
> and so this series refactors BPF's use of those functions to permit
> architectures to change this behavior.
> 
Hi Ard,

I am looking at adding optional BPF JIT in vmalloc functionality for x86 that
would use this refactor. In fact I have done the same thing with just different
names.

My implementation intends to use the module space until a usage limit is reached
and then overflow into vmalloc, so it would be an additional knob like
"bpf_jit_limit". Wondering if that should be a cross-arch concept that connects
to this. Does it fit in with what you are trying to do for arm64 here?

Thanks,

Rick

[PATCH v2 0/2] bpf: permit JIT allocations to be served outside the module region

[Edgecombe, Rick P]

On Wed, 2018-11-21 at 14:17 +0100, Ard Biesheuvel wrote:
> On arm64, modules are allocated from a 128 MB window which is close to
> the core kernel, so that relative direct branches are guaranteed to be
> in range (except in some KASLR configurations). Also, module_alloc()
> is in charge of allocating KASAN shadow memory when running with KASAN
> enabled.
> 
> This means that the way BPF reuses module_alloc()/module_memfree() is
> undesirable on arm64 (and potentially other architectures as well),
> and so this series refactors BPF's use of those functions to permit
> architectures to change this behavior.
> 
Hi Ard,

I am looking at adding optional BPF JIT in vmalloc functionality for x86 that
would use this refactor. In fact I have done the same thing with just different
names.

My implementation intends to use the module space until a usage limit is reached
and then overflow into vmalloc, so it would be an additional knob like
"bpf_jit_limit". Wondering if that should be a cross-arch concept that connects
to this. Does it fit in with what you are trying to do for arm64 here?

Thanks,

Rick

Re: [PATCH v2 0/2] bpf: permit JIT allocations to be served outside the module region

[Ard Biesheuvel]

On Wed, 21 Nov 2018 at 20:48, Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
>
> On Wed, 2018-11-21 at 14:17 +0100, Ard Biesheuvel wrote:
> > On arm64, modules are allocated from a 128 MB window which is close to
> > the core kernel, so that relative direct branches are guaranteed to be
> > in range (except in some KASLR configurations). Also, module_alloc()
> > is in charge of allocating KASAN shadow memory when running with KASAN
> > enabled.
> >
> > This means that the way BPF reuses module_alloc()/module_memfree() is
> > undesirable on arm64 (and potentially other architectures as well),
> > and so this series refactors BPF's use of those functions to permit
> > architectures to change this behavior.
> >
> Hi Ard,
>
> I am looking at adding optional BPF JIT in vmalloc functionality for x86 that
> would use this refactor. In fact I have done the same thing with just different
> names.
>
> My implementation intends to use the module space until a usage limit is reached
> and then overflow into vmalloc, so it would be an additional knob like
> "bpf_jit_limit". Wondering if that should be a cross-arch concept that connects
> to this. Does it fit in with what you are trying to do for arm64 here?
>

Hi Rick,

As I understand it, x86 requires the BPF allocations to be located
within 2 GB of the core kernel, so that RIP-relative 32-bit jumps are
in range (I read that in a comment somewhere, or a git commit log
perhaps)

That requirement does not exist on arm64: ordinary function calls and
tail calls emitted by the BPF JIT code have unlimited range, and so
there is simply no reason to prefer the module region for these
allocations. I guess we could achieve the same when reusing your
approach by setting the threshold to zero.

[PATCH v2 0/2] bpf: permit JIT allocations to be served outside the module region

[Ard Biesheuvel]

On Wed, 21 Nov 2018 at 20:48, Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
>
> On Wed, 2018-11-21 at 14:17 +0100, Ard Biesheuvel wrote:
> > On arm64, modules are allocated from a 128 MB window which is close to
> > the core kernel, so that relative direct branches are guaranteed to be
> > in range (except in some KASLR configurations). Also, module_alloc()
> > is in charge of allocating KASAN shadow memory when running with KASAN
> > enabled.
> >
> > This means that the way BPF reuses module_alloc()/module_memfree() is
> > undesirable on arm64 (and potentially other architectures as well),
> > and so this series refactors BPF's use of those functions to permit
> > architectures to change this behavior.
> >
> Hi Ard,
>
> I am looking at adding optional BPF JIT in vmalloc functionality for x86 that
> would use this refactor. In fact I have done the same thing with just different
> names.
>
> My implementation intends to use the module space until a usage limit is reached
> and then overflow into vmalloc, so it would be an additional knob like
> "bpf_jit_limit". Wondering if that should be a cross-arch concept that connects
> to this. Does it fit in with what you are trying to do for arm64 here?
>

Hi Rick,

As I understand it, x86 requires the BPF allocations to be located
within 2 GB of the core kernel, so that RIP-relative 32-bit jumps are
in range (I read that in a comment somewhere, or a git commit log
perhaps)

That requirement does not exist on arm64: ordinary function calls and
tail calls emitted by the BPF JIT code have unlimited range, and so
there is simply no reason to prefer the module region for these
allocations. I guess we could achieve the same when reusing your
approach by setting the threshold to zero.

Re: [PATCH v2 2/2] arm64/bpf: don't allocate BPF JIT programs in module memory

[Daniel Borkmann]

On 11/21/2018 02:17 PM, Ard Biesheuvel wrote:
> The arm64 module region is a 128 MB region that is kept close to
> the core kernel, in order to ensure that relative branches are
> always in range. So using the same region for programs that do
> not have this restriction is wasteful, and preferably avoided.
> 
> Now that the core BPF JIT code permits the alloc/free routines to
> be overridden, implement them by simple vmalloc_exec()/vfree()
> calls, which can be served from anywere. This also solves an
> issue under KASAN, where shadow memory is needlessly allocated for
> all BPF programs (which don't require KASAN shadow pages since
> they are not KASAN instrumented)
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/net/bpf_jit_comp.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index a6fdaea07c63..f91b7c157841 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -940,3 +940,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>  					   tmp : orig_prog);
>  	return prog;
>  }
> +
> +void *bpf_jit_alloc_exec(unsigned long size)
> +{
> +	return vmalloc_exec(size);
> +}
> +
> +void bpf_jit_free_exec(const void *addr)
> +{
> +	return vfree(size);
> +}

Hmm, could you elaborate in the commit log on the potential performance
regression for JITed progs on arm64 after this change?

I think this change would also break JITing of BPF to BPF calls. You might
have the same issue as ppc64 folks where the offset might not fit into imm
anymore and would have to transfer it via fp->aux->func[off]->bpf_func
instead.

[PATCH v2 2/2] arm64/bpf: don't allocate BPF JIT programs in module memory

[Daniel Borkmann]

On 11/21/2018 02:17 PM, Ard Biesheuvel wrote:
> The arm64 module region is a 128 MB region that is kept close to
> the core kernel, in order to ensure that relative branches are
> always in range. So using the same region for programs that do
> not have this restriction is wasteful, and preferably avoided.
> 
> Now that the core BPF JIT code permits the alloc/free routines to
> be overridden, implement them by simple vmalloc_exec()/vfree()
> calls, which can be served from anywere. This also solves an
> issue under KASAN, where shadow memory is needlessly allocated for
> all BPF programs (which don't require KASAN shadow pages since
> they are not KASAN instrumented)
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/net/bpf_jit_comp.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index a6fdaea07c63..f91b7c157841 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -940,3 +940,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>  					   tmp : orig_prog);
>  	return prog;
>  }
> +
> +void *bpf_jit_alloc_exec(unsigned long size)
> +{
> +	return vmalloc_exec(size);
> +}
> +
> +void bpf_jit_free_exec(const void *addr)
> +{
> +	return vfree(size);
> +}

Hmm, could you elaborate in the commit log on the potential performance
regression for JITed progs on arm64 after this change?

I think this change would also break JITing of BPF to BPF calls. You might
have the same issue as ppc64 folks where the offset might not fit into imm
anymore and would have to transfer it via fp->aux->func[off]->bpf_func
instead.

Re: [PATCH v2 2/2] arm64/bpf: don't allocate BPF JIT programs in module memory

[Ard Biesheuvel]

On Thu, 22 Nov 2018 at 00:20, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 11/21/2018 02:17 PM, Ard Biesheuvel wrote:
> > The arm64 module region is a 128 MB region that is kept close to
> > the core kernel, in order to ensure that relative branches are
> > always in range. So using the same region for programs that do
> > not have this restriction is wasteful, and preferably avoided.
> >
> > Now that the core BPF JIT code permits the alloc/free routines to
> > be overridden, implement them by simple vmalloc_exec()/vfree()
> > calls, which can be served from anywere. This also solves an
> > issue under KASAN, where shadow memory is needlessly allocated for
> > all BPF programs (which don't require KASAN shadow pages since
> > they are not KASAN instrumented)
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  arch/arm64/net/bpf_jit_comp.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > index a6fdaea07c63..f91b7c157841 100644
> > --- a/arch/arm64/net/bpf_jit_comp.c
> > +++ b/arch/arm64/net/bpf_jit_comp.c
> > @@ -940,3 +940,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
> >                                          tmp : orig_prog);
> >       return prog;
> >  }
> > +
> > +void *bpf_jit_alloc_exec(unsigned long size)
> > +{
> > +     return vmalloc_exec(size);
> > +}
> > +
> > +void bpf_jit_free_exec(const void *addr)
> > +{
> > +     return vfree(size);
> > +}
>
> Hmm, could you elaborate in the commit log on the potential performance
> regression for JITed progs on arm64 after this change?
>

This does not affect the generated code, so I don't anticipate a
performance hit. Did you have anything in particular in mind?

> I think this change would also break JITing of BPF to BPF calls. You might
> have the same issue as ppc64 folks where the offset might not fit into imm
> anymore and would have to transfer it via fp->aux->func[off]->bpf_func
> instead.

If we are relying on BPF programs to remain within 128 MB of each
other, then we already have a potential problem, given that the
module_alloc() spills over into a 4 GB window if the 128 MB window is
exhausted. Perhaps we should do something like

void *bpf_jit_alloc_exec(unsigned long size) {
  return __vmalloc_node_range(size, MODULE_ALIGN,
    BPF_REGION_START, BPF_REGION_END,
    GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
    __builtin_return_address(0));
}

and make [BPF_REGION_START, BPF_REGION_END) a separate 128 MB window
at the top of the vmalloc space. That way, it is guaranteed that BPF
programs are within branching range of each other, and we still solve
the original problem. I also like that it becomes impossible to infer
anything about the state of the vmalloc space, placement of the kernel
and modules etc from the placement of the BPF programs (in case it
leaks this information in one way or the other)

That would only give you space for 128M/4K == 32768 programs (or
128M/64K == 2048 on 64k pages kernels). So I guess we'd still need a
spillover window as well, in which case we'd need a fix for the
BPF-to-BPF branching issue (but we need that at the moment anyway)

[PATCH v2 2/2] arm64/bpf: don't allocate BPF JIT programs in module memory

[Ard Biesheuvel]

On Thu, 22 Nov 2018 at 00:20, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 11/21/2018 02:17 PM, Ard Biesheuvel wrote:
> > The arm64 module region is a 128 MB region that is kept close to
> > the core kernel, in order to ensure that relative branches are
> > always in range. So using the same region for programs that do
> > not have this restriction is wasteful, and preferably avoided.
> >
> > Now that the core BPF JIT code permits the alloc/free routines to
> > be overridden, implement them by simple vmalloc_exec()/vfree()
> > calls, which can be served from anywere. This also solves an
> > issue under KASAN, where shadow memory is needlessly allocated for
> > all BPF programs (which don't require KASAN shadow pages since
> > they are not KASAN instrumented)
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  arch/arm64/net/bpf_jit_comp.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > index a6fdaea07c63..f91b7c157841 100644
> > --- a/arch/arm64/net/bpf_jit_comp.c
> > +++ b/arch/arm64/net/bpf_jit_comp.c
> > @@ -940,3 +940,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
> >                                          tmp : orig_prog);
> >       return prog;
> >  }
> > +
> > +void *bpf_jit_alloc_exec(unsigned long size)
> > +{
> > +     return vmalloc_exec(size);
> > +}
> > +
> > +void bpf_jit_free_exec(const void *addr)
> > +{
> > +     return vfree(size);
> > +}
>
> Hmm, could you elaborate in the commit log on the potential performance
> regression for JITed progs on arm64 after this change?
>

This does not affect the generated code, so I don't anticipate a
performance hit. Did you have anything in particular in mind?

> I think this change would also break JITing of BPF to BPF calls. You might
> have the same issue as ppc64 folks where the offset might not fit into imm
> anymore and would have to transfer it via fp->aux->func[off]->bpf_func
> instead.

If we are relying on BPF programs to remain within 128 MB of each
other, then we already have a potential problem, given that the
module_alloc() spills over into a 4 GB window if the 128 MB window is
exhausted. Perhaps we should do something like

void *bpf_jit_alloc_exec(unsigned long size) {
  return __vmalloc_node_range(size, MODULE_ALIGN,
    BPF_REGION_START, BPF_REGION_END,
    GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
    __builtin_return_address(0));
}

and make [BPF_REGION_START, BPF_REGION_END) a separate 128 MB window
at the top of the vmalloc space. That way, it is guaranteed that BPF
programs are within branching range of each other, and we still solve
the original problem. I also like that it becomes impossible to infer
anything about the state of the vmalloc space, placement of the kernel
and modules etc from the placement of the BPF programs (in case it
leaks this information in one way or the other)

That would only give you space for 128M/4K == 32768 programs (or
128M/64K == 2048 on 64k pages kernels). So I guess we'd still need a
spillover window as well, in which case we'd need a fix for the
BPF-to-BPF branching issue (but we need that at the moment anyway)

Re: [PATCH v2 1/2] bpf: add __weak hook for allocating executable memory

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]

Hi Ard,

I love your patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on v4.20-rc3 next-20181122]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/bpf-permit-JIT-allocations-to-be-served-outside-the-module-region/20181123-033144
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-x002-201846 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   kernel//bpf/core.c: In function 'bpf_jit_free_exec':
>> kernel//bpf/core.c:632:24: error: 'size' undeclared (first use in this function); did you mean 'ksize'?
     return module_memfree(size);
                           ^~~~
                           ksize
   kernel//bpf/core.c:632:24: note: each undeclared identifier is reported only once for each function it appears in
>> kernel//bpf/core.c:632:9: warning: 'return' with a value, in function returning void
     return module_memfree(size);
            ^~~~~~~~~~~~~~
   kernel//bpf/core.c:630:13: note: declared here
    void __weak bpf_jit_free_exec(const void *addr)
                ^~~~~~~~~~~~~~~~~

vim +632 kernel//bpf/core.c

   629	
   630	void __weak bpf_jit_free_exec(const void *addr)
   631	{
 > 632		return module_memfree(size);
   633	}
   634	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23659 bytes --]

[PATCH v2 1/2] bpf: add __weak hook for allocating executable memory

[kbuild test robot]

Hi Ard,

I love your patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on v4.20-rc3 next-20181122]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/bpf-permit-JIT-allocations-to-be-served-outside-the-module-region/20181123-033144
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-x002-201846 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   kernel//bpf/core.c: In function 'bpf_jit_free_exec':
>> kernel//bpf/core.c:632:24: error: 'size' undeclared (first use in this function); did you mean 'ksize'?
     return module_memfree(size);
                           ^~~~
                           ksize
   kernel//bpf/core.c:632:24: note: each undeclared identifier is reported only once for each function it appears in
>> kernel//bpf/core.c:632:9: warning: 'return' with a value, in function returning void
     return module_memfree(size);
            ^~~~~~~~~~~~~~
   kernel//bpf/core.c:630:13: note: declared here
    void __weak bpf_jit_free_exec(const void *addr)
                ^~~~~~~~~~~~~~~~~

vim +632 kernel//bpf/core.c

   629	
   630	void __weak bpf_jit_free_exec(const void *addr)
   631	{
 > 632		return module_memfree(size);
   633	}
   634	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 23659 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181123/cee26021/attachment-0001.gz>

Re: [PATCH v2 1/2] bpf: add __weak hook for allocating executable memory

[Ard Biesheuvel]

> On 22 Nov 2018, at 21:01, kbuild test robot <lkp@intel.com> wrote:
> 
> Hi Ard,
> 
> I love your patch! Yet something to improve:
> 

Ugh, apologies for this. I’m sure i build tested /something/ but clearly not what i should have tested.

In any case, following up on the discussion I’ll have to respin this in any case. I’ll triple check next time that what i send out was tested properly.


> [auto build test ERROR on bpf-next/master]
> [also build test ERROR on v4.20-rc3 next-20181122]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/bpf-permit-JIT-allocations-to-be-served-outside-the-module-region/20181123-033144
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> config: x86_64-randconfig-x002-201846 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
>        # save the attached .config to linux build tree
>        make ARCH=x86_64 
> 
> All error/warnings (new ones prefixed by >>):
> 
>   kernel//bpf/core.c: In function 'bpf_jit_free_exec':
>>> kernel//bpf/core.c:632:24: error: 'size' undeclared (first use in this function); did you mean 'ksize'?
>     return module_memfree(size);
>                           ^~~~
>                           ksize
>   kernel//bpf/core.c:632:24: note: each undeclared identifier is reported only once for each function it appears in
>>> kernel//bpf/core.c:632:9: warning: 'return' with a value, in function returning void
>     return module_memfree(size);
>            ^~~~~~~~~~~~~~
>   kernel//bpf/core.c:630:13: note: declared here
>    void __weak bpf_jit_free_exec(const void *addr)
>                ^~~~~~~~~~~~~~~~~
> 
> vim +632 kernel//bpf/core.c
> 
>   629    
>   630    void __weak bpf_jit_free_exec(const void *addr)
>   631    {
>> 632        return module_memfree(size);
>   633    }
>   634    
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> <.config.gz>

[PATCH v2 1/2] bpf: add __weak hook for allocating executable memory

[Ard Biesheuvel]

> On 22 Nov 2018, at 21:01, kbuild test robot <lkp@intel.com> wrote:
> 
> Hi Ard,
> 
> I love your patch! Yet something to improve:
> 

Ugh, apologies for this. I?m sure i build tested /something/ but clearly not what i should have tested.

In any case, following up on the discussion I?ll have to respin this in any case. I?ll triple check next time that what i send out was tested properly.


> [auto build test ERROR on bpf-next/master]
> [also build test ERROR on v4.20-rc3 next-20181122]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/bpf-permit-JIT-allocations-to-be-served-outside-the-module-region/20181123-033144
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> config: x86_64-randconfig-x002-201846 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
>        # save the attached .config to linux build tree
>        make ARCH=x86_64 
> 
> All error/warnings (new ones prefixed by >>):
> 
>   kernel//bpf/core.c: In function 'bpf_jit_free_exec':
>>> kernel//bpf/core.c:632:24: error: 'size' undeclared (first use in this function); did you mean 'ksize'?
>     return module_memfree(size);
>                           ^~~~
>                           ksize
>   kernel//bpf/core.c:632:24: note: each undeclared identifier is reported only once for each function it appears in
>>> kernel//bpf/core.c:632:9: warning: 'return' with a value, in function returning void
>     return module_memfree(size);
>            ^~~~~~~~~~~~~~
>   kernel//bpf/core.c:630:13: note: declared here
>    void __weak bpf_jit_free_exec(const void *addr)
>                ^~~~~~~~~~~~~~~~~
> 
> vim +632 kernel//bpf/core.c
> 
>   629    
>   630    void __weak bpf_jit_free_exec(const void *addr)
>   631    {
>> 632        return module_memfree(size);
>   633    }
>   634    
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> <.config.gz>

Re: [PATCH v2 1/2] bpf: add __weak hook for allocating executable memory

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

Hi Ard,

I love your patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on v4.20-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/bpf-permit-JIT-allocations-to-be-served-outside-the-module-region/20181123-033144
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: i386-randconfig-s0-11191736 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   kernel/bpf/core.c: In function 'bpf_jit_free_exec':
>> kernel/bpf/core.c:632:24: error: 'size' undeclared (first use in this function)
     return module_memfree(size);
                           ^~~~
   kernel/bpf/core.c:632:24: note: each undeclared identifier is reported only once for each function it appears in
   kernel/bpf/core.c:632:9: warning: 'return' with a value, in function returning void
     return module_memfree(size);
            ^~~~~~~~~~~~~~
   kernel/bpf/core.c:630:13: note: declared here
    void __weak bpf_jit_free_exec(const void *addr)
                ^~~~~~~~~~~~~~~~~

vim +/size +632 kernel/bpf/core.c

   629	
   630	void __weak bpf_jit_free_exec(const void *addr)
   631	{
 > 632		return module_memfree(size);
   633	}
   634	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36733 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/2] bpf: add __weak hook for allocating executable memory

[kbuild test robot]

Hi Ard,

I love your patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on v4.20-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/bpf-permit-JIT-allocations-to-be-served-outside-the-module-region/20181123-033144
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: i386-randconfig-s0-11191736 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   kernel/bpf/core.c: In function 'bpf_jit_free_exec':
>> kernel/bpf/core.c:632:24: error: 'size' undeclared (first use in this function)
     return module_memfree(size);
                           ^~~~
   kernel/bpf/core.c:632:24: note: each undeclared identifier is reported only once for each function it appears in
   kernel/bpf/core.c:632:9: warning: 'return' with a value, in function returning void
     return module_memfree(size);
            ^~~~~~~~~~~~~~
   kernel/bpf/core.c:630:13: note: declared here
    void __weak bpf_jit_free_exec(const void *addr)
                ^~~~~~~~~~~~~~~~~

vim +/size +632 kernel/bpf/core.c

   629	
   630	void __weak bpf_jit_free_exec(const void *addr)
   631	{
 > 632		return module_memfree(size);
   633	}
   634	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 36733 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181123/479932ec/attachment-0001.gz>

Re: [PATCH v2 2/2] arm64/bpf: don't allocate BPF JIT programs in module memory

[Daniel Borkmann]

On 11/22/2018 09:02 AM, Ard Biesheuvel wrote:
> On Thu, 22 Nov 2018 at 00:20, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 11/21/2018 02:17 PM, Ard Biesheuvel wrote:
>>> The arm64 module region is a 128 MB region that is kept close to
>>> the core kernel, in order to ensure that relative branches are
>>> always in range. So using the same region for programs that do
>>> not have this restriction is wasteful, and preferably avoided.
>>>
>>> Now that the core BPF JIT code permits the alloc/free routines to
>>> be overridden, implement them by simple vmalloc_exec()/vfree()
>>> calls, which can be served from anywere. This also solves an
>>> issue under KASAN, where shadow memory is needlessly allocated for
>>> all BPF programs (which don't require KASAN shadow pages since
>>> they are not KASAN instrumented)
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  arch/arm64/net/bpf_jit_comp.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
>>> index a6fdaea07c63..f91b7c157841 100644
>>> --- a/arch/arm64/net/bpf_jit_comp.c
>>> +++ b/arch/arm64/net/bpf_jit_comp.c
>>> @@ -940,3 +940,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>>>                                          tmp : orig_prog);
>>>       return prog;
>>>  }
>>> +
>>> +void *bpf_jit_alloc_exec(unsigned long size)
>>> +{
>>> +     return vmalloc_exec(size);
>>> +}
>>> +
>>> +void bpf_jit_free_exec(const void *addr)
>>> +{
>>> +     return vfree(size);
>>> +}
>>
>> Hmm, could you elaborate in the commit log on the potential performance
>> regression for JITed progs on arm64 after this change?
> 
> This does not affect the generated code, so I don't anticipate a
> performance hit. Did you have anything in particular in mind?

We do optimize immediate emission in the JIT, I was mostly wondering that
once the code is much further away from core kernel how much more insns we
might need to emit in some worst case for each BPF helper call, but then
unlike some other archs we always use absolute addresses so nothing would
change here, so never mind. (And BPF to BPF calls emits unoptimized 64
immediates since this is needed as we pass through the JIT several times
so we need this as place holder for later once the address is actually
known.)

>> I think this change would also break JITing of BPF to BPF calls. You might
>> have the same issue as ppc64 folks where the offset might not fit into imm
>> anymore and would have to transfer it via fp->aux->func[off]->bpf_func
>> instead.
> 
> If we are relying on BPF programs to remain within 128 MB of each
> other, then we already have a potential problem, given that the
> module_alloc() spills over into a 4 GB window if the 128 MB window is
> exhausted. Perhaps we should do something like

Hmm, good point, presumably you mean this one here fd045f6cd98e ("arm64:
add support for module PLTs"). Agree that this needs fixing.

> void *bpf_jit_alloc_exec(unsigned long size) {
>   return __vmalloc_node_range(size, MODULE_ALIGN,
>     BPF_REGION_START, BPF_REGION_END,
>     GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
>     __builtin_return_address(0));
> }
> 
> and make [BPF_REGION_START, BPF_REGION_END) a separate 128 MB window
> at the top of the vmalloc space. That way, it is guaranteed that BPF
> programs are within branching range of each other, and we still solve
> the original problem. I also like that it becomes impossible to infer
> anything about the state of the vmalloc space, placement of the kernel
> and modules etc from the placement of the BPF programs (in case it
> leaks this information in one way or the other)
> 
> That would only give you space for 128M/4K == 32768 programs (or
> 128M/64K == 2048 on 64k pages kernels). So I guess we'd still need a

Note that it's 4k BPF insns which do not map 1:1, if possible I'd actually
prefer if we could enlarge this space a bit.

> spillover window as well, in which case we'd need a fix for the
> BPF-to-BPF branching issue (but we need that at the moment anyway)

Yeah, or spillover window instead. I think as a fix starting out with
its own region without the spillover window would be less complex and
better suited for stable?

Thanks,
Daniel

Re: [PATCH v2 2/2] arm64/bpf: don't allocate BPF JIT programs in module memory

[Daniel Borkmann]

On 11/22/2018 09:02 AM, Ard Biesheuvel wrote:
> On Thu, 22 Nov 2018 at 00:20, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 11/21/2018 02:17 PM, Ard Biesheuvel wrote:
>>> The arm64 module region is a 128 MB region that is kept close to
>>> the core kernel, in order to ensure that relative branches are
>>> always in range. So using the same region for programs that do
>>> not have this restriction is wasteful, and preferably avoided.
>>>
>>> Now that the core BPF JIT code permits the alloc/free routines to
>>> be overridden, implement them by simple vmalloc_exec()/vfree()
>>> calls, which can be served from anywere. This also solves an
>>> issue under KASAN, where shadow memory is needlessly allocated for
>>> all BPF programs (which don't require KASAN shadow pages since
>>> they are not KASAN instrumented)
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  arch/arm64/net/bpf_jit_comp.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
>>> index a6fdaea07c63..f91b7c157841 100644
>>> --- a/arch/arm64/net/bpf_jit_comp.c
>>> +++ b/arch/arm64/net/bpf_jit_comp.c
>>> @@ -940,3 +940,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>>>                                          tmp : orig_prog);
>>>       return prog;
>>>  }
>>> +
>>> +void *bpf_jit_alloc_exec(unsigned long size)
>>> +{
>>> +     return vmalloc_exec(size);
>>> +}
>>> +
>>> +void bpf_jit_free_exec(const void *addr)
>>> +{
>>> +     return vfree(size);
>>> +}
>>
>> Hmm, could you elaborate in the commit log on the potential performance
>> regression for JITed progs on arm64 after this change?
> 
> This does not affect the generated code, so I don't anticipate a
> performance hit. Did you have anything in particular in mind?

We do optimize immediate emission in the JIT, I was mostly wondering that
once the code is much further away from core kernel how much more insns we
might need to emit in some worst case for each BPF helper call, but then
unlike some other archs we always use absolute addresses so nothing would
change here, so never mind. (And BPF to BPF calls emits unoptimized 64
immediates since this is needed as we pass through the JIT several times
so we need this as place holder for later once the address is actually
known.)

>> I think this change would also break JITing of BPF to BPF calls. You might
>> have the same issue as ppc64 folks where the offset might not fit into imm
>> anymore and would have to transfer it via fp->aux->func[off]->bpf_func
>> instead.
> 
> If we are relying on BPF programs to remain within 128 MB of each
> other, then we already have a potential problem, given that the
> module_alloc() spills over into a 4 GB window if the 128 MB window is
> exhausted. Perhaps we should do something like

Hmm, good point, presumably you mean this one here fd045f6cd98e ("arm64:
add support for module PLTs"). Agree that this needs fixing.

> void *bpf_jit_alloc_exec(unsigned long size) {
>   return __vmalloc_node_range(size, MODULE_ALIGN,
>     BPF_REGION_START, BPF_REGION_END,
>     GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
>     __builtin_return_address(0));
> }
> 
> and make [BPF_REGION_START, BPF_REGION_END) a separate 128 MB window
> at the top of the vmalloc space. That way, it is guaranteed that BPF
> programs are within branching range of each other, and we still solve
> the original problem. I also like that it becomes impossible to infer
> anything about the state of the vmalloc space, placement of the kernel
> and modules etc from the placement of the BPF programs (in case it
> leaks this information in one way or the other)
> 
> That would only give you space for 128M/4K == 32768 programs (or
> 128M/64K == 2048 on 64k pages kernels). So I guess we'd still need a

Note that it's 4k BPF insns which do not map 1:1, if possible I'd actually
prefer if we could enlarge this space a bit.

> spillover window as well, in which case we'd need a fix for the
> BPF-to-BPF branching issue (but we need that at the moment anyway)

Yeah, or spillover window instead. I think as a fix starting out with
its own region without the spillover window would be less complex and
better suited for stable?

Thanks,
Daniel

[PATCH v2 2/2] arm64/bpf: don't allocate BPF JIT programs in module memory

[Daniel Borkmann]

On 11/22/2018 09:02 AM, Ard Biesheuvel wrote:
> On Thu, 22 Nov 2018 at 00:20, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 11/21/2018 02:17 PM, Ard Biesheuvel wrote:
>>> The arm64 module region is a 128 MB region that is kept close to
>>> the core kernel, in order to ensure that relative branches are
>>> always in range. So using the same region for programs that do
>>> not have this restriction is wasteful, and preferably avoided.
>>>
>>> Now that the core BPF JIT code permits the alloc/free routines to
>>> be overridden, implement them by simple vmalloc_exec()/vfree()
>>> calls, which can be served from anywere. This also solves an
>>> issue under KASAN, where shadow memory is needlessly allocated for
>>> all BPF programs (which don't require KASAN shadow pages since
>>> they are not KASAN instrumented)
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  arch/arm64/net/bpf_jit_comp.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
>>> index a6fdaea07c63..f91b7c157841 100644
>>> --- a/arch/arm64/net/bpf_jit_comp.c
>>> +++ b/arch/arm64/net/bpf_jit_comp.c
>>> @@ -940,3 +940,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>>>                                          tmp : orig_prog);
>>>       return prog;
>>>  }
>>> +
>>> +void *bpf_jit_alloc_exec(unsigned long size)
>>> +{
>>> +     return vmalloc_exec(size);
>>> +}
>>> +
>>> +void bpf_jit_free_exec(const void *addr)
>>> +{
>>> +     return vfree(size);
>>> +}
>>
>> Hmm, could you elaborate in the commit log on the potential performance
>> regression for JITed progs on arm64 after this change?
> 
> This does not affect the generated code, so I don't anticipate a
> performance hit. Did you have anything in particular in mind?

We do optimize immediate emission in the JIT, I was mostly wondering that
once the code is much further away from core kernel how much more insns we
might need to emit in some worst case for each BPF helper call, but then
unlike some other archs we always use absolute addresses so nothing would
change here, so never mind. (And BPF to BPF calls emits unoptimized 64
immediates since this is needed as we pass through the JIT several times
so we need this as place holder for later once the address is actually
known.)

>> I think this change would also break JITing of BPF to BPF calls. You might
>> have the same issue as ppc64 folks where the offset might not fit into imm
>> anymore and would have to transfer it via fp->aux->func[off]->bpf_func
>> instead.
> 
> If we are relying on BPF programs to remain within 128 MB of each
> other, then we already have a potential problem, given that the
> module_alloc() spills over into a 4 GB window if the 128 MB window is
> exhausted. Perhaps we should do something like

Hmm, good point, presumably you mean this one here fd045f6cd98e ("arm64:
add support for module PLTs"). Agree that this needs fixing.

> void *bpf_jit_alloc_exec(unsigned long size) {
>   return __vmalloc_node_range(size, MODULE_ALIGN,
>     BPF_REGION_START, BPF_REGION_END,
>     GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
>     __builtin_return_address(0));
> }
> 
> and make [BPF_REGION_START, BPF_REGION_END) a separate 128 MB window
> at the top of the vmalloc space. That way, it is guaranteed that BPF
> programs are within branching range of each other, and we still solve
> the original problem. I also like that it becomes impossible to infer
> anything about the state of the vmalloc space, placement of the kernel
> and modules etc from the placement of the BPF programs (in case it
> leaks this information in one way or the other)
> 
> That would only give you space for 128M/4K == 32768 programs (or
> 128M/64K == 2048 on 64k pages kernels). So I guess we'd still need a

Note that it's 4k BPF insns which do not map 1:1, if possible I'd actually
prefer if we could enlarge this space a bit.

> spillover window as well, in which case we'd need a fix for the
> BPF-to-BPF branching issue (but we need that at the moment anyway)

Yeah, or spillover window instead. I think as a fix starting out with
its own region without the spillover window would be less complex and
better suited for stable?

Thanks,
Daniel

Re: [PATCH v2 2/2] arm64/bpf: don't allocate BPF JIT programs in module memory

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3883 bytes --]

Hi Ard,

I love your patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on v4.20-rc3 next-20181123]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/bpf-permit-JIT-allocations-to-be-served-outside-the-module-region/20181123-033144
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   include/linux/slab.h:332:43: warning: dubious: x & !y
   kernel/bpf/core.c:625:6: warning: symbol 'bpf_jit_alloc_exec' was not declared. Should it be static?
>> kernel/bpf/core.c:632:31: error: undefined identifier 'size'
>> kernel/bpf/core.c:632:30: error: return expression in void function
   include/linux/slab.h:332:43: warning: dubious: x & !y
   kernel/bpf/core.c:1621:9: warning: incorrect type in argument 1 (different address spaces)
   include/linux/slab.h:332:43: warning: dubious: x & !y
   kernel/bpf/core.c:1695:44: warning: incorrect type in initializer (different address spaces)
   kernel/bpf/core.c:1719:26: warning: incorrect type in assignment (different address spaces)
   kernel/bpf/core.c:1753:26: warning: incorrect type in assignment (different address spaces)
   include/trace/events/xdp.h:28:1: warning: Using plain integer as NULL pointer
   include/trace/events/xdp.h:53:1: warning: Using plain integer as NULL pointer
   include/trace/events/xdp.h:111:1: warning: Using plain integer as NULL pointer
   include/trace/events/xdp.h:126:1: warning: Using plain integer as NULL pointer
   include/trace/events/xdp.h:161:1: warning: Using plain integer as NULL pointer
   include/trace/events/xdp.h:196:1: warning: Using plain integer as NULL pointer
   include/trace/events/xdp.h:231:1: warning: Using plain integer as NULL pointer
   kernel/bpf/core.c:1012:18: warning: Initializer entry defined twice
   kernel/bpf/core.c: In function 'bpf_jit_free_exec':
   kernel/bpf/core.c:632:24: error: 'size' undeclared (first use in this function); did you mean 'ksize'?
     return module_memfree(size);
                           ^~~~
                           ksize
   kernel/bpf/core.c:632:24: note: each undeclared identifier is reported only once for each function it appears in
   kernel/bpf/core.c:632:9: warning: 'return' with a value, in function returning void
     return module_memfree(size);
            ^~~~~~~~~~~~~~
   kernel/bpf/core.c:630:13: note: declared here
    void __weak bpf_jit_free_exec(const void *addr)
                ^~~~~~~~~~~~~~~~~

vim +/size +632 kernel/bpf/core.c

ede95a63b Daniel Borkmann 2018-10-23  624  
f65149135 Ard Biesheuvel  2018-11-21 @625  void *__weak bpf_jit_alloc_exec(unsigned long size)
f65149135 Ard Biesheuvel  2018-11-21  626  {
f65149135 Ard Biesheuvel  2018-11-21  627  	return module_alloc(size);
f65149135 Ard Biesheuvel  2018-11-21  628  }
f65149135 Ard Biesheuvel  2018-11-21  629  
f65149135 Ard Biesheuvel  2018-11-21  630  void __weak bpf_jit_free_exec(const void *addr)
f65149135 Ard Biesheuvel  2018-11-21  631  {
f65149135 Ard Biesheuvel  2018-11-21 @632  	return module_memfree(size);
f65149135 Ard Biesheuvel  2018-11-21  633  }
f65149135 Ard Biesheuvel  2018-11-21  634  

:::::: The code at line 632 was first introduced by commit
:::::: f6514913515dca448eb8ebd150918cc6d0a5515e bpf: add __weak hook for allocating executable memory

:::::: TO: Ard Biesheuvel <ard.biesheuvel@linaro.org>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66569 bytes --]

[PATCH v2 2/2] arm64/bpf: don't allocate BPF JIT programs in module memory

[kbuild test robot]

Hi Ard,

I love your patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on v4.20-rc3 next-20181123]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/bpf-permit-JIT-allocations-to-be-served-outside-the-module-region/20181123-033144
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   include/linux/slab.h:332:43: warning: dubious: x & !y
   kernel/bpf/core.c:625:6: warning: symbol 'bpf_jit_alloc_exec' was not declared. Should it be static?
>> kernel/bpf/core.c:632:31: error: undefined identifier 'size'
>> kernel/bpf/core.c:632:30: error: return expression in void function
   include/linux/slab.h:332:43: warning: dubious: x & !y
   kernel/bpf/core.c:1621:9: warning: incorrect type in argument 1 (different address spaces)
   include/linux/slab.h:332:43: warning: dubious: x & !y
   kernel/bpf/core.c:1695:44: warning: incorrect type in initializer (different address spaces)
   kernel/bpf/core.c:1719:26: warning: incorrect type in assignment (different address spaces)
   kernel/bpf/core.c:1753:26: warning: incorrect type in assignment (different address spaces)
   include/trace/events/xdp.h:28:1: warning: Using plain integer as NULL pointer
   include/trace/events/xdp.h:53:1: warning: Using plain integer as NULL pointer
   include/trace/events/xdp.h:111:1: warning: Using plain integer as NULL pointer
   include/trace/events/xdp.h:126:1: warning: Using plain integer as NULL pointer
   include/trace/events/xdp.h:161:1: warning: Using plain integer as NULL pointer
   include/trace/events/xdp.h:196:1: warning: Using plain integer as NULL pointer
   include/trace/events/xdp.h:231:1: warning: Using plain integer as NULL pointer
   kernel/bpf/core.c:1012:18: warning: Initializer entry defined twice
   kernel/bpf/core.c: In function 'bpf_jit_free_exec':
   kernel/bpf/core.c:632:24: error: 'size' undeclared (first use in this function); did you mean 'ksize'?
     return module_memfree(size);
                           ^~~~
                           ksize
   kernel/bpf/core.c:632:24: note: each undeclared identifier is reported only once for each function it appears in
   kernel/bpf/core.c:632:9: warning: 'return' with a value, in function returning void
     return module_memfree(size);
            ^~~~~~~~~~~~~~
   kernel/bpf/core.c:630:13: note: declared here
    void __weak bpf_jit_free_exec(const void *addr)
                ^~~~~~~~~~~~~~~~~

vim +/size +632 kernel/bpf/core.c

ede95a63b Daniel Borkmann 2018-10-23  624  
f65149135 Ard Biesheuvel  2018-11-21 @625  void *__weak bpf_jit_alloc_exec(unsigned long size)
f65149135 Ard Biesheuvel  2018-11-21  626  {
f65149135 Ard Biesheuvel  2018-11-21  627  	return module_alloc(size);
f65149135 Ard Biesheuvel  2018-11-21  628  }
f65149135 Ard Biesheuvel  2018-11-21  629  
f65149135 Ard Biesheuvel  2018-11-21  630  void __weak bpf_jit_free_exec(const void *addr)
f65149135 Ard Biesheuvel  2018-11-21  631  {
f65149135 Ard Biesheuvel  2018-11-21 @632  	return module_memfree(size);
f65149135 Ard Biesheuvel  2018-11-21  633  }
f65149135 Ard Biesheuvel  2018-11-21  634  

:::::: The code at line 632 was first introduced by commit
:::::: f6514913515dca448eb8ebd150918cc6d0a5515e bpf: add __weak hook for allocating executable memory

:::::: TO: Ard Biesheuvel <ard.biesheuvel@linaro.org>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 66569 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181124/65a7e461/attachment-0001.gz>