On Tue, May 07, 2024 at 03:35:58PM -0700, Yang Shi wrote:
The atomic RMW instructions, for example, ldadd, actually does load +I'd also argue that this should be optimised in openjdk. Is an LDADD
add + store in one instruction, it may trigger two page faults, the
first fault is a read fault, the second fault is a write fault.
Some applications use atomic RMW instructions to populate memory, for
example, openjdk uses atomic-add-0 to do pretouch (populate heap memory
at launch time) between v18 and v22.
more efficient on your hardware than a plain STR? I hope it only does
one operation per page rather than per long. There's also MAP_POPULATE
that openjdk can use to pre-fault the pages with no additional fault.
This would be even more efficient than any store or atomic operation.
Not sure the reason for the architecture to report a read fault only on
atomics. Looking at the pseudocode, it checks for both but the read
permission takes priority. Also in case of a translation fault (which is
what we get on the first fault), I think the syndrome write bit is
populated as (!read && write), so 0 since 'read' is 1 for atomics.
But the double page fault has some problems:I can see why the current behaviour is not ideal but I can't tell why
1. Noticeable TLB overhead. The kernel actually installs zero page with
readonly PTE for the read fault. The write fault will trigger a
write-protection fault (CoW). The CoW will allocate a new page and
make the PTE point to the new page, this needs TLB invalidations. The
tlb invalidation and the mandatory memory barriers may incur
significant overhead, particularly on the machines with many cores.
openjdk does it this way either.
A bigger hammer would be to implement mm_forbids_zeropage() but this may
affect some workloads that rely on sparsely populated large arrays.
diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.hThis looks correct, it covers the LDADD and SWP instructions. However,
index db1aeacd4cd9..5d5a3fbeecc0 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -319,6 +319,7 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
* "-" means "don't care"
*/
__AARCH64_INSN_FUNCS(class_branch_sys, 0x1c000000, 0x14000000)
+__AARCH64_INSN_FUNCS(class_atomic, 0x3b200c00, 0x38200000)
one concern is whether future architecture versions will add some
instructions in this space that are allowed to do a read only operation
(e.g. skip writing if the value is the same or fails some comparison).
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.cI'd avoid the goto if possible. Even better, move this higher up into
index 8251e2fea9c7..f7bceedf5ef3 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -529,6 +529,7 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
unsigned int mm_flags = FAULT_FLAG_DEFAULT;
unsigned long addr = untagged_addr(far);
struct vm_area_struct *vma;
+ unsigned int insn;
if (kprobe_page_fault(regs, esr))
return 0;
@@ -586,6 +587,24 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
if (!vma)
goto lock_mmap;
+ if (mm_flags & (FAULT_FLAG_WRITE | FAULT_FLAG_INSTRUCTION))
+ goto continue_fault;
the block of if/else statements building the vm_flags and mm_flags.
Factor out the checks into a different function - is_el0_atomic_instr()
or something.
+This prevents recursively entering do_page_fault() but it may be worth
+ pagefault_disable();
testing it with an execute-only permission.
+The above would need to check if the fault is coming from a 64-bit user
+ if (get_user(insn, (unsigned int __user *) instruction_pointer(regs))) {
+ pagefault_enable();
+ goto continue_fault;
+ }
+
+ if (aarch64_insn_is_class_atomic(insn)) {
+ vm_flags = VM_WRITE;
+ mm_flags |= FAULT_FLAG_WRITE;
+ }
mode, otherwise the decoding wouldn't make sense:
if (!user_mode(regs) || compat_user_mode(regs))
return false;
(assuming a separate function that checks the above and returns a bool;
you'd need to re-enable the page faults)
You also need to take care of endianness since the instructions are
always little-endian. We use a similar pattern in user_insn_read():
u32 instr;
__le32 instr_le;
if (get_user(instr_le, (__le32 __user *)instruction_pointer(regs)))
return false;
instr = le32_to_cpu(inst_le);
That said, I'm not keen on this kernel workaround. If openjdk decides to
improve some security and goes for PROT_EXEC-only mappings of its text
sections, the above trick will no longer work.