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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
