* [PATCH] um: fix _nofault accesses
@ 2025-04-04 15:05 Johannes Berg
2025-04-04 21:12 ` Nathan Chancellor
0 siblings, 1 reply; 2+ messages in thread
From: Johannes Berg @ 2025-04-04 15:05 UTC (permalink / raw)
To: linux-um; +Cc: Johannes Berg, Nathan Chancellor
From: Johannes Berg <johannes.berg@intel.com>
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")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
arch/um/include/asm/uaccess.h | 2 ++
arch/um/kernel/trap.c | 26 ++++++++++++------------
arch/x86/um/shared/sysdep/faultinfo_32.h | 2 +-
arch/x86/um/shared/sysdep/faultinfo_64.h | 2 +-
4 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/arch/um/include/asm/uaccess.h b/arch/um/include/asm/uaccess.h
index 3a08f9029a3f..1c6e0ae41b0c 100644
--- 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 ce073150dc20..ef2272e92a43 100644
--- 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 ab5c8e47049c..9193a7790a71 100644
--- 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 26fb4835d3e9..61e4ca1e0ab5 100644
--- 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;" \
--
2.49.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] um: fix _nofault accesses
2025-04-04 15:05 [PATCH] um: fix _nofault accesses Johannes Berg
@ 2025-04-04 21:12 ` Nathan Chancellor
0 siblings, 0 replies; 2+ messages in thread
From: Nathan Chancellor @ 2025-04-04 21:12 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-um, Johannes Berg
On Fri, Apr 04, 2025 at 05:05:19PM +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> 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")
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Thanks, this makes my test case happy.
Tested-by: Nathan Chancellor <nathan@kernel.org>
> ---
> arch/um/include/asm/uaccess.h | 2 ++
> arch/um/kernel/trap.c | 26 ++++++++++++------------
> arch/x86/um/shared/sysdep/faultinfo_32.h | 2 +-
> arch/x86/um/shared/sysdep/faultinfo_64.h | 2 +-
> 4 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/arch/um/include/asm/uaccess.h b/arch/um/include/asm/uaccess.h
> index 3a08f9029a3f..1c6e0ae41b0c 100644
> --- 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 ce073150dc20..ef2272e92a43 100644
> --- 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 ab5c8e47049c..9193a7790a71 100644
> --- 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 26fb4835d3e9..61e4ca1e0ab5 100644
> --- 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;" \
> --
> 2.49.0
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-04-04 21:12 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-04 15:05 [PATCH] um: fix _nofault accesses Johannes Berg
2025-04-04 21:12 ` Nathan Chancellor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox