| Message ID | 20181117185715.25198-1-ard.biesheuvel@linaro.org |
|---|---|
| Headers | show |
| Series | bpf: permit JIT allocations to be served outside the module region | expand |
On Sat, Nov 17, 2018 at 6:58 PM Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. > > Patch #1 fixes a bug introduced during the merge window, where the new > alloc/free tracking does not account for memory that is freed by some > arch code. > > Patch #2 refactors the freeing path so that architectures can switch to > something other than module_memfree(). > > Patch #3 does the same for module_alloc(). > > Patch #4 implements the new alloc/free overrides for arm64 Except a minor comment, the whole patch set looks good to me. Acked-by: Yonghong Song <yhs@fb.com> > > 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: Ralf Baechle <ralf@linux-mips.org> > Cc: Paul Burton <paul.burton@mips.com> > Cc: James Hogan <jhogan@kernel.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-mips@linux-mips.org > Cc: linuxppc-dev@lists.ozlabs.org > Cc: sparclinux@vger.kernel.org > Cc: netdev@vger.kernel.org > > Ard Biesheuvel (4): > bpf: account for freed JIT allocations in arch code > net/bpf: refactor freeing of executable allocations > 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 | 11 ++++++++++ > arch/mips/net/bpf_jit.c | 7 ++----- > arch/powerpc/net/bpf_jit_comp.c | 7 ++----- > arch/powerpc/net/bpf_jit_comp64.c | 12 +++-------- > arch/sparc/net/bpf_jit_comp_32.c | 7 ++----- > kernel/bpf/core.c | 22 ++++++++++---------- > 6 files changed, 31 insertions(+), 35 deletions(-) > > -- > 2.17.1 >
On 11/17/2018 07:57 PM, Ard Biesheuvel wrote: > Commit ede95a63b5e84 ("bpf: add bpf_jit_limit knob to restrict unpriv > allocations") added a call to bpf_jit_uncharge_modmem() to the routine > bpf_jit_binary_free() which is called from the __weak bpf_jit_free(). > This function is overridden by arches, some of which do not call > bpf_jit_binary_free() to release the memory, and so the released > memory is not accounted for, potentially leading to spurious allocation > failures. > > So replace the direct calls to module_memfree() in the arch code with > calls to bpf_jit_binary_free(). Sorry but this patch is completely buggy, and above description on the accounting incorrect as well. Looks like this patch was not tested at all. The below cBPF JITs that use module_memfree() which you replace with bpf_jit_binary_free() are using module_alloc() internally to get the JIT image buffer ... > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/mips/net/bpf_jit.c | 2 +- > arch/powerpc/net/bpf_jit_comp.c | 2 +- > arch/powerpc/net/bpf_jit_comp64.c | 5 +---- > arch/sparc/net/bpf_jit_comp_32.c | 2 +- > 4 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c > index 4d8cb9bb8365..1b69897274a1 100644 > --- a/arch/mips/net/bpf_jit.c > +++ b/arch/mips/net/bpf_jit.c > @@ -1264,7 +1264,7 @@ void bpf_jit_compile(struct bpf_prog *fp) > void bpf_jit_free(struct bpf_prog *fp) > { > if (fp->jited) > - module_memfree(fp->bpf_func); > + bpf_jit_binary_free(bpf_jit_binary_hdr(fp)); > > bpf_prog_unlock_free(fp); > } > diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c > index d5bfe24bb3b5..a1ea1ea6b40d 100644 > --- a/arch/powerpc/net/bpf_jit_comp.c > +++ b/arch/powerpc/net/bpf_jit_comp.c > @@ -683,7 +683,7 @@ void bpf_jit_compile(struct bpf_prog *fp) > void bpf_jit_free(struct bpf_prog *fp) > { > if (fp->jited) > - module_memfree(fp->bpf_func); > + bpf_jit_binary_free(bpf_jit_binary_hdr(fp)); > > bpf_prog_unlock_free(fp); > } > diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c > index 50b129785aee..84c8f013a6c6 100644 > --- a/arch/powerpc/net/bpf_jit_comp64.c > +++ b/arch/powerpc/net/bpf_jit_comp64.c > @@ -1024,11 +1024,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp) > /* Overriding bpf_jit_free() as we don't set images read-only. */ > void bpf_jit_free(struct bpf_prog *fp) > { > - unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK; > - struct bpf_binary_header *bpf_hdr = (void *)addr; > - > if (fp->jited) > - bpf_jit_binary_free(bpf_hdr); > + bpf_jit_binary_free(bpf_jit_binary_hdr(fp)); > > bpf_prog_unlock_free(fp); > } > diff --git a/arch/sparc/net/bpf_jit_comp_32.c b/arch/sparc/net/bpf_jit_comp_32.c > index a5ff88643d5c..01bda6bc9e7f 100644 > --- a/arch/sparc/net/bpf_jit_comp_32.c > +++ b/arch/sparc/net/bpf_jit_comp_32.c > @@ -759,7 +759,7 @@ cond_branch: f_offset = addrs[i + filter[i].jf]; > void bpf_jit_free(struct bpf_prog *fp) > { > if (fp->jited) > - module_memfree(fp->bpf_func); > + bpf_jit_binary_free(bpf_jit_binary_hdr(fp)); > > bpf_prog_unlock_free(fp); > } >