From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758324AbbJHUOx (ORCPT ); Thu, 8 Oct 2015 16:14:53 -0400 Received: from shadbolt.e.decadent.org.uk ([88.96.1.126]:34370 "EHLO shadbolt.e.decadent.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754713AbbJHUOv (ORCPT ); Thu, 8 Oct 2015 16:14:51 -0400 Message-ID: <1444335269.2956.268.camel@decadent.org.uk> Subject: Re: [PATCH] nbd: Add locking for tasks From: Ben Hutchings To: Markus Pargmann Cc: Jens Axboe , nbd-general@lists.sourceforge.net, Michal Belczyk , Hermann Lauer , linux-kernel@vger.kernel.org Date: Thu, 08 Oct 2015 21:14:29 +0100 In-Reply-To: <1444154634-24927-1-git-send-email-mpa@pengutronix.de> References: <20151001060436.GN19121@pengutronix.de> <1444154634-24927-1-git-send-email-mpa@pengutronix.de> Content-Type: multipart/signed; micalg="pgp-sha512"; protocol="application/pgp-signature"; boundary="=-KMfPO9XPdiCR7OiP8Qrv" X-Mailer: Evolution 3.16.5-1 Mime-Version: 1.0 X-SA-Exim-Connect-IP: 192.168.4.247 X-SA-Exim-Mail-From: ben@decadent.org.uk X-SA-Exim-Scanned: No (on shadbolt.decadent.org.uk); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-KMfPO9XPdiCR7OiP8Qrv Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2015-10-06 at 20:03 +0200, Markus Pargmann wrote: > The timeout handling introduced in > > 7e2893a16d3e (nbd: Fix timeout detection) > introduces a race condition which may lead to killing of tasks that are > not in nbd context anymore. This was not observed or reproducable yet. >=20 > This patch adds locking to critical use of task_recv and task_send to > avoid killing tasks that already left the NBD thread functions. This > lock is only acquired if a timeout occures or the nbd device > starts/stops. >=20 > Reported-by: Ben Hutchings > Signed-off-by: Markus Pargmann Reviewed-by: Ben Hutchings You could add 'Fixes: 7e2893a16d3e ("nbd: Fix timeout detection")' to the commit message as well. nbd_dbg_tasks_show() can still race with thread exit and two tasks can=20 race to become the receive thread, but those aren't new bugs. Ben. > --- > drivers/block/nbd.c | 36 ++++++++++++++++++++++++++++++------ > 1 file changed, 30 insertions(+), 6 deletions(-) >=20 > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c > index 039c7c4f0539..1a70852ac808 100644 > --- a/drivers/block/nbd.c > +++ b/drivers/block/nbd.c > @@ -60,6 +60,7 @@ struct nbd_device { > > > bool disconnect; /* a disconnect has been requested by user */ > =20 > > > struct timer_list timeout_timer; > +> > spinlock_t tasks_lock; > > > struct task_struct *task_recv; > > > struct task_struct *task_send; > =20 > @@ -140,21 +141,23 @@ static void sock_shutdown(struct nbd_device *nbd) > static void nbd_xmit_timeout(unsigned long arg) > { > > > struct nbd_device *nbd =3D (struct nbd_device *)arg; > -> > struct task_struct *task; > +> > unsigned long flags; > =20 > > > if (list_empty(&nbd->queue_head)) > > > > return; > =20 > > > nbd->disconnect =3D true; > =20 > -> > task =3D READ_ONCE(nbd->task_recv); > -> > if (task) > -> > > force_sig(SIGKILL, task); > +> > spin_lock_irqsave(&nbd->tasks_lock, flags); > + > +> > if (nbd->task_recv) > +> > > force_sig(SIGKILL, nbd->task_recv); > =20 > -> > task =3D READ_ONCE(nbd->task_send); > -> > if (task) > +> > if (nbd->task_send) > > > > force_sig(SIGKILL, nbd->task_send); > =20 > +> > spin_unlock_irqrestore(&nbd->tasks_lock, flags); > + > > > dev_err(nbd_to_dev(nbd), "Connection timed out, killed receiver and= sender, shutting down connection\n"); > } > =20 > @@ -403,17 +406,24 @@ static int nbd_thread_recv(struct nbd_device *nbd) > { > > > struct request *req; > > > int ret; > +> > unsigned long flags; > =20 > > > BUG_ON(nbd->magic !=3D NBD_MAGIC); > =20 > > > sk_set_memalloc(nbd->sock->sk); > =20 > +> > spin_lock_irqsave(&nbd->tasks_lock, flags); > > > nbd->task_recv =3D current; > +> > spin_unlock_irqrestore(&nbd->tasks_lock, flags); > =20 > > > ret =3D 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 =3D NULL; > +> > > spin_unlock_irqrestore(&nbd->tasks_lock, flags); > + > > > > return ret; > > > } > =20 > @@ -429,7 +439,9 @@ static int nbd_thread_recv(struct nbd_device *nbd) > =20 > > > device_remove_file(disk_to_dev(nbd->disk), &pid_attr); > =20 > +> > spin_lock_irqsave(&nbd->tasks_lock, flags); > > > nbd->task_recv =3D NULL; > +> > spin_unlock_irqrestore(&nbd->tasks_lock, flags); > =20 > > > if (signal_pending(current)) { > > > > siginfo_t info; > @@ -534,8 +546,11 @@ static int nbd_thread_send(void *data) > { > > > struct nbd_device *nbd =3D data; > > > struct request *req; > +> > unsigned long flags; > =20 > +> > spin_lock_irqsave(&nbd->tasks_lock, flags); > > > nbd->task_send =3D current; > +> > spin_unlock_irqrestore(&nbd->tasks_lock, flags); > =20 > > > set_user_nice(current, MIN_NICE); > > > while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) = { > @@ -572,7 +587,15 @@ static int nbd_thread_send(void *data) > > > > nbd_handle_req(nbd, req); > > > } > =20 > +> > spin_lock_irqsave(&nbd->tasks_lock, flags); > > > nbd->task_send =3D NULL; > +> > spin_unlock_irqrestore(&nbd->tasks_lock, flags); > + > +> > /* Clear maybe pending signals */ > +> > if (signal_pending(current)) { > +> > > siginfo_t info; > +> > > dequeue_signal_lock(current, =C2=A4t->blocked, &info); > +> > } > =20 > > > return 0; > } > @@ -1027,6 +1050,7 @@ static int __init nbd_init(void) > > > > nbd_dev[i].magic =3D 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); > > > > INIT_LIST_HEAD(&nbd_dev[i].queue_head); > > > > mutex_init(&nbd_dev[i].tx_lock); > > > > init_timer(&nbd_dev[i].timeout_timer); --=20 Ben Hutchings Once a job is fouled up, anything done to improve it makes it worse. --=-KMfPO9XPdiCR7OiP8Qrv Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIVAwUAVhbOpue/yOyVhhEJAQrX2Q//bHq+3AOkJxldWwqGSk51ceW4DsktgGrN zlpSSiNctI/MQck8ZcW4AxzCm0MowLweD8OzGQKVpH2M5P4zXGyM4X4mwr6ZAtSj fK20gJJqeJRzQWdTNWKbjxlFubMUZqIt9EM5I8K2WlKa3eT18+zZTcsEljek8sy6 wSqLg3v/vBoKXIVLJj/r6/OVXoO/yLAkj4kVN3BEc8WmQFxBde0W4T4QT5IbtSPC HCJ0nw8R4gFODYn4IBYEDg9bnBdmHJBGRfUuOVkvnEV8gbeiFGyIygJF8kV1M2rp cSAHFlxjmKbb89ZY1iCTuCCvf4fovFlt29e0HWEW83GvBkp+NLfGKfZtZippMwWo I2oFn+VwBtkm2zF6sWXuYtKoIL6rbro/Y4DIxW1Sgz+Wd5TASjfpCzDeOyzoQh4o WINlX6LV5FpFwRE2HNcJd6twtkUPW7rqzYxndJK9ep+gT2KHyTQWo+eyu/KFkWM3 ERZH0a3WSgUxmB39s/lQ950/pFqM3m/AiuaIUIodp86DUmgvlPRWSdZ9GWnDYLTC WQ+vflEQKrYNn+/c+tH4rJCf8h3eZqZ6Eytr77amSd/MKUyI5UIbcgvN3uMN/qJr 8pz4HHce78b2o79S65Ge7wgfUs6xLwYWUL/JdbbClmg1A6sDpVj/dnHxzzxYwcA4 +Wp8Q6QfYy0= =Dxf5 -----END PGP SIGNATURE----- --=-KMfPO9XPdiCR7OiP8Qrv--