Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752596AbbKASIq (ORCPT ); Sun, 1 Nov 2015 13:08:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55955 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751481AbbKASIo (ORCPT ); Sun, 1 Nov 2015 13:08:44 -0500 Date: Sun, 1 Nov 2015 20:05:00 +0100 From: Oleg Nesterov To: Markus Pargmann Cc: nbd-general@lists.sourceforge.net, Christoph Hellwig , linux-kernel@vger.kernel.org Subject: Re: [PATCH 0/4] nbd: Remove signal usage Message-ID: <20151101190500.GA1019@redhat.com> References: <1446133360-30652-1-git-send-email-mpa@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1446133360-30652-1-git-send-email-mpa@pengutronix.de> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3487 Lines: 117 Hi Markus, Sorry again for delay. I was offlist. again. On 10/29, Markus Pargmann wrote: > > Hi, > > this is a try to remove all the signals from NBD. The first patch replaces the > signals. The other patches are some cleanups I made on the way. > > This should solve the kthread_run() problems as well. I obviously can't review these changes, I do not understand this code enough. But they look good imo. However, I do not understand the usage of ->task_recv and ->task_send. pid_show() doesn't even check nbd->task_recv != NULL. Honestly, I simply do not know if it can race with device_remove_file() or not. I think it can, but I can be easily wrong... nbd_dbg_tasks_show() looks racy too even if it checks task_recv/task_send, at least this needs READ_ONCE() but in theory this is not enough, task_pid_nr() can read the freed task_struct. Again, I can easily miss something. But whatever I missed, perhaps the trivial (but uncompiled/untested) patch below makes sense anyway? Oleg. diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index f547005..67c1e09 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -63,8 +63,8 @@ struct nbd_device { struct timer_list timeout_timer; /* protects initialization and shutdown of the socket */ spinlock_t sock_lock; - struct task_struct *task_recv; - struct task_struct *task_send; + pid_t task_recv; + pid_t task_send; #if IS_ENABLED(CONFIG_DEBUG_FS) struct dentry *dbg_dir; @@ -392,7 +392,7 @@ static ssize_t pid_show(struct device *dev, struct gendisk *disk = dev_to_disk(dev); struct nbd_device *nbd = (struct nbd_device *)disk->private_data; - return sprintf(buf, "%d\n", task_pid_nr(nbd->task_recv)); + return sprintf(buf, "%d\n", nbd->task_recv); } static struct device_attribute pid_attr = { @@ -409,13 +409,13 @@ static int nbd_thread_recv(struct nbd_device *nbd) sk_set_memalloc(nbd->sock->sk); - nbd->task_recv = current; + nbd->task_recv = task_pid_nr(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; + nbd->task_recv = 0; return ret; } @@ -432,7 +432,7 @@ static int nbd_thread_recv(struct nbd_device *nbd) device_remove_file(disk_to_dev(nbd->disk), &pid_attr); - nbd->task_recv = NULL; + nbd->task_recv = 0; return ret; } @@ -526,7 +526,7 @@ static int nbd_thread_send(void *data) struct nbd_device *nbd = data; struct request *req; - nbd->task_send = current; + nbd->task_send = task_pid_nr(current); set_user_nice(current, MIN_NICE); while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) { @@ -549,7 +549,7 @@ static int nbd_thread_send(void *data) nbd_handle_req(nbd, req); } - nbd->task_send = NULL; + nbd->task_send = 0; return 0; } @@ -827,9 +827,9 @@ static int nbd_dbg_tasks_show(struct seq_file *s, void *unused) struct nbd_device *nbd = s->private; if (nbd->task_recv) - seq_printf(s, "recv: %d\n", task_pid_nr(nbd->task_recv)); + seq_printf(s, "recv: %d\n", nbd->task_recv); if (nbd->task_send) - seq_printf(s, "send: %d\n", task_pid_nr(nbd->task_send)); + seq_printf(s, "send: %d\n", nbd->task_send); return 0; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/