[PATCH 0/5][SRU][XENIAL] Fix nbd panic on ubuntu_nbd_smoke_test

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/5][SRU][XENIAL] Fix nbd panic on ubuntu_nbd_smoke_test

Colin Ian King-2
From: Colin Ian King <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1793464

== SRU Justification ==

When running the Ubuntu nbd autotest regression test we trip a hang
and then a little later a panic message.  There are two upstream
fixes required as this is actually two issues in one. One fix is to
not to shutdown the sock when IRQs are disable and a second to fix is
to race in the nbd ioctl.

== Fix ==

Upstream commits:

23272a6754b81ff6503e09c743bb4ceeeab39997
  nbd: Remove signal usage

1f7b5cf1be4351e60cf8ae7aab976503dd73c5f8
  nbd: Timeouts are not user requested disconnects

0e4f0f6f63d3416a9e529d99febfe98545427b81
  nbd: Cleanup reset of nbd and bdev after a disconnect

c261189862c6f65117eb3b1748622a08ef49c262
  nbd: don't shutdown sock with irq's disabled

97240963eb308d8d21a89c0459822f7ea98463b4
  nbd: fix race in ioctl

The first 3 patches are prerequisites required for the latter two fixes to apply and work correctly.  Most of these backports are minor patch wiggles
required because later patches have been applied to the driver in earlier fixes to this driver.
   

== Regression Potential ==

These fixes just touch nbd, so the regression potential is just limited to this. Secondly, we are pulling in upstream fixes that exist in Bionic and Cosmic kernels, so these are tried and tested fixes.

== Test Case ==

  1. Deploy a node with 4.4 Xenial
  2. Run the ubuntu_nbd_smoke_test

Without the fix, we get hang/crashes.  With the fix one can run this test
multiple times without any issues at all.

----

Josef Bacik (1):
  nbd: don't shutdown sock with irq's disabled

Markus Pargmann (3):
  nbd: Remove signal usage
  nbd: Timeouts are not user requested disconnects
  nbd: Cleanup reset of nbd and bdev after a disconnect

Vegard Nossum (1):
  nbd: fix race in ioctl

 drivers/block/nbd.c | 200 ++++++++++++++++++++++++++--------------------------
 1 file changed, 99 insertions(+), 101 deletions(-)

--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/5][SRU][XENIAL] nbd: Remove signal usage

Colin Ian King-2
From: Markus Pargmann <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1793464

As discussed on the mailing list, the usage of signals for timeout
handling has a lot of potential issues. The nbd driver used for some
time signals for timeouts. These signals where able to get the threads
out of the blocking socket operations.

This patch removes all signal usage and uses a socket shutdown instead.
The socket descriptor itself is cleared later when the whole nbd device
is closed.

The tasks_lock is removed as we do not depend on this anymore. Instead
a new lock for the socket is introduced so we can safely work with the
socket in the timeout handler outside of the two main threads.

