try_to_freeze() called with IRQs disabled on ARM

On Thu, Aug 25, 2011 at 03:09:07PM +0200, Tejun Heo wrote:
> Hey, Russell.
>
> If you can fix it properly without going through temporary step,
> that's awesome.  Let's put the arguments behind, okay?

Here's the patch.  As the kernel I've run this against doesn't have the
change to try_to_freeze(), I added a might_sleep() in do_signal() during
my testing to verify that it fixes Mark's problem (which it does.)

I've tested functions returning -ERESTARTSYS, -ERESTARTNOHAND and
-ERESTART_RESTARTBLOCK, all of which seem to behave as expected with
signals such as SIGCONT (without handler) and SIGALRM (with handler).
I haven't tested -ERESTARTNOINTR.

I don't have a test case for the race condition I mentioned (which is
admittedly pretty difficult to construct, requiring an explicit
signal, schedule, signal sequence) but this should plug that too.

How do we achieve this?  Effectively the steps in this patch are:

1. Undo Arnd's fixups to the syscall restart processing (but don't worry,
   we restore it in step 3).

2. Introduce TIF_SYS_RESTART, which is set when we enter signal handling
   and the syscall has returned one of the restart codes.  This is used
   as a flag to indicate that we have some syscall restart processing to
   do at some point.

3. Clear TIF_SYS_RESTART whenever ptrace is used to set the GP registers
   (thereby restoring Arnd's fixup for his gdb testsuite problem - it
   would be good if Arnd could reconfirm that.)

4. When we setup a user handler to run, check TIF_SYS_RESTART and clear it.
   If it was set, we need to set things up to return -EINTR or restart the
   syscall as appropriate.  As we've cleared it, no further restart
   processing will occur.

5. Once we've run all work (signal delivery, and rescheduling events), and
   we're about to return to userspace, make a final check for TIF_SYS_RESTART.
   If it's still set, then we're returning to userspace having not setup
   any user handlers, and we need to restart the syscall.  This is mostly
   trivial, except for OABI restartblock which requires the user stack to
   be written.  We have to re-enable IRQs for this write, which means we
   have to manually re-check for rescheduling events, abort the restart,
   and try again later.

One of the side effects of reverting Arnd's patch is that we restore the
strace behaviour which we've had for years on ARM, and can still be seen
on x86: strace can see the -ERESTART return codes from the kernel syscalls,
rather than what seems to be the signal number:

Before:
rt_sigsuspend([] <unfinished ...>
--- SIGIO (I/O possible) ---
<... rt_sigsuspend resumed> )           = 29
sigreturn()                             = ? (mask now [])
vs:
rt_sigsuspend([])                       = ? ERESTARTNOHAND (To be restarted)
--- SIGIO (I/O possible) @ 0 (0) ---
sigreturn()                             = ? (mask now [])
x86:
rt_sigsuspend([])                       = ? ERESTARTNOHAND (To be restarted)
--- {si_signo=SIGIO, si_code=SI_USER} (I/O possible) ---
sigreturn()                             = ? (mask now [])
So, this patch should fix:
1. The race which I identified in the signal handling code (I think x86
   and other architectures can suffer from it too.)
2. The warning from try_to_freeze.
3. The unanticipated change to strace output.

Arnd, can you test this to make sure your gdb test case still works, and
Mark, can you test this to make sure it fixes your problem please?

Thanks.

The issue apperas with item 12 in [1], specifically with
CONFIG_DEBUG_ATOMIC_SLEEP=y [2]. A patch to fix this is discussed in [3] and [4]

1. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob_plain;f=Documentation/SubmitChecklist;hb=HEAD
2. http://us.generation-nt.com/bug-sleeping-function-called-invalid-context-at-usr-local-src-linux-3-4-rc1-include-linux-freezer-h-46-help-207181171.html
3. http://lists.infradead.org/pipermail/linux-arm-kernel/2012-January/079917.html
4. https://lkml.org/lkml/2011/8/25/231

Signed-off-by: Russell King <linux@arm.linux.org.uk>