nbd: fix/cleanup the usage of kernel_dequeue_signal()

nbd_thread_recv() is called by userspace, it is very wrong to dequeue and throw out a signal.

I do not understand why nbd_thread_recv() (and nbd_thread_send() btw) does sock_shutdown(); the caller, __nbd_ioctl(NBD_DO_IT) does this too.

Signed-off-by Oleg Nesterov <[email protected]>

---
 drivers/block/nbd.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

drivers/block/nbd.c +2 -3

index e5d96e5..0ffd73c 100644
@@ -444,9 +444,8 @@ static int nbd_thread_recv(struct nbd_device *nbd)
 	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);
+		dev_warn(nbd_to_dev(nbd), "pid %d, %s, got signal",
+			 task_pid_nr(current), current->comm);
 		mutex_lock(&nbd->tx_lock);
 		sock_shutdown(nbd);
 		mutex_unlock(&nbd->tx_lock);

Signature · 1 line

1.5.5.1

nbd_thread_send() does kernel_dequeue_signal() at the end for no reason, it is fine to exit with the pending SIGKILL.

Not sure it really needs another kernel_dequeue_signal() inside the main loop, we know that signal_pending() means SIGKILL. But probably we want to clear TIF_SIGPENDING before sock_shutdown().

Signed-off-by Oleg Nesterov <[email protected]>

---
 drivers/block/nbd.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

drivers/block/nbd.c +0 -4

index b85e7a0..e5d96e5 100644
@@ -586,10 +586,6 @@ static int nbd_thread_send(void *data)
 	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;
 }
 

Signature · 1 line

1.5.5.1

Both nbd_thread_recv() and nbd_thread_send() are might_sleep() and called with irqs enabled(), irqsave/irqrestore make no sense and imo look confusing.

Signed-off-by Oleg Nesterov <[email protected]>

---
 drivers/block/nbd.c |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)

drivers/block/nbd.c +10 -12

index 0ffd73c..fd79405 100644
@@ -406,23 +406,22 @@ static int nbd_thread_recv(struct nbd_device *nbd)
 {
 	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);
+	spin_lock_irq(&nbd->tasks_lock);
 	nbd->task_recv = current;
-	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+	spin_unlock_irq(&nbd->tasks_lock);
 
 	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);
+		spin_lock_irq(&nbd->tasks_lock);
 		nbd->task_recv = NULL;
-		spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+		spin_unlock_irq(&nbd->tasks_lock);
 
 		return ret;
 	}

@@ -439,9 +438,9 @@ static int nbd_thread_recv(struct nbd_device *nbd)
 
 	device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
 
-	spin_lock_irqsave(&nbd->tasks_lock, flags);
+	spin_lock_irq(&nbd->tasks_lock);
 	nbd->task_recv = NULL;
-	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+	spin_unlock_irq(&nbd->tasks_lock);
 
 	if (signal_pending(current)) {
 		dev_warn(nbd_to_dev(nbd), "pid %d, %s, got signal",

@@ -543,11 +542,10 @@ 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);
+	spin_lock_irq(&nbd->tasks_lock);
 	nbd->task_send = current;
-	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+	spin_unlock_irq(&nbd->tasks_lock);
 
 	set_user_nice(current, MIN_NICE);
 	while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) {

@@ -581,9 +579,9 @@ static int nbd_thread_send(void *data)
 		nbd_handle_req(nbd, req);
 	}
 
-	spin_lock_irqsave(&nbd->tasks_lock, flags);
+	spin_lock_irq(&nbd->tasks_lock);
 	nbd->task_send = NULL;
-	spin_unlock_irqrestore(&nbd->tasks_lock, flags);
+	spin_unlock_irq(&nbd->tasks_lock);
 
 	return 0;
 }

Signature · 1 line

1.5.5.1

Hi Oleg,

Quoted 2 lines · Oleg Nesterov

nbd_thread_recv() is called by userspace, it is very wrong to dequeue and throw out a signal.

This signal handling for a userspace process is implicitly implemented for several years already through the timeout handling. This is nothing new and could potentially break userspace if someone disconnects NBD using the kill command. As we expose the appropriate PID of the process as well this is possible to be used in an init script.

So I am not sure about this patch yet.