| Message ID | 20230908144320.2474-3-puranjay12@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf, arm64: use BPF prog pack allocator in BPF JIT | expand |
On 9/8/2023 10:43 PM, Puranjay Mohan wrote: > The BPF JIT needs to write invalid instructions to RX regions of memory > to invalidate removed BPF programs. This needs a function like memset() > that can work with RX memory. > > Implement aarch64_insn_set() which is similar to text_poke_set() of x86. > > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> > --- > arch/arm64/include/asm/patching.h | 1 + > arch/arm64/kernel/patching.c | 40 +++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+) > > diff --git a/arch/arm64/include/asm/patching.h b/arch/arm64/include/asm/patching.h > index f78a0409cbdb..551933338739 100644 > --- a/arch/arm64/include/asm/patching.h > +++ b/arch/arm64/include/asm/patching.h > @@ -8,6 +8,7 @@ int aarch64_insn_read(void *addr, u32 *insnp); > int aarch64_insn_write(void *addr, u32 insn); > > int aarch64_insn_write_literal_u64(void *addr, u64 val); > +int aarch64_insn_set(void *dst, const u32 insn, size_t len); > void *aarch64_insn_copy(void *dst, const void *src, size_t len); > > int aarch64_insn_patch_text_nosync(void *addr, u32 insn); > diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c > index 243d6ae8d2d8..63d9e0e77806 100644 > --- a/arch/arm64/kernel/patching.c > +++ b/arch/arm64/kernel/patching.c > @@ -146,6 +146,46 @@ noinstr void *aarch64_insn_copy(void *dst, const void *src, size_t len) > return dst; > } > > +/** > + * aarch64_insn_set - memset for RX memory regions. > + * @dst: address to modify > + * @c: value to set insn > + * @len: length of memory region. > + * > + * Useful for JITs to fill regions of RX memory with illegal instructions. > + */ > +noinstr int aarch64_insn_set(void *dst, const u32 insn, size_t len) const is unnecessary > +{ > + unsigned long flags; > + size_t patched = 0; > + size_t size; > + void *waddr; > + void *ptr; > + > + /* A64 instructions must be word aligned */ > + if ((uintptr_t)dst & 0x3) > + return -EINVAL; > + > + raw_spin_lock_irqsave(&patch_lock, flags); > + > + while (patched < len) { > + ptr = dst + patched; > + size = min_t(size_t, PAGE_SIZE - offset_in_page(ptr), > + len - patched); > + > + waddr = patch_map(ptr, FIX_TEXT_POKE0); > + memset32(waddr, insn, size / 4); > + patch_unmap(FIX_TEXT_POKE0); > + > + patched += size; > + } > + raw_spin_unlock_irqrestore(&patch_lock, flags); > + > + caches_clean_inval_pou((uintptr_t)dst, (uintptr_t)dst + len); > + > + return 0; > +} > + this function shares most of the code with aarch64_insn_copy(), how about extracting the shared code to a separate function? > int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn) > { > u32 *tp = addr;
Xu Kuohai <xukuohai@huaweicloud.com> writes: > On 9/8/2023 10:43 PM, Puranjay Mohan wrote: >> The BPF JIT needs to write invalid instructions to RX regions of memory >> to invalidate removed BPF programs. This needs a function like memset() >> that can work with RX memory. >> >> Implement aarch64_insn_set() which is similar to text_poke_set() of x86. >> >> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> >> --- >> arch/arm64/include/asm/patching.h | 1 + >> arch/arm64/kernel/patching.c | 40 +++++++++++++++++++++++++++++++ >> 2 files changed, 41 insertions(+) >> >> diff --git a/arch/arm64/include/asm/patching.h b/arch/arm64/include/asm/patching.h >> index f78a0409cbdb..551933338739 100644 >> --- a/arch/arm64/include/asm/patching.h >> +++ b/arch/arm64/include/asm/patching.h >> @@ -8,6 +8,7 @@ int aarch64_insn_read(void *addr, u32 *insnp); >> int aarch64_insn_write(void *addr, u32 insn); >> >> int aarch64_insn_write_literal_u64(void *addr, u64 val); >> +int aarch64_insn_set(void *dst, const u32 insn, size_t len); >> void *aarch64_insn_copy(void *dst, const void *src, size_t len); >> >> int aarch64_insn_patch_text_nosync(void *addr, u32 insn); >> diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c >> index 243d6ae8d2d8..63d9e0e77806 100644 >> --- a/arch/arm64/kernel/patching.c >> +++ b/arch/arm64/kernel/patching.c >> @@ -146,6 +146,46 @@ noinstr void *aarch64_insn_copy(void *dst, const void *src, size_t len) >> return dst; >> } >> >> +/** >> + * aarch64_insn_set - memset for RX memory regions. >> + * @dst: address to modify >> + * @c: value to set > > insn Thanks for catching. >> + * @len: length of memory region. >> + * >> + * Useful for JITs to fill regions of RX memory with illegal instructions. >> + */ >> +noinstr int aarch64_insn_set(void *dst, const u32 insn, size_t len) > > const is unnecessary > Will remove in next version. >> +{ >> + unsigned long flags; >> + size_t patched = 0; >> + size_t size; >> + void *waddr; >> + void *ptr; >> + >> + /* A64 instructions must be word aligned */ >> + if ((uintptr_t)dst & 0x3) >> + return -EINVAL; >> + >> + raw_spin_lock_irqsave(&patch_lock, flags); >> + >> + while (patched < len) { >> + ptr = dst + patched; >> + size = min_t(size_t, PAGE_SIZE - offset_in_page(ptr), >> + len - patched); >> + >> + waddr = patch_map(ptr, FIX_TEXT_POKE0); >> + memset32(waddr, insn, size / 4); >> + patch_unmap(FIX_TEXT_POKE0); >> + >> + patched += size; >> + } >> + raw_spin_unlock_irqrestore(&patch_lock, flags); >> + >> + caches_clean_inval_pou((uintptr_t)dst, (uintptr_t)dst + len); >> + >> + return 0; >> +} >> + > > this function shares most of the code with aarch64_insn_copy(), how about > extracting the shared code to a separate function? I was thinking of writing it like the text_poke api of x86. Where you can provide a function as an argument to work on a memory area. Essentially, it will look like: typedef int text_poke_f(void *dst, const void *src, size_t len); static void *aarch64_insn_poke(text_poke_f func, void *addr, const void *src, size_t len) We can call this function with a wrapper of `copy_to_kernel_nofault` for copy and with a wrapper of memset32 for setting. Do you think this is a good approach? > >> int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn) >> { >> u32 *tp = addr; Thanks, Puranjay
On 9/21/2023 10:50 PM, Puranjay Mohan wrote: > Xu Kuohai <xukuohai@huaweicloud.com> writes: > >> On 9/8/2023 10:43 PM, Puranjay Mohan wrote: >>> The BPF JIT needs to write invalid instructions to RX regions of memory >>> to invalidate removed BPF programs. This needs a function like memset() >>> that can work with RX memory. >>> >>> Implement aarch64_insn_set() which is similar to text_poke_set() of x86. >>> >>> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> >>> --- >>> arch/arm64/include/asm/patching.h | 1 + >>> arch/arm64/kernel/patching.c | 40 +++++++++++++++++++++++++++++++ >>> 2 files changed, 41 insertions(+) >>> >>> diff --git a/arch/arm64/include/asm/patching.h b/arch/arm64/include/asm/patching.h >>> index f78a0409cbdb..551933338739 100644 >>> --- a/arch/arm64/include/asm/patching.h >>> +++ b/arch/arm64/include/asm/patching.h >>> @@ -8,6 +8,7 @@ int aarch64_insn_read(void *addr, u32 *insnp); >>> int aarch64_insn_write(void *addr, u32 insn); >>> >>> int aarch64_insn_write_literal_u64(void *addr, u64 val); >>> +int aarch64_insn_set(void *dst, const u32 insn, size_t len); >>> void *aarch64_insn_copy(void *dst, const void *src, size_t len); >>> >>> int aarch64_insn_patch_text_nosync(void *addr, u32 insn); >>> diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c >>> index 243d6ae8d2d8..63d9e0e77806 100644 >>> --- a/arch/arm64/kernel/patching.c >>> +++ b/arch/arm64/kernel/patching.c >>> @@ -146,6 +146,46 @@ noinstr void *aarch64_insn_copy(void *dst, const void *src, size_t len) >>> return dst; >>> } >>> >>> +/** >>> + * aarch64_insn_set - memset for RX memory regions. >>> + * @dst: address to modify >>> + * @c: value to set >> >> insn > > Thanks for catching. > >>> + * @len: length of memory region. >>> + * >>> + * Useful for JITs to fill regions of RX memory with illegal instructions. >>> + */ >>> +noinstr int aarch64_insn_set(void *dst, const u32 insn, size_t len) >> >> const is unnecessary >> > > Will remove in next version. > >>> +{ >>> + unsigned long flags; >>> + size_t patched = 0; >>> + size_t size; >>> + void *waddr; >>> + void *ptr; >>> + >>> + /* A64 instructions must be word aligned */ >>> + if ((uintptr_t)dst & 0x3) >>> + return -EINVAL; >>> + >>> + raw_spin_lock_irqsave(&patch_lock, flags); >>> + >>> + while (patched < len) { >>> + ptr = dst + patched; >>> + size = min_t(size_t, PAGE_SIZE - offset_in_page(ptr), >>> + len - patched); >>> + >>> + waddr = patch_map(ptr, FIX_TEXT_POKE0); >>> + memset32(waddr, insn, size / 4); >>> + patch_unmap(FIX_TEXT_POKE0); >>> + >>> + patched += size; >>> + } >>> + raw_spin_unlock_irqrestore(&patch_lock, flags); >>> + >>> + caches_clean_inval_pou((uintptr_t)dst, (uintptr_t)dst + len); >>> + >>> + return 0; >>> +} >>> + >> >> this function shares most of the code with aarch64_insn_copy(), how about >> extracting the shared code to a separate function? > > I was thinking of writing it like the text_poke api of x86. Where you > can provide a function as an argument to work on a memory area. > Essentially, it will look like: > > typedef int text_poke_f(void *dst, const void *src, size_t len); > > static void *aarch64_insn_poke(text_poke_f func, void *addr, const void *src, size_t len) > > We can call this function with a wrapper of `copy_to_kernel_nofault` for copy > and with a wrapper of memset32 for setting. > > Do you think this is a good approach? > Sounds great, thanks. >> >>> int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn) >>> { >>> u32 *tp = addr; > > Thanks, > Puranjay
On Fri, Sep 08, 2023 at 02:43:19PM +0000, Puranjay Mohan wrote: > The BPF JIT needs to write invalid instructions to RX regions of memory > to invalidate removed BPF programs. This needs a function like memset() > that can work with RX memory. > > Implement aarch64_insn_set() which is similar to text_poke_set() of x86. > > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> > --- > arch/arm64/include/asm/patching.h | 1 + > arch/arm64/kernel/patching.c | 40 +++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+) > > diff --git a/arch/arm64/include/asm/patching.h b/arch/arm64/include/asm/patching.h > index f78a0409cbdb..551933338739 100644 > --- a/arch/arm64/include/asm/patching.h > +++ b/arch/arm64/include/asm/patching.h > @@ -8,6 +8,7 @@ int aarch64_insn_read(void *addr, u32 *insnp); > int aarch64_insn_write(void *addr, u32 insn); > > int aarch64_insn_write_literal_u64(void *addr, u64 val); > +int aarch64_insn_set(void *dst, const u32 insn, size_t len); > void *aarch64_insn_copy(void *dst, const void *src, size_t len); > > int aarch64_insn_patch_text_nosync(void *addr, u32 insn); > diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c > index 243d6ae8d2d8..63d9e0e77806 100644 > --- a/arch/arm64/kernel/patching.c > +++ b/arch/arm64/kernel/patching.c > @@ -146,6 +146,46 @@ noinstr void *aarch64_insn_copy(void *dst, const void *src, size_t len) > return dst; > } > > +/** > + * aarch64_insn_set - memset for RX memory regions. > + * @dst: address to modify > + * @c: value to set > + * @len: length of memory region. > + * > + * Useful for JITs to fill regions of RX memory with illegal instructions. > + */ > +noinstr int aarch64_insn_set(void *dst, const u32 insn, size_t len) > +{ > + unsigned long flags; > + size_t patched = 0; > + size_t size; > + void *waddr; > + void *ptr; > + > + /* A64 instructions must be word aligned */ > + if ((uintptr_t)dst & 0x3) > + return -EINVAL; > + > + raw_spin_lock_irqsave(&patch_lock, flags); > + > + while (patched < len) { > + ptr = dst + patched; > + size = min_t(size_t, PAGE_SIZE - offset_in_page(ptr), > + len - patched); > + > + waddr = patch_map(ptr, FIX_TEXT_POKE0); > + memset32(waddr, insn, size / 4); Do we need to use a specific instruction passed by the caller, or can we hard-code a trapping instruction here? If we don't care about the specific instruction, it'd be best to memset this to 0, as 0x00000000 is UDF #0 (which will trap), and that way memset can use DC ZVA to zero the memory, which is faster than 4 bytes at a time. If we did that, we can rename this to something like: aarch64_insn_clear(void *dst, size_t len) > + patch_unmap(FIX_TEXT_POKE0); > + > + patched += size; > + } > + raw_spin_unlock_irqrestore(&patch_lock, flags); > + > + caches_clean_inval_pou((uintptr_t)dst, (uintptr_t)dst + len); Assuming the point of this is to trap if/when we accidentally execute the freed instructions, we need an IPI here, and so this should use flush_icache_range() or make it the caller's responsibility to do so. Mark. > + > + return 0; > +} > + > int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn) > { > u32 *tp = addr; > -- > 2.40.1 > >
diff --git a/arch/arm64/include/asm/patching.h b/arch/arm64/include/asm/patching.h index f78a0409cbdb..551933338739 100644 --- a/arch/arm64/include/asm/patching.h +++ b/arch/arm64/include/asm/patching.h @@ -8,6 +8,7 @@ int aarch64_insn_read(void *addr, u32 *insnp); int aarch64_insn_write(void *addr, u32 insn); int aarch64_insn_write_literal_u64(void *addr, u64 val); +int aarch64_insn_set(void *dst, const u32 insn, size_t len); void *aarch64_insn_copy(void *dst, const void *src, size_t len); int aarch64_insn_patch_text_nosync(void *addr, u32 insn); diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c index 243d6ae8d2d8..63d9e0e77806 100644 --- a/arch/arm64/kernel/patching.c +++ b/arch/arm64/kernel/patching.c @@ -146,6 +146,46 @@ noinstr void *aarch64_insn_copy(void *dst, const void *src, size_t len) return dst; } +/** + * aarch64_insn_set - memset for RX memory regions. + * @dst: address to modify + * @c: value to set + * @len: length of memory region. + * + * Useful for JITs to fill regions of RX memory with illegal instructions. + */ +noinstr int aarch64_insn_set(void *dst, const u32 insn, size_t len) +{ + unsigned long flags; + size_t patched = 0; + size_t size; + void *waddr; + void *ptr; + + /* A64 instructions must be word aligned */ + if ((uintptr_t)dst & 0x3) + return -EINVAL; + + raw_spin_lock_irqsave(&patch_lock, flags); + + while (patched < len) { + ptr = dst + patched; + size = min_t(size_t, PAGE_SIZE - offset_in_page(ptr), + len - patched); + + waddr = patch_map(ptr, FIX_TEXT_POKE0); + memset32(waddr, insn, size / 4); + patch_unmap(FIX_TEXT_POKE0); + + patched += size; + } + raw_spin_unlock_irqrestore(&patch_lock, flags); + + caches_clean_inval_pou((uintptr_t)dst, (uintptr_t)dst + len); + + return 0; +} + int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn) { u32 *tp = addr;
The BPF JIT needs to write invalid instructions to RX regions of memory to invalidate removed BPF programs. This needs a function like memset() that can work with RX memory. Implement aarch64_insn_set() which is similar to text_poke_set() of x86. Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> --- arch/arm64/include/asm/patching.h | 1 + arch/arm64/kernel/patching.c | 40 +++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+)