Cc: Oleg Nesterov <[hidden email]>
Cc: Christoph Hellwig <[hidden email]>
Signed-off-by: Markus Pargmann <[hidden email]>
Reviewed-by: Christoph Hellwig <[hidden email]>
(backported from commit 23272a6754b81ff6503e09c743bb4ceeeab39997)
Signed-off-by: Colin Ian King <[hidden email]>
---
 drivers/block/nbd.c | 126 ++++++++++++++++++++--------------------------------
 1 file changed, 48 insertions(+), 78 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 754b6fd..d65a6f5 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -60,7 +60,8 @@ struct nbd_device {
  bool disconnect; /* a disconnect has been requested by user */
 
  struct timer_list timeout_timer;
- spinlock_t tasks_lock;
+ /* protects initialization and shutdown of the socket */
+ spinlock_t sock_lock;
  struct task_struct *task_recv;
  struct task_struct *task_send;
 
@@ -170,13 +171,20 @@ static void nbd_end_request(struct nbd_device *nbd, struct request *req)
  */
 static void sock_shutdown(struct nbd_device *nbd)
 {
- if (!nbd->sock)
+ spin_lock_irq(&nbd->sock_lock);
+
+ if (!nbd->sock) {
+ spin_unlock_irq(&nbd->sock_lock);
  return;
+ }
 
  dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
  kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
+ sockfd_put(nbd->sock);
  nbd->sock = NULL;
- del_timer_sync(&nbd->timeout_timer);
+ spin_unlock_irq(&nbd->sock_lock);
+
+ del_timer(&nbd->timeout_timer);
 }
 
 static void nbd_xmit_timeout(unsigned long arg)
@@ -189,17 +197,15 @@ static void nbd_xmit_timeout(unsigned long arg)
 
  nbd->disconnect = true;
 
- spin_lock_irqsave(&nbd->tasks_lock, flags);
+ spin_lock_irqsave(&nbd->sock_lock, flags);
 
- if (nbd->task_recv)
- force_sig(SIGKILL, nbd->task_recv);
 
- if (nbd->task_send)
- force_sig(SIGKILL, nbd->task_send);
+ if (nbd->sock)
+ kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
 
- spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+ spin_unlock_irqrestore(&nbd->sock_lock, flags);
 
- dev_err(nbd_to_dev(nbd), "Connection timed out, killed receiver and sender, shutting down connection\n");
+ dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
 }
 
 /*
@@ -212,7 +218,6 @@ static int sock_xmit(struct nbd_device *nbd, int send, void *buf, int size,
  int result;
  struct msghdr msg;
  struct kvec iov;
- sigset_t blocked, oldset;
  unsigned long pflags = current->flags;
 
  if (unlikely(!sock)) {
@@ -222,11 +227,6 @@ static int sock_xmit(struct nbd_device *nbd, int send, void *buf, int size,
  return -EINVAL;
  }
 
- /* Allow interception of SIGKILL only
- * Don't allow other signals to interrupt the transmission */
- siginitsetinv(&blocked, sigmask(SIGKILL));
- sigprocmask(SIG_SETMASK, &blocked, &oldset);
-
  current->flags |= PF_MEMALLOC;
  do {
  sock->sk->sk_allocation = GFP_NOIO | __GFP_MEMALLOC;
@@ -253,7 +253,6 @@ static int sock_xmit(struct nbd_device *nbd, int send, void *buf, int size,
  buf += result;
  } while (size > 0);
 
- sigprocmask(SIG_SETMASK, &oldset, NULL);
  tsk_restore_flags(current, pflags, PF_MEMALLOC);
 
  if (!send && nbd->xmit_timeout)
@@ -447,23 +446,18 @@ static int nbd_thread_recv(struct nbd_device *nbd, struct block_device *bdev)
 {
  struct request *req;
  int ret;
- unsigned long flags;
 
  BUG_ON(nbd->magic != NBD_MAGIC);
 
  sk_set_memalloc(nbd->sock->sk);
 
- spin_lock_irqsave(&nbd->tasks_lock, flags);
  nbd->task_recv = current;
- spin_unlock_irqrestore(&nbd->tasks_lock, flags);
 
  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");
 
- spin_lock_irqsave(&nbd->tasks_lock, flags);
  nbd->task_recv = NULL;
- spin_unlock_irqrestore(&nbd->tasks_lock, flags);
 
  return ret;
  }
@@ -484,19 +478,7 @@ static int nbd_thread_recv(struct nbd_device *nbd, struct block_device *bdev)
 
  device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
 
- spin_lock_irqsave(&nbd->tasks_lock, flags);
  nbd->task_recv = NULL;
- spin_unlock_irqrestore(&nbd->tasks_lock, flags);
-
- if (signal_pending(current)) {
- ret = kernel_dequeue_signal(NULL);
- dev_warn(nbd_to_dev(nbd), "pid %d, %s, got signal %d\n",
- task_pid_nr(current), current->comm, ret);
- mutex_lock(&nbd->tx_lock);
- sock_shutdown(nbd);
- mutex_unlock(&nbd->tx_lock);
- ret = -ETIMEDOUT;
- }
 
  return ret;
 }
