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 <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Rick Edgecombe <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jann Horn <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Jessica Yu <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
On Wed, 21 Nov 2018 at 20:48, Edgecombe, Rick P
<[email protected]> 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.
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 <[email protected]>
> ---
> 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.
On Thu, 22 Nov 2018 at 00:20, Daniel Borkmann <[email protected]> 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 <[email protected]>
> > ---
> > 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)
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
> On 22 Nov 2018, at 21:01, kbuild test robot <[email protected]> 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>
On 11/22/2018 09:02 AM, Ard Biesheuvel wrote:
> On Thu, 22 Nov 2018 at 00:20, Daniel Borkmann <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
:::::: CC: 0day robot <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation