[PATCH v2 0/4] Fix no signal delivery at do_signal

[PATCH v2 0/4] Fix no signal delivery at do_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 woken up by the signal.  In this situation, a signal
handler is called correctly, but it seems that the pselect is
restarted.  Expected behavior is woken pselect returning -EINTR.

Looking the code, my theory is:
It is a fake signal by suspend which wakes up 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.

I found that, in arch/arm, it does differently.  When it finds no
signal delivery at do_signal, it restarts the system call soon,
breaking the loop.  This approach also works.

Changes from v1:
* Title change of cover leter
* Avoid User Mode Linux breakage
* Only call nosig_restart_syscall when really needed
* Add 'unlikely'.

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

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

-- 
2.11.0

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

[NIIBE Yutaka]

Make a function for swing at the ball but actually no ball.

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 v2 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 v2 3/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 woken up 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 e709b54a01b8..9c24df5ca2d4 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

[PATCH v2 4/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 one of them actually
delivers signal 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.

In this patch, the function name of do_signal is changed, to avoid
breakage of building User Mode Linux.

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

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 74f6eee15179..14e4da298887 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 sig_result = 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);
+			sig_result |= do_signal_x86(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 (unlikely(sig_result == DO_SIGNAL_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..32a861950a63 100644
--- a/arch/x86/include/asm/signal.h
+++ b/arch/x86/include/asm/signal.h
@@ -34,8 +34,10 @@ typedef sigset_t compat_sigset_t;
 
 #endif /* __ASSEMBLY__ */
 #include <uapi/asm/signal.h>
+#define DO_SIGNAL_DELIVERED	1
+#define DO_SIGNAL_NOSIG		2
 #ifndef __ASSEMBLY__
-extern void do_signal(struct pt_regs *regs);
+extern int do_signal_x86(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 9c24df5ca2d4..4a852aa60118 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_x86(struct pt_regs *regs)
 {
 	struct ksignal ksig;
 
 	if (get_signal(&ksig)) {
 		/* Whee! Actually deliver the signal.  */
 		handle_signal(&ksig, regs);
-		return;
+		return DO_SIGNAL_DELIVERED;
 	}
 
-	nosig_restart_syscall(regs);
+	return DO_SIGNAL_NOSIG;
 }
 
 void nosig_restart_syscall(struct pt_regs *regs)
-- 
2.11.0