[PATCH 0/4] Unblocked by fake_signal_wake_up then real signal

[PATCH 0/4] Unblocked by fake_signal_wake_up then real signal

[NIIBE Yutaka]

Hello,

Background:
I tried to fix my own problem in suspend-to-ram with USB devices (some
device doesn't work well after suspend/resume).  I know it's common
problems.  Well, while I identified some problems of USB devices and
drivers, I did try my best.  I fixed a USB device implementation of
mine.  Then, I also located and fixed bugs in a USB user space driver
which I maintain.

Now, I think I found a kernel problem.  After suspend/resume, in my
USB user space driver, a task sends a signal to another task, then,
pselect is unblocked by the signal.  In this situation, a signal
handler is called correctly, but it seems that the pselect is
restarted.  Expected behavior is unblocked pselect returning -EINTR.

Looking the code, my theory is: 
It is a fake signal by suspend which unblocks pselect to be frozen,
and after resume, real signal is sent.  When both of signals are
handled in a single call of exit_to_usermode_loop, by two calls of
do_signal, firstly for fake signal and secondly for real signal, the
system call is restarted wrongly, where it should return -EINTR.

Here is a patch series of mine for x86.  I'd suggest architecture
maintainers to do similar if an architecture supports suspend/resume.

NIIBE Yutaka (4):
  signal/x86: Factor out nosig handling.
  signal/x86: do_signal: syscall restart should be done only once.
  signal/x86: Move nosig handling at the end of exit_to_usermode_loop.
  signal/x86: Move restore_saved_sigmask().

 arch/x86/entry/common.c       |  6 +++++-
 arch/x86/include/asm/signal.h |  3 ++-
 arch/x86/kernel/signal.c      | 22 ++++++++++++++--------
 3 files changed, 21 insertions(+), 10 deletions(-)

-- 
2.11.0

[PATCH 1/4] signal/x86: Factor out nosig handling.

[NIIBE Yutaka]

Make a function for swing at the ball and miss.

Signed-off-by: NIIBE Yutaka <gniibe@fsij.org>
---
 arch/x86/include/asm/signal.h | 1 +
 arch/x86/kernel/signal.c      | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/signal.h b/arch/x86/include/asm/signal.h
index 5f9012ff52ed..6517df6d9938 100644
--- a/arch/x86/include/asm/signal.h
+++ b/arch/x86/include/asm/signal.h
@@ -36,6 +36,7 @@ typedef sigset_t compat_sigset_t;
 #include <uapi/asm/signal.h>
 #ifndef __ASSEMBLY__
 extern void do_signal(struct pt_regs *regs);
+extern void nosig_restart_syscall(struct pt_regs *regs);
 
 #define __ARCH_HAS_SA_RESTORER
 
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 4cdc0b27ec82..cdfb82031243 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -812,6 +812,11 @@ void do_signal(struct pt_regs *regs)
 		return;
 	}
 
+	nosig_restart_syscall(regs);
+}
+
+void nosig_restart_syscall(struct pt_regs *regs)
+{
 	/* Did we come from a system call? */
 	if (syscall_get_nr(current, regs) >= 0) {
 		/* Restart the system call - no handlers present */
-- 
2.11.0

[PATCH 2/4] signal/x86: do_signal: syscall restart should be done only once.

[NIIBE Yutaka]

do_signal may be called multiple times from exit_to_usermode_loop.  In
those multiple calls, only the one should handle restarting the system
call.

When actually delivering a signal, make sure the register will not be
examined again as syscall errno by another call of do_signal.

Signed-off-by: NIIBE Yutaka <gniibe@fsij.org>
---
 arch/x86/kernel/signal.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index cdfb82031243..e709b54a01b8 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -729,6 +729,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 			regs->ip -= 2;
 			break;
 		}
+		regs->orig_ax = -1;
 	}
 
 	/*
-- 
2.11.0

[PATCH 3/4] signal/x86: Move nosig handling at the end of exit_to_usermode_loop.

[NIIBE Yutaka]

Handling of restarting a system call for nosig should be done after
all possible calls of do_signal, to see if it's actually delivering
signal(s) or not.

Before this change, restarting a system call for nosig may be followed
by a call of handle_signal which would try to change the system call
returning -EINTR instead, but it is too late to do so.

Signed-off-by: NIIBE Yutaka <gniibe@fsij.org>
---
 arch/x86/entry/common.c       | 6 +++++-
 arch/x86/include/asm/signal.h | 2 +-
 arch/x86/kernel/signal.c      | 6 +++---
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 74f6eee15179..38e0939c0aeb 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -137,6 +137,8 @@ static long syscall_trace_enter(struct pt_regs *regs)
 
 static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags)
 {
+	int nosig = 0;
+
 	/*
 	 * In order to return to user mode, we need to have IRQs off with
 	 * none of EXIT_TO_USERMODE_LOOP_FLAGS set.  Several of these flags
@@ -159,7 +161,7 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags)
 
 		/* deal with pending signal delivery */
 		if (cached_flags & _TIF_SIGPENDING)
-			do_signal(regs);
+			nosig |= do_signal(regs);
 
 		if (cached_flags & _TIF_NOTIFY_RESUME) {
 			clear_thread_flag(TIF_NOTIFY_RESUME);
@@ -177,6 +179,8 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags)
 		if (!(cached_flags & EXIT_TO_USERMODE_LOOP_FLAGS))
 			break;
 	}
+	if (nosig)
+		nosig_restart_syscall(regs);
 }
 
 /* Called with IRQs disabled. */
diff --git a/arch/x86/include/asm/signal.h b/arch/x86/include/asm/signal.h
index 6517df6d9938..31e68266c2f6 100644
--- a/arch/x86/include/asm/signal.h
+++ b/arch/x86/include/asm/signal.h
@@ -35,7 +35,7 @@ typedef sigset_t compat_sigset_t;
 #endif /* __ASSEMBLY__ */
 #include <uapi/asm/signal.h>
 #ifndef __ASSEMBLY__
-extern void do_signal(struct pt_regs *regs);
+extern int do_signal(struct pt_regs *regs);
 extern void nosig_restart_syscall(struct pt_regs *regs);
 
 #define __ARCH_HAS_SA_RESTORER
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index e709b54a01b8..57576988a01f 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -803,17 +803,17 @@ static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
  * want to handle. Thus you cannot kill init even with a SIGKILL even by
  * mistake.
  */
-void do_signal(struct pt_regs *regs)
+int do_signal(struct pt_regs *regs)
 {
 	struct ksignal ksig;
 
 	if (get_signal(&ksig)) {
 		/* Whee! Actually deliver the signal.  */
 		handle_signal(&ksig, regs);
-		return;
+		return 0;
 	}
 
-	nosig_restart_syscall(regs);
+	return 1;
 }
 
 void nosig_restart_syscall(struct pt_regs *regs)
-- 
2.11.0

[PATCH 4/4] signal/x86: Move restore_saved_sigmask().

[NIIBE Yutaka]

Call to restore_saved_sigmask is only needed when it is from a system
call.  It is only unblocked system call which uses saved_sigmask.
Specifically, they are pselect, ppoll, and epoll_pwait.

Signed-off-by: NIIBE Yutaka <gniibe@fsij.org>
---
 arch/x86/kernel/signal.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 57576988a01f..c3928718a33a 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -834,13 +834,13 @@ void nosig_restart_syscall(struct pt_regs *regs)
 			regs->ip -= 2;
 			break;
 		}
-	}
 
-	/*
-	 * If there's no signal to deliver, we just put the saved sigmask
-	 * back.
-	 */
-	restore_saved_sigmask();
+		/*
+		 * If there's no signal to deliver, we just put the
+		 * saved sigmask back.
+		 */
+		restore_saved_sigmask();
+	}
 }
 
 void signal_fault(struct pt_regs *regs, void __user *frame, char *where)
-- 
2.11.0

Re: [PATCH 3/4] signal/x86: Move nosig handling at the end of exit_to_usermode_loop.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3540 bytes --]

Hi NIIBE,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[also build test ERROR on next-20180320]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/NIIBE-Yutaka/Unblocked-by-fake_signal_wake_up-then-real-signal/20180321-051147
config: um-x86_64_defconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=um SUBARCH=x86_64

All errors (new ones prefixed by >>):

   In file included from arch/um/kernel/irq.c:17:0:
>> arch/um/include/shared/kern_util.h:26:13: error: conflicting types for 'do_signal'
    extern void do_signal(struct pt_regs *regs);
                ^~~~~~~~~
   In file included from include/uapi/linux/signal.h:5:0,
                    from include/linux/signal_types.h:10,
                    from include/linux/sched.h:27,
                    from include/linux/kernel_stat.h:10,
                    from arch/um/kernel/irq.c:11:
   arch/x86/include/asm/signal.h:38:12: note: previous declaration of 'do_signal' was here
    extern int do_signal(struct pt_regs *regs);
               ^~~~~~~~~
--
   In file included from arch/um/kernel/signal.c:13:0:
>> arch/um/include/shared/kern_util.h:26:13: error: conflicting types for 'do_signal'
    extern void do_signal(struct pt_regs *regs);
                ^~~~~~~~~
   In file included from include/uapi/linux/signal.h:5:0,
                    from include/linux/signal_types.h:10,
                    from include/linux/sched.h:27,
                    from include/linux/ptrace.h:6,
                    from arch/um/kernel/signal.c:7:
   arch/x86/include/asm/signal.h:38:12: note: previous declaration of 'do_signal' was here
    extern int do_signal(struct pt_regs *regs);
               ^~~~~~~~~
>> arch/um/kernel/signal.c:67:6: error: conflicting types for 'do_signal'
    void do_signal(struct pt_regs *regs)
         ^~~~~~~~~
   In file included from include/uapi/linux/signal.h:5:0,
                    from include/linux/signal_types.h:10,
                    from include/linux/sched.h:27,
                    from include/linux/ptrace.h:6,
                    from arch/um/kernel/signal.c:7:
   arch/x86/include/asm/signal.h:38:12: note: previous declaration of 'do_signal' was here
    extern int do_signal(struct pt_regs *regs);
               ^~~~~~~~~

vim +/do_signal +26 arch/um/include/shared/kern_util.h

edea13858 arch/um/include/kern_util.h        Jeff Dike     2008-02-04  24  
ccaee5f85 arch/um/include/shared/kern_util.h Ingo Molnar   2015-07-03  25  struct pt_regs;
ccaee5f85 arch/um/include/shared/kern_util.h Ingo Molnar   2015-07-03 @26  extern void do_signal(struct pt_regs *regs);
edea13858 arch/um/include/kern_util.h        Jeff Dike     2008-02-04  27  extern void interrupt_end(void);
d3c1cfcdb arch/um/include/shared/kern_util.h Martin Pärtel 2012-08-02  28  extern void relay_signal(int sig, struct siginfo *si, struct uml_pt_regs *regs);
edea13858 arch/um/include/kern_util.h        Jeff Dike     2008-02-04  29  

:::::: The code at line 26 was first introduced by commit
:::::: ccaee5f851470dec6894a6835b6fadffc2bb7514 um: Fix do_signal() prototype

:::::: TO: Ingo Molnar <mingo@kernel.org>
:::::: CC: Ingo Molnar <mingo@kernel.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 8114 bytes --]