Re: [RFC] TIF_NOTIFY_RESUME, arch/*/*/*signal*.c and all such

[Al Viro <viro@ZenIV.linux.org.uk>]

On Sun, Apr 29, 2012 at 06:18:18PM +0200, Oleg Nesterov wrote:

> Please look at 29a2e2836ff9ea65a603c89df217f4198973a74f
> x86-32: Fix endless loop when processing signals for kernel tasks
> 
> > At least i386, arm and mips have
> > ret_from_fork going straight to "return from syscall" path, no checks for
> > return to user mode done.  And process created by kernel_thread() will
> > go there.
> 
> Looks like, the patch above fixes that.

Yes, found that shortly after posting.  No such luck for arm, though...

> But, we call do_notify_resume() first, it would be nice to avoid this
> and remove the user_mode() check in do_signal() or lift into
> do_notify_resume().

See the proposed patch.  Does exactly that - lifts all the way to caller
of do_notify_resume(), buggers off if test fits.

> Question. So far I know that on x86 do_notify_resume() && !user_mode()
> is only possible on 32bit system, and the possible callers are
> ret_from_fork or kernel_execve (if it fails).
> 
> Plus, perhaps CONFIG_VM86 makes a difference?
> 
> Could you please clarify?

VM86 doesn't make a difference; we form normal pt_regs for it in case
we have a pending signal, but after that has been done, we don't need
to care.  Look:
	* NOTIFY_RESUME handling doesn't need to be done unless we are
returning to userland.  IOW, the first step is to lift that if (!user_mode(...
into do_notify_resume().  Agreed?
	* Now, if do_notify_resume() does nothing in case !user_mode(regs),
let's lift that check to (32bit) caller.  What we have right now is
	do_notify_resume(%esp, NULL, %ecx)
	goto resume_userspace_sig;
resume_userspace_sig:
	if (!user_mode_vm(%esp))
		goto resume_kernel;
resume_userspace:
So after lifting the check we get
	if (user_mode(%esp))
		do_notify_resume(%esp, NULL, %ecx)
	goto resume_userspace_sig;
resume_userspace_sig:
	if (!user_mode_vm(%esp))
		goto resume_kernel;
resume_userspace:
but user_mode(regs) being true means that user_mode_vm(regs) is also true,
so this code is equivalent to
	if (!user_mode(%esp))
		goto resume_kernel;
	do_notify_resume(%esp, NULL, %ecx)
	goto resume_userspace;
(with stuff around resume_userspace_sig left without changes).

diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index 7b784f4..e858462 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -321,7 +321,6 @@ ret_from_exception:
 	preempt_stop(CLBR_ANY)
 ret_from_intr:
 	GET_THREAD_INFO(%ebp)
-resume_userspace_sig:
 #ifdef CONFIG_VM86
 	movl PT_EFLAGS(%esp), %eax	# mix EFLAGS and CS
 	movb PT_CS(%esp), %al
@@ -628,9 +627,13 @@ work_notifysig:				# deal with pending signals and
 					# vm86-space
 	TRACE_IRQS_ON
 	ENABLE_INTERRUPTS(CLBR_NONE)
+	movb PT_CS(%esp), %bl
+	andl $SEGMENT_RPL_MASK, %ebx
+	cmpl $USER_RPL, %ebx
+	jb resume_kernel
 	xorl %edx, %edx
 	call do_notify_resume
-	jmp resume_userspace_sig
+	jmp resume_userspace
 
 	ALIGN
 work_notifysig_v86:
@@ -643,9 +646,13 @@ work_notifysig_v86:
 #endif
 	TRACE_IRQS_ON
 	ENABLE_INTERRUPTS(CLBR_NONE)
+	movb PT_CS(%esp), %bl
+	andl $SEGMENT_RPL_MASK, %ebx
+	cmpl $USER_RPL, %ebx
+	jb resume_kernel
 	xorl %edx, %edx
 	call do_notify_resume
-	jmp resume_userspace_sig
+	jmp resume_userspace
 END(work_pending)
 
 	# perform syscall exit tracing
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 595969f..c4aa7c5 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -738,16 +738,6 @@ static void do_signal(struct pt_regs *regs)
 	siginfo_t info;
 	int signr;
 
-	/*
-	 * We want the common case to go fast, which is why we may in certain
-	 * cases get here from kernel mode. Just return without doing anything
-	 * if so.
-	 * X86_32: vm86 regs switched out by assembly code before reaching
-	 * here, so testing against kernel CS suffices.
-	 */
-	if (!user_mode(regs))
-		return;
-
 	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
 	if (signr > 0) {
 		/* Whee! Actually deliver the signal.  */