@@ -589,11 +571,8 @@ static int nbd_thread_send(void *data)
 {
  struct nbd_device *nbd = data;
  struct request *req;
- unsigned long flags;
 
- spin_lock_irqsave(&nbd->tasks_lock, flags);
  nbd->task_send = current;
- spin_unlock_irqrestore(&nbd->tasks_lock, flags);
 
  set_user_nice(current, MIN_NICE);
  while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) {
@@ -602,17 +581,6 @@ static int nbd_thread_send(void *data)
  kthread_should_stop() ||
  !list_empty(&nbd->waiting_queue));
 
- if (signal_pending(current)) {
- int ret = kernel_dequeue_signal(NULL);
-
- dev_warn(nbd_to_dev(nbd), "pid %d, %s, got signal %d\n",
- task_pid_nr(current), current->comm, ret);
- mutex_lock(&nbd->tx_lock);
- sock_shutdown(nbd);
- mutex_unlock(&nbd->tx_lock);
- break;
- }
-
  /* extract request */
  if (list_empty(&nbd->waiting_queue))
  continue;
@@ -627,13 +595,7 @@ static int nbd_thread_send(void *data)
  nbd_handle_req(nbd, req);
  }
 
- spin_lock_irqsave(&nbd->tasks_lock, flags);
  nbd->task_send = NULL;
- spin_unlock_irqrestore(&nbd->tasks_lock, flags);
-
- /* Clear maybe pending signals */
- if (signal_pending(current))
- kernel_dequeue_signal(NULL);
 
  return 0;
 }
@@ -681,6 +643,25 @@ static void nbd_request_handler(struct request_queue *q)
  }
 }
 
+static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
+{
+ int ret = 0;
+
+ spin_lock_irq(&nbd->sock_lock);
+
+ if (nbd->sock) {
+ ret = -EBUSY;
+ goto out;
+ }
+
+ nbd->sock = sock;
+
+out:
+ spin_unlock_irq(&nbd->sock_lock);
+
+ return ret;
+}
+
 static int nbd_dev_dbg_init(struct nbd_device *nbd);
 static void nbd_dev_dbg_close(struct nbd_device *nbd);
 
@@ -713,32 +694,26 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
  return 0;
  }
 
