diff options
author | Ian Abbott <abbotti@mev.co.uk> | 2014-04-10 22:41:57 +0400 |
---|---|---|
committer | Greg Kroah-Hartman <gregkh@linuxfoundation.org> | 2014-04-16 22:41:45 +0400 |
commit | b34aa86f12e8848ba453215602c8c50fa63c4cb3 (patch) | |
tree | de0df46918495ebf9930f8cdd6cb693bb91fad75 /drivers/staging | |
parent | 2c33d7cc3875e75311d24a3bfe8d40ca201da2da (diff) | |
download | linux-b34aa86f12e8848ba453215602c8c50fa63c4cb3.tar.xz |
staging: comedi: fix circular locking dependency in comedi_mmap()
Mmapping a comedi data buffer with lockdep checking enabled produced the
following kernel debug messages:
======================================================
[ INFO: possible circular locking dependency detected ]
3.5.0-rc3-ija1+ #9 Tainted: G C
-------------------------------------------------------
comedi_test/4160 is trying to acquire lock:
(&dev->mutex#2){+.+.+.}, at: [<ffffffffa00313f4>] comedi_mmap+0x57/0x1d9 [comedi]
but task is already holding lock:
(&mm->mmap_sem){++++++}, at: [<ffffffff810c96fe>] vm_mmap_pgoff+0x41/0x76
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&mm->mmap_sem){++++++}:
[<ffffffff8106d0e8>] lock_acquire+0x97/0x105
[<ffffffff810ce3bc>] might_fault+0x6d/0x90
[<ffffffffa0031ffb>] do_devinfo_ioctl.isra.7+0x11e/0x14c [comedi]
[<ffffffffa003227f>] comedi_unlocked_ioctl+0x256/0xe48 [comedi]
[<ffffffff810f7fcd>] vfs_ioctl+0x18/0x34
[<ffffffff810f87fd>] do_vfs_ioctl+0x382/0x43c
[<ffffffff810f88f9>] sys_ioctl+0x42/0x65
[<ffffffff81415c62>] system_call_fastpath+0x16/0x1b
-> #0 (&dev->mutex#2){+.+.+.}:
[<ffffffff8106c528>] __lock_acquire+0x101d/0x1591
[<ffffffff8106d0e8>] lock_acquire+0x97/0x105
[<ffffffff8140c894>] mutex_lock_nested+0x46/0x2a4
[<ffffffffa00313f4>] comedi_mmap+0x57/0x1d9 [comedi]
[<ffffffff810d5816>] mmap_region+0x281/0x492
[<ffffffff810d5c92>] do_mmap_pgoff+0x26b/0x2a7
[<ffffffff810c971a>] vm_mmap_pgoff+0x5d/0x76
[<ffffffff810d493f>] sys_mmap_pgoff+0xc7/0x10d
[<ffffffff81004d36>] sys_mmap+0x16/0x20
[<ffffffff81415c62>] system_call_fastpath+0x16/0x1b
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&mm->mmap_sem);
lock(&dev->mutex#2);
lock(&mm->mmap_sem);
lock(&dev->mutex#2);
*** DEADLOCK ***
To avoid the circular dependency, just try to get the lock in
`comedi_mmap()` instead of blocking. Since the comedi device's main mutex
is heavily used, do a down-read of its `attach_lock` rwsemaphore
instead. Trying to down-read `attach_lock` should only fail if
some task has down-write locked it, and that is only done while the
comedi device is being attached to or detached from a low-level hardware
device.
Unfortunately, acquiring the `attach_lock` doesn't prevent another
task replacing the comedi data buffer we are trying to mmap. The
details of the buffer are held in a `struct comedi_buf_map` and pointed
to by `s->async->buf_map` where `s` is the comedi subdevice whose buffer
we are trying to map. The `struct comedi_buf_map` is already reference
counted with a `struct kref`, so we can stop it being freed prematurely.
Modify `comedi_mmap()` to call new function
`comedi_buf_map_from_subdev_get()` to read the subdevice's current
buffer map pointer and increment its reference instead of accessing
`async->buf_map` directly. Call `comedi_buf_map_put()` to decrement the
reference once the buffer map structure has been dealt with. (Note that
`comedi_buf_map_put()` does nothing if passed a NULL pointer.)
`comedi_buf_map_from_subdev_get()` checks the subdevice's buffer map
pointer has been set and the buffer map has been initialized enough for
`comedi_mmap()` to deal with it (specifically, check the `n_pages`
member has been set to a non-zero value). If all is well, the buffer
map's reference is incremented and a pointer to it is returned. The
comedi subdevice's spin-lock is used to protect the checks. Also use
the spin-lock in `__comedi_buf_alloc()` and `__comedi_buf_free()` to
protect changes to the subdevice's buffer map structure pointer and the
buffer map structure's `n_pages` member. (This checking of `n_pages` is
a bit clunky and I [Ian Abbott] plan to deal with it in the future.)
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Cc: <stable@vger.kernel.org> # 3.14.x, 3.15.x
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Diffstat (limited to 'drivers/staging')
-rw-r--r-- | drivers/staging/comedi/comedi_buf.c | 37 | ||||
-rw-r--r-- | drivers/staging/comedi/comedi_fops.c | 18 | ||||
-rw-r--r-- | drivers/staging/comedi/comedi_internal.h | 2 |
3 files changed, 51 insertions, 6 deletions
diff --git a/drivers/staging/comedi/comedi_buf.c b/drivers/staging/comedi/comedi_buf.c index 924fce977985..257595016161 100644 --- a/drivers/staging/comedi/comedi_buf.c +++ b/drivers/staging/comedi/comedi_buf.c @@ -61,6 +61,8 @@ static void __comedi_buf_free(struct comedi_device *dev, struct comedi_subdevice *s) { struct comedi_async *async = s->async; + struct comedi_buf_map *bm; + unsigned long flags; if (async->prealloc_buf) { vunmap(async->prealloc_buf); @@ -68,8 +70,11 @@ static void __comedi_buf_free(struct comedi_device *dev, async->prealloc_bufsz = 0; } - comedi_buf_map_put(async->buf_map); + spin_lock_irqsave(&s->spin_lock, flags); + bm = async->buf_map; async->buf_map = NULL; + spin_unlock_irqrestore(&s->spin_lock, flags); + comedi_buf_map_put(bm); } static void __comedi_buf_alloc(struct comedi_device *dev, @@ -80,6 +85,7 @@ static void __comedi_buf_alloc(struct comedi_device *dev, struct page **pages = NULL; struct comedi_buf_map *bm; struct comedi_buf_page *buf; + unsigned long flags; unsigned i; if (!IS_ENABLED(CONFIG_HAS_DMA) && s->async_dma_dir != DMA_NONE) { @@ -92,8 +98,10 @@ static void __comedi_buf_alloc(struct comedi_device *dev, if (!bm) return; - async->buf_map = bm; kref_init(&bm->refcount); + spin_lock_irqsave(&s->spin_lock, flags); + async->buf_map = bm; + spin_unlock_irqrestore(&s->spin_lock, flags); bm->dma_dir = s->async_dma_dir; if (bm->dma_dir != DMA_NONE) /* Need ref to hardware device to free buffer later. */ @@ -127,7 +135,9 @@ static void __comedi_buf_alloc(struct comedi_device *dev, pages[i] = virt_to_page(buf->virt_addr); } + spin_lock_irqsave(&s->spin_lock, flags); bm->n_pages = i; + spin_unlock_irqrestore(&s->spin_lock, flags); /* vmap the prealloc_buf if all the pages were allocated */ if (i == n_pages) @@ -150,6 +160,29 @@ int comedi_buf_map_put(struct comedi_buf_map *bm) return 1; } +/* returns s->async->buf_map and increments its kref refcount */ +struct comedi_buf_map * +comedi_buf_map_from_subdev_get(struct comedi_subdevice *s) +{ + struct comedi_async *async = s->async; + struct comedi_buf_map *bm = NULL; + unsigned long flags; + + if (!async) + return NULL; + + spin_lock_irqsave(&s->spin_lock, flags); + bm = async->buf_map; + /* only want it if buffer pages allocated */ + if (bm && bm->n_pages) + comedi_buf_map_get(bm); + else + bm = NULL; + spin_unlock_irqrestore(&s->spin_lock, flags); + + return bm; +} + bool comedi_buf_is_mmapped(struct comedi_async *async) { struct comedi_buf_map *bm = async->buf_map; diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index ea6dc36d753b..acc80197e35e 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -1926,14 +1926,21 @@ static int comedi_mmap(struct file *file, struct vm_area_struct *vma) struct comedi_device *dev = file->private_data; struct comedi_subdevice *s; struct comedi_async *async; - struct comedi_buf_map *bm; + struct comedi_buf_map *bm = NULL; unsigned long start = vma->vm_start; unsigned long size; int n_pages; int i; int retval; - mutex_lock(&dev->mutex); + /* + * 'trylock' avoids circular dependency with current->mm->mmap_sem + * and down-reading &dev->attach_lock should normally succeed without + * contention unless the device is in the process of being attached + * or detached. + */ + if (!down_read_trylock(&dev->attach_lock)) + return -EAGAIN; if (!dev->attached) { dev_dbg(dev->class_dev, "no driver attached\n"); @@ -1973,7 +1980,9 @@ static int comedi_mmap(struct file *file, struct vm_area_struct *vma) } n_pages = size >> PAGE_SHIFT; - bm = async->buf_map; + + /* get reference to current buf map (if any) */ + bm = comedi_buf_map_from_subdev_get(s); if (!bm || n_pages > bm->n_pages) { retval = -EINVAL; goto done; @@ -1997,7 +2006,8 @@ static int comedi_mmap(struct file *file, struct vm_area_struct *vma) retval = 0; done: - mutex_unlock(&dev->mutex); + up_read(&dev->attach_lock); + comedi_buf_map_put(bm); /* put reference to buf map - okay if NULL */ return retval; } diff --git a/drivers/staging/comedi/comedi_internal.h b/drivers/staging/comedi/comedi_internal.h index 9a746570f161..a492f2d2436e 100644 --- a/drivers/staging/comedi/comedi_internal.h +++ b/drivers/staging/comedi/comedi_internal.h @@ -19,6 +19,8 @@ void comedi_buf_reset(struct comedi_async *async); bool comedi_buf_is_mmapped(struct comedi_async *async); void comedi_buf_map_get(struct comedi_buf_map *bm); int comedi_buf_map_put(struct comedi_buf_map *bm); +struct comedi_buf_map *comedi_buf_map_from_subdev_get( + struct comedi_subdevice *s); unsigned int comedi_buf_write_n_allocated(struct comedi_async *async); void comedi_device_cancel_all(struct comedi_device *dev); |