Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1255412 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH -mm 2/3] nbd: nbd_thread_recv: remove the buggy  kernel_dequeue_signal() Oleg Nesterov <oleg@redhat.com> - 2015-10-25 15:40 +0100
    Re: [PATCH -mm 2/3] nbd: nbd_thread_recv: remove the buggy  kernel_dequeue_signal() Markus Pargmann <mpa@pengutronix.de> - 2015-10-26 08:50 +0100
      Re: [PATCH -mm 2/3] nbd: nbd_thread_recv: remove the buggy  kernel_dequeue_signal() Oleg Nesterov <oleg@redhat.com> - 2015-10-28 16:00 +0100

#1255412 — [PATCH -mm 2/3] nbd: nbd_thread_recv: remove the buggy 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 <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

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

[toc] | [next] | [standalone]

#1255716

[Multipart message — attachments visible in raw view] — view raw

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 |

[toc] | [prev] | [next] | [standalone]

#1258221

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.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web