Re: [PATCH 1/2] um: mark rodata read-only and implement _nofault accesses

[Nathan Chancellor <nathan@kernel.org>]

On Thu, Apr 03, 2025 at 08:20:23AM +0200, Benjamin Berg wrote:
> Hi Nathan,
> 
> oops, that is a little logic bug in the code. When we are coming from
> userspace, we may be running as part of a user task and have an mm. And
> then the new "current->pagefault_disabled" logic is skipped. So when
> prepend_copy is doing its nofault copy to evade a lock it is crashing
> instead of failing gracefully and retrying.
> 
> In segv_handler, can you try moving that into a separate "else if"
> block just above the "current->mm == NULL" check?
> 
> Something like the patch I copied below.

Thanks, I applied that change, which shows a slightly different crash
message now:

Modules linked in:
Pid: 24, comm: mount Not tainted 6.14.0-rc5-00002-gd1d7f01f7cd3-dirty
RIP: 0033:_end_0+0x38/0x45
RSP: 00000000648bfca0  EFLAGS: 00010206
RAX: 0000000000000000 RBX: 00000000609c7ff8 RCX: 00000000606350b8
RDX: 00000000648bfc5e RSI: 0000000000001000 RDI: 0000000060c0c000
RBP: 00000000648bfcc0 R08: 0000000000000000 R09: 0000000060c0c780
R10: 0000000000000000 R11: 0000000000000206 R12: 00000000648bfd60
R13: 0000000060c0c900 R14: 0000000060c0c9f8 R15: 0000000000000007
Kernel panic - not syncing: Segfault without recovery target
CPU: 0 UID: 0 PID: 24 Comm: mount Not tainted 6.14.0-rc5-00002-gd1d7f01f7cd3-dirty #1
Stack:
 600c25f9 00000007 609c7ff8 608c4780
 648bfcf0 601533da 601533bb 648bfd60
 648bfdb0 608c4780 648bfd10 60152ece
Call Trace:
 [<600c25f9>] ? copy_from_kernel_nofault+0x0/0x64
 [<601533da>] prepend_copy+0x1f/0x51
 [<601533bb>] ? prepend_copy+0x0/0x51
 [<60152ece>] prepend+0x60/0x67
 [<60153389>] prepend_name+0x1f/0x51
 [<6015336a>] ? prepend_name+0x0/0x51
 [<60152bbd>] prepend_path+0xb9/0x1d8
 [<6003cb91>] ? um_set_signals+0x0/0x3b
 [<60152e27>] d_path+0xce/0x115
 [<60186805>] proc_pid_readlink+0xa1/0x17d
 [<6012b192>] vfs_readlink+0xec/0xee
 [<60138930>] ? touch_atime+0x0/0x162
 [<60120351>] do_readlinkat+0xbe/0x13a
 [<6011f92b>] sys_readlinkat+0x10/0x14
 [<6002cabb>] handle_syscall+0x7b/0xaa
 [<6002ca40>] ? handle_syscall+0x0/0xaa
 [<6003f695>] userspace+0x289/0x4a5
 [<6002a8e3>] fork_handler+0x56/0x5d

> diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
> index cbe924a0fa87..8a2e68d07de6 100644
> --- a/arch/um/kernel/trap.c
> +++ b/arch/um/kernel/trap.c
> @@ -330,20 +330,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");
>  	}