Bug 11823 - dv1394: possible deadlock if accessed by multithreaded app
dv1394: possible deadlock if accessed by multithreaded app
Status: CLOSED CODE_FIX
Product: Drivers
Classification: Unclassified
Component: IEEE1394
All Linux
: P1 low
Assigned To: Stefan Richter
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-25 01:56 UTC by Stefan Richter
Modified: 2008-11-07 09:28 UTC (History)
0 users

See Also:
Kernel Version: all
Tree: Mainline
Regression: ---


Attachments

Description Stefan Richter 2008-10-25 01:56:17 UTC
Latest working kernel version: unknown
Earliest failing kernel version: unknown

Reported by Johannes Weiner in http://lkml.org/lkml/2008/10/23/192:

  - dv1394_write() locks video->mtx, then calls copy_from_user() which
    may lock mmap_sem if a page fault occurs.

  - dv1394_mmap() is called with mmap_sem held, then locks video->mtx.
Comment 1 Anonymous Emailer 2008-10-26 04:04:14 UTC
Reply-To: stefanr@s5r6.in-berlin.de

Fix a possible though highly unlikely deadlock:

Thread A:                  Thread B:
 - acquire mmap_sem         - dv1394_ioctl/read/write()
 - dv1394_mmap()            - acquire video->mtx
 - acquire video->mtx       - copy_to/from_user(), possible page fault:
                              acquire mmap_sem

The simplest fix is to use mutex_trylock() instead of mutex_lock() in
dv1394_mmap().  This changes the behavior under contention in a way
which is visible to userspace clients.  However, my guess is that no
clients exist which use mmap vs. ioctl/read/write on the dv1394
character device file interface in concurrent threads.

Reported-by: Johannes Weiner <hannes@saeurebad.de>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/dv1394.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Index: linux/drivers/ieee1394/dv1394.c
===================================================================
--- linux.orig/drivers/ieee1394/dv1394.c
+++ linux/drivers/ieee1394/dv1394.c
@@ -1270,8 +1270,14 @@ static int dv1394_mmap(struct file *file
 	struct video_card *video = file_to_video_card(file);
 	int retval = -EINVAL;
 
-	/* serialize mmap */
-	mutex_lock(&video->mtx);
+	/*
+	 * We cannot use the blocking variant mutex_lock here because .mmap
+	 * is called with mmap_sem held, while .ioctl, .read, .write acquire
+	 * video->mtx and subsequently call copy_to/from_user which will
+	 * grab mmap_sem in case of a page fault.
+	 */
+	if (!mutex_trylock(&video->mtx))
+		return -EAGAIN;
 
 	if ( ! video_card_initialized(video) ) {
 		retval = do_dv1394_init_default(video);

Comment 2 Stefan Richter 2008-11-07 09:28:01 UTC
fixed by commit 8449fc3ae58bf8ee5acbd2280754cde67b5db128

Note You need to log in before you can comment on or make changes to this bug.