From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-16 08:51:05
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
Hi,
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystmes
that need to allocate code, such as ftrace, kprobes and BPF to modules and
puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
A centralized infrastructure for code allocation allows allocations of
executable memory as ROX, and future optimizations such as caching large
pages for better iTLB performance and providing sub-page allocations for
users that only need small jit code snippets.
Rick Edgecombe proposed perm_alloc extension to vmalloc [1] and Song Liu
proposed execmem_alloc [2], but both these approaches were targeting BPF
allocations and lacked the ground work to abstract executable allocations
and split them from the modules core.
Thomas Gleixner's suggested to express module allocation restrictions and
requirements as struct mod_alloc_type_params [3] that would define ranges,
protections and other parameters for different types of allocations used by
modules and following that suggestion Song separated allocations of
different types in modules (commit ac3b43283923 ("module: replace
module_layout with module_memory")) and posted "Type aware module
allocator" set [4].
I liked the idea of parametrising code allocation requirements as a
structure, but I believe the original proposal and Song's module allocator
were too module centric, so I came up with these patches.
This set splits code allocation from modules by introducing
execmem_text_alloc(), execmem_data_alloc(), execmem_free(),
jit_text_alloc() and jit_free() APIs, replaces call sites of module_alloc()
and module_memfree() with the new APIs and implements core text and related
allocation in a central place.
Instead of architecture specific overrides for module_alloc(), the
architectures that require non-default behaviour for text allocation must
fill execmem_alloc_params structure and implement execmem_arch_params()
that returns a pointer to that structure. If an architecture does not
implement execmem_arch_params(), the defaults compatible with the current
modules::module_alloc() are used.
The intended semantics of the new APIs is that execmem APIs should be used
to allocate memory that must reside close to the kernel image because of
addressing mode restrictions, e.g modules on many architectures or dynamic
ftrace trampolines on x86.
The jit APIs are intended for users that can place code anywhere in vmalloc
area, like kprobes on most architectures and BPF on arm/arm64.
While two distinct API cover the major cases, there is still might be need
for arch-specific overrides for some of the usecases. For example, riscv
uses a dedicated range for BPF allocations in order to be able to use
relative addressing, but for kprobes riscv can use the entire vmalloc area.
For such overrides we might introduce jit_text_alloc variant that gets
start + end parameters to restrict the range like Mark Rutland suggested
and then use that variant in arch override.
The new infrastructure allows decoupling of kprobes and ftrace from
modules, and most importantly it paves the way for ROX allocations for
executable memory.
For now I've dropped patches that enable ROX allocations on x86 because
with them modprobe takes ten times more. To make modprobe fast with ROX
allocations more work is required to text poking infrastructure, but this
work is not a prerequisite for this series.
[1] https://lore.kernel.org/lkml/20201120202426.18009-1-rick.p.edgecombe@intel.com/
[2] https://lore.kernel.org/all/20221107223921.3451913-1-song@kernel.org/
[3] https://lore.kernel.org/all/87v8mndy3y.ffs@tglx/
[4] https://lore.kernel.org/all/20230526051529.3387103-1-song@kernel.org
v2 changes:
* Separate "module" and "others" allocations with execmem_text_alloc()
and jit_text_alloc()
* Drop ROX entablement on x86
* Add ack for nios2 changes, thanks Dinh Nguyen
v1: https://lore.kernel.org/all/20230601101257.530867-1-rppt@kernel.org
Mike Rapoport (IBM) (12):
nios2: define virtual address space for modules
mm: introduce execmem_text_alloc() and jit_text_alloc()
mm/execmem, arch: convert simple overrides of module_alloc to execmem
mm/execmem, arch: convert remaining overrides of module_alloc to execmem
modules, execmem: drop module_alloc
mm/execmem: introduce execmem_data_alloc()
arm64, execmem: extend execmem_params for generated code definitions
riscv: extend execmem_params for kprobes allocations
powerpc: extend execmem_params for kprobes allocations
arch: make execmem setup available regardless of CONFIG_MODULES
x86/ftrace: enable dynamic ftrace without CONFIG_MODULES
kprobes: remove dependcy on CONFIG_MODULES
arch/Kconfig | 2 +-
arch/arm/kernel/module.c | 32 ------
arch/arm/mm/init.c | 36 ++++++
arch/arm64/include/asm/memory.h | 8 ++
arch/arm64/include/asm/module.h | 6 -
arch/arm64/kernel/kaslr.c | 3 +-
arch/arm64/kernel/module.c | 47 --------
arch/arm64/kernel/probes/kprobes.c | 7 --
arch/arm64/mm/init.c | 56 +++++++++
arch/loongarch/kernel/module.c | 6 -
arch/loongarch/mm/init.c | 20 ++++
arch/mips/kernel/module.c | 10 +-
arch/mips/mm/init.c | 19 ++++
arch/nios2/include/asm/pgtable.h | 5 +-
arch/nios2/kernel/module.c | 28 +++--
arch/parisc/kernel/module.c | 12 +-
arch/parisc/mm/init.c | 22 +++-
arch/powerpc/kernel/kprobes.c | 16 +--
arch/powerpc/kernel/module.c | 37 ------
arch/powerpc/mm/mem.c | 59 ++++++++++
arch/riscv/kernel/module.c | 10 --
arch/riscv/kernel/probes/kprobes.c | 10 --
arch/riscv/mm/init.c | 34 ++++++
arch/s390/kernel/ftrace.c | 4 +-
arch/s390/kernel/kprobes.c | 4 +-
arch/s390/kernel/module.c | 42 +------
arch/s390/mm/init.c | 41 +++++++
arch/sparc/kernel/module.c | 33 +-----
arch/sparc/mm/Makefile | 2 +
arch/sparc/mm/execmem.c | 25 ++++
arch/sparc/net/bpf_jit_comp_32.c | 8 +-
arch/x86/Kconfig | 1 +
arch/x86/kernel/ftrace.c | 16 +--
arch/x86/kernel/kprobes/core.c | 4 +-
arch/x86/kernel/module.c | 51 ---------
arch/x86/mm/init.c | 54 +++++++++
include/linux/execmem.h | 155 +++++++++++++++++++++++++
include/linux/moduleloader.h | 15 ---
kernel/bpf/core.c | 14 +--
kernel/kprobes.c | 51 +++++----
kernel/module/Kconfig | 1 +
kernel/module/main.c | 45 ++------
kernel/trace/trace_kprobe.c | 11 ++
mm/Kconfig | 3 +
mm/Makefile | 1 +
mm/execmem.c | 177 +++++++++++++++++++++++++++++
mm/mm_init.c | 2 +
47 files changed, 813 insertions(+), 432 deletions(-)
create mode 100644 arch/sparc/mm/execmem.c
create mode 100644 include/linux/execmem.h
create mode 100644 mm/execmem.c
base-commit: 44c026a73be8038f03dbdeef028b642880cf1511
--
2.35.1
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-16 08:51:20
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
nios2 uses kmalloc() to implement module_alloc() because CALL26/PCREL26
cannot reach all of vmalloc address space.
Define module space as 32MiB below the kernel base and switch nios2 to
use vmalloc for module allocations.
Suggested-by: Thomas Gleixner <redacted>
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
Acked-by: Dinh Nguyen <dinguyen@kernel.org>
---
arch/nios2/include/asm/pgtable.h | 5 ++++-
arch/nios2/kernel/module.c | 19 ++++---------------
2 files changed, 8 insertions(+), 16 deletions(-)
@@ -21,23 +21,12 @@#include<asm/cacheflush.h>-/*-*ModulesshouldNOTbeallocatedwithkmallocfor(obvious)reasons.-*Butwedoitfornowtoavoidrelocationissues.CALL26/PCREL26cannotreach-*from0x80000000(vmallocarea)to0xc00000000(kernel)(kmallocreturns-*addressesin0xc0000000)-*/void*module_alloc(unsignedlongsize){-if(size==0)-returnNULL;-returnkmalloc(size,GFP_KERNEL);-}--/* Free memory returned from module_alloc */-voidmodule_memfree(void*module_region)-{-kfree(module_region);+return__vmalloc_node_range(size,1,MODULES_VADDR,MODULES_END,+GFP_KERNEL,PAGE_KERNEL_EXEC,+VM_FLUSH_RESET_PERMS,NUMA_NO_NODE,+__builtin_return_address(0));}intapply_relocate_add(Elf32_Shdr*sechdrs,constchar*strtab,
I wonder if the (size == 0) check is really needed, but
__vmalloc_node_range() will WARN on this case where the old code won't.
module_alloc() should not be called with zero size, so a warning there
would be appropriate.
Besides, no other module_alloc() had this check.
--
Sincerely yours,
Mike.
From: Song Liu <song@kernel.org> Date: 2023-06-16 18:14:58
On Fri, Jun 16, 2023 at 1:51 AM Mike Rapoport [off-list ref] wrote:
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
nios2 uses kmalloc() to implement module_alloc() because CALL26/PCREL26
cannot reach all of vmalloc address space.
Define module space as 32MiB below the kernel base and switch nios2 to
use vmalloc for module allocations.
Suggested-by: Thomas Gleixner <redacted>
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
Acked-by: Dinh Nguyen <dinguyen@kernel.org>
@@ -21,23 +21,12 @@#include<asm/cacheflush.h>-/*-*ModulesshouldNOTbeallocatedwithkmallocfor(obvious)reasons.-*Butwedoitfornowtoavoidrelocationissues.CALL26/PCREL26cannotreach-*from0x80000000(vmallocarea)to0xc00000000(kernel)(kmallocreturns-*addressesin0xc0000000)-*/void*module_alloc(unsignedlongsize){-if(size==0)-returnNULL;-returnkmalloc(size,GFP_KERNEL);-}--/* Free memory returned from module_alloc */-voidmodule_memfree(void*module_region)-{-kfree(module_region);+return__vmalloc_node_range(size,1,MODULES_VADDR,MODULES_END,+GFP_KERNEL,PAGE_KERNEL_EXEC,+VM_FLUSH_RESET_PERMS,NUMA_NO_NODE,+__builtin_return_address(0));}intapply_relocate_add(Elf32_Shdr*sechdrs,constchar*strtab,--
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-16 08:51:38
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
The names execmem_text_alloc() and jit_text_alloc() emphasize that the
allocated memory is for executable code, the allocations of the
associated data, like data sections of a module will use
execmem_data_alloc() interface that will be added later.
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
---
arch/powerpc/kernel/kprobes.c | 4 +--
arch/s390/kernel/ftrace.c | 4 +--
arch/s390/kernel/kprobes.c | 4 +--
arch/s390/kernel/module.c | 5 +--
arch/sparc/net/bpf_jit_comp_32.c | 8 ++---
arch/x86/kernel/ftrace.c | 6 ++--
arch/x86/kernel/kprobes/core.c | 4 +--
include/linux/execmem.h | 52 ++++++++++++++++++++++++++++++++
include/linux/moduleloader.h | 3 --
kernel/bpf/core.c | 14 ++++-----
kernel/kprobes.c | 8 ++---
kernel/module/Kconfig | 1 +
kernel/module/main.c | 25 +++++----------
mm/Kconfig | 3 ++
mm/Makefile | 1 +
mm/execmem.c | 36 ++++++++++++++++++++++
16 files changed, 130 insertions(+), 48 deletions(-)
create mode 100644 include/linux/execmem.h
create mode 100644 mm/execmem.c
@@ -261,15 +262,14 @@ void arch_ftrace_update_code(int command)#ifdef CONFIG_X86_64#ifdef CONFIG_MODULES-#include<linux/moduleloader.h>/* Module allocation simplifies allocating memory for code */staticinlinevoid*alloc_tramp(unsignedlongsize){-returnmodule_alloc(size);+returnexecmem_text_alloc(size);}staticinlinevoidtramp_free(void*tramp){-module_memfree(tramp);+execmem_free(tramp);}#else/* Trampolines can only be created if modules are supported */
@@ -29,9 +29,6 @@ unsigned int arch_mod_section_prepend(struct module *mod, unsigned int section);sections.ReturnsNULLonfailure.*/void*module_alloc(unsignedlongsize);-/* Free memory returned from module_alloc. */-voidmodule_memfree(void*module_region);-/* Determines if the section name is an init section (that is only used during*moduleloading).*/
From: Kent Overstreet <kent.overstreet@linux.dev> Date: 2023-06-16 16:48:20
On Fri, Jun 16, 2023 at 11:50:28AM +0300, Mike Rapoport wrote:
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
The names execmem_text_alloc() and jit_text_alloc() emphasize that the
allocated memory is for executable code, the allocations of the
associated data, like data sections of a module will use
execmem_data_alloc() interface that will be added later.
I like the API split - at the risk of further bikeshedding, perhaps
near_text_alloc() and far_text_alloc()? Would be more explicit.
Reviewed-by: Kent Overstreet <kent.overstreet@linux.dev>
From: Song Liu <song@kernel.org> Date: 2023-06-16 18:19:02
On Fri, Jun 16, 2023 at 9:48 AM Kent Overstreet
[off-list ref] wrote:
On Fri, Jun 16, 2023 at 11:50:28AM +0300, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
The names execmem_text_alloc() and jit_text_alloc() emphasize that the
allocated memory is for executable code, the allocations of the
associated data, like data sections of a module will use
execmem_data_alloc() interface that will be added later.
I like the API split - at the risk of further bikeshedding, perhaps
near_text_alloc() and far_text_alloc()? Would be more explicit.
Reviewed-by: Kent Overstreet <kent.overstreet@linux.dev>
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-17 05:58:28
On Fri, Jun 16, 2023 at 12:48:02PM -0400, Kent Overstreet wrote:
On Fri, Jun 16, 2023 at 11:50:28AM +0300, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
The names execmem_text_alloc() and jit_text_alloc() emphasize that the
allocated memory is for executable code, the allocations of the
associated data, like data sections of a module will use
execmem_data_alloc() interface that will be added later.
I like the API split - at the risk of further bikeshedding, perhaps
near_text_alloc() and far_text_alloc()? Would be more explicit.
With near and far it should mention from where and that's getting too long.
I don't mind changing the names, but I couldn't think about something
better than Song's execmem and your jit.
Reviewed-by: Kent Overstreet <kent.overstreet@linux.dev>
On Fri, Jun 16, 2023, at 1:50 AM, Mike Rapoport wrote:
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-18 08:01:21
On Sat, Jun 17, 2023 at 01:38:29PM -0700, Andy Lutomirski wrote:
On Fri, Jun 16, 2023, at 1:50 AM, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
Is there anything in this series to help users do the appropriate
synchronization when the actually populate the allocated memory with
code? See here, for example:
This series only factors out the executable allocations from modules and
puts them in a central place.
Anything else would go on top after this lands.
On Sun, Jun 18, 2023, at 1:00 AM, Mike Rapoport wrote:
On Sat, Jun 17, 2023 at 01:38:29PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Jun 16, 2023, at 1:50 AM, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
Is there anything in this series to help users do the appropriate
synchronization when the actually populate the allocated memory with
code? See here, for example:
This series only factors out the executable allocations from modules and
puts them in a central place.
Anything else would go on top after this lands.
Hmm.
On the one hand, there's nothing wrong with factoring out common code. On the other hand, this is probably the right time to at least start thinking about synchronization, at least to the extent that it might make us want to change this API. (I'm not at all saying that this series should require changes -- I'm just saying that this is a good time to think about how this should work.)
The current APIs, *and* the proposed jit_text_alloc() API, don't actually look like the one think in the Linux ecosystem that actually intelligently and efficiently maps new text into an address space: mmap().
On x86, you can mmap() an existing file full of executable code PROT_EXEC and jump to it with minimal synchronization (just the standard implicit ordering in the kernel that populates the pages before setting up the PTEs and whatever user synchronization is needed to avoid jumping into the mapping before mmap() finishes). It works across CPUs, and the only possible way userspace can screw it up (for a read-only mapping of read-only text, anyway) is to jump to the mapping too early, in which case userspace gets a page fault. Incoherence is impossible, and no one needs to "serialize" (in the SDM sense).
I think the same sequence (from userspace's perspective) works on other architectures, too, although I think more cache management is needed on the kernel's end. As far as I know, no Linux SMP architecture needs an IPI to map executable text into usermode, but I could easily be wrong. (IIRC RISC-V has very developer-unfriendly icache management, but I don't remember the details.)
Of course, using ptrace or any other FOLL_FORCE to modify text on x86 is rather fraught, and I bet many things do it wrong when userspace is multithreaded. But not in production because it's mostly not used in production.)
But jit_text_alloc() can't do this, because the order of operations doesn't match. With jit_text_alloc(), the executable mapping shows up before the text is populated, so there is no atomic change from not-there to populated-and-executable. Which means that there is an opportunity for CPUs, speculatively or otherwise, to start filling various caches with intermediate states of the text, which means that various architectures (even x86!) may need serialization.
For eBPF- and module- like use cases, where JITting/code gen is quite coarse-grained, perhaps something vaguely like:
jit_text_alloc() -> returns a handle and an executable virtual address, but does *not* map it there
jit_text_write() -> write to that handle
jit_text_map() -> map it and synchronize if needed (no sync needed on x86, I think)
could be more efficient and/or safer.
(Modules could use this too. Getting alternatives right might take some fiddling, because off the top of my head, this doesn't match how it works now.)
To make alternatives easier, this could work, maybe (haven't fully thought it through):
jit_text_alloc()
jit_text_map_rw_inplace() -> map at the target address, but RW, !X
write the text and apply alternatives
jit_text_finalize() -> change from RW to RX *and synchronize*
jit_text_finalize() would either need to wait for RCU (possibly extra heavy weight RCU to get "serialization") or send an IPI.
This is slower than the alloc, write, map solution, but allows alternatives to be applied at the final address.
Even fancier variants where the writing is some using something like use_temporary_mm() might even make sense.
To what extent does performance matter for the various users? module loading is slow, and I don't think we care that much. eBPF loaded is not super fast, and we care to a limited extent. I *think* the bcachefs use case needs to be very fast, but I'm not sure it can be fast and supportable.
Anyway, food for thought.
From: Nadav Amit <hidden> Date: 2023-06-19 20:18:45
On Jun 19, 2023, at 10:09 AM, Andy Lutomirski [off-list ref] wrote:
But jit_text_alloc() can't do this, because the order of operations doesn't match. With jit_text_alloc(), the executable mapping shows up before the text is populated, so there is no atomic change from not-there to populated-and-executable. Which means that there is an opportunity for CPUs, speculatively or otherwise, to start filling various caches with intermediate states of the text, which means that various architectures (even x86!) may need serialization.
For eBPF- and module- like use cases, where JITting/code gen is quite coarse-grained, perhaps something vaguely like:
jit_text_alloc() -> returns a handle and an executable virtual address, but does *not* map it there
jit_text_write() -> write to that handle
jit_text_map() -> map it and synchronize if needed (no sync needed on x86, I think)
Andy, would you mind explaining why you think a sync is not needed? I mean I have a “feeling” that perhaps TSO can guarantee something based on the order of write and page-table update. Is that the argument?
On this regard, one thing that I clearly do not understand is why *today* it is ok for users of bpf_arch_text_copy() not to call text_poke_sync(). Am I missing something?
On Mon, Jun 19, 2023, at 1:18 PM, Nadav Amit wrote:
quoted
On Jun 19, 2023, at 10:09 AM, Andy Lutomirski [off-list ref] wrote:
But jit_text_alloc() can't do this, because the order of operations doesn't match. With jit_text_alloc(), the executable mapping shows up before the text is populated, so there is no atomic change from not-there to populated-and-executable. Which means that there is an opportunity for CPUs, speculatively or otherwise, to start filling various caches with intermediate states of the text, which means that various architectures (even x86!) may need serialization.
For eBPF- and module- like use cases, where JITting/code gen is quite coarse-grained, perhaps something vaguely like:
jit_text_alloc() -> returns a handle and an executable virtual address, but does *not* map it there
jit_text_write() -> write to that handle
jit_text_map() -> map it and synchronize if needed (no sync needed on x86, I think)
Andy, would you mind explaining why you think a sync is not needed? I
mean I have a “feeling” that perhaps TSO can guarantee something based
on the order of write and page-table update. Is that the argument?
Sorry, when I say "no sync" I mean no cross-CPU synchronization. I'm assuming the underlying sequence of events is:
allocate physical pages (jit_text_alloc)
write to them (with MOV, memcpy, whatever), via the direct map or via a temporary mm
do an appropriate *local* barrier (which, on x86, is probably implied by TSO, as the subsequent pagetable change is at least a release; also, any any previous temporary mm stuff would have done MOV CR3 afterwards, which is a full "serializing" barrier)
optionally zap the direct map via IPI, assuming the pages are direct mapped (but this could be avoided with a smart enough allocator and temporary_mm above)
install the final RX PTE (jit_text_map), which does a MOV or maybe a LOCK CMPXCHG16B. Note that the virtual address in question was not readable or executable before this, and all CPUs have serialized since the last time it was executable.
either jump to the new text locally, or:
1. Do a store-release to tell other CPUs that the text is mapped
2. Other CPU does a load-acquire to detect that the text is mapped and jumps to the text
This is all approximately the same thing that plain old mmap(..., PROT_EXEC, ...) does.
On this regard, one thing that I clearly do not understand is why
*today* it is ok for users of bpf_arch_text_copy() not to call
text_poke_sync(). Am I missing something?
I cannot explain this, because I suspect the current code is wrong. But it's only wrong across CPUs, because bpf_arch_text_copy goes through text_poke_copy, which calls unuse_temporary_mm(), which is serializing. And it's plausible that most eBPF use cases don't actually cause the loaded program to get used on a different CPU without first serializing on the CPU that ends up using it. (Context switches and interrupts are serializing.)
FRED could make interrupts non-serializing. I sincerely hope that FRED doesn't cause this all to fall apart.
--Andy
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-25 16:15:21
On Mon, Jun 19, 2023 at 10:09:02AM -0700, Andy Lutomirski wrote:
On Sun, Jun 18, 2023, at 1:00 AM, Mike Rapoport wrote:
quoted
On Sat, Jun 17, 2023 at 01:38:29PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Jun 16, 2023, at 1:50 AM, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
Is there anything in this series to help users do the appropriate
synchronization when the actually populate the allocated memory with
code? See here, for example:
This series only factors out the executable allocations from modules and
puts them in a central place.
Anything else would go on top after this lands.
Hmm.
On the one hand, there's nothing wrong with factoring out common code. On
the other hand, this is probably the right time to at least start
thinking about synchronization, at least to the extent that it might make
us want to change this API. (I'm not at all saying that this series
should require changes -- I'm just saying that this is a good time to
think about how this should work.)
The current APIs, *and* the proposed jit_text_alloc() API, don't actually
look like the one think in the Linux ecosystem that actually
intelligently and efficiently maps new text into an address space:
mmap().
On x86, you can mmap() an existing file full of executable code PROT_EXEC
and jump to it with minimal synchronization (just the standard implicit
ordering in the kernel that populates the pages before setting up the
PTEs and whatever user synchronization is needed to avoid jumping into
the mapping before mmap() finishes). It works across CPUs, and the only
possible way userspace can screw it up (for a read-only mapping of
read-only text, anyway) is to jump to the mapping too early, in which
case userspace gets a page fault. Incoherence is impossible, and no one
needs to "serialize" (in the SDM sense).
I think the same sequence (from userspace's perspective) works on other
architectures, too, although I think more cache management is needed on
the kernel's end. As far as I know, no Linux SMP architecture needs an
IPI to map executable text into usermode, but I could easily be wrong.
(IIRC RISC-V has very developer-unfriendly icache management, but I don't
remember the details.)
Of course, using ptrace or any other FOLL_FORCE to modify text on x86 is
rather fraught, and I bet many things do it wrong when userspace is
multithreaded. But not in production because it's mostly not used in
production.)
But jit_text_alloc() can't do this, because the order of operations
doesn't match. With jit_text_alloc(), the executable mapping shows up
before the text is populated, so there is no atomic change from not-there
to populated-and-executable. Which means that there is an opportunity
for CPUs, speculatively or otherwise, to start filling various caches
with intermediate states of the text, which means that various
architectures (even x86!) may need serialization.
For eBPF- and module- like use cases, where JITting/code gen is quite
coarse-grained, perhaps something vaguely like:
jit_text_alloc() -> returns a handle and an executable virtual address,
but does *not* map it there
jit_text_write() -> write to that handle
jit_text_map() -> map it and synchronize if needed (no sync needed on
x86, I think)
could be more efficient and/or safer.
(Modules could use this too. Getting alternatives right might take some
fiddling, because off the top of my head, this doesn't match how it works
now.)
To make alternatives easier, this could work, maybe (haven't fully
thought it through):
jit_text_alloc()
jit_text_map_rw_inplace() -> map at the target address, but RW, !X
write the text and apply alternatives
jit_text_finalize() -> change from RW to RX *and synchronize*
jit_text_finalize() would either need to wait for RCU (possibly extra
heavy weight RCU to get "serialization") or send an IPI.
This essentially how modules work now. The memory is allocated RW, written
and updated with alternatives and then made ROX in the end with set_memory
APIs.
The issue with not having the memory mapped X when it's written is that we
cannot use large pages to map it. One of the goals is to have executable
memory mapped with large pages and make code allocator able to divide that
page among several callers.
So the idea was that jit_text_alloc() will have a cache of large pages
mapped ROX, will allocate memory from those caches and there will be
jit_update() that uses text poking for writing to that memory.
Upon allocation of a large page to increase the cache, that large page will
be "invalidated" by filling it with breakpoint instructions (e.g int3 on
x86)
To improve the performance of this process, we can write to !X copy and
then text_poke it to the actual address in one go. This will require some
changes to get the alternatives right.
--
Sincerely yours,
Mike.
On Sun, Jun 25, 2023, at 9:14 AM, Mike Rapoport wrote:
On Mon, Jun 19, 2023 at 10:09:02AM -0700, Andy Lutomirski wrote:
quoted
On Sun, Jun 18, 2023, at 1:00 AM, Mike Rapoport wrote:
quoted
On Sat, Jun 17, 2023 at 01:38:29PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Jun 16, 2023, at 1:50 AM, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
Is there anything in this series to help users do the appropriate
synchronization when the actually populate the allocated memory with
code? See here, for example:
This series only factors out the executable allocations from modules and
puts them in a central place.
Anything else would go on top after this lands.
Hmm.
On the one hand, there's nothing wrong with factoring out common code. On
the other hand, this is probably the right time to at least start
thinking about synchronization, at least to the extent that it might make
us want to change this API. (I'm not at all saying that this series
should require changes -- I'm just saying that this is a good time to
think about how this should work.)
The current APIs, *and* the proposed jit_text_alloc() API, don't actually
look like the one think in the Linux ecosystem that actually
intelligently and efficiently maps new text into an address space:
mmap().
On x86, you can mmap() an existing file full of executable code PROT_EXEC
and jump to it with minimal synchronization (just the standard implicit
ordering in the kernel that populates the pages before setting up the
PTEs and whatever user synchronization is needed to avoid jumping into
the mapping before mmap() finishes). It works across CPUs, and the only
possible way userspace can screw it up (for a read-only mapping of
read-only text, anyway) is to jump to the mapping too early, in which
case userspace gets a page fault. Incoherence is impossible, and no one
needs to "serialize" (in the SDM sense).
I think the same sequence (from userspace's perspective) works on other
architectures, too, although I think more cache management is needed on
the kernel's end. As far as I know, no Linux SMP architecture needs an
IPI to map executable text into usermode, but I could easily be wrong.
(IIRC RISC-V has very developer-unfriendly icache management, but I don't
remember the details.)
Of course, using ptrace or any other FOLL_FORCE to modify text on x86 is
rather fraught, and I bet many things do it wrong when userspace is
multithreaded. But not in production because it's mostly not used in
production.)
But jit_text_alloc() can't do this, because the order of operations
doesn't match. With jit_text_alloc(), the executable mapping shows up
before the text is populated, so there is no atomic change from not-there
to populated-and-executable. Which means that there is an opportunity
for CPUs, speculatively or otherwise, to start filling various caches
with intermediate states of the text, which means that various
architectures (even x86!) may need serialization.
For eBPF- and module- like use cases, where JITting/code gen is quite
coarse-grained, perhaps something vaguely like:
jit_text_alloc() -> returns a handle and an executable virtual address,
but does *not* map it there
jit_text_write() -> write to that handle
jit_text_map() -> map it and synchronize if needed (no sync needed on
x86, I think)
could be more efficient and/or safer.
(Modules could use this too. Getting alternatives right might take some
fiddling, because off the top of my head, this doesn't match how it works
now.)
To make alternatives easier, this could work, maybe (haven't fully
thought it through):
jit_text_alloc()
jit_text_map_rw_inplace() -> map at the target address, but RW, !X
write the text and apply alternatives
jit_text_finalize() -> change from RW to RX *and synchronize*
jit_text_finalize() would either need to wait for RCU (possibly extra
heavy weight RCU to get "serialization") or send an IPI.
This essentially how modules work now. The memory is allocated RW, written
and updated with alternatives and then made ROX in the end with set_memory
APIs.
The issue with not having the memory mapped X when it's written is that we
cannot use large pages to map it. One of the goals is to have executable
memory mapped with large pages and make code allocator able to divide that
page among several callers.
So the idea was that jit_text_alloc() will have a cache of large pages
mapped ROX, will allocate memory from those caches and there will be
jit_update() that uses text poking for writing to that memory.
Upon allocation of a large page to increase the cache, that large page will
be "invalidated" by filling it with breakpoint instructions (e.g int3 on
x86)
Is this actually valid? In between int3 and real code, there’s a potential torn read of real code mixed up with 0xcc.
To improve the performance of this process, we can write to !X copy and
then text_poke it to the actual address in one go. This will require some
changes to get the alternatives right.
--
Sincerely yours,
Mike.
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-25 17:44:30
On Sun, Jun 25, 2023 at 09:59:34AM -0700, Andy Lutomirski wrote:
On Sun, Jun 25, 2023, at 9:14 AM, Mike Rapoport wrote:
quoted
On Mon, Jun 19, 2023 at 10:09:02AM -0700, Andy Lutomirski wrote:
quoted
On Sun, Jun 18, 2023, at 1:00 AM, Mike Rapoport wrote:
quoted
On Sat, Jun 17, 2023 at 01:38:29PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Jun 16, 2023, at 1:50 AM, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
Is there anything in this series to help users do the appropriate
synchronization when the actually populate the allocated memory with
code? See here, for example:
This series only factors out the executable allocations from modules and
puts them in a central place.
Anything else would go on top after this lands.
Hmm.
On the one hand, there's nothing wrong with factoring out common code. On
the other hand, this is probably the right time to at least start
thinking about synchronization, at least to the extent that it might make
us want to change this API. (I'm not at all saying that this series
should require changes -- I'm just saying that this is a good time to
think about how this should work.)
The current APIs, *and* the proposed jit_text_alloc() API, don't actually
look like the one think in the Linux ecosystem that actually
intelligently and efficiently maps new text into an address space:
mmap().
On x86, you can mmap() an existing file full of executable code PROT_EXEC
and jump to it with minimal synchronization (just the standard implicit
ordering in the kernel that populates the pages before setting up the
PTEs and whatever user synchronization is needed to avoid jumping into
the mapping before mmap() finishes). It works across CPUs, and the only
possible way userspace can screw it up (for a read-only mapping of
read-only text, anyway) is to jump to the mapping too early, in which
case userspace gets a page fault. Incoherence is impossible, and no one
needs to "serialize" (in the SDM sense).
I think the same sequence (from userspace's perspective) works on other
architectures, too, although I think more cache management is needed on
the kernel's end. As far as I know, no Linux SMP architecture needs an
IPI to map executable text into usermode, but I could easily be wrong.
(IIRC RISC-V has very developer-unfriendly icache management, but I don't
remember the details.)
Of course, using ptrace or any other FOLL_FORCE to modify text on x86 is
rather fraught, and I bet many things do it wrong when userspace is
multithreaded. But not in production because it's mostly not used in
production.)
But jit_text_alloc() can't do this, because the order of operations
doesn't match. With jit_text_alloc(), the executable mapping shows up
before the text is populated, so there is no atomic change from not-there
to populated-and-executable. Which means that there is an opportunity
for CPUs, speculatively or otherwise, to start filling various caches
with intermediate states of the text, which means that various
architectures (even x86!) may need serialization.
For eBPF- and module- like use cases, where JITting/code gen is quite
coarse-grained, perhaps something vaguely like:
jit_text_alloc() -> returns a handle and an executable virtual address,
but does *not* map it there
jit_text_write() -> write to that handle
jit_text_map() -> map it and synchronize if needed (no sync needed on
x86, I think)
could be more efficient and/or safer.
(Modules could use this too. Getting alternatives right might take some
fiddling, because off the top of my head, this doesn't match how it works
now.)
To make alternatives easier, this could work, maybe (haven't fully
thought it through):
jit_text_alloc()
jit_text_map_rw_inplace() -> map at the target address, but RW, !X
write the text and apply alternatives
jit_text_finalize() -> change from RW to RX *and synchronize*
jit_text_finalize() would either need to wait for RCU (possibly extra
heavy weight RCU to get "serialization") or send an IPI.
This essentially how modules work now. The memory is allocated RW, written
and updated with alternatives and then made ROX in the end with set_memory
APIs.
The issue with not having the memory mapped X when it's written is that we
cannot use large pages to map it. One of the goals is to have executable
memory mapped with large pages and make code allocator able to divide that
page among several callers.
So the idea was that jit_text_alloc() will have a cache of large pages
mapped ROX, will allocate memory from those caches and there will be
jit_update() that uses text poking for writing to that memory.
Upon allocation of a large page to increase the cache, that large page will
be "invalidated" by filling it with breakpoint instructions (e.g int3 on
x86)
Is this actually valid? In between int3 and real code, there’s a
potential torn read of real code mixed up with 0xcc.
You mean while doing text poking?
quoted
To improve the performance of this process, we can write to !X copy and
then text_poke it to the actual address in one go. This will require some
changes to get the alternatives right.
--
Sincerely yours,
Mike.
From: Kent Overstreet <kent.overstreet@linux.dev> Date: 2023-06-25 18:08:03
On Sun, Jun 25, 2023 at 08:42:57PM +0300, Mike Rapoport wrote:
On Sun, Jun 25, 2023 at 09:59:34AM -0700, Andy Lutomirski wrote:
quoted
On Sun, Jun 25, 2023, at 9:14 AM, Mike Rapoport wrote:
quoted
On Mon, Jun 19, 2023 at 10:09:02AM -0700, Andy Lutomirski wrote:
quoted
On Sun, Jun 18, 2023, at 1:00 AM, Mike Rapoport wrote:
quoted
On Sat, Jun 17, 2023 at 01:38:29PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Jun 16, 2023, at 1:50 AM, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
Is there anything in this series to help users do the appropriate
synchronization when the actually populate the allocated memory with
code? See here, for example:
This series only factors out the executable allocations from modules and
puts them in a central place.
Anything else would go on top after this lands.
Hmm.
On the one hand, there's nothing wrong with factoring out common code. On
the other hand, this is probably the right time to at least start
thinking about synchronization, at least to the extent that it might make
us want to change this API. (I'm not at all saying that this series
should require changes -- I'm just saying that this is a good time to
think about how this should work.)
The current APIs, *and* the proposed jit_text_alloc() API, don't actually
look like the one think in the Linux ecosystem that actually
intelligently and efficiently maps new text into an address space:
mmap().
On x86, you can mmap() an existing file full of executable code PROT_EXEC
and jump to it with minimal synchronization (just the standard implicit
ordering in the kernel that populates the pages before setting up the
PTEs and whatever user synchronization is needed to avoid jumping into
the mapping before mmap() finishes). It works across CPUs, and the only
possible way userspace can screw it up (for a read-only mapping of
read-only text, anyway) is to jump to the mapping too early, in which
case userspace gets a page fault. Incoherence is impossible, and no one
needs to "serialize" (in the SDM sense).
I think the same sequence (from userspace's perspective) works on other
architectures, too, although I think more cache management is needed on
the kernel's end. As far as I know, no Linux SMP architecture needs an
IPI to map executable text into usermode, but I could easily be wrong.
(IIRC RISC-V has very developer-unfriendly icache management, but I don't
remember the details.)
Of course, using ptrace or any other FOLL_FORCE to modify text on x86 is
rather fraught, and I bet many things do it wrong when userspace is
multithreaded. But not in production because it's mostly not used in
production.)
But jit_text_alloc() can't do this, because the order of operations
doesn't match. With jit_text_alloc(), the executable mapping shows up
before the text is populated, so there is no atomic change from not-there
to populated-and-executable. Which means that there is an opportunity
for CPUs, speculatively or otherwise, to start filling various caches
with intermediate states of the text, which means that various
architectures (even x86!) may need serialization.
For eBPF- and module- like use cases, where JITting/code gen is quite
coarse-grained, perhaps something vaguely like:
jit_text_alloc() -> returns a handle and an executable virtual address,
but does *not* map it there
jit_text_write() -> write to that handle
jit_text_map() -> map it and synchronize if needed (no sync needed on
x86, I think)
could be more efficient and/or safer.
(Modules could use this too. Getting alternatives right might take some
fiddling, because off the top of my head, this doesn't match how it works
now.)
To make alternatives easier, this could work, maybe (haven't fully
thought it through):
jit_text_alloc()
jit_text_map_rw_inplace() -> map at the target address, but RW, !X
write the text and apply alternatives
jit_text_finalize() -> change from RW to RX *and synchronize*
jit_text_finalize() would either need to wait for RCU (possibly extra
heavy weight RCU to get "serialization") or send an IPI.
This essentially how modules work now. The memory is allocated RW, written
and updated with alternatives and then made ROX in the end with set_memory
APIs.
The issue with not having the memory mapped X when it's written is that we
cannot use large pages to map it. One of the goals is to have executable
memory mapped with large pages and make code allocator able to divide that
page among several callers.
So the idea was that jit_text_alloc() will have a cache of large pages
mapped ROX, will allocate memory from those caches and there will be
jit_update() that uses text poking for writing to that memory.
Upon allocation of a large page to increase the cache, that large page will
be "invalidated" by filling it with breakpoint instructions (e.g int3 on
x86)
Is this actually valid? In between int3 and real code, there’s a
potential torn read of real code mixed up with 0xcc.
You mean while doing text poking?
I think we've been getting distracted by text_poke(). text_poke() does
updates via a different virtual address which introduce new
synchroniation wrinkles, but it's not the main issue.
As _think_ I understand it, the root of the issue is that speculative
execution - and that per Andy, speculative execution doesn't obey memory
barriers.
I have _not_ dug into the details of how retpolines work and all the
spectre stuff that was going on, but - retpoline uses lfence, doesn't
it? And if speculative execution is the issue here, isn't retpoline what
we need?
For this particular issue, I'm not sure "invalidate by filling with
illegal instructions" makes sense. For that to work, would the processor
have to execute a serialize operation and a retry on hitting an illegal
instruction - or perhaps we do in the interrupt handler?
But if filling with illegal instructions does act as a speculation
barrier, then the issue is that a torn read could generate a legal but
incorrect instruction.
From: Song Liu <song@kernel.org> Date: 2023-06-26 06:13:38
On Sun, Jun 25, 2023 at 11:07 AM Kent Overstreet
[off-list ref] wrote:
On Sun, Jun 25, 2023 at 08:42:57PM +0300, Mike Rapoport wrote:
quoted
On Sun, Jun 25, 2023 at 09:59:34AM -0700, Andy Lutomirski wrote:
quoted
On Sun, Jun 25, 2023, at 9:14 AM, Mike Rapoport wrote:
quoted
On Mon, Jun 19, 2023 at 10:09:02AM -0700, Andy Lutomirski wrote:
quoted
On Sun, Jun 18, 2023, at 1:00 AM, Mike Rapoport wrote:
quoted
On Sat, Jun 17, 2023 at 01:38:29PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Jun 16, 2023, at 1:50 AM, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
Is there anything in this series to help users do the appropriate
synchronization when the actually populate the allocated memory with
code? See here, for example:
This series only factors out the executable allocations from modules and
puts them in a central place.
Anything else would go on top after this lands.
Hmm.
On the one hand, there's nothing wrong with factoring out common code. On
the other hand, this is probably the right time to at least start
thinking about synchronization, at least to the extent that it might make
us want to change this API. (I'm not at all saying that this series
should require changes -- I'm just saying that this is a good time to
think about how this should work.)
The current APIs, *and* the proposed jit_text_alloc() API, don't actually
look like the one think in the Linux ecosystem that actually
intelligently and efficiently maps new text into an address space:
mmap().
On x86, you can mmap() an existing file full of executable code PROT_EXEC
and jump to it with minimal synchronization (just the standard implicit
ordering in the kernel that populates the pages before setting up the
PTEs and whatever user synchronization is needed to avoid jumping into
the mapping before mmap() finishes). It works across CPUs, and the only
possible way userspace can screw it up (for a read-only mapping of
read-only text, anyway) is to jump to the mapping too early, in which
case userspace gets a page fault. Incoherence is impossible, and no one
needs to "serialize" (in the SDM sense).
I think the same sequence (from userspace's perspective) works on other
architectures, too, although I think more cache management is needed on
the kernel's end. As far as I know, no Linux SMP architecture needs an
IPI to map executable text into usermode, but I could easily be wrong.
(IIRC RISC-V has very developer-unfriendly icache management, but I don't
remember the details.)
Of course, using ptrace or any other FOLL_FORCE to modify text on x86 is
rather fraught, and I bet many things do it wrong when userspace is
multithreaded. But not in production because it's mostly not used in
production.)
But jit_text_alloc() can't do this, because the order of operations
doesn't match. With jit_text_alloc(), the executable mapping shows up
before the text is populated, so there is no atomic change from not-there
to populated-and-executable. Which means that there is an opportunity
for CPUs, speculatively or otherwise, to start filling various caches
with intermediate states of the text, which means that various
architectures (even x86!) may need serialization.
For eBPF- and module- like use cases, where JITting/code gen is quite
coarse-grained, perhaps something vaguely like:
jit_text_alloc() -> returns a handle and an executable virtual address,
but does *not* map it there
jit_text_write() -> write to that handle
jit_text_map() -> map it and synchronize if needed (no sync needed on
x86, I think)
could be more efficient and/or safer.
(Modules could use this too. Getting alternatives right might take some
fiddling, because off the top of my head, this doesn't match how it works
now.)
To make alternatives easier, this could work, maybe (haven't fully
thought it through):
jit_text_alloc()
jit_text_map_rw_inplace() -> map at the target address, but RW, !X
write the text and apply alternatives
jit_text_finalize() -> change from RW to RX *and synchronize*
jit_text_finalize() would either need to wait for RCU (possibly extra
heavy weight RCU to get "serialization") or send an IPI.
This essentially how modules work now. The memory is allocated RW, written
and updated with alternatives and then made ROX in the end with set_memory
APIs.
The issue with not having the memory mapped X when it's written is that we
cannot use large pages to map it. One of the goals is to have executable
memory mapped with large pages and make code allocator able to divide that
page among several callers.
So the idea was that jit_text_alloc() will have a cache of large pages
mapped ROX, will allocate memory from those caches and there will be
jit_update() that uses text poking for writing to that memory.
Upon allocation of a large page to increase the cache, that large page will
be "invalidated" by filling it with breakpoint instructions (e.g int3 on
x86)
Is this actually valid? In between int3 and real code, there’s a
potential torn read of real code mixed up with 0xcc.
You mean while doing text poking?
I think we've been getting distracted by text_poke(). text_poke() does
updates via a different virtual address which introduce new
synchroniation wrinkles, but it's not the main issue.
As _think_ I understand it, the root of the issue is that speculative
execution - and that per Andy, speculative execution doesn't obey memory
barriers.
I have _not_ dug into the details of how retpolines work and all the
spectre stuff that was going on, but - retpoline uses lfence, doesn't
it? And if speculative execution is the issue here, isn't retpoline what
we need?
For this particular issue, I'm not sure "invalidate by filling with
illegal instructions" makes sense. For that to work, would the processor
have to execute a serialize operation and a retry on hitting an illegal
instruction - or perhaps we do in the interrupt handler?
But if filling with illegal instructions does act as a speculation
barrier, then the issue is that a torn read could generate a legal but
incorrect instruction.
What is a "torn read" here? I assume it is an instruction read that
goes at the wrong instruction boundary (CISC). If this is correct, do
we need to handle torn read caused by software bug, or hardware
bit flip, or both?
Thanks,
Song
On Mon, Jun 26, 2023 at 8:13 AM Song Liu [off-list ref] wrote:
On Sun, Jun 25, 2023 at 11:07 AM Kent Overstreet
[off-list ref] wrote:
quoted
On Sun, Jun 25, 2023 at 08:42:57PM +0300, Mike Rapoport wrote:
quoted
On Sun, Jun 25, 2023 at 09:59:34AM -0700, Andy Lutomirski wrote:
quoted
On Sun, Jun 25, 2023, at 9:14 AM, Mike Rapoport wrote:
quoted
On Mon, Jun 19, 2023 at 10:09:02AM -0700, Andy Lutomirski wrote:
quoted
On Sun, Jun 18, 2023, at 1:00 AM, Mike Rapoport wrote:
quoted
On Sat, Jun 17, 2023 at 01:38:29PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Jun 16, 2023, at 1:50 AM, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
Is there anything in this series to help users do the appropriate
synchronization when the actually populate the allocated memory with
code? See here, for example:
This series only factors out the executable allocations from modules and
puts them in a central place.
Anything else would go on top after this lands.
Hmm.
On the one hand, there's nothing wrong with factoring out common code. On
the other hand, this is probably the right time to at least start
thinking about synchronization, at least to the extent that it might make
us want to change this API. (I'm not at all saying that this series
should require changes -- I'm just saying that this is a good time to
think about how this should work.)
The current APIs, *and* the proposed jit_text_alloc() API, don't actually
look like the one think in the Linux ecosystem that actually
intelligently and efficiently maps new text into an address space:
mmap().
On x86, you can mmap() an existing file full of executable code PROT_EXEC
and jump to it with minimal synchronization (just the standard implicit
ordering in the kernel that populates the pages before setting up the
PTEs and whatever user synchronization is needed to avoid jumping into
the mapping before mmap() finishes). It works across CPUs, and the only
possible way userspace can screw it up (for a read-only mapping of
read-only text, anyway) is to jump to the mapping too early, in which
case userspace gets a page fault. Incoherence is impossible, and no one
needs to "serialize" (in the SDM sense).
I think the same sequence (from userspace's perspective) works on other
architectures, too, although I think more cache management is needed on
the kernel's end. As far as I know, no Linux SMP architecture needs an
IPI to map executable text into usermode, but I could easily be wrong.
(IIRC RISC-V has very developer-unfriendly icache management, but I don't
remember the details.)
Of course, using ptrace or any other FOLL_FORCE to modify text on x86 is
rather fraught, and I bet many things do it wrong when userspace is
multithreaded. But not in production because it's mostly not used in
production.)
But jit_text_alloc() can't do this, because the order of operations
doesn't match. With jit_text_alloc(), the executable mapping shows up
before the text is populated, so there is no atomic change from not-there
to populated-and-executable. Which means that there is an opportunity
for CPUs, speculatively or otherwise, to start filling various caches
with intermediate states of the text, which means that various
architectures (even x86!) may need serialization.
For eBPF- and module- like use cases, where JITting/code gen is quite
coarse-grained, perhaps something vaguely like:
jit_text_alloc() -> returns a handle and an executable virtual address,
but does *not* map it there
jit_text_write() -> write to that handle
jit_text_map() -> map it and synchronize if needed (no sync needed on
x86, I think)
could be more efficient and/or safer.
(Modules could use this too. Getting alternatives right might take some
fiddling, because off the top of my head, this doesn't match how it works
now.)
To make alternatives easier, this could work, maybe (haven't fully
thought it through):
jit_text_alloc()
jit_text_map_rw_inplace() -> map at the target address, but RW, !X
write the text and apply alternatives
jit_text_finalize() -> change from RW to RX *and synchronize*
jit_text_finalize() would either need to wait for RCU (possibly extra
heavy weight RCU to get "serialization") or send an IPI.
This essentially how modules work now. The memory is allocated RW, written
and updated with alternatives and then made ROX in the end with set_memory
APIs.
The issue with not having the memory mapped X when it's written is that we
cannot use large pages to map it. One of the goals is to have executable
memory mapped with large pages and make code allocator able to divide that
page among several callers.
So the idea was that jit_text_alloc() will have a cache of large pages
mapped ROX, will allocate memory from those caches and there will be
jit_update() that uses text poking for writing to that memory.
Upon allocation of a large page to increase the cache, that large page will
be "invalidated" by filling it with breakpoint instructions (e.g int3 on
x86)
Is this actually valid? In between int3 and real code, there’s a
potential torn read of real code mixed up with 0xcc.
You mean while doing text poking?
I think we've been getting distracted by text_poke(). text_poke() does
updates via a different virtual address which introduce new
synchroniation wrinkles, but it's not the main issue.
As _think_ I understand it, the root of the issue is that speculative
execution - and that per Andy, speculative execution doesn't obey memory
barriers.
I have _not_ dug into the details of how retpolines work and all the
spectre stuff that was going on, but - retpoline uses lfence, doesn't
it? And if speculative execution is the issue here, isn't retpoline what
we need?
For this particular issue, I'm not sure "invalidate by filling with
illegal instructions" makes sense. For that to work, would the processor
have to execute a serialize operation and a retry on hitting an illegal
instruction - or perhaps we do in the interrupt handler?
But if filling with illegal instructions does act as a speculation
barrier, then the issue is that a torn read could generate a legal but
incorrect instruction.
What is a "torn read" here? I assume it is an instruction read that
goes at the wrong instruction boundary (CISC). If this is correct, do
we need to handle torn read caused by software bug, or hardware
bit flip, or both?
On ARM64 (RISC), torn reads can't happen because the instruction fetch
is word aligned. If we replace the whole instruction atomically then there
won't be half old - half new instruction fetches.
Thanks,
Puranjay
From: Mark Rutland <mark.rutland@arm.com> Date: 2023-06-26 12:25:39
On Mon, Jun 26, 2023 at 11:54:02AM +0200, Puranjay Mohan wrote:
On Mon, Jun 26, 2023 at 8:13 AM Song Liu [off-list ref] wrote:
quoted
On Sun, Jun 25, 2023 at 11:07 AM Kent Overstreet
[off-list ref] wrote:
quoted
On Sun, Jun 25, 2023 at 08:42:57PM +0300, Mike Rapoport wrote:
quoted
On Sun, Jun 25, 2023 at 09:59:34AM -0700, Andy Lutomirski wrote:
quoted
On Sun, Jun 25, 2023, at 9:14 AM, Mike Rapoport wrote:
quoted
On Mon, Jun 19, 2023 at 10:09:02AM -0700, Andy Lutomirski wrote:
quoted
On Sun, Jun 18, 2023, at 1:00 AM, Mike Rapoport wrote:
quoted
On Sat, Jun 17, 2023 at 01:38:29PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Jun 16, 2023, at 1:50 AM, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
Is there anything in this series to help users do the appropriate
synchronization when the actually populate the allocated memory with
code? See here, for example:
This series only factors out the executable allocations from modules and
puts them in a central place.
Anything else would go on top after this lands.
Hmm.
On the one hand, there's nothing wrong with factoring out common code. On
the other hand, this is probably the right time to at least start
thinking about synchronization, at least to the extent that it might make
us want to change this API. (I'm not at all saying that this series
should require changes -- I'm just saying that this is a good time to
think about how this should work.)
The current APIs, *and* the proposed jit_text_alloc() API, don't actually
look like the one think in the Linux ecosystem that actually
intelligently and efficiently maps new text into an address space:
mmap().
On x86, you can mmap() an existing file full of executable code PROT_EXEC
and jump to it with minimal synchronization (just the standard implicit
ordering in the kernel that populates the pages before setting up the
PTEs and whatever user synchronization is needed to avoid jumping into
the mapping before mmap() finishes). It works across CPUs, and the only
possible way userspace can screw it up (for a read-only mapping of
read-only text, anyway) is to jump to the mapping too early, in which
case userspace gets a page fault. Incoherence is impossible, and no one
needs to "serialize" (in the SDM sense).
I think the same sequence (from userspace's perspective) works on other
architectures, too, although I think more cache management is needed on
the kernel's end. As far as I know, no Linux SMP architecture needs an
IPI to map executable text into usermode, but I could easily be wrong.
(IIRC RISC-V has very developer-unfriendly icache management, but I don't
remember the details.)
Of course, using ptrace or any other FOLL_FORCE to modify text on x86 is
rather fraught, and I bet many things do it wrong when userspace is
multithreaded. But not in production because it's mostly not used in
production.)
But jit_text_alloc() can't do this, because the order of operations
doesn't match. With jit_text_alloc(), the executable mapping shows up
before the text is populated, so there is no atomic change from not-there
to populated-and-executable. Which means that there is an opportunity
for CPUs, speculatively or otherwise, to start filling various caches
with intermediate states of the text, which means that various
architectures (even x86!) may need serialization.
For eBPF- and module- like use cases, where JITting/code gen is quite
coarse-grained, perhaps something vaguely like:
jit_text_alloc() -> returns a handle and an executable virtual address,
but does *not* map it there
jit_text_write() -> write to that handle
jit_text_map() -> map it and synchronize if needed (no sync needed on
x86, I think)
could be more efficient and/or safer.
(Modules could use this too. Getting alternatives right might take some
fiddling, because off the top of my head, this doesn't match how it works
now.)
To make alternatives easier, this could work, maybe (haven't fully
thought it through):
jit_text_alloc()
jit_text_map_rw_inplace() -> map at the target address, but RW, !X
write the text and apply alternatives
jit_text_finalize() -> change from RW to RX *and synchronize*
jit_text_finalize() would either need to wait for RCU (possibly extra
heavy weight RCU to get "serialization") or send an IPI.
This essentially how modules work now. The memory is allocated RW, written
and updated with alternatives and then made ROX in the end with set_memory
APIs.
The issue with not having the memory mapped X when it's written is that we
cannot use large pages to map it. One of the goals is to have executable
memory mapped with large pages and make code allocator able to divide that
page among several callers.
So the idea was that jit_text_alloc() will have a cache of large pages
mapped ROX, will allocate memory from those caches and there will be
jit_update() that uses text poking for writing to that memory.
Upon allocation of a large page to increase the cache, that large page will
be "invalidated" by filling it with breakpoint instructions (e.g int3 on
x86)
Is this actually valid? In between int3 and real code, there’s a
potential torn read of real code mixed up with 0xcc.
You mean while doing text poking?
I think we've been getting distracted by text_poke(). text_poke() does
updates via a different virtual address which introduce new
synchroniation wrinkles, but it's not the main issue.
As _think_ I understand it, the root of the issue is that speculative
execution - and that per Andy, speculative execution doesn't obey memory
barriers.
I have _not_ dug into the details of how retpolines work and all the
spectre stuff that was going on, but - retpoline uses lfence, doesn't
it? And if speculative execution is the issue here, isn't retpoline what
we need?
For this particular issue, I'm not sure "invalidate by filling with
illegal instructions" makes sense. For that to work, would the processor
have to execute a serialize operation and a retry on hitting an illegal
instruction - or perhaps we do in the interrupt handler?
But if filling with illegal instructions does act as a speculation
barrier, then the issue is that a torn read could generate a legal but
incorrect instruction.
What is a "torn read" here? I assume it is an instruction read that
goes at the wrong instruction boundary (CISC). If this is correct, do
we need to handle torn read caused by software bug, or hardware
bit flip, or both?
On ARM64 (RISC), torn reads can't happen because the instruction fetch
is word aligned. If we replace the whole instruction atomically then there
won't be half old - half new instruction fetches.
Unfortunately, that's only guaranteed for a subset of instructions (e.g. B,
NOP), and in general CPUs can fetch an instruction multiple times, and could
fetch arbitrary portions of the instruction each time.
Please see the "Concurrent Modification and Execution of instructions" rules in
the ARM ARM.
For arm64, in general, you need to inhibit any concurrent execution (e.g. by
stopping-the-world) when patching text, and afterwards you need cache maintence
followed by a context-syncrhonization-event (aking to an x86 serializing
instruction) on CPUs which will execute the new instruction(s).
There are a bunch of special cases where we can omit some of that, but in
general the architectural guarnatees are *very* weak and require SW to perform
several bits of work to guarantee the new instructions will be executed without issues.
Thanks,
Mark.
From: Mark Rutland <mark.rutland@arm.com> Date: 2023-06-26 12:31:32
On Sun, Jun 25, 2023 at 07:14:17PM +0300, Mike Rapoport wrote:
On Mon, Jun 19, 2023 at 10:09:02AM -0700, Andy Lutomirski wrote:
quoted
On Sun, Jun 18, 2023, at 1:00 AM, Mike Rapoport wrote:
quoted
On Sat, Jun 17, 2023 at 01:38:29PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Jun 16, 2023, at 1:50 AM, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
Is there anything in this series to help users do the appropriate
synchronization when the actually populate the allocated memory with
code? See here, for example:
This series only factors out the executable allocations from modules and
puts them in a central place.
Anything else would go on top after this lands.
Hmm.
On the one hand, there's nothing wrong with factoring out common code. On
the other hand, this is probably the right time to at least start
thinking about synchronization, at least to the extent that it might make
us want to change this API. (I'm not at all saying that this series
should require changes -- I'm just saying that this is a good time to
think about how this should work.)
The current APIs, *and* the proposed jit_text_alloc() API, don't actually
look like the one think in the Linux ecosystem that actually
intelligently and efficiently maps new text into an address space:
mmap().
On x86, you can mmap() an existing file full of executable code PROT_EXEC
and jump to it with minimal synchronization (just the standard implicit
ordering in the kernel that populates the pages before setting up the
PTEs and whatever user synchronization is needed to avoid jumping into
the mapping before mmap() finishes). It works across CPUs, and the only
possible way userspace can screw it up (for a read-only mapping of
read-only text, anyway) is to jump to the mapping too early, in which
case userspace gets a page fault. Incoherence is impossible, and no one
needs to "serialize" (in the SDM sense).
I think the same sequence (from userspace's perspective) works on other
architectures, too, although I think more cache management is needed on
the kernel's end. As far as I know, no Linux SMP architecture needs an
IPI to map executable text into usermode, but I could easily be wrong.
(IIRC RISC-V has very developer-unfriendly icache management, but I don't
remember the details.)
Of course, using ptrace or any other FOLL_FORCE to modify text on x86 is
rather fraught, and I bet many things do it wrong when userspace is
multithreaded. But not in production because it's mostly not used in
production.)
But jit_text_alloc() can't do this, because the order of operations
doesn't match. With jit_text_alloc(), the executable mapping shows up
before the text is populated, so there is no atomic change from not-there
to populated-and-executable. Which means that there is an opportunity
for CPUs, speculatively or otherwise, to start filling various caches
with intermediate states of the text, which means that various
architectures (even x86!) may need serialization.
For eBPF- and module- like use cases, where JITting/code gen is quite
coarse-grained, perhaps something vaguely like:
jit_text_alloc() -> returns a handle and an executable virtual address,
but does *not* map it there
jit_text_write() -> write to that handle
jit_text_map() -> map it and synchronize if needed (no sync needed on
x86, I think)
could be more efficient and/or safer.
(Modules could use this too. Getting alternatives right might take some
fiddling, because off the top of my head, this doesn't match how it works
now.)
To make alternatives easier, this could work, maybe (haven't fully
thought it through):
jit_text_alloc()
jit_text_map_rw_inplace() -> map at the target address, but RW, !X
write the text and apply alternatives
jit_text_finalize() -> change from RW to RX *and synchronize*
jit_text_finalize() would either need to wait for RCU (possibly extra
heavy weight RCU to get "serialization") or send an IPI.
This essentially how modules work now. The memory is allocated RW, written
and updated with alternatives and then made ROX in the end with set_memory
APIs.
The issue with not having the memory mapped X when it's written is that we
cannot use large pages to map it. One of the goals is to have executable
memory mapped with large pages and make code allocator able to divide that
page among several callers.
So the idea was that jit_text_alloc() will have a cache of large pages
mapped ROX, will allocate memory from those caches and there will be
jit_update() that uses text poking for writing to that memory.
Upon allocation of a large page to increase the cache, that large page will
be "invalidated" by filling it with breakpoint instructions (e.g int3 on
x86)
Does that work on x86?
That is in no way gauranteed for other architectures; on arm64 you need
explicit cache maintenance (with I-cache maintenance at the VA to be executed
from) followed by context-synchronization-events (e.g. via ISB instructions, or
IPIs).
Mark.
From: Song Liu <song@kernel.org> Date: 2023-06-26 17:49:27
On Mon, Jun 26, 2023 at 5:31 AM Mark Rutland [off-list ref] wrote:
[...]
quoted
So the idea was that jit_text_alloc() will have a cache of large pages
mapped ROX, will allocate memory from those caches and there will be
jit_update() that uses text poking for writing to that memory.
Upon allocation of a large page to increase the cache, that large page will
be "invalidated" by filling it with breakpoint instructions (e.g int3 on
x86)
Does that work on x86?
That is in no way gauranteed for other architectures; on arm64 you need
explicit cache maintenance (with I-cache maintenance at the VA to be executed
from) followed by context-synchronization-events (e.g. via ISB instructions, or
IPIs).
I guess we need:
1) Invalidate unused part of the huge ROX pages;
2) Do not put two jit users (including module text, bpf, etc.) in the
same cache line;
3) Explicit cache maintenance;
4) context-synchronization-events.
Would these (or a subset of them) be sufficient to protect us from torn read?
Thanks,
Song
On Mon, Jun 26, 2023, at 10:48 AM, Song Liu wrote:
On Mon, Jun 26, 2023 at 5:31 AM Mark Rutland [off-list ref] wrote:
quoted
[...]
quoted
quoted
So the idea was that jit_text_alloc() will have a cache of large pages
mapped ROX, will allocate memory from those caches and there will be
jit_update() that uses text poking for writing to that memory.
Upon allocation of a large page to increase the cache, that large page will
be "invalidated" by filling it with breakpoint instructions (e.g int3 on
x86)
Does that work on x86?
That is in no way gauranteed for other architectures; on arm64 you need
explicit cache maintenance (with I-cache maintenance at the VA to be executed
from) followed by context-synchronization-events (e.g. via ISB instructions, or
IPIs).
I guess we need:
1) Invalidate unused part of the huge ROX pages;
2) Do not put two jit users (including module text, bpf, etc.) in the
same cache line;
3) Explicit cache maintenance;
4) context-synchronization-events.
Would these (or a subset of them) be sufficient to protect us from torn read?
Maybe? #4 is sufficiently vague that I can't really interpret it.
I have a half-drafted email asking for official clarification on the rules that might help shed light on this. I find that this type of request works best when it's really well written :)
From: Mark Rutland <mark.rutland@arm.com> Date: 2023-06-26 13:02:29
On Mon, Jun 19, 2023 at 10:09:02AM -0700, Andy Lutomirski wrote:
On Sun, Jun 18, 2023, at 1:00 AM, Mike Rapoport wrote:
quoted
On Sat, Jun 17, 2023 at 01:38:29PM -0700, Andy Lutomirski wrote:
quoted
On Fri, Jun 16, 2023, at 1:50 AM, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
Is there anything in this series to help users do the appropriate
synchronization when the actually populate the allocated memory with
code? See here, for example:
This series only factors out the executable allocations from modules and
puts them in a central place.
Anything else would go on top after this lands.
Hmm.
On the one hand, there's nothing wrong with factoring out common code. On the
other hand, this is probably the right time to at least start thinking about
synchronization, at least to the extent that it might make us want to change
this API. (I'm not at all saying that this series should require changes --
I'm just saying that this is a good time to think about how this should
work.)
The current APIs, *and* the proposed jit_text_alloc() API, don't actually
look like the one think in the Linux ecosystem that actually intelligently
and efficiently maps new text into an address space: mmap().
On x86, you can mmap() an existing file full of executable code PROT_EXEC and
jump to it with minimal synchronization (just the standard implicit ordering
in the kernel that populates the pages before setting up the PTEs and
whatever user synchronization is needed to avoid jumping into the mapping
before mmap() finishes). It works across CPUs, and the only possible way
userspace can screw it up (for a read-only mapping of read-only text, anyway)
is to jump to the mapping too early, in which case userspace gets a page
fault. Incoherence is impossible, and no one needs to "serialize" (in the
SDM sense).
I think the same sequence (from userspace's perspective) works on other
architectures, too, although I think more cache management is needed on the
kernel's end. As far as I know, no Linux SMP architecture needs an IPI to
map executable text into usermode, but I could easily be wrong. (IIRC RISC-V
has very developer-unfriendly icache management, but I don't remember the
details.)
That's my understanding too, with a couple of details:
1) After the copy we perform and complete all the data + instruction cache
maintenance *before* marking the mapping as executable.
2) Even *after* the mapping is marked executable, a thread could take a
spurious fault on an instruction fetch for the new instructions. One way to
think about this is that the CPU attempted to speculate the instructions
earlier, saw that the mapping was faulting, and placed a "generate a fault
here" operation into its pipeline to generate that later.
The CPU pipeline/OoO-engine/whatever is effectively a transient cache for
operations in-flight which is only ever "invalidated" by a
context-synchronization-event (akin to an x86 serializing effect).
We're only guarnateed to have a new instruction fetch (from the I-cache into
the CPU pipeline) after the next context synchronization event (akin to an x86
serializing effect), and luckily out exception entry/exit is architecturally
guarnateed to provide that (unless we explicitly opt out via a control bit).
I know we're a bit lax with that today: I think we omit the
context-synchronization-event when enabling ftrace callsites, and worse, for
static keys. Those are both on my TODO list of nasty problems that require
careful auditing...
Of course, using ptrace or any other FOLL_FORCE to modify text on x86 is
rather fraught, and I bet many things do it wrong when userspace is
multithreaded. But not in production because it's mostly not used in
production.)
I suspect uprobes needs a look too...
I'll need to go dig into all that a bit before I have more of an opinion on the
shape of the API.
Thanks,
Mark.
But jit_text_alloc() can't do this, because the order of operations doesn't
match. With jit_text_alloc(), the executable mapping shows up before the
text is populated, so there is no atomic change from not-there to
populated-and-executable. Which means that there is an opportunity for CPUs,
speculatively or otherwise, to start filling various caches with intermediate
states of the text, which means that various architectures (even x86!) may
need serialization.
For eBPF- and module- like use cases, where JITting/code gen is quite
coarse-grained, perhaps something vaguely like:
jit_text_alloc() -> returns a handle and an executable virtual address, but does *not* map it there
jit_text_write() -> write to that handle
jit_text_map() -> map it and synchronize if needed (no sync needed on x86, I think)
could be more efficient and/or safer.
(Modules could use this too. Getting alternatives right might take some
fiddling, because off the top of my head, this doesn't match how it works
now.)
To make alternatives easier, this could work, maybe (haven't fully thought it through):
jit_text_alloc()
jit_text_map_rw_inplace() -> map at the target address, but RW, !X
write the text and apply alternatives
jit_text_finalize() -> change from RW to RX *and synchronize*
jit_text_finalize() would either need to wait for RCU (possibly extra heavy
weight RCU to get "serialization") or send an IPI.
This is slower than the alloc, write, map solution, but allows alternatives
to be applied at the final address.
Even fancier variants where the writing is some using something like
use_temporary_mm() might even make sense.
To what extent does performance matter for the various users? module loading
is slow, and I don't think we care that much. eBPF loaded is not super fast,
and we care to a limited extent. I *think* the bcachefs use case needs to be
very fast, but I'm not sure it can be fast and supportable.
Anyway, food for thought.
From: Kent Overstreet <kent.overstreet@linux.dev> Date: 2023-06-19 11:34:41
On Sat, Jun 17, 2023 at 01:38:29PM -0700, Andy Lutomirski wrote:
On Fri, Jun 16, 2023, at 1:50 AM, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
module_alloc() is used everywhere as a mean to allocate memory for code.
Beside being semantically wrong, this unnecessarily ties all subsystems
that need to allocate code, such as ftrace, kprobes and BPF to modules
and puts the burden of code allocation to the modules code.
Several architectures override module_alloc() because of various
constraints where the executable memory can be located and this causes
additional obstacles for improvements of code allocation.
Start splitting code allocation from modules by introducing
execmem_text_alloc(), execmem_free(), jit_text_alloc(), jit_free() APIs.
Initially, execmem_text_alloc() and jit_text_alloc() are wrappers for
module_alloc() and execmem_free() and jit_free() are replacements of
module_memfree() to allow updating all call sites to use the new APIs.
The intention semantics for new allocation APIs:
* execmem_text_alloc() should be used to allocate memory that must reside
close to the kernel image, like loadable kernel modules and generated
code that is restricted by relative addressing.
* jit_text_alloc() should be used to allocate memory for generated code
when there are no restrictions for the code placement. For
architectures that require that any code is within certain distance
from the kernel image, jit_text_alloc() will be essentially aliased to
execmem_text_alloc().
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-16 08:51:59
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
Several architectures override module_alloc() only to define address
range for code allocations different than VMALLOC address space.
Provide a generic implementation in execmem that uses the parameters
for address space ranges, required alignment and page protections
provided by architectures.
The architecures must fill execmem_params structure and implement
execmem_arch_params() that returns a pointer to that structure. This
way the execmem initialization won't be called from every architecure,
but rather from a central place, namely initialization of the core
memory management.
The execmem provides execmem_text_alloc() API that wraps
__vmalloc_node_range() with the parameters defined by the architecures.
If an architeture does not implement execmem_arch_params(),
execmem_text_alloc() will fall back to module_alloc().
The name execmem_text_alloc() emphasizes that the allocated memory is
for executable code, the allocations of the associated data, like data
sections of a module will use execmem_data_alloc() interface that will
be added later.
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
---
arch/loongarch/kernel/module.c | 18 ++++++++++--
arch/mips/kernel/module.c | 18 +++++++++---
arch/nios2/kernel/module.c | 19 +++++++++----
arch/parisc/kernel/module.c | 23 +++++++++------
arch/riscv/kernel/module.c | 20 +++++++++----
arch/sparc/kernel/module.c | 44 +++++++++++++---------------
include/linux/execmem.h | 52 ++++++++++++++++++++++++++++++++++
mm/execmem.c | 52 +++++++++++++++++++++++++++++++---
mm/mm_init.c | 2 ++
9 files changed, 195 insertions(+), 53 deletions(-)
@@ -173,15 +174,21 @@ static inline int reassemble_22(int as22)((as22&0x0003ff)<<3));}-void*module_alloc(unsignedlongsize)+staticstructexecmem_paramsexecmem_params={+.modules={+.text={+.pgprot=PAGE_KERNEL_RWX,+.alignment=1,+},+},+};++structexecmem_params__init*execmem_arch_params(void){-/* using RWX means less protection for modules, but it's-*easierthantryingtomapthetext,data,init_textand-*init_datacorrectly*/-return__vmalloc_node_range(size,1,VMALLOC_START,VMALLOC_END,-GFP_KERNEL,-PAGE_KERNEL_RWX,0,NUMA_NO_NODE,-__builtin_return_address(0));+execmem_params.modules.text.start=VMALLOC_START;+execmem_params.modules.text.end=VMALLOC_END;++return&execmem_params;}#ifndef CONFIG_64BIT
From: Song Liu <song@kernel.org> Date: 2023-06-16 18:36:52
On Fri, Jun 16, 2023 at 1:51 AM Mike Rapoport [off-list ref] wrote:
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
Several architectures override module_alloc() only to define address
range for code allocations different than VMALLOC address space.
Provide a generic implementation in execmem that uses the parameters
for address space ranges, required alignment and page protections
provided by architectures.
The architecures must fill execmem_params structure and implement
execmem_arch_params() that returns a pointer to that structure. This
way the execmem initialization won't be called from every architecure,
but rather from a central place, namely initialization of the core
memory management.
The execmem provides execmem_text_alloc() API that wraps
__vmalloc_node_range() with the parameters defined by the architecures.
If an architeture does not implement execmem_arch_params(),
execmem_text_alloc() will fall back to module_alloc().
The name execmem_text_alloc() emphasizes that the allocated memory is
for executable code, the allocations of the associated data, like data
sections of a module will use execmem_data_alloc() interface that will
be added later.
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-16 08:52:14
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
Extend execmem parameters to accommodate more complex overrides of
module_alloc() by architectures.
This includes specification of a fallback range required by arm, arm64
and powerpc and support for allocation of KASAN shadow required by
arm64, s390 and x86.
The core implementation of execmem_alloc() takes care of suppressing
warnings when the initial allocation fails but there is a fallback range
defined.
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
---
arch/arm/kernel/module.c | 36 ++++++++++---------
arch/arm64/kernel/module.c | 68 ++++++++++++++++--------------------
arch/powerpc/kernel/module.c | 52 +++++++++++++--------------
arch/s390/kernel/module.c | 33 ++++++++---------
arch/x86/kernel/module.c | 33 +++++++++--------
include/linux/execmem.h | 14 ++++++++
mm/execmem.c | 50 ++++++++++++++++++++++----
7 files changed, 168 insertions(+), 118 deletions(-)
@@ -17,56 +17,50 @@#include<linux/moduleloader.h>#include<linux/scs.h>#include<linux/vmalloc.h>+#include<linux/execmem.h>#include<asm/alternative.h>#include<asm/insn.h>#include<asm/scs.h>#include<asm/sections.h>-void*module_alloc(unsignedlongsize)+staticstructexecmem_paramsexecmem_params={+.modules={+.flags=EXECMEM_KASAN_SHADOW,+.text={+.alignment=MODULE_ALIGN,+},+},+};++structexecmem_params__init*execmem_arch_params(void){u64module_alloc_end=module_alloc_base+MODULES_VSIZE;-gfp_tgfp_mask=GFP_KERNEL;-void*p;--/* Silence the initial allocation */-if(IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))-gfp_mask|=__GFP_NOWARN;-if(IS_ENABLED(CONFIG_KASAN_GENERIC)||-IS_ENABLED(CONFIG_KASAN_SW_TAGS))-/* don't exceed the static module region - see below */-module_alloc_end=MODULES_END;+execmem_params.modules.text.pgprot=PAGE_KERNEL;+execmem_params.modules.text.start=module_alloc_base;+execmem_params.modules.text.end=module_alloc_end;-p=__vmalloc_node_range(size,MODULE_ALIGN,module_alloc_base,-module_alloc_end,gfp_mask,PAGE_KERNEL,VM_DEFER_KMEMLEAK,-NUMA_NO_NODE,__builtin_return_address(0));--if(!p&&IS_ENABLED(CONFIG_ARM64_MODULE_PLTS)&&+/*+*KASANwithoutKASAN_VMALLOCcanonlydealwithmodule+*allocationsbeingservedfromthereservedmoduleregion,+*sincetheremainderofthevmallocregionisalready+*backedbyzeroshadowpages,andpunchingholesintoit+*isnon-trivial.Sincethemoduleregionisnotrandomized+*whenKASANisenabledwithoutKASAN_VMALLOC,itiseven+*lesslikelythatthemoduleregiongetsexhausted,sowe+*cansimplyomitthisfallbackinthatcase.+*/+if(IS_ENABLED(CONFIG_ARM64_MODULE_PLTS)&&(IS_ENABLED(CONFIG_KASAN_VMALLOC)||(!IS_ENABLED(CONFIG_KASAN_GENERIC)&&-!IS_ENABLED(CONFIG_KASAN_SW_TAGS))))-/*-*KASANwithoutKASAN_VMALLOCcanonlydealwithmodule-*allocationsbeingservedfromthereservedmoduleregion,-*sincetheremainderofthevmallocregionisalready-*backedbyzeroshadowpages,andpunchingholesintoit-*isnon-trivial.Sincethemoduleregionisnotrandomized-*whenKASANisenabledwithoutKASAN_VMALLOC,itiseven-*lesslikelythatthemoduleregiongetsexhausted,sowe-*cansimplyomitthisfallbackinthatcase.-*/-p=__vmalloc_node_range(size,MODULE_ALIGN,module_alloc_base,-module_alloc_base+SZ_2G,GFP_KERNEL,-PAGE_KERNEL,0,NUMA_NO_NODE,-__builtin_return_address(0));--if(p&&(kasan_alloc_module_shadow(p,size,gfp_mask)<0)){-vfree(p);-returnNULL;+!IS_ENABLED(CONFIG_KASAN_SW_TAGS)))){+unsignedlongend=module_alloc_base+SZ_2G;++execmem_params.modules.text.fallback_start=module_alloc_base;+execmem_params.modules.text.fallback_end=end;}-/* Memory is intended to be executable, reset the pointer tag. */-returnkasan_reset_tag(p);+return&execmem_params;}enumaarch64_reloc_op{
@@ -89,39 +90,38 @@ int module_finalize(const Elf_Ehdr *hdr,return0;}-static__always_inlinevoid*-__module_alloc(unsignedlongsize,unsignedlongstart,unsignedlongend,boolnowarn)+staticstructexecmem_paramsexecmem_params={+.modules={+.text={+.alignment=1,+},+},+};+++structexecmem_params__init*execmem_arch_params(void){pgprot_tprot=strict_module_rwx_enabled()?PAGE_KERNEL:PAGE_KERNEL_EXEC;-gfp_tgfp=GFP_KERNEL|(nowarn?__GFP_NOWARN:0);--/*-*Don'tdohugepageallocationsformodulesyetuntilmoretesting-*isdone.STRICT_MODULE_RWXmayrequireextraworktosupportthis-*too.-*/-return__vmalloc_node_range(size,1,start,end,gfp,prot,-VM_FLUSH_RESET_PERMS,-NUMA_NO_NODE,__builtin_return_address(0));-}-void*module_alloc(unsignedlongsize)-{#ifdef MODULES_VADDRunsignedlonglimit=(unsignedlong)_etext-SZ_32M;-void*ptr=NULL;--BUILD_BUG_ON(TASK_SIZE>MODULES_VADDR);/* First try within 32M limit from _etext to avoid branch trampolines */-if(MODULES_VADDR<PAGE_OFFSET&&MODULES_END>limit)-ptr=__module_alloc(size,limit,MODULES_END,true);--if(!ptr)-ptr=__module_alloc(size,MODULES_VADDR,MODULES_END,false);--returnptr;+if(MODULES_VADDR<PAGE_OFFSET&&MODULES_END>limit){+execmem_params.modules.text.start=limit;+execmem_params.modules.text.end=MODULES_END;+execmem_params.modules.text.fallback_start=MODULES_VADDR;+execmem_params.modules.text.fallback_end=MODULES_END;+}else{+execmem_params.modules.text.start=MODULES_VADDR;+execmem_params.modules.text.end=MODULES_END;+}#else-return__module_alloc(size,VMALLOC_START,VMALLOC_END,false);+execmem_params.modules.text.start=VMALLOC_START;+execmem_params.modules.text.end=VMALLOC_END;#endif++execmem_params.modules.text.pgprot=prot;++return&execmem_params;}
@@ -65,26 +66,24 @@ static unsigned long int get_module_load_offset(void)}#endif-void*module_alloc(unsignedlongsize)-{-gfp_tgfp_mask=GFP_KERNEL;-void*p;--if(PAGE_ALIGN(size)>MODULES_LEN)-returnNULL;+staticstructexecmem_paramsexecmem_params={+.modules={+.flags=EXECMEM_KASAN_SHADOW,+.text={+.alignment=MODULE_ALIGN,+},+},+};-p=__vmalloc_node_range(size,MODULE_ALIGN,-MODULES_VADDR+get_module_load_offset(),-MODULES_END,gfp_mask,PAGE_KERNEL,-VM_FLUSH_RESET_PERMS|VM_DEFER_KMEMLEAK,-NUMA_NO_NODE,__builtin_return_address(0));+structexecmem_params__init*execmem_arch_params(void)+{+unsignedlongstart=MODULES_VADDR+get_module_load_offset();-if(p&&(kasan_alloc_module_shadow(p,size,gfp_mask)<0)){-vfree(p);-returnNULL;-}+execmem_params.modules.text.start=start;+execmem_params.modules.text.end=MODULES_END;+execmem_params.modules.text.pgprot=PAGE_KERNEL;-returnp;+return&execmem_params;}#ifdef CONFIG_X86_32
Did you consider making these execmem_params's ro_after_init? Not that
it is security sensitive, but it's a nice hint to the reader that it is
only modified at init. And I guess basically free sanitizing of buggy
writes to it.
Did you consider making these execmem_params's ro_after_init? Not that
it is security sensitive, but it's a nice hint to the reader that it is
only modified at init. And I guess basically free sanitizing of buggy
writes to it.
I think we can drop the mutex's in get_module_load_offset() now, since
execmem_arch_params() should only be called once at init.
Right. Even more, the entire get_module_load_offset() can be folded into
execmem_arch_params() as
if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) && kaslr_enabled())
module_load_offset =
get_random_u32_inclusive(1, 1024) * PAGE_SIZE;
@@ -17,56 +17,50 @@#include<linux/moduleloader.h>#include<linux/scs.h>#include<linux/vmalloc.h>+#include<linux/execmem.h>#include<asm/alternative.h>#include<asm/insn.h>#include<asm/scs.h>#include<asm/sections.h>-void*module_alloc(unsignedlongsize)+staticstructexecmem_paramsexecmem_params={+.modules={+.flags=EXECMEM_KASAN_SHADOW,+.text={+.alignment=MODULE_ALIGN,+},+},+};++structexecmem_params__init*execmem_arch_params(void){u64module_alloc_end=module_alloc_base+MODULES_VSIZE;-gfp_tgfp_mask=GFP_KERNEL;-void*p;--/* Silence the initial allocation */-if(IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))-gfp_mask|=__GFP_NOWARN;-if(IS_ENABLED(CONFIG_KASAN_GENERIC)||-IS_ENABLED(CONFIG_KASAN_SW_TAGS))-/* don't exceed the static module region - see below */-module_alloc_end=MODULES_END;+execmem_params.modules.text.pgprot=PAGE_KERNEL;+execmem_params.modules.text.start=module_alloc_base;
I think I mentioned this earlier. For arm64 with CONFIG_RANDOMIZE_BASE,
module_alloc_base is not yet set when execmem_arch_params() is
called. So we will need some extra logic for this.
Thanks,
Song
+ execmem_params.modules.text.end = module_alloc_end;
- p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
- module_alloc_end, gfp_mask, PAGE_KERNEL, VM_DEFER_KMEMLEAK,
- NUMA_NO_NODE, __builtin_return_address(0));
-
- if (!p && IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
+ /*
+ * KASAN without KASAN_VMALLOC can only deal with module
+ * allocations being served from the reserved module region,
+ * since the remainder of the vmalloc region is already
+ * backed by zero shadow pages, and punching holes into it
+ * is non-trivial. Since the module region is not randomized
+ * when KASAN is enabled without KASAN_VMALLOC, it is even
+ * less likely that the module region gets exhausted, so we
+ * can simply omit this fallback in that case.
+ */
+ if (IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
(IS_ENABLED(CONFIG_KASAN_VMALLOC) ||
(!IS_ENABLED(CONFIG_KASAN_GENERIC) &&
- !IS_ENABLED(CONFIG_KASAN_SW_TAGS))))
- /*
- * KASAN without KASAN_VMALLOC can only deal with module
- * allocations being served from the reserved module region,
- * since the remainder of the vmalloc region is already
- * backed by zero shadow pages, and punching holes into it
- * is non-trivial. Since the module region is not randomized
- * when KASAN is enabled without KASAN_VMALLOC, it is even
- * less likely that the module region gets exhausted, so we
- * can simply omit this fallback in that case.
- */
- p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
- module_alloc_base + SZ_2G, GFP_KERNEL,
- PAGE_KERNEL, 0, NUMA_NO_NODE,
- __builtin_return_address(0));
-
- if (p && (kasan_alloc_module_shadow(p, size, gfp_mask) < 0)) {
- vfree(p);
- return NULL;
+ !IS_ENABLED(CONFIG_KASAN_SW_TAGS)))) {
+ unsigned long end = module_alloc_base + SZ_2G;
+
+ execmem_params.modules.text.fallback_start = module_alloc_base;
+ execmem_params.modules.text.fallback_end = end;
}
- /* Memory is intended to be executable, reset the pointer tag. */
- return kasan_reset_tag(p);
+ return &execmem_params;
}
enum aarch64_reloc_op {
@@ -17,56 +17,50 @@#include<linux/moduleloader.h>#include<linux/scs.h>#include<linux/vmalloc.h>+#include<linux/execmem.h>#include<asm/alternative.h>#include<asm/insn.h>#include<asm/scs.h>#include<asm/sections.h>-void*module_alloc(unsignedlongsize)+staticstructexecmem_paramsexecmem_params={+.modules={+.flags=EXECMEM_KASAN_SHADOW,+.text={+.alignment=MODULE_ALIGN,+},+},+};++structexecmem_params__init*execmem_arch_params(void){u64module_alloc_end=module_alloc_base+MODULES_VSIZE;-gfp_tgfp_mask=GFP_KERNEL;-void*p;--/* Silence the initial allocation */-if(IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))-gfp_mask|=__GFP_NOWARN;-if(IS_ENABLED(CONFIG_KASAN_GENERIC)||-IS_ENABLED(CONFIG_KASAN_SW_TAGS))-/* don't exceed the static module region - see below */-module_alloc_end=MODULES_END;+execmem_params.modules.text.pgprot=PAGE_KERNEL;+execmem_params.modules.text.start=module_alloc_base;
I think I mentioned this earlier. For arm64 with CONFIG_RANDOMIZE_BASE,
module_alloc_base is not yet set when execmem_arch_params() is
called. So we will need some extra logic for this.
Right, this is taken care of in a later patch, but it actually belongs here.
Good catch!
Thanks,
Song
quoted
+ execmem_params.modules.text.end = module_alloc_end;
- p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
- module_alloc_end, gfp_mask, PAGE_KERNEL, VM_DEFER_KMEMLEAK,
- NUMA_NO_NODE, __builtin_return_address(0));
-
- if (!p && IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
+ /*
+ * KASAN without KASAN_VMALLOC can only deal with module
+ * allocations being served from the reserved module region,
+ * since the remainder of the vmalloc region is already
+ * backed by zero shadow pages, and punching holes into it
+ * is non-trivial. Since the module region is not randomized
+ * when KASAN is enabled without KASAN_VMALLOC, it is even
+ * less likely that the module region gets exhausted, so we
+ * can simply omit this fallback in that case.
+ */
+ if (IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
(IS_ENABLED(CONFIG_KASAN_VMALLOC) ||
(!IS_ENABLED(CONFIG_KASAN_GENERIC) &&
- !IS_ENABLED(CONFIG_KASAN_SW_TAGS))))
- /*
- * KASAN without KASAN_VMALLOC can only deal with module
- * allocations being served from the reserved module region,
- * since the remainder of the vmalloc region is already
- * backed by zero shadow pages, and punching holes into it
- * is non-trivial. Since the module region is not randomized
- * when KASAN is enabled without KASAN_VMALLOC, it is even
- * less likely that the module region gets exhausted, so we
- * can simply omit this fallback in that case.
- */
- p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
- module_alloc_base + SZ_2G, GFP_KERNEL,
- PAGE_KERNEL, 0, NUMA_NO_NODE,
- __builtin_return_address(0));
-
- if (p && (kasan_alloc_module_shadow(p, size, gfp_mask) < 0)) {
- vfree(p);
- return NULL;
+ !IS_ENABLED(CONFIG_KASAN_SW_TAGS)))) {
+ unsigned long end = module_alloc_base + SZ_2G;
+
+ execmem_params.modules.text.fallback_start = module_alloc_base;
+ execmem_params.modules.text.fallback_end = end;
}
- /* Memory is intended to be executable, reset the pointer tag. */
- return kasan_reset_tag(p);
+ return &execmem_params;
}
enum aarch64_reloc_op {
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-16 08:52:35
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
Define default parameters for address range for code allocations using
the current values in module_alloc() and make execmem_text_alloc() use
these defaults when an architecure does not supply its specific
parameters.
With this, execmem_text_alloc() implements memory allocation in a way
compatible with module_alloc() and can be used as a replacement for
module_alloc().
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
---
include/linux/execmem.h | 8 ++++++++
include/linux/moduleloader.h | 12 ------------
kernel/module/main.c | 7 -------
mm/execmem.c | 12 ++++++++----
4 files changed, 16 insertions(+), 23 deletions(-)
@@ -25,10 +25,6 @@ int module_frob_arch_sections(Elf_Ehdr *hdr,/* Additional bytes needed by arch in front of individual sections */unsignedintarch_mod_section_prepend(structmodule*mod,unsignedintsection);-/* Allocator used for allocating struct module, core sections and init-sections.ReturnsNULLonfailure.*/-void*module_alloc(unsignedlongsize);-/* Determines if the section name is an init section (that is only used during*moduleloading).*/
From: Song Liu <song@kernel.org> Date: 2023-06-16 18:56:31
On Fri, Jun 16, 2023 at 1:51 AM Mike Rapoport [off-list ref] wrote:
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
Define default parameters for address range for code allocations using
the current values in module_alloc() and make execmem_text_alloc() use
these defaults when an architecure does not supply its specific
parameters.
With this, execmem_text_alloc() implements memory allocation in a way
compatible with module_alloc() and can be used as a replacement for
module_alloc().
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
@@ -25,10 +25,6 @@ int module_frob_arch_sections(Elf_Ehdr *hdr,/* Additional bytes needed by arch in front of individual sections */unsignedintarch_mod_section_prepend(structmodule*mod,unsignedintsection);-/* Allocator used for allocating struct module, core sections and init-sections.ReturnsNULLonfailure.*/-void*module_alloc(unsignedlongsize);-/* Determines if the section name is an init section (that is only used during*moduleloading).*/
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-16 08:52:39
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
Data related to code allocations, such as module data section, need to
comply with architecture constraints for its placement and its
allocation right now was done using execmem_text_alloc().
Create a dedicated API for allocating data related to code allocations
and allow architectures to define address ranges for data allocations.
Since currently this is only relevant for powerpc variants that use the
VMALLOC address space for module data allocations, automatically reuse
address ranges defined for text unless address range for data is
explicitly defined by an architecture.
With separation of code and data allocations, data sections of the
modules are now mapped as PAGE_KERNEL rather than PAGE_KERNEL_EXEC which
was a default on many architectures.
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
---
arch/powerpc/kernel/module.c | 8 +++++++
include/linux/execmem.h | 18 +++++++++++++++
kernel/module/main.c | 15 +++----------
mm/execmem.c | 43 ++++++++++++++++++++++++++++++++++++
4 files changed, 72 insertions(+), 12 deletions(-)
From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com> Date: 2023-06-16 16:59:13
On Fri, 2023-06-16 at 11:50 +0300, Mike Rapoport wrote:
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
Data related to code allocations, such as module data section, need
to
comply with architecture constraints for its placement and its
allocation right now was done using execmem_text_alloc().
Create a dedicated API for allocating data related to code
allocations
and allow architectures to define address ranges for data
allocations.
Right now the cross-arch way to specify kernel memory permissions is
encoded in the function names of all the set_memory_foo()'s. You can't
just have unified prot names because some arch's have NX and some have
X bits, etc. CPA wouldn't know if it needs to set or unset a bit if you
pass in a PROT.
But then you end up with a new function for *each* combination (i.e.
set_memory_rox()). I wish CPA has flags like mmap() does, and I wonder
if it makes sense here instead of execmem_data_alloc().
Maybe that is an overhaul for another day though...
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-17 06:45:41
On Fri, Jun 16, 2023 at 04:55:53PM +0000, Edgecombe, Rick P wrote:
On Fri, 2023-06-16 at 11:50 +0300, Mike Rapoport wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
Data related to code allocations, such as module data section, need
to
comply with architecture constraints for its placement and its
allocation right now was done using execmem_text_alloc().
Create a dedicated API for allocating data related to code
allocations
and allow architectures to define address ranges for data
allocations.
Right now the cross-arch way to specify kernel memory permissions is
encoded in the function names of all the set_memory_foo()'s. You can't
just have unified prot names because some arch's have NX and some have
X bits, etc. CPA wouldn't know if it needs to set or unset a bit if you
pass in a PROT.
The idea is that allocator never uses CPA. It allocates with the
permissions defined by architecture at the first place and then the callers
might use CPA to change them. Ideally, that shouldn't be needed for
anything but ro_after_init, but we are far from there.
But then you end up with a new function for *each* combination (i.e.
set_memory_rox()). I wish CPA has flags like mmap() does, and I wonder
if it makes sense here instead of execmem_data_alloc().
I don't think execmem should handle all the combinations. The code is
always allocated as ROX for architectures that support text poking and as
RWX for those that don't.
Maybe execmem_data_alloc() will need to able to handle RW and RO data
differently at some point, but that's the only variant I can think of, but
even then I don't expect CPA will be used by execmem.
We also might move resetting the permissions to default from vmalloc to
execmem, but again, we are far from there.
Maybe that is an overhaul for another day though...
CPA surely needs some love :)
--
Sincerely yours,
Mike.
From: Song Liu <song@kernel.org> Date: 2023-06-16 20:02:18
On Fri, Jun 16, 2023 at 1:51 AM Mike Rapoport [off-list ref] wrote:
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
Data related to code allocations, such as module data section, need to
comply with architecture constraints for its placement and its
allocation right now was done using execmem_text_alloc().
Create a dedicated API for allocating data related to code allocations
and allow architectures to define address ranges for data allocations.
Since currently this is only relevant for powerpc variants that use the
VMALLOC address space for module data allocations, automatically reuse
address ranges defined for text unless address range for data is
explicitly defined by an architecture.
With separation of code and data allocations, data sections of the
modules are now mapped as PAGE_KERNEL rather than PAGE_KERNEL_EXEC which
was a default on many architectures.
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
Do we really need to check each of these? IOW, can we do:
if (!pgprot_val(execmem_params.modules.data.pgprot)) {
execmem_params.modules.data.pgprot = PAGE_KERNEL;
execmem_params.modules.data.alignment = m->text.alignment;
execmem_params.modules.data.start = m->text.start;
execmem_params.modules.data.end = m->text.end;
execmem_params.modules.data.fallback_start = m->text.fallback_start;
execmem_params.modules.data.fallback_end = m->text.fallback_end;
}
Thanks,
Song
[...]
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-17 06:52:05
On Fri, Jun 16, 2023 at 01:01:08PM -0700, Song Liu wrote:
On Fri, Jun 16, 2023 at 1:51 AM Mike Rapoport [off-list ref] wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
Data related to code allocations, such as module data section, need to
comply with architecture constraints for its placement and its
allocation right now was done using execmem_text_alloc().
Create a dedicated API for allocating data related to code allocations
and allow architectures to define address ranges for data allocations.
Since currently this is only relevant for powerpc variants that use the
VMALLOC address space for module data allocations, automatically reuse
address ranges defined for text unless address range for data is
explicitly defined by an architecture.
With separation of code and data allocations, data sections of the
modules are now mapped as PAGE_KERNEL rather than PAGE_KERNEL_EXEC which
was a default on many architectures.
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
This also fills in jit.text (next patch), so _data doesn't work here :)
And it's not really a default, the defaults are set explicitly for arches
that don't have execmem_arch_params.
Do we really need to check each of these? IOW, can we do:
if (!pgprot_val(execmem_params.modules.data.pgprot)) {
execmem_params.modules.data.pgprot = PAGE_KERNEL;
execmem_params.modules.data.alignment = m->text.alignment;
execmem_params.modules.data.start = m->text.start;
execmem_params.modules.data.end = m->text.end;
execmem_params.modules.data.fallback_start = m->text.fallback_start;
execmem_params.modules.data.fallback_end = m->text.fallback_end;
}
Having _seven_ intermediate variables to fill _eight_ arguments of a
function instead of handing in @size and a proper struct pointer is
tasteless and disgusting at best.
Six out of those seven parameters are from:
execmem_params.module.data
while the KASAN shadow part is retrieved from
execmem_params.module.flags
So what prevents you from having a uniform data structure, which is
extensible and decribes _all_ types of allocations?
Absolutely nothing. The flags part can either be in the type dependend
part or you make the type configs an array as I had suggested originally
and then execmem_alloc() becomes:
void *execmem_alloc(type, size)
and
static inline void *execmem_data_alloc(size_t size)
{
return execmem_alloc(EXECMEM_TYPE_DATA, size);
}
which gets the type independent parts from @execmem_param.
Just read through your own series and watch the evolution of
execmem_alloc():
static void *execmem_alloc(size_t size)
static void *execmem_alloc(size_t size, unsigned long start,
unsigned long end, unsigned int align,
pgprot_t pgprot)
static void *execmem_alloc(size_t len, unsigned long start,
unsigned long end, unsigned int align,
pgprot_t pgprot,
unsigned long fallback_start,
unsigned long fallback_end,
bool kasan)
In a month from now this function will have _ten_ parameters and tons of
horrible wrappers which convert an already existing data structure into
individual function arguments.
Seriously?
If you want this function to be [ab]used outside of the exec_param
configuration space for whatever non-sensical reasons then this still
can be either:
void *execmem_alloc(params, type, size)
static inline void *execmem_data_alloc(size_t size)
{
return execmem_alloc(&exec_param, EXECMEM_TYPE_DATA, size);
}
or
void *execmem_alloc(type_params, size);
static inline void *execmem_data_alloc(size_t size)
{
return execmem_alloc(&exec_param.data, size);
}
which both allows you to provide alternative params, right?
Coming back to my conversation with Song:
"Bad programmers worry about the code. Good programmers worry about
data structures and their relationships."
You might want to reread:
https://lore.kernel.org/r/87lenuukj0.ffs@tglx
and the subsequent thread.
The fact that my suggestions had a 'mod_' namespace prefix does not make
any of my points moot.
Song did an extremly good job in abstracting things out, but you decided
to ditch his ground work instead of building on it and keeping the good
parts. That's beyond sad.
Worse, you went the full 'not invented here' path with an outcome which is
even worse than the original hackery from Song which started the above
referenced thread.
I don't know what caused you to decide that ad hoc hackery is better
than proper data structure based design patterns. I actually don't want
to know.
As much as my voice counts:
NAK-ed-by: Thomas Gleixner [off-list ref]
Thanks,
tglx
Having _seven_ intermediate variables to fill _eight_ arguments of a
function instead of handing in @size and a proper struct pointer is
tasteless and disgusting at best.
Six out of those seven parameters are from:
execmem_params.module.data
while the KASAN shadow part is retrieved from
execmem_params.module.flags
So what prevents you from having a uniform data structure, which is
extensible and decribes _all_ types of allocations?
Absolutely nothing. The flags part can either be in the type dependend
part or you make the type configs an array as I had suggested originally
and then execmem_alloc() becomes:
void *execmem_alloc(type, size)
and
static inline void *execmem_data_alloc(size_t size)
{
return execmem_alloc(EXECMEM_TYPE_DATA, size);
}
which gets the type independent parts from @execmem_param.
Just read through your own series and watch the evolution of
execmem_alloc():
static void *execmem_alloc(size_t size)
static void *execmem_alloc(size_t size, unsigned long start,
unsigned long end, unsigned int align,
pgprot_t pgprot)
static void *execmem_alloc(size_t len, unsigned long start,
unsigned long end, unsigned int align,
pgprot_t pgprot,
unsigned long fallback_start,
unsigned long fallback_end,
bool kasan)
In a month from now this function will have _ten_ parameters and tons of
horrible wrappers which convert an already existing data structure into
individual function arguments.
Seriously?
If you want this function to be [ab]used outside of the exec_param
configuration space for whatever non-sensical reasons then this still
can be either:
void *execmem_alloc(params, type, size)
static inline void *execmem_data_alloc(size_t size)
{
return execmem_alloc(&exec_param, EXECMEM_TYPE_DATA, size);
}
or
void *execmem_alloc(type_params, size);
static inline void *execmem_data_alloc(size_t size)
{
return execmem_alloc(&exec_param.data, size);
}
which both allows you to provide alternative params, right?
Coming back to my conversation with Song:
"Bad programmers worry about the code. Good programmers worry about
data structures and their relationships."
Thomas, you're confusing an internal interface with external, I made the
same mistake reviewing Song's patchset...
From: Thomas Gleixner <hidden> Date: 2023-06-19 00:44:09
Kent!
On Sun, Jun 18 2023 at 19:14, Kent Overstreet wrote:
On Mon, Jun 19, 2023 at 12:32:55AM +0200, Thomas Gleixner wrote:
Thomas, you're confusing an internal interface with external
No. I am not.
Whether that's an internal function or not does not make any difference
at all.
Having a function growing _eight_ parameters where _six_ of them are
derived from a well defined data structure is a clear sign of design
fail.
It's not rocket science to do:
struct generic_allocation_info {
....
};
struct execmem_info {
....
struct generic_allocation_info alloc_info;
;
struct execmem_param {
...
struct execmem_info[NTYPES];
};
and having a function which can either operate on execmem_param and type
or on generic_allocation_info itself. It does not matter as long as the
data structure which is handed into this internal function is
describing it completely or needs a supplementary argument, i.e. flags.
Having tons of wrappers which do:
a = generic_info.a;
b = generic_info.b;
....
n = generic_info.n;
internal_func(a, b, ....,, n);
is just hillarious and to repeat myself tasteless and therefore
disgusting.
That's CS course first semester hackery, but TBH, I can only tell from
imagination because I did not take CS courses - maybe that's the
problem...
Data structure driven design works not from the usage site down to the
internals. It's the other way round:
1) Define a data structure which describes what the internal function
needs to know
2) Implement use case specific variants which describe that
3) Hand the use case specific variant to the internal function
eventually with some minimal supplementary information.
Object oriented basics, right?
There is absolutely nothing wrong with having
internal_func(generic_info *, modifier);
but having
internal_func(a, b, ... n)
is fundamentally wrong in the context of an extensible and future proof
internal function.
Guess what. Today it's sufficient to have _eight_ arguments and we are
happy to have 10 nonsensical wrappers around this internal
function. Tomorrow there happens to be a use case which needs another
argument so you end up:
Changing 10 wrappers plus the function declaration and definition in
one go
instead of adding
One new naturally 0 initialized member to generic_info and be done
with it.
Look at the evolution of execmem_alloc() in this very pathset which I
pointed out. That very patchset covers _two_ of at least _six_ cases
Song and myself identified. It already had _three_ steps of evolution
from _one_ to _five_ to _eight_ parameters.
C is not like e.g. python where you can "solve" that problem by simply
doing:
- internal_func(a, b, c):
+ internal_func(a, b, c, d=None, ..., n=None):
But that's not a solution either. That's a horrible workaround even in
python once your parameter space gets sufficiently large. The way out in
python is to have **kwargs. But that's not an option in C, and not
necessarily the best option for python either.
Even in python or any other object oriented language you get to the
point where you have to rethink your approach, go back to the drawing
board and think about data representation.
But creating a new interface based on "let's see what we need over
time and add parameters as we see fit" is simply wrong to begin with
independent of the programming language.
Even if the _eight_ parameters are the end of the range, then they are
beyond justifyable because that's way beyond the natural register
argument space of any architecture and you are offloading your lazyness
to wrappers and the compiler to emit pointlessly horrible code.
There is a reason why new syscalls which need more than a few parameters
are based on 'struct DATA_WHICH_I_NEED_TO_KNOW' and 'flags'.
We've got burned on the non-extensibilty often enough. Why would a new
internal function have any different requirements especially as it is
neither implemented to the full extent nor a hotpath function?
Now you might argue that it _is_ a "hotpath" due to the BPF usage, but
then even more so as any intermediate wrapper which converts from one
data representation to another data representation is not going to
increase performance, right?
... I made the same mistake reviewing Song's patchset...
Songs series had rough edges, but was way more data structure driven
and palatable than this hackery.
The fact that you made a mistake while reviewing Songs series has
absolutely nothing to do with the above or my previous reply to Mike.
Thanks,
tglx
From: Kent Overstreet <kent.overstreet@linux.dev> Date: 2023-06-19 02:12:25
On Mon, Jun 19, 2023 at 02:43:58AM +0200, Thomas Gleixner wrote:
Kent!
Hi Thomas :)
No. I am not.
Ok.
Whether that's an internal function or not does not make any difference
at all.
Well, at the risk of this discussion going completely off the rails, I
have to disagree with you there. External interfaces and high level
semantics are more important to get right from the outset, internal
implementation details can be cleaned up later, within reason.
And the discussion on this patchset has been more focused on those
external interfaces, which seems like the right approach to me.
quoted
... I made the same mistake reviewing Song's patchset...
Songs series had rough edges, but was way more data structure driven
and palatable than this hackery.
I liked that aspect of Song's patchset too, and I'm actually inclined to
agree with you that this patchset might get a bit cleaner with more of
that, but really, this semes like just quibbling over calling convention
for an internal helper function.
From: Steven Rostedt <rostedt@goodmis.org> Date: 2023-06-20 14:51:24
On Mon, 19 Jun 2023 02:43:58 +0200
Thomas Gleixner [off-list ref] wrote:
Now you might argue that it _is_ a "hotpath" due to the BPF usage, but
then even more so as any intermediate wrapper which converts from one
data representation to another data representation is not going to
increase performance, right?
Just as a side note. BPF can not attach its return calling code to
functions that have more than 6 parameters (3 on 32 bit x86), because of
the way BPF return path trampoline works. It is a requirement that all
parameters live in registers, and none on the stack.
-- Steve
On Tue, Jun 20, 2023 at 7:51 AM Steven Rostedt [off-list ref] wrote:
On Mon, 19 Jun 2023 02:43:58 +0200
Thomas Gleixner [off-list ref] wrote:
quoted
Now you might argue that it _is_ a "hotpath" due to the BPF usage, but
then even more so as any intermediate wrapper which converts from one
data representation to another data representation is not going to
increase performance, right?
Just as a side note. BPF can not attach its return calling code to
functions that have more than 6 parameters (3 on 32 bit x86), because of
the way BPF return path trampoline works. It is a requirement that all
parameters live in registers, and none on the stack.
It's actually 7 and that restriction is being lifted.
The patch set to attach to <= 12 is being discussed.
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-19 15:24:22
On Mon, Jun 19, 2023 at 12:32:55AM +0200, Thomas Gleixner wrote:
Mike!
Sorry for being late on this ...
On Fri, Jun 16 2023 at 11:50, Mike Rapoport wrote:
The fact that my suggestions had a 'mod_' namespace prefix does not make
any of my points moot.
The prefix does not matter. What matters is what we are trying to abstract.
Your suggestion is based of the memory used by modules. I'm abstracting
address spaces for different types of executable and related memory. They
are similar, yes, but they are not the same.
The TEXT, INIT_TEXT and *_DATA do not match to what we have from arch POV.
They have modules with text, rw data, ro data and ro after init data and
the memory for the generated code. The memory for modules and memory for
other users have different restrictions for their placement, so using a
single TEXT type for them is semantically wrong. BPF and kprobes do not
necessarily must be at the same address range as modules and init text does
not differ from normal text.
Song did an extremly good job in abstracting things out, but you decided
to ditch his ground work instead of building on it and keeping the good
parts. That's beyond sad.
Actually not. The core idea to describe address range suitable for code
allocations with a structure and have arch code initialize this structure
at boot and be done with it is the same. But I don't think vmalloc
parameters belong there, they should be completely encapsulated in the
allocator. Having fallback range named explicitly is IMO clearer than an
array of address spaces.
I accept your point that the structures describing ranges for different
types should be unified and I've got carried away with making the wrappers
to convert that structure to parameters to the core allocation function.
I've chosen to define ranges as fields in the containing structure rather
than enum with types and an array because I strongly feel that the callers
should not care about these parameters. These parameters are defined by
architecture and the callers should not need to know how each and every
arch defines restrictions suitable for modules, bpf or kprobes.
That's also the reason to have different names for API calls, exactly to
avoid having alloc(KPROBES,...), alloc(BPF, ...), alloc(MODULES, ...) an so
on.
All in all, if I filter all the ranting, this boils down to having a
unified structure for all the address ranges and passing this structure
from the wrappers to the core alloc as is rather that translating it to
separate parameters, with which I agree.
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-16 08:52:58
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
The memory allocations for kprobes on arm64 can be placed anywhere in
vmalloc address space and currently this is implemented with an override
of alloc_insn_page() in arm64.
Extend execmem_params with a range for generated code allocations and
make kprobes on arm64 use this extension rather than override
alloc_insn_page().
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
---
arch/arm64/kernel/module.c | 9 +++++++++
arch/arm64/kernel/probes/kprobes.c | 7 -------
include/linux/execmem.h | 11 +++++++++++
mm/execmem.c | 14 +++++++++++++-
4 files changed, 33 insertions(+), 8 deletions(-)
@@ -129,13 +129,6 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)return0;}-void*alloc_insn_page(void)-{-return__vmalloc_node_range(PAGE_SIZE,1,VMALLOC_START,VMALLOC_END,-GFP_KERNEL,PAGE_KERNEL_ROX,VM_FLUSH_RESET_PERMS,-NUMA_NO_NODE,__builtin_return_address(0));-}-/* arm kprobe: install breakpoint in text */void__kprobesarch_arm_kprobe(structkprobe*p){
From: Song Liu <song@kernel.org> Date: 2023-06-16 20:05:50
On Fri, Jun 16, 2023 at 1:52 AM Mike Rapoport [off-list ref] wrote:
quoted hunk
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
The memory allocations for kprobes on arm64 can be placed anywhere in
vmalloc address space and currently this is implemented with an override
of alloc_insn_page() in arm64.
Extend execmem_params with a range for generated code allocations and
make kprobes on arm64 use this extension rather than override
alloc_insn_page().
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
---
arch/arm64/kernel/module.c | 9 +++++++++
arch/arm64/kernel/probes/kprobes.c | 7 -------
include/linux/execmem.h | 11 +++++++++++
mm/execmem.c | 14 +++++++++++++-
4 files changed, 33 insertions(+), 8 deletions(-)
This is growing fast. :) We have 3 now: text, data, jit. And it will be
5 when we split data into rw data, ro data, ro after init data. I wonder
whether we should still do some type enum here. But we can revisit
this topic later.
Other than that
Acked-by: Song Liu <song@kernel.org>
From: Mike Rapoport <rppt@kernel.org> Date: 2023-06-17 06:58:56
On Fri, Jun 16, 2023 at 01:05:29PM -0700, Song Liu wrote:
On Fri, Jun 16, 2023 at 1:52 AM Mike Rapoport [off-list ref] wrote:
quoted
From: "Mike Rapoport (IBM)" <rppt@kernel.org>
The memory allocations for kprobes on arm64 can be placed anywhere in
vmalloc address space and currently this is implemented with an override
of alloc_insn_page() in arm64.
Extend execmem_params with a range for generated code allocations and
make kprobes on arm64 use this extension rather than override
alloc_insn_page().
Signed-off-by: Mike Rapoport (IBM) <rppt@kernel.org>
---
arch/arm64/kernel/module.c | 9 +++++++++
arch/arm64/kernel/probes/kprobes.c | 7 -------
include/linux/execmem.h | 11 +++++++++++
mm/execmem.c | 14 +++++++++++++-
4 files changed, 33 insertions(+), 8 deletions(-)
This is growing fast. :) We have 3 now: text, data, jit. And it will be
5 when we split data into rw data, ro data, ro after init data. I wonder
whether we should still do some type enum here. But we can revisit
this topic later.
I don't think we'd need 5. Four at most :)
I don't know yet what would be the best way to differentiate RW and RO
data, but ro_after_init surely won't need a new type. It either will be
allocated as RW and then the caller will have to set it RO after
initialization is done, or it will be allocated as RO and the caller will
have to do something like text_poke to update it.
Other than that
Acked-by: Song Liu <song@kernel.org>