Re: [Qemu-devel] [PATCH v2 2/3] filter-rewriter: fix memory leak for connection in connection_track_table

[Jason Wang <jasowang@redhat.com>]

On 2017年02月27日 18:29, Hailiang Zhang wrote:
> On 2017/2/27 17:05, Jason Wang wrote:
>>
>>
>> On 2017年02月27日 14:53, Hailiang Zhang wrote:
>>>> I think the issue is that your code can not differ A from B.
>>>>
>>>
>>> We have a parameter 'fin_ack_seq' recording the sequence of
>>> 'FIN=1,ACK=1,seq=w,ack=u+1' and if the ack value from the opposite
>>> side is is 'w+1', we can consider this connection is closed, no ?
>>
>
> Hi Jason,
>
> Thanks very much for your patience.
>
>> Let's see what happens, consider VM is doing active close (reuse the
>> figure above):
>>
>
> (We didn't support tracking the connection start
> by the VM in current rewriter codes.
> I mean the Client side is VM).

So the questions are still there:

1) Can this patch knows whether or not the connection is started by VM? 
If yes, any specific reason to do this?
2) Even if it only support VM as server, server can still do active 
close for many reasons. What if VM send FIN first?

>
> Your figure is not quite correct, the process should be:
>                                           (VM)
> Client:                                    Server:
>
> ESTABLISHED|                               |
>            | -> FIN=1,seq=u   ->           |
> FIN_WAIT_1 |                               |
>            | <- ACK=1,seq=v,ack=u+1 <-     |
> FINA_WAIT_2|                               |CLOSE_WAIT
>            | <- FIN=1,ACK=1,seq=w,ack=u+1<-|
> handle_secondary():
> fin_ack_seq = w
> tcp_state = TCPS_LAST_ACK
>
>            |                               |LAST+ACK
>            |   -> ACK=1,seq=u+1,ack=w+1    |
> TIME_WAIT  |                               |CLOSED
> CLOSED     |                               |
>
> handle_primary():
> if (ack = fin_ack_seq + 1)
>    g_hash_table_remove()

Yes, that's what I said. The code looks correct for passive close.

Thanks

>
>> (VM)
>> Client:                                    Server:
>>
>> ESTABLISHED|                               |
>>              | -> FIN=1,seq=u   ->           |
>>
>> handle_secondary():
>> fin_ack_seq = u
>> tcp_state = TCPS_LAST_ACK
>>
>> FIN_WAIT_1 |                               |
>>              | <- ACK=1,seq=v,ack=u+1 <-     |
>>
>> handle_primary():
>> fin_ack_seq = ack + 1
>> g_hash_table_remove()
>>
>> But we probably want it to be removed in TIME_WAIT_CLOSED.
>>
>
> Yes, we should removed it after 2MSL, because the last
> the sever side may not get the 'ACK=1,seq=v,ack=u+1' packet,
> and it will resend the 'FIN=1,ACK=1,seq=w,ack=u+1'.
>
> Thanks.
>
>
>> Thanks
>>
>> .
>>
>
>