um: fix _nofault accesses

Nathan reported [1] that when built with clang, the um kernel
crashes pretty much immediately. This turned out to be an issue
with the inline assembly I had added, when clang used %rax/%eax
for both operands. Reorder it so current->thread.segv_continue
is written first, and then the lifetime of _faulted won't have
overlap with the lifetime of segv_continue.

In the email thread Benjamin also pointed out that current->mm
is only NULL for true kernel tasks, but we could do this for a
userspace task, so the current->thread.segv_continue logic must
be lifted out of the mm==NULL check.

Finally, while looking at this, put a barrier() so the NULL
assignment to thread.segv_continue cannot be reorder before
the possibly faulting operation.

Reported-by: Nathan Chancellor <nathan@kernel.org>
Closes: https://lore.kernel.org/r/20250402221254.GA384@ax162 [1]
Fixes: d1d7f01f7cd3 ("um: mark rodata read-only and implement _nofault accesses")
Tested-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>

diff --git a/arch/um/include/asm/uaccess.h b/arch/um/include/asm/uaccess.h
index 3a08f9029a3f9a22bbab4b720250d9a34050049c..1c6e0ae41b0c5a7251e5ae81bfc57b21f739cec3 100644 (file)
--- a/arch/um/include/asm/uaccess.h
+++ b/arch/um/include/asm/uaccess.h
@@ -55,6 +55,7 @@ do {                                                                  \
                goto err_label;                                         \
        }                                                               \
        *((type *)dst) = get_unaligned((type *)(src));                  \
+       barrier();                                                      \
        current->thread.segv_continue = NULL;                           \
 } while (0)
 
@@ -66,6 +67,7 @@ do {                                                                  \
        if (__faulted)                                                  \
                goto err_label;                                         \
        put_unaligned(*((type *)src), (type *)(dst));                   \
+       barrier();                                                      \
        current->thread.segv_continue = NULL;                           \
 } while (0)
 

diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
index ce073150dc20a8a72ca9f2e4e7d22595fa4a7fcd..ef2272e92a4321110f7faa3d82da763ed2eff271 100644 (file)
--- a/arch/um/kernel/trap.c
+++ b/arch/um/kernel/trap.c
@@ -225,20 +225,20 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
                        panic("Failed to sync kernel TLBs: %d", err);
                goto out;
        }
-       else if (current->mm == NULL) {
-               if (current->pagefault_disabled) {
-                       if (!mc) {
-                               show_regs(container_of(regs, struct pt_regs, regs));
-                               panic("Segfault with pagefaults disabled but no mcontext");
-                       }
-                       if (!current->thread.segv_continue) {
-                               show_regs(container_of(regs, struct pt_regs, regs));
-                               panic("Segfault without recovery target");
-                       }
-                       mc_set_rip(mc, current->thread.segv_continue);
-                       current->thread.segv_continue = NULL;
-                       goto out;
+       else if (current->pagefault_disabled) {
+               if (!mc) {
+                       show_regs(container_of(regs, struct pt_regs, regs));
+                       panic("Segfault with pagefaults disabled but no mcontext");
                }
+               if (!current->thread.segv_continue) {
+                       show_regs(container_of(regs, struct pt_regs, regs));
+                       panic("Segfault without recovery target");
+               }
+               mc_set_rip(mc, current->thread.segv_continue);
+               current->thread.segv_continue = NULL;
+               goto out;
+       }
+       else if (current->mm == NULL) {
                show_regs(container_of(regs, struct pt_regs, regs));
                panic("Segfault with no mm");
        }

diff --git a/arch/x86/um/shared/sysdep/faultinfo_32.h b/arch/x86/um/shared/sysdep/faultinfo_32.h
index ab5c8e47049c31ced1986e029dbb99e55d500b17..9193a7790a71a4c61f6bea2615fc7b40412908d8 100644 (file)
--- a/arch/x86/um/shared/sysdep/faultinfo_32.h
+++ b/arch/x86/um/shared/sysdep/faultinfo_32.h
@@ -31,8 +31,8 @@ struct faultinfo {
 
 #define ___backtrack_faulted(_faulted)                                 \
        asm volatile (                                                  \
-               "mov $0, %0\n"                                          \
                "movl $__get_kernel_nofault_faulted_%=,%1\n"            \
+               "mov $0, %0\n"                                          \
                "jmp _end_%=\n"                                         \
                "__get_kernel_nofault_faulted_%=:\n"                    \
                "mov $1, %0;"                                           \

diff --git a/arch/x86/um/shared/sysdep/faultinfo_64.h b/arch/x86/um/shared/sysdep/faultinfo_64.h
index 26fb4835d3e9a32b1f93fe4b390aa9b0f28765fe..61e4ca1e0ab58d8b37af68d1232bcde0931f24e1 100644 (file)
--- a/arch/x86/um/shared/sysdep/faultinfo_64.h
+++ b/arch/x86/um/shared/sysdep/faultinfo_64.h
@@ -31,8 +31,8 @@ struct faultinfo {
 
 #define ___backtrack_faulted(_faulted)                                 \
        asm volatile (                                                  \
-               "mov $0, %0\n"                                          \
                "movq $__get_kernel_nofault_faulted_%=,%1\n"            \
+               "mov $0, %0\n"                                          \
                "jmp _end_%=\n"                                         \
                "__get_kernel_nofault_faulted_%=:\n"                    \
                "mov $1, %0;"                                           \