All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@elte.hu>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Andrew Morton <akpm@osdl.org>,
	linux-kernel@vger.kernel.org,
	Alexander Viro <viro@ftp.linux.org.uk>
Subject: [patch, validator] fix files_lock related deadlock
Date: Wed, 25 Jan 2006 19:14:41 +0100	[thread overview]
Message-ID: <20060125181441.GA14541@elte.hu> (raw)
In-Reply-To: <20060125180811.GA12762@elte.hu>


> to solve this we must either change files_lock to be softirq-safe too 
> (bleh!), or we must forbid remove_proc_entry() use from softirq 
> contexts. Neither is a happy solution - remove_proc_entry() is used 
> within free_irq(), and who knows how many drivers do free_irq() in 
> softirq/tasklet context ...
> 
> Andrew, this needs to be resolved before v2.6.16, correct? Steve's 
> patch solves a real bug in the upstream kernel.

the patch below does the easier and safer change: it makes files_lock 
softirq-safe. (A quick test shows that the validator does not complain 
when this patch is applied too - so it seems the 'softirq effect' does 
not spread to other VFS locks.)

	Ingo

-----
the validator just found another problem with this lock, pointing out 
that files_lock nests inside of proc_subdir_lock, and that files_lock is 
a softirq-unsafe lock, creating another (unlikely but possible) deadlock 
scenario:

 =====================================
 [ BUG: lock inversion bug detected! ]
 -------------------------------------
 grep/2290 just changed the state of lock {proc_subdir_lock} at:
  [<c0196e53>] remove_proc_entry+0x33/0x1f0
 but this lock took lock {files_lock} in the past,
 acquired at: [<c0196ece>] remove_proc_entry+0xae/0x1f0
 and interrupts could create an inverse lock dependency between them,
 which could lead to deadlocks!
 other info that might help in debugging this:
 ------------------------------
 | showing all locks held by: |  (grep/2290 [c321c790, 125]):
 ------------------------------

  [<c010432d>] show_trace+0xd/0x10
  [<c0104347>] dump_stack+0x17/0x20
  [<c0137b11>] check_no_lock_2_mask+0x131/0x180
  [<c0137ffb>] mark_lock+0xfb/0x2a0
  [<c01387b3>] debug_lock_chain+0x613/0x10d0
  [<c01392ad>] debug_lock_chain_spin+0x3d/0x60
  [<c02656ed>] _raw_spin_lock+0x2d/0x90
  [<c04d88d2>] _spin_lock_bh+0x12/0x20
  [<c0196e53>] remove_proc_entry+0x33/0x1f0
  [<c01427c9>] unregister_handler_proc+0x19/0x20
  [<c0141f8b>] free_irq+0x7b/0xe0
  [<c02f2302>] floppy_release_irq_and_dma+0x1b2/0x210
  [<c02f07f7>] set_dor+0xc7/0x1b0
  [<c02f3871>] motor_off_callback+0x21/0x30
  [<c01273a5>] run_timer_softirq+0xf5/0x1f0
  [<c0122cf7>] __do_softirq+0x97/0x130
  [<c0105519>] do_softirq+0x69/0x100
  =======================
  [<c01229a9>] irq_exit+0x39/0x50
  [<c010f4cc>] smp_apic_timer_interrupt+0x4c/0x50
  [<c010393b>] apic_timer_interrupt+0x27/0x2c

to solve this we must either change files_lock to be softirq-safe too 
(bleh!), or we must forbid remove_proc_entry() use from softirq 
contexts. Neither is a happy solution - remove_proc_entry() is used 
within free_irq(), and who knows how many drivers do free_irq() in 
softirq/tasklet context ...

the patch below makes files_lock softirq-safe.

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

----

 include/linux/fs.h |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h
+++ linux/include/linux/fs.h
@@ -648,9 +648,13 @@ struct file {
 #endif /* #ifdef CONFIG_EPOLL */
 	struct address_space	*f_mapping;
 };
+/*
+ * files_lock can also be taken from softirq context:
+ */
 extern spinlock_t files_lock;
-#define file_list_lock() spin_lock(&files_lock);
-#define file_list_unlock() spin_unlock(&files_lock);
+
+#define file_list_lock()	spin_lock_bh(&files_lock);
+#define file_list_unlock()	spin_unlock_bh(&files_lock);
 
 #define get_file(x)	atomic_inc(&(x)->f_count)
 #define file_count(x)	atomic_read(&(x)->f_count)

  reply	other threads:[~2006-01-25 18:14 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-01-25 17:03 [patch, validator] fix proc_subdir_lock related deadlock Ingo Molnar
2006-01-25 17:14 ` Steven Rostedt
2006-01-25 18:08   ` Ingo Molnar
2006-01-25 18:14     ` Ingo Molnar [this message]
2006-01-25 18:23     ` Andrew Morton
2006-01-25 20:21       ` Ingo Molnar
2006-01-26  0:02       ` [patch, lock validator] fix proc_inum_lock " Ingo Molnar
2006-01-26  0:11         ` Ingo Molnar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20060125181441.GA14541@elte.hu \
    --to=mingo@elte.hu \
    --cc=akpm@osdl.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=viro@ftp.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.