- case NBD_CLEAR_SOCK: {
- struct socket *sock = nbd->sock;
- nbd->sock = NULL;
+ case NBD_CLEAR_SOCK:
+ sock_shutdown(nbd);
  nbd_clear_que(nbd);
  BUG_ON(!list_empty(&nbd->queue_head));
  BUG_ON(!list_empty(&nbd->waiting_queue));
  kill_bdev(bdev);
- if (sock)
- sockfd_put(sock);
  return 0;
- }
 
  case NBD_SET_SOCK: {
- struct socket *sock;
  int err;
- if (nbd->sock)
- return -EBUSY;
- sock = sockfd_lookup(arg, &err);
- if (sock) {
- nbd->sock = sock;
- if (max_part > 0)
- bdev->bd_invalidated = 1;
- nbd->disconnect = false; /* we're connected now */
- return 0;
- }
- return -EINVAL;
+ struct socket *sock = sockfd_lookup(arg, &err);
+
+ if (!sock)
+ return err;
+
+ err = nbd_set_socket(nbd, sock);
+ if (!err && max_part)
+ bdev->bd_invalidated = 1;
+
+ return err;
  }
 
  case NBD_SET_BLKSIZE: {
@@ -771,7 +746,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 
  case NBD_DO_IT: {
  struct task_struct *thread;
- struct socket *sock;
  int error;
 
  if (nbd->task_recv)
@@ -806,14 +780,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
  mutex_lock(&nbd->tx_lock);
 
  sock_shutdown(nbd);
- sock = nbd->sock;
- nbd->sock = NULL;
  nbd_clear_que(nbd);
  kill_bdev(bdev);
  queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
  set_device_ro(bdev, false);
- if (sock)
- sockfd_put(sock);
  nbd->flags = 0;
  nbd->bytesize = 0;
  bdev->bd_inode->i_size = 0;
@@ -1105,7 +1075,7 @@ static int __init nbd_init(void)
  nbd_dev[i].magic = NBD_MAGIC;
  INIT_LIST_HEAD(&nbd_dev[i].waiting_queue);
  spin_lock_init(&nbd_dev[i].queue_lock);
- spin_lock_init(&nbd_dev[i].tasks_lock);
+ spin_lock_init(&nbd_dev[i].sock_lock);
  INIT_LIST_HEAD(&nbd_dev[i].queue_head);
  mutex_init(&nbd_dev[i].tx_lock);
  init_timer(&nbd_dev[i].timeout_timer);
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/5][SRU][XENIAL] nbd: Timeouts are not user requested disconnects

Colin Ian King-2
In reply to this post by Colin Ian King-2
From: Markus Pargmann <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1793464

It may be useful to know in the client that a connection timed out. The
current code returns success for a timeout.

This patch reports the error code -ETIMEDOUT for a timeout.

Signed-off-by: Markus Pargmann <[hidden email]>
(backported from commit 1f7b5cf1be4351e60cf8ae7aab976503dd73c5f8)
Signed-off-by: Colin Ian King <[hidden email]>
---
 drivers/block/nbd.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index d65a6f5..cc2844f 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -57,6 +57,7 @@ struct nbd_device {
  loff_t blksize;
  loff_t bytesize;
  int xmit_timeout;
+ bool timedout;
  bool disconnect; /* a disconnect has been requested by user */
 
  struct timer_list timeout_timer;
@@ -195,10 +196,9 @@ static void nbd_xmit_timeout(unsigned long arg)
  if (list_empty(&nbd->queue_head))
  return;
 
- nbd->disconnect = true;
-
  spin_lock_irqsave(&nbd->sock_lock, flags);
 
+ nbd->timedout = true;
 
  if (nbd->sock)
  kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
@@ -791,7 +791,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
  if (max_part > 0)
  blkdev_reread_part(bdev);
  if (nbd->disconnect) /* user requested, ignore socket errors */
- return 0;
+ error = 0;
+ if (nbd->timedout)
+ error = -ETIMEDOUT;
+
  return error;
  }
 
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/5][SRU][XENIAL] nbd: Cleanup reset of nbd and bdev after a disconnect

Colin Ian King-2
In reply to this post by Colin Ian King-2
From: Markus Pargmann <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1793464

Group all variables that are reset after a disconnect into reset
functions. This patch adds two of these functions, nbd_reset() and
nbd_bdev_reset().

Signed-off-by: Markus Pargmann <[hidden email]>
(cherry picked from commit 0e4f0f6f63d3416a9e529d99febfe98545427b81)
Signed-off-by: Colin Ian King <[hidden email]>
---
 drivers/block/nbd.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index cc2844f..910c3c6 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -662,6 +662,30 @@ out:
  return ret;
 }
 
+/* Reset all properties of an NBD device */
+static void nbd_reset(struct nbd_device *nbd)
+{
+ nbd->disconnect = false;
+ nbd->timedout = false;
+ nbd->blksize = 1024;
+ nbd->bytesize = 0;
+ set_capacity(nbd->disk, 0);
+ nbd->flags = 0;
+ nbd->xmit_timeout = 0;
+ queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
+ del_timer_sync(&nbd->timeout_timer);
+}
+
+static void nbd_bdev_reset(struct block_device *bdev)
+{
+ set_device_ro(bdev, false);
+ bdev->bd_inode->i_size = 0;
+ if (max_part > 0) {
+ blkdev_reread_part(bdev);
+ bdev->bd_invalidated = 1;
+ }
+}
+
 static int nbd_dev_dbg_init(struct nbd_device *nbd);
 static void nbd_dev_dbg_close(struct nbd_device *nbd);
 
@@ -782,19 +806,15 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
  sock_shutdown(nbd);
  nbd_clear_que(nbd);
  kill_bdev(bdev);
- queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
- set_device_ro(bdev, false);
- nbd->flags = 0;
- nbd->bytesize = 0;
- bdev->bd_inode->i_size = 0;
- set_capacity(nbd->disk, 0);
- if (max_part > 0)
- blkdev_reread_part(bdev);
+ nbd_bdev_reset(bdev);
+
  if (nbd->disconnect) /* user requested, ignore socket errors */
  error = 0;
  if (nbd->timedout)
  error = -ETIMEDOUT;
 
+ nbd_reset(nbd);
+
  return error;
  }
 
@@ -1086,14 +1106,12 @@ static int __init nbd_init(void)
  nbd_dev[i].timeout_timer.data = (unsigned long)&nbd_dev[i];
  init_waitqueue_head(&nbd_dev[i].active_wq);
  init_waitqueue_head(&nbd_dev[i].waiting_wq);
- nbd_dev[i].blksize = 1024;
- nbd_dev[i].bytesize = 0;
  disk->major = NBD_MAJOR;
  disk->first_minor = i << part_shift;
  disk->fops = &nbd_fops;
  disk->private_data = &nbd_dev[i];
  sprintf(disk->disk_name, "nbd%d", i);
- set_capacity(disk, 0);
+ nbd_reset(&nbd_dev[i]);
  add_disk(disk);
  }
 
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 4/5][SRU][XENIAL] nbd: don't shutdown sock with irq's disabled

Colin Ian King-2
In reply to this post by Colin Ian King-2
From: Josef Bacik <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1793464

We hit a warning when shutting down the nbd connection because we have irq's
disabled.  We don't really need to do the shutdown under the lock, just clear
the nbd->sock.  So do the shutdown outside of the irq.  This gets rid of the
warning.

Signed-off-by: Josef Bacik <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(backported from commit c261189862c6f65117eb3b1748622a08ef49c262)
Signed-off-by: Colin Ian King <[hidden email]>
---
 drivers/block/nbd.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 910c3c6..c2a688b 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -172,6 +172,8 @@ static void nbd_end_request(struct nbd_device *nbd, struct request *req)
  */
 static void sock_shutdown(struct nbd_device *nbd)
 {
+ struct socket *sock;
+
  spin_lock_irq(&nbd->sock_lock);
 
  if (!nbd->sock) {
@@ -179,18 +181,21 @@ static void sock_shutdown(struct nbd_device *nbd)
  return;
  }
 
+ sock = nbd->sock;
  dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
- kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
- sockfd_put(nbd->sock);
  nbd->sock = NULL;
  spin_unlock_irq(&nbd->sock_lock);
 
+ kernel_sock_shutdown(sock, SHUT_RDWR);
+ sockfd_put(sock);
+
  del_timer(&nbd->timeout_timer);
 }
 
 static void nbd_xmit_timeout(unsigned long arg)
 {
  struct nbd_device *nbd = (struct nbd_device *)arg;
+ struct socket *sock = NULL;
  unsigned long flags;
 
  if (list_empty(&nbd->queue_head))
@@ -200,10 +205,16 @@ static void nbd_xmit_timeout(unsigned long arg)
 
  nbd->timedout = true;
 
- if (nbd->sock)
- kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
+ if (nbd->sock) {
+ sock = nbd->sock;
+ get_file(sock->file);
+ }
 
  spin_unlock_irqrestore(&nbd->sock_lock, flags);
+ if (sock) {
+ kernel_sock_shutdown(sock, SHUT_RDWR);
+ sockfd_put(sock);
+ }
 
  dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
 }
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 5/5][SRU][XENIAL] nbd: fix race in ioctl

Colin Ian King-2
In reply to this post by Colin Ian King-2
From: Vegard Nossum <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1793464

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
 0000000000000000 ffff8820330bba68 ffffffff814b8791 ffff8820330bbac8
 0000000000000000 ffff8820330bbab8 ffffffff810d04ab ffff8820330bbaa8
 0000001f00000296 0000000000017681 ffff8810380bf000 ffffffffa0001790
