[patch] warn on release_region() from irq context

[patch] warn on release_region() from irq context

[Ingo Molnar]

it is not legal to call release_region() from hardirq/softirq context.  
Add a WARN_ON() so that we find such cases.

Signed-off-by: Ingo Molnar <mingo@elte.hu>

----

 kernel/resource.c |    3 +++
 1 files changed, 3 insertions(+)

Index: linux/kernel/resource.c
===================================================================
--- linux.orig/kernel/resource.c
+++ linux/kernel/resource.c
@@ -18,6 +18,7 @@
 #include <linux/fs.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#include <linux/interrupt.h>
 #include <asm/io.h>
 
 
@@ -486,6 +487,8 @@ void __release_region(struct resource *p
 	struct resource **p;
 	unsigned long end;
 
+	WARN_ON(in_interrupt());
+
 	p = &parent->child;
 	end = start + n - 1;
 

Re: [patch] warn on release_region() from irq context

[Ingo Molnar]

the rationale for this patch is that the lock validator (which now 
tracks rwlock dependencies too, see output below) noticed that floppy.c 
does a release_region() from softirq context -> ouch. I'm working on the 
floppy.c fix for that.

	Ingo

  ============================
  [ BUG: illegal lock usage! ]
  ----------------------------
  illegal {softirq-on} -> {in-softirq} usage.
  rcu_torture_rea/264 [HC0[0]:SC1[2]:HE1:SE0] takes:
   (resource_lock){+-}, at: [<c0123997>] __release_region+0x37/0xf0
  {softirq-on} state was registered at:
   [<c0122c85>] irq_exit+0x45/0x50
  hardirqs last enabled at: [<c011d39e>] vprintk+0x32e/0x3e0
  softirqs last enabled at: [<c0122c85>] irq_exit+0x45/0x50
  
  other info that might help us debug this:
  no locks held by rcu_torture_rea/264.
  
  stack backtrace:
   [<c010432d>] show_trace+0xd/0x10
   [<c0104347>] dump_stack+0x17/0x20
   [<c0139078>] print_usage_bug+0x1d8/0x230
   [<c0139436>] mark_lock+0x366/0x3a0
   [<c0139903>] debug_lock_chain+0x493/0x1090
   [<c013a53d>] debug_lock_chain_spin+0x3d/0x60
   [<c0268522>] _raw_write_lock+0x32/0x1a0
   [<c04dc158>] _write_lock+0x8/0x10
   [<c0123997>] __release_region+0x37/0xf0
   [<c02f5010>] floppy_release_irq_and_dma+0x140/0x200
   [<c02f3577>] set_dor+0xc7/0x1b0
   [<c02f65e1>] motor_off_callback+0x21/0x30
   [<c01276c5>] run_timer_softirq+0xf5/0x1f0
   [<c0122fc7>] __do_softirq+0x97/0x130
   [<c01054a9>] do_softirq+0x69/0x100
   =======================
   [<c0122c85>] irq_exit+0x45/0x50
   [<c010f504>] smp_apic_timer_interrupt+0x54/0x60
   [<c010393b>] apic_timer_interrupt+0x27/0x2c
   [<c0266b79>] __delay+0x9/0x10
   [<c0266be1>] __udelay+0x31/0x40
   [<c01445e3>] rcu_torture_reader+0x83/0x140
   [<c0131cca>] kthread+0xca/0xd0
   [<c0100ef5>] kernel_thread_helper+0x5/0x10

Re: [patch] warn on release_region() from irq context

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> the rationale for this patch is that the lock validator (which now 
> tracks rwlock dependencies too, see output below) noticed that 
> floppy.c does a release_region() from softirq context -> ouch. I'm 
> working on the floppy.c fix for that.

ugh. floppy.c is a mess. I did the patch below - but that's not enough, 
it does stupid things like request_region() and release_region() from 
the timer irq - so floppy.c file needs a serious redesign to fix these 
deadlocks :-(

	Ingo

floppy.c does alot of irq-unsafe work within floppy_release_irq_and_dma():
free_irq(), release_region() ... so when executing in irq context, push
the whole function into keventd.

Signed-off-by: Ingo Molnar <mingo@elte.hu>

----

 drivers/block/floppy.c |   27 +++++++++++++++++++++++++--
 1 files changed, 25 insertions(+), 2 deletions(-)

Index: linux/drivers/block/floppy.c
===================================================================
--- linux.orig/drivers/block/floppy.c
+++ linux/drivers/block/floppy.c
@@ -561,6 +561,21 @@ static int floppy_grab_irq_and_dma(void)
 static void floppy_release_irq_and_dma(void);
 
 /*
+ * Interrupt, DMA and region freeing must not be done from IRQ
+ * context - e.g. irq-unregistration means /proc VFS work, region
+ * release takes an irq-unsafe lock, etc. So we push this work
+ * into keventd:
+ */
+static void fd_release_fn(void *data)
+{
+	mutex_lock(&open_lock);
+	floppy_release_irq_and_dma();
+	mutex_unlock(&open_lock);
+}
+
+static DECLARE_WORK(floppy_release_irq_and_dma_work, fd_release_fn, NULL);
+
+/*
  * The "reset" variable should be tested whenever an interrupt is scheduled,
  * after the commands have been sent. This is to ensure that the driver doesn't
  * get wedged when the interrupt doesn't come because of a failed command.
@@ -824,7 +839,7 @@ static int set_dor(int fdc, char mask, c
 	if (newdor & FLOPPY_MOTOR_MASK)
 		floppy_grab_irq_and_dma();
 	if (olddor & FLOPPY_MOTOR_MASK)
-		floppy_release_irq_and_dma();
+		schedule_work(&floppy_release_irq_and_dma_work);
 	return olddor;
 }
 
@@ -905,6 +920,8 @@ static int _lock_fdc(int drive, int inte
 
 		set_current_state(TASK_RUNNING);
 		remove_wait_queue(&fdc_wait, &wait);
+
+		flush_scheduled_work();
 	}
 	command_status = FD_COMMAND_NONE;
 
@@ -938,7 +955,7 @@ static inline void unlock_fdc(void)
 	if (elv_next_request(floppy_queue))
 		do_fd_request(floppy_queue);
 	spin_unlock_irqrestore(&floppy_lock, flags);
-	floppy_release_irq_and_dma();
+	schedule_work(&floppy_release_irq_and_dma_work);
 	wake_up(&fdc_wait);
 }
 
@@ -4628,6 +4645,12 @@ void cleanup_module(void)
 	del_timer_sync(&fd_timer);
 	blk_cleanup_queue(floppy_queue);
 
+	/*
+	 * Wait for any asynchronous floppy_release_irq_and_dma()
+	 * calls to finish first:
+	 */
+	flush_scheduled_work();
+
 	if (usage_count)
 		floppy_release_irq_and_dma();
 

Re: [patch] warn on release_region() from irq context

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

>   ============================
>   [ BUG: illegal lock usage! ]
>   ----------------------------
>   illegal {softirq-on} -> {in-softirq} usage.
>   rcu_torture_rea/264 [HC0[0]:SC1[2]:HE1:SE0] takes:
>    (resource_lock){+-}, at: [<c0123997>] __release_region+0x37/0xf0
>   {softirq-on} state was registered at:
>    [<c0122c85>] irq_exit+0x45/0x50
>   hardirqs last enabled at: [<c011d39e>] vprintk+0x32e/0x3e0
>   softirqs last enabled at: [<c0122c85>] irq_exit+0x45/0x50
>   
>   other info that might help us debug this:
>   no locks held by rcu_torture_rea/264.
>   
>   stack backtrace:
>    [<c010432d>] show_trace+0xd/0x10
>    [<c0104347>] dump_stack+0x17/0x20
>    [<c0139078>] print_usage_bug+0x1d8/0x230
>    [<c0139436>] mark_lock+0x366/0x3a0
>    [<c0139903>] debug_lock_chain+0x493/0x1090
>    [<c013a53d>] debug_lock_chain_spin+0x3d/0x60
>    [<c0268522>] _raw_write_lock+0x32/0x1a0
>    [<c04dc158>] _write_lock+0x8/0x10
>    [<c0123997>] __release_region+0x37/0xf0
>    [<c02f5010>] floppy_release_irq_and_dma+0x140/0x200
>    [<c02f3577>] set_dor+0xc7/0x1b0
>    [<c02f65e1>] motor_off_callback+0x21/0x30
>    [<c01276c5>] run_timer_softirq+0xf5/0x1f0
>    [<c0122fc7>] __do_softirq+0x97/0x130
>    [<c01054a9>] do_softirq+0x69/0x100

btw., Arjan discovered that this bug is not theoretical, and the 
deadlock scenario is experienced by users:

  http://i15.photobucket.com/albums/a396/johnny_blue/fedora-devel/28012006026.jpg

	Ingo