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

Groups > linux.kernel > #1233804 > unrolled thread

Re: [PATCH 01/10] nbd: Fix timeout detection

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

  Re: [PATCH 01/10] nbd: Fix timeout detection Ben Hutchings <ben@decadent.org.uk> - 2015-09-28 02:30 +0200
    Re: [PATCH 01/10] nbd: Fix timeout detection Markus Pargmann <mpa@pengutronix.de> - 2015-10-01 08:10 +0200

#1233804 — Re: [PATCH 01/10] nbd: Fix timeout detection

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

On Mon, 2015-08-17 at 08:20 +0200, Markus Pargmann wrote:
> At the moment the nbd timeout just detects hanging tcp operations. This
> is not enough to detect a hanging or bad connection as expected of a
> timeout.
> 
> This patch redesigns the timeout detection to include some more cases.
> The timeout is now in relation to replies from the server. If the server
> does not send replies within the timeout the connection will be shut
> down.
> 
> The patch adds a continous timer 'timeout_timer' that is setup in one of
> two cases:
>  - The request list is empty and we are sending the first request out to
>    the server. We want to have a reply within the given timeout,
>    otherwise we consider the connection to be dead.
>  - A server response was received. This means the server is still
>    communicating with us. The timer is reset to the timeout value.
> 
> The timer is not stopped if the list becomes empty. It will just trigger
> a timeout which will directly leave the handling routine again as the
> request list is empty.
> 
> The whole patch does not use any additional explicit locking. The
> list_empty() calls are safe to be used concurrently. The timer is locked
> internally as we just use mod_timer and del_timer_sync().

This is crazy.  The timer is locked internally but the tasks are not.
So it is possible for the timeout handler to kill a task after it
exited from nbd_do_it()/nbd_thread_recv(), or after it exited entirely
(use-after-free).

[...]
> +> 	> task = READ_ONCE(nbd->task_send);
> +> 	> if (task)
> +> 	> 	> force_sig(SIGKILL, nbd->task_send);
[...]

And this is just... what?  What is the point of using READ_ONCE() if
you're going to look up nbd->task_send again?

Ben.

-- 
Ben Hutchings
All extremists should be taken out and shot.

[toc] | [next] | [standalone]

#1237003

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

Hi,

On Mon, Sep 28, 2015 at 01:27:44AM +0100, Ben Hutchings wrote:
> On Mon, 2015-08-17 at 08:20 +0200, Markus Pargmann wrote:
> > At the moment the nbd timeout just detects hanging tcp operations. This
> > is not enough to detect a hanging or bad connection as expected of a
> > timeout.
> > 
> > This patch redesigns the timeout detection to include some more cases.
> > The timeout is now in relation to replies from the server. If the server
> > does not send replies within the timeout the connection will be shut
> > down.
> > 
> > The patch adds a continous timer 'timeout_timer' that is setup in one of
> > two cases:
> >  - The request list is empty and we are sending the first request out to
> >    the server. We want to have a reply within the given timeout,
> >    otherwise we consider the connection to be dead.
> >  - A server response was received. This means the server is still
> >    communicating with us. The timer is reset to the timeout value.
> > 
> > The timer is not stopped if the list becomes empty. It will just trigger
> > a timeout which will directly leave the handling routine again as the
> > request list is empty.
> > 
> > The whole patch does not use any additional explicit locking. The
> > list_empty() calls are safe to be used concurrently. The timer is locked
> > internally as we just use mod_timer and del_timer_sync().
> 
> This is crazy.  The timer is locked internally but the tasks are not.
> So it is possible for the timeout handler to kill a task after it
> exited from nbd_do_it()/nbd_thread_recv(), or after it exited entirely
> (use-after-free).

Indeed, thanks. I am working on a patch to fix this issue.

Best Regards,

Markus

> 
> [...]
> > +> 	> task = READ_ONCE(nbd->task_send);
> > +> 	> if (task)
> > +> 	> 	> force_sig(SIGKILL, nbd->task_send);
> [...]
> 
> And this is just... what?  What is the point of using READ_ONCE() if
> you're going to look up nbd->task_send again?
> 
> Ben.
> 
> -- 
> Ben Hutchings
> All extremists should be taken out and shot.



-- 
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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web