Call Trace:
 [<ffffffff814b8791>] dump_stack+0x4d/0x6c
 [<ffffffff810d04ab>] __warn+0xdb/0x100
 [<ffffffff810d0574>] warn_slowpath_fmt+0x44/0x50
 [<ffffffff81218c65>] sysfs_warn_dup+0x65/0x80
 [<ffffffff81218a02>] sysfs_add_file_mode_ns+0x172/0x180
 [<ffffffff81218a35>] sysfs_create_file_ns+0x25/0x30
 [<ffffffff81594a76>] device_create_file+0x36/0x90
 [<ffffffffa0000e8d>] __nbd_ioctl+0x32d/0x9b0 [nbd]
 [<ffffffff814cc8e8>] ? find_next_bit+0x18/0x20
 [<ffffffff810f7c29>] ? select_idle_sibling+0xe9/0x120
 [<ffffffff810f6cd7>] ? __enqueue_entity+0x67/0x70
 [<ffffffff810f9bf0>] ? enqueue_task_fair+0x630/0xe20
 [<ffffffff810efa76>] ? resched_curr+0x36/0x70
 [<ffffffff810f0078>] ? check_preempt_curr+0x78/0x90
 [<ffffffff810f00a2>] ? ttwu_do_wakeup+0x12/0x80
 [<ffffffff810f01b1>] ? ttwu_do_activate.constprop.86+0x61/0x70
 [<ffffffff810f0c15>] ? try_to_wake_up+0x185/0x2d0
 [<ffffffff810f0d6d>] ? default_wake_function+0xd/0x10
 [<ffffffff81105471>] ? autoremove_wake_function+0x11/0x40
 [<ffffffffa0001577>] nbd_ioctl+0x67/0x94 [nbd]
 [<ffffffff814ac0fd>] blkdev_ioctl+0x14d/0x940
 [<ffffffff811b0da2>] ? put_pipe_info+0x22/0x60
 [<ffffffff811d96cc>] block_ioctl+0x3c/0x40
 [<ffffffff811ba08d>] do_vfs_ioctl+0x8d/0x5e0
 [<ffffffff811aa329>] ? ____fput+0x9/0x10
 [<ffffffff810e9092>] ? task_work_run+0x72/0x90
 [<ffffffff811ba627>] SyS_ioctl+0x47/0x80
 [<ffffffff8185f5df>] 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 <[hidden email]>
Cc: Markus Pargmann <[hidden email]>
Cc: Paul Clements <[hidden email]>
Cc: Pavel Machek <[hidden email]>
Cc: Jens Axboe <[hidden email]>
Cc: Al Viro <[hidden email]>
Signed-off-by: Vegard Nossum <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(backported from commit 97240963eb308d8d21a89c0459822f7ea98463b4)
Signed-off-by: Colin Ian King <[hidden email]>
---
 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 c2a688b..cfbd129 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -462,14 +462,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;
  }
 
@@ -488,9 +483,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);
 
  if (nbd->flags & NBD_FLAG_READ_ONLY)
@@ -804,6 +798,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);
  }
 
@@ -813,6 +808,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);
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH 0/5][SRU][XENIAL] Fix nbd panic on ubuntu_nbd_smoke_test

Stefan Bader-2
In reply to this post by Colin Ian King-2
On 11.10.2018 16:41, Colin King wrote:

