[PATCH -mm 0/3] nbd: fix/cleanup the usage of kernel_dequeue_signal()

[PATCH -mm 0/3] nbd: fix/cleanup the usage of kernel_dequeue_signal()

[Oleg Nesterov]

On top of

	signal-turn-dequeue_signal_lock-into-kernel_dequeue_signal
	
in -mm plus

	signal-turn-dequeue_signal_lock-into-kernel_dequeue_signal-fix

I sent today.

Another untested (but simple) series. Needs an ack from  Markus or
should be ignored, I can easily miss something.

Oleg.

[PATCH -mm 1/3] nbd: nbd_thread_send: remove the unnecessary kernel_dequeue_signal()

[Oleg Nesterov]

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 <oleg@redhat.com>
---
 drivers/block/nbd.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index b85e7a0..e5d96e5 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -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;
 }
 
-- 
1.5.5.1

[PATCH -mm 2/3] nbd: nbd_thread_recv: remove the buggy kernel_dequeue_signal()

[Oleg Nesterov]

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 <oleg@redhat.com>
---
 drivers/block/nbd.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index e5d96e5..0ffd73c 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -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);
-- 
1.5.5.1

[PATCH -mm 3/3] nbd: don't abuse irqsave/irqrestore while taking nbd->tasks_lock

[Oleg Nesterov]

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 <oleg@redhat.com>
---
 drivers/block/nbd.c |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 0ffd73c..fd79405 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -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;
 }
-- 
1.5.5.1

Re: [PATCH -mm 2/3] nbd: nbd_thread_recv: remove the buggy kernel_dequeue_signal()

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]

Hi Oleg,

On Sun, Oct 25, 2015 at 04:26:39PM +0100, Oleg Nesterov wrote:
> 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.

> 
> 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.

Yes indeed. This has to be fixed as well, thanks.

Best Regards,

Markus


> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  drivers/block/nbd.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index e5d96e5..0ffd73c 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -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);
> -- 
> 1.5.5.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH -mm 1/3] nbd: nbd_thread_send: remove the unnecessary kernel_dequeue_signal()

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]

On Sun, Oct 25, 2015 at 04:26:37PM +0100, Oleg Nesterov wrote:
> 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().

Yes, I don't think it would be good to go into a tcp socket shutdown
with a pending signal although I don't know if this would make
difficulties.

I will apply the patch to my tree and send it out as pull request to
Jens as usual.

Thanks,

Markus

> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  drivers/block/nbd.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index b85e7a0..e5d96e5 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -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;
>  }
>  
> -- 
> 1.5.5.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH -mm 3/3] nbd: don't abuse irqsave/irqrestore while taking nbd->tasks_lock

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 3072 bytes --]

On Sun, Oct 25, 2015 at 04:26:41PM +0100, Oleg Nesterov wrote:
> 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.

Thanks, applied.

Regards,

Markus

> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  drivers/block/nbd.c |   22 ++++++++++------------
>  1 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 0ffd73c..fd79405 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -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;
>  }
> -- 
> 1.5.5.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH -mm 2/3] nbd: nbd_thread_recv: remove the buggy kernel_dequeue_signal()

[Oleg Nesterov]

On 10/26, Markus Pargmann wrote:
>
> Hi Oleg,
>
> On Sun, Oct 25, 2015 at 04:26:39PM +0100, Oleg Nesterov wrote:
> > 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.

I strongly believe this kernel_dequeue_signal() must die, it is very
wrong and I can't even enumerate all problems.

And why do you want it? Probably to "hide" the SIGKILL sent while this
process was exposed as ->task_recv. This can not work even in the simplest
case when this process is single-threaded. kernel_dequeue_signal() won't
clear SIGNAL_GROUP_EXIT set by SIGKILL, it won't remove SIGKILL from
shared_pending if it was sent by the kill command.

Note also that SIGKILL can be sent from another thread which does
exec/group_exit/coredump. In this case the state of this thread group
will be very wrong after kernel_dequeue_signal() removes SIGKILL from
task_struct->pending.

Finally. We can not even know which signal we are going to dequeue and
throw out. Say, it can be SIGCHLD or any other unrelated signal.

No, this can't be right.

Oleg.