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>
This commit is contained in:
parent
2c33d7cc38
commit
b34aa86f12
|
@ -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;
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
|
|
@ -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);
|
||||
|
||||
|
|
Loading…
Reference in a new issue