[PATCH] arm: Enable interrupts before calling schedule()

[PATCH] arm: Enable interrupts before calling schedule()

[Thomas Gleixner]

do_work_pending() calls schedule() with interrupts disabled, which is just
wrong. Fix it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/arm/kernel/signal.c |    1 +
 1 file changed, 1 insertion(+)

--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -573,6 +573,7 @@ do_work_pending(struct pt_regs *regs, un
 	trace_hardirqs_off();
 	do {
 		if (likely(thread_flags & _TIF_NEED_RESCHED)) {
+			local_irq_enable();
 			schedule();
 		} else {
 			if (unlikely(!user_mode(regs)))

[PATCH] arm: Enable interrupts before calling schedule()

[Catalin Marinas]

Hi Thomas,

On Fri, May 20, 2016 at 05:42:17PM +0200, Thomas Gleixner wrote:
> do_work_pending() calls schedule() with interrupts disabled, which is just
> wrong. Fix it.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/arm/kernel/signal.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -573,6 +573,7 @@ do_work_pending(struct pt_regs *regs, un
>  	trace_hardirqs_off();
>  	do {
>  		if (likely(thread_flags & _TIF_NEED_RESCHED)) {
> +			local_irq_enable();
>  			schedule();
>  		} else {
>  			if (unlikely(!user_mode(regs)))

We may have the same bug on arm64 (arch/arm64/kernel/entry.S). Is there
a more fundamental problem with calling schedule() with IRQs off? The
__schedule() function disables the IRQs shortly after it is entered.

To silence IRQ trace warnings on arm64, we merged commit db3899a6477a
("arm64: Add trace_hardirqs_off annotation in ret_to_user"). But we were
also debating whether enabling the IRQs before calling schedule() in
arch/arm64/kernel/entry.S would make more sense. It looks like we need
to revisit this patch:

https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/commit/?h=arm64/entry-deasm&id=d244472af6e88c55603dc1ba342fae4e85cde31c

Thanks.

-- 
Catalin

[PATCH] arm: Enable interrupts before calling schedule()

[Russell King - ARM Linux]

On Mon, May 23, 2016 at 11:54:20AM +0100, Catalin Marinas wrote:
> We may have the same bug on arm64 (arch/arm64/kernel/entry.S). Is there
> a more fundamental problem with calling schedule() with IRQs off? The
> __schedule() function disables the IRQs shortly after it is entered.

schedule() does other stuff before entering __schedule() though, such
as calling into the block layer.  This code may have the expectation
that interrupts are enabled.

However, having interrupts enabled in this path (which I'd argue is
special in respect of the "thou shalt not enter schedule() with IRQs
off" rule) opens up the possibility to call into schedule() with the
need_resched flag cleared:

- need_resched was set when returning to userspace, we enter
  do_work_pending().
- do_work_pending() enables IRQs, and an IRQ was pending.
- IRQ is processed, and during that kernel preemption happens, clearing
  this thread's need_resched flag.
- we return to this thread, and now we will enter schedule() with
  need_resched clear.

Whether that matters or not is a different question - and I guess
is a question for scheduler people.

The likelyhood of this happening depends on the IRQ load, but the
requirements are quite simple: need_resched set while returning to
userspace with a pending IRQ.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] arm: Enable interrupts before calling schedule()

[Peter Zijlstra]

On Fri, May 20, 2016 at 05:42:17PM +0200, Thomas Gleixner wrote:
> do_work_pending() calls schedule() with interrupts disabled, which is just
> wrong. Fix it.

Thomas; lockdep cannot currently catch this. It doesn't do IRQ state
validation other than ensuring the state matches with the hardware.

So things like:

	local_irq_disable();
	local_irq_disable();
	local_irq_save();
	local_irq_enable();

(and 'obviously' suspect sequence of IRQ events)

Are _fine_ by it. The only time it will yell is if flipping IRQ state
ends up marking an actual held lock with ENABLED_HARDIRQ while it
already had USED_IN_HARDIRQ.