code review 6123058: ssh: fix deadlock

ssh: fix deadlock

The code was taking locks in the wrong order.

Fixes issue 3570.

[albert.strasheim, 2012-04-26 16:02:01]

LGTM

At least according to the test. Before the patch I could reproduce the hang,
after not.

Fixes issue 3570.

[agl1, 2012-04-26 16:05:33]

Hello fullung@gmail.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://agl%40golang.org@code.google.com/p/go.crypto/

[agl1, 2012-04-26 16:05:42]

*** Submitted as
http://code.google.com/p/go/source/detail?r=bd78e6211ff8&repo=crypto ***

ssh: fix deadlock

The code was taking locks in the wrong order.

Fixes issue 3570.

R=fullung
CC=golang-dev
http://codereview.appspot.com/6123058

[kardia, 2012-04-26 16:32:15]

I do NOT think this is good. Ok as a stop-gap only.

This solution is acceptable only when the server's window size matches the
packet size AND the amount written and read by the client is large. In cases for
terminal access, this is not so good. If the default window size was adjusted to
be larger, this is not so good.

There are other ways to make this work.


On 2012/04/26 16:05:42, agl1 wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=bd78e6211ff8&repo=crypto ***
> 
> ssh: fix deadlock
> 
> The code was taking locks in the wrong order.
> 
> Fixes issue 3570.
> 
> R=fullung
> CC=golang-dev
> http://codereview.appspot.com/6123058

[agl1, 2012-04-26 16:36:08]

On Thu, Apr 26, 2012 at 12:32 PM,  <kardianos@gmail.com> wrote:
> This solution is acceptable only when the server's window size matches
> the packet size AND the amount written and read by the client is large.
> In cases for terminal access, this is not so good. If the default window
> size was adjusted to be larger, this is not so good.

The logic hasn't been changed here, only the order in which the locks
were taken, which was previously incorrect.

So I think any concerns about the logic are independent of this
change, although I must admit that I don't understand your concern.


Cheers

AGL

[kardia, 2012-04-26 16:50:34]

> On Thu, Apr 26, 2012 at 12:32 PM,<kardianos@gmail.com>  wrote:
>> This solution is acceptable only when the server's window size matches
>> the packet size AND the amount written and read by the client is large.
>> In cases for terminal access, this is not so good. If the default window
>> size was adjusted to be larger, this is not so good.
>
> The logic hasn't been changed here, only the order in which the locks
> were taken, which was previously incorrect.

I stand corrected here.

> So I think any concerns about the logic are independent of this
> change, although I must admit that I don't understand your concern.

I suppose my issue is with the current code then, independent of this 
change. Sorry for the noise.

If I understand correctly, each read from the server channel will result 
in a packet to the client. This seems unnecessary, especially if the use 
case is for a terminal where single charters are being read.

[agl1, 2012-04-26 16:59:55]

On Thu, Apr 26, 2012 at 12:50 PM, Daniel <kardianos@gmail.com> wrote:
> If I understand correctly, each read from the server channel will result in
> a packet to the client. This seems unnecessary, especially if the use case
> is for a terminal where single charters are being read.

Well, at the TCP layer, the server will be ACKing the client's packets
and we can't do much about that. (At best you get to save a few
because of TCP delayed ACK.)

In the SSH code we have this:


windowAdjustment = uint32(len(c.pendingData)-c.length) - c.myWindow
if windowAdjustment < uint32(len(c.pendingData)/2) {
»·······windowAdjustment = 0
}

uint32(len(c.pendingData)-c.length) is how much space we actually have
in the buffer and c.myWindow is how much window space the client
thinks is remaining. So, that's the amount of extra window that we
*could* give to the client. If that's less than half of the buffer
size, we don't bother sending any extra window.

So for the case where the server is reading byte-by-byte, we'll only
send window adjustments very rarely. (Only every 16K reads with
default sizes.)

Over high bandwidth-delay-product links, the buffer would have to be
larger to allow the flow to reach full TCP speeds and it might be
suitable to tweak that limit (i.e. if windowAdjustment > 16KB, always
send an adjustment, even if our buffer is 4MB). But I didn't write
that in because I haven't hit it and have no basis for what the limit
should be.


Cheers

AGL

[kardia, 2012-04-26 17:24:49]

Thanks for taking time to reply. I missed assigning the windowAdjustment 
to zero condition. I'll try to read more throughly :/.

Thanks again,
-Daniel


On 04/26/2012 09:59 AM, Adam Langley wrote:
> On Thu, Apr 26, 2012 at 12:50 PM, Daniel<kardianos@gmail.com>  wrote:
>> If I understand correctly, each read from the server channel will result in
>> a packet to the client. This seems unnecessary, especially if the use case
>> is for a terminal where single charters are being read.
>
> Well, at the TCP layer, the server will be ACKing the client's packets
> and we can't do much about that. (At best you get to save a few
> because of TCP delayed ACK.)
>
> In the SSH code we have this:
>
>
> windowAdjustment = uint32(len(c.pendingData)-c.length) - c.myWindow
> if windowAdjustment<  uint32(len(c.pendingData)/2) {
> »·······windowAdjustment = 0
> }
>
> uint32(len(c.pendingData)-c.length) is how much space we actually have
> in the buffer and c.myWindow is how much window space the client
> thinks is remaining. So, that's the amount of extra window that we
> *could* give to the client. If that's less than half of the buffer
> size, we don't bother sending any extra window.
>
> So for the case where the server is reading byte-by-byte, we'll only
> send window adjustments very rarely. (Only every 16K reads with
> default sizes.)
>
> Over high bandwidth-delay-product links, the buffer would have to be
> larger to allow the flow to reach full TCP speeds and it might be
> suitable to tweak that limit (i.e. if windowAdjustment>  16KB, always
> send an adjustment, even if our buffer is 4MB). But I didn't write
> that in because I haven't hit it and have no basis for what the limit
> should be.
>
>
> Cheers
>
> AGL