lkml.org 
[lkml]   [2011]   [Aug]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: try_to_freeze() called with IRQs disabled on ARM
Date
On Tuesday, August 23, 2011, Russell King - ARM Linux wrote:
> On Tue, Aug 23, 2011 at 04:19:36PM +0100, Mark Brown wrote:
> > The recent series of commits reworking the freezer appear to have
> > caused serious issues on ARM. The kernel constantly complains that
> > try_to_freeze() is bring called with interrupts disabled:
> >
> > [ 75.380000] BUG: sleeping function called from invalid context at include/linux/freezer.h:44
> > [ 75.380000] in_atomic(): 0, irqs_disabled(): 128, pid: 1517, name: Xorg
> > [ 75.380000] no locks held by Xorg/1517.
> > [ 75.380000] [<c0014308>] (unwind_backtrace+0x0/0x12c) from
> > [<c0464400>] (dump_stack+0x20/0x24)
> > [ 75.380000] [<c0464400>] (dump_stack+0x20/0x24) from [<c0022b80>]
> > (__might_sleep+0xfc/0x11c)
> > [ 75.380000] [<c0022b80>] (__might_sleep+0xfc/0x11c) from [<c0011520>]
> > (do_signal+0x94/0x230)
> > [ 75.380000] [<c0011520>] (do_signal+0x94/0x230) from [<c00116e4>]
> > (do_notify_resume+0x28/0x6c)
> > [ 75.380000] [<c00116e4>] (do_notify_resume+0x28/0x6c) from
> > [<c000eaf8>] (work_pending+0x24/0x28)
> >
> > and the boot runs very slowly. Reverting the series merged in 56f0be
> > appears to resolve the issue, though looking at the changes I'd expect
> > there's some underlying bug here that just doesn't trigger very often.
> > I don't really have the bandwidth to understand what's gone wrong right
> > now but should be able to run tests if you've got anything you'd like
> > looking atl.
>
> The signal handling is rather yucky, and it seems it needs to run with
> IRQs enabled - but we run it with IRQs disabled.
>
> There's horrible issues in there. One such issue is the syscall restart
> handling. Here's an example:
>
> sys_ppoll
> ...
> receives signal
> returns ERESTARTNOHAND (restart syscall if no handler)
>
> On the exit path, we notice that we have TIF_SIGPENDING set. So we call
> do_notify_resume() and eventually do_signal(). At this point, 'syscall'
> is set.
>
> The first thing do_signal() does is fiddle with the stack to fake a
> restart. If we don't gain a signal here, everything is fine and we
> return. So far so good.
>
> Lets say for the sake of argument that we've run do_signal() with IRQs
> enabled. Now, a scheduling event has come along and set TIF_NEED_RESCHED.
>
> So, having returned from do_notify_resume(), we disable interrupts, reload
> the work mask, and notice that TIF_NEED_RESCHED is set. We now switch
> away from this thread and do something else.
>
> While something else is running, lets say a SIGHUP is sent to our process,
> and there's a handler installed.
>
> When we switch back, we again disable interrupts, reload the work mask,
> and notice this time that TIF_SIGPENDING is again set. So, just like
> last time, we call do_notify_resume() and eventually do_signal(). This
> time, syscall is false.
>
> So we obtain the signal, save the current state, and set userspace up to
> call the handler.
>
> The state which we've saved though is the state required for the syscall
> restart - but that's wrong, because the return code said 'restart if
> no handler' and we're now invoking a handler.
>
> What should happen is that the syscall is not restarted, and -EINTR is
> returned instead.
>
> What has this got to do with the issue you raise? Having interrupts
> off during signal handling helps to avoid this problem by ensuring that
> we can't get resched events if there's a signal with no handler present.
> Enabling interrupts for do_signal opens that race up.
>
> Now, having interrupts disabled for writing to the processes stack is
> technically bad news (although in the last 18 or so years its never
> caused a problem.) That said, we really should enable interrupts
> before calling handle_signal() - which should be safe from the above
> race.
>
> But... without serious amounts of rework of the signal handling code
> to avoid the above race I don't see how we can allow try_to_freeze()
> to run with IRQs enabled and still have race-free syscall restarting.

I'm not sure if it's necessary to call try_to_freeze() from do_signal().
At least x86 doesn't do that.

Rafael


\
 
 \ /
  Last update: 2011-08-24 00:09    [W:0.103 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site