> From: Colin Ian King <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1793464
>
> == SRU Justification ==
>
> When running the Ubuntu nbd autotest regression test we trip a hang
> and then a little later a panic message.  There are two upstream
> fixes required as this is actually two issues in one. One fix is to
> not to shutdown the sock when IRQs are disable and a second to fix is
> to race in the nbd ioctl.
>
> == Fix ==
>
> Upstream commits:
>
> 23272a6754b81ff6503e09c743bb4ceeeab39997
>   nbd: Remove signal usage
>
> 1f7b5cf1be4351e60cf8ae7aab976503dd73c5f8
>   nbd: Timeouts are not user requested disconnects
>
> 0e4f0f6f63d3416a9e529d99febfe98545427b81
>   nbd: Cleanup reset of nbd and bdev after a disconnect
>
> c261189862c6f65117eb3b1748622a08ef49c262
>   nbd: don't shutdown sock with irq's disabled
>
> 97240963eb308d8d21a89c0459822f7ea98463b4
>   nbd: fix race in ioctl
>
> The first 3 patches are prerequisites required for the latter two fixes to apply and work correctly.  Most of these backports are minor patch wiggles
> required because later patches have been applied to the driver in earlier fixes to this driver.
>    
>
> == Regression Potential ==
>
> These fixes just touch nbd, so the regression potential is just limited to this. Secondly, we are pulling in upstream fixes that exist in Bionic and Cosmic kernels, so these are tried and tested fixes.
>
> == Test Case ==
>
>   1. Deploy a node with 4.4 Xenial
>   2. Run the ubuntu_nbd_smoke_test
>
> Without the fix, we get hang/crashes.  With the fix one can run this test
> multiple times without any issues at all.
>
> ----
>
> Josef Bacik (1):
>   nbd: don't shutdown sock with irq's disabled
>
> Markus Pargmann (3):
>   nbd: Remove signal usage
>   nbd: Timeouts are not user requested disconnects
>   nbd: Cleanup reset of nbd and bdev after a disconnect
>
> Vegard Nossum (1):
>   nbd: fix race in ioctl
>
>  drivers/block/nbd.c | 200 ++++++++++++++++++++++++++--------------------------
>  1 file changed, 99 insertions(+), 101 deletions(-)
>
Limited to single driver which was verified to be fixed.

Acked-by: Stefan Bader <[hidden email]>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH 0/5][SRU][XENIAL] Fix nbd panic on ubuntu_nbd_smoke_test

Kleber Souza
In reply to this post by Colin Ian King-2
On 10/11/18 16:41, Colin King wrote:

> From: Colin Ian King <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1793464
>
> == SRU Justification ==
>
> When running the Ubuntu nbd autotest regression test we trip a hang
> and then a little later a panic message.  There are two upstream
> fixes required as this is actually two issues in one. One fix is to
> not to shutdown the sock when IRQs are disable and a second to fix is
> to race in the nbd ioctl.
>
> == Fix ==
>
> Upstream commits:
>
> 23272a6754b81ff6503e09c743bb4ceeeab39997
>   nbd: Remove signal usage
>
> 1f7b5cf1be4351e60cf8ae7aab976503dd73c5f8
>   nbd: Timeouts are not user requested disconnects
>
> 0e4f0f6f63d3416a9e529d99febfe98545427b81
>   nbd: Cleanup reset of nbd and bdev after a disconnect
>
> c261189862c6f65117eb3b1748622a08ef49c262
>   nbd: don't shutdown sock with irq's disabled
>
> 97240963eb308d8d21a89c0459822f7ea98463b4
>   nbd: fix race in ioctl
>
> The first 3 patches are prerequisites required for the latter two fixes to apply and work correctly.  Most of these backports are minor patch wiggles
> required because later patches have been applied to the driver in earlier fixes to this driver.
>    
>
> == Regression Potential ==
>
> These fixes just touch nbd, so the regression potential is just limited to this. Secondly, we are pulling in upstream fixes that exist in Bionic and Cosmic kernels, so these are tried and tested fixes.
>
> == Test Case ==
>
>   1. Deploy a node with 4.4 Xenial
>   2. Run the ubuntu_nbd_smoke_test
>
> Without the fix, we get hang/crashes.  With the fix one can run this test
> multiple times without any issues at all.
>
> ----
>
> Josef Bacik (1):
>   nbd: don't shutdown sock with irq's disabled
>
> Markus Pargmann (3):
>   nbd: Remove signal usage
>   nbd: Timeouts are not user requested disconnects
>   nbd: Cleanup reset of nbd and bdev after a disconnect
>
> Vegard Nossum (1):
>   nbd: fix race in ioctl
>
>  drivers/block/nbd.c | 200 ++++++++++++++++++++++++++--------------------------
>  1 file changed, 99 insertions(+), 101 deletions(-)
>

Limited to nbd, tested.

Acked-by: Kleber Sacilotto de Souza <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team