Hi,
On Friday 27 May 2016 12:59:35 Vegard Nossum wrote:
> Quentin ran into this bug:
>
> WARNING: CPU: 64 PID: 10085 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x65/0x80
> sysfs: cannot create duplicate filename '/devices/virtual/block/nbd3/pid'
> Modules linked in: nbd
> CPU: 64 PID: 10085 Comm: qemu-nbd Tainted: G D 4.6.0+ #7
> 8820330bba68 814b8791 8820330bbac8
> 8820330bbab8 810d04ab 8820330bbaa8
> 001f0296 00017681 8810380bf000 a0001790
> Call Trace:
> [] dump_stack+0x4d/0x6c
> [] __warn+0xdb/0x100
> [] warn_slowpath_fmt+0x44/0x50
> [] sysfs_warn_dup+0x65/0x80
> [] sysfs_add_file_mode_ns+0x172/0x180
> [] sysfs_create_file_ns+0x25/0x30
> [] device_create_file+0x36/0x90
> [] __nbd_ioctl+0x32d/0x9b0 [nbd]
> [] ? find_next_bit+0x18/0x20
> [] ? select_idle_sibling+0xe9/0x120
> [] ? __enqueue_entity+0x67/0x70
> [] ? enqueue_task_fair+0x630/0xe20
> [] ? resched_curr+0x36/0x70
> [] ? check_preempt_curr+0x78/0x90
> [] ? ttwu_do_wakeup+0x12/0x80
> [] ? ttwu_do_activate.constprop.86+0x61/0x70
> [] ? try_to_wake_up+0x185/0x2d0
> [] ? default_wake_function+0xd/0x10
> [] ? autoremove_wake_function+0x11/0x40
> [] nbd_ioctl+0x67/0x94 [nbd]
> [] blkdev_ioctl+0x14d/0x940
> [] ? put_pipe_info+0x22/0x60
> [] block_ioctl+0x3c/0x40
> [] do_vfs_ioctl+0x8d/0x5e0
> [] ? fput+0x9/0x10
> [] ? task_work_run+0x72/0x90
> [] SyS_ioctl+0x47/0x80
> [] entry_SYSCALL_64_fastpath+0x17/0x93
> ---[ end trace 7899b295e4f850c8 ]---
>
> It seems fairly obvious that device_create_file() is not being protected
> from being run concurrently on the same nbd.
>
> Quentin found the following relevant commits:
>
> 1a2ad21 nbd: add locking to nbd_ioctl
> 90b8f28 [PATCH] end of methods switch: remove the old ones
> d4430d6 [PATCH] beginning of methods conversion
> 08f8585 [PATCH] move block_device_operations to blkdev.h
>
> It would seem that the race was introduced in the process of moving nbd
> from BKL to unlocked ioctls.
>
> By setting nbd->task_recv while the mutex is held, we can prevent other
> processes from running concurrently (since nbd->task_recv is also checked
> while the mutex is held).
>
> Reported-and-tested-by: Quentin Casasnovas
> Cc: Markus Pargmann
> Cc: Paul Clements
> Cc: Pavel Machek
> Cc: Jens Axboe
> Cc: Al Viro
> Signed-off-by: Vegard Nossum
Thanks, applied.
Best Regards,
Markus
> ---
> drivers/block/nbd.c | 12
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 31e73a7..a831f2b 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -451,14 +451,9 @@ static int nbd_thread_recv(struct nbd_device *nbd,
> struct block_device *bdev)
>
> sk_set_memalloc(nbd->sock->sk);
>
> - nbd->task_recv = current;
> -
> ret = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
> if (ret) {
> dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
> -
> - nbd->task_recv = NULL;
> -
> return ret;
> }
>
> @@ -477,9 +472,6 @@ static int nbd_thread_recv(struct nbd_device *nbd, struct
> block_device *bdev)
> nbd_size_clear(nbd, bdev);
>
> device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
> -
> - nbd->task_recv = NULL;
> -
> return ret;
> }
>
> @@ -788,6 +780,8 @@ static int __nbd_ioctl(struct block_device *bdev, struct
> nbd_device *nbd,
> if (!nbd->sock)
> return -EINVAL;
>
> + /* We have to claim the device under the lock */
> + nbd->task_recv = current;
> mutex_unlock(&nbd->tx_lock);
>
> nbd_parse_flags(nbd, bdev);
> @@ -796,6 +790,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct
> nbd_device *nbd,
>nbd_name(nbd));
> if (IS_ERR(thread)) {
> mutex_lock(&nbd->tx_lock);
> + nbd->task_recv = NULL;
> return PTR_ERR(thread);
> }
>
> @@ -805,6 +800,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct
> nbd_device *nbd,
> kthread_stop(thread);
>
> mutex_lock(&nbd->tx_lock);
> + nbd->task_recv = NULL;
>
> sock_shutdown(nbd);
> nbd_clear_que(nbd);
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
signature.asc
Description: This is a digitally signed message part.
