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

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

On 2017/2/27 13:35, Jason Wang wrote:
>
>
> On 2017年02月27日 12:09, Hailiang Zhang wrote:
>> On 2017/2/27 11:40, Jason Wang wrote:
>>>
>>>
>>> On 2017年02月27日 11:11, Hailiang Zhang wrote:
>>>> On 2017/2/23 12:16, Jason Wang wrote:
>>>>>
>>>>>
>>>>> On 2017年02月22日 16:51, Hailiang Zhang wrote:
>>>>>> On 2017/2/22 16:45, Hailiang Zhang wrote:
>>>>>>> On 2017/2/22 16:07, Jason Wang wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2017年02月22日 11:46, zhanghailiang wrote:
>>>>>>>>> After a net connection is closed, we didn't clear its releated
>>>>>>>>> resources
>>>>>>>>> in connection_track_table, which will lead to memory leak.
>>>>>>>>
>>>>>>>> Not a real leak but would lead reset of hash table if too many
>>>>>>>> closed
>>>>>>>> connections.
>>>>>>>>
>>>>>>>
>>>>>>> Yes, you are right, there will be lots of stale connection data in
>>>>>>> hash table
>>>>>>> if we don't remove it while it is been closed. Which
>>>>>
>>>>>
>>>>> Ok, so let's come up with a better title of the patch.
>>>>>
>>>>
>>>> OK.
>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>> Let't track the state of net connection, if it is closed, its
>>>>>>>>> related
>>>>>>>>> resources will be cleared up.
>>>>>>>>
>>>>>>>> The issue is the state were tracked partially, do we need a full
>>>>>>>> state
>>>>>>>> machine here?
>>>>>>>>
>>>>>>>
>>>>>>> Not, IMHO, we only care about the last state of it, because, we will
>>>>>>> do nothing
>>>>>>> even if we track the intermedial states.
>>>>>
>>>>> Well, you care at least syn state too. Without a complete state
>>>>> machine,
>>>>> it's very hard to track even partial state I believe. And you will
>>>>> fail
>>>>> to track some state transition for sure which makes the code fragile.
>>>>>
>>>>
>>>> Agree, but here things are a little different. There are some extreme
>>>> cases
>>>> that we may can't track the complete process of closing connection.
>>>> For example (I have explained that in the bellow, it seems that you
>>>> didn't
>>>> got it ;) ).
>>>> If VM is running before we want to make it goes into COLO FT state,
>>>> there maybe some connections exist already, in extreme case, VM is
>>>> going into
>>>> COLO state while some connections are in half closing state, we can
>>>> only track
>>>> the bellow half closing state in filter-rewriter and colo compare
>>>> object.
>>>>
>>>>
>
> [...]
>
>>>> Er, here we track the last two states 'FIN=1, ACK=1' and  'ACK=1' (
>>>> which asks
>>>> the 'FIN=1,ACK=1' packet, We will remove the connection while got the
>>>> 'ACK=1'
>>>> packet, so  is it enough ?
>>>
>>> But the connection is not closed in fact, no? It's legal for remote to
>>> continue sending tons of packet to us even after this.
>>>
>>
>> Er, I'm a little confused, Here, for server side,
>> i think after got the 'ACK=1,seq=u+1,ack=w+1', it is closed,
>> so i remove it from hash table, wrong ?
>>
>> Client:                                    Server:
>>
>> ESTABLISHED|                               |
>>             | -> FIN=1,seq=u   ->           |
>
> This is case A and ACK should be set in this segment too.
>
>> FIN_WAIT_1 |                               |
>>             | <- ACK=1,seq=v,ack=u+1 <-     |
>> FINA_WAIT_2|                               |CLOSE_WAIT
>>             | <- FIN=1,ACK=1,seq=w,ack=u+1<-|
>>             |                               |LAST+ACK
>
> This is case B.
>
>> |   -> ACK=1,seq=u+1,ack=w+1    |
>> TIME_WAIT  |                               |CLOSED
>> CLOSED     |                               |
>>
>
> 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 ?

> Thanks
>
>>>>
>>>>> What's more I don't think we can decide passive or active close by:
>>>>>
>>>>>
>>>>> +    if ((tcp_pkt->th_flags & (TH_ACK | TH_FIN)) == (TH_ACK |
>>>>> TH_FIN)) {
>>>>>
>>>>> Since both cases will send FIN,ACK for sure.
>>>>>
>>>>
>>>> I didn't quite understand, here we have tracked the closing request no
>>>> matter
>>>> it is from the server side (passive close ?) or client side ( active
>>>> close ?).
>>>> You can refer to the comment in codes, 'Case 1' and 'Case 2' comments.
>>>
>>> I think you need differ them since passive close is much simpler, and it
>>> seems that your code may work only in this case.
>>>
>>>>
>>>> Here, it seems that we can't track one case which both sides send the
>>>> closing
>>>> requests at the same time, in that case, there are only 'FIN=1' and
>>>> 'ACK=1'
>>>> packets.
>>>>
>>>
>>> Yes, RFC allows this.
>>>
>>
>> Hmm, I'd like to remove this patch from this series,
>> And send it as a single patch after all the above questions
>> been solved. How about the other patches ?
>>
>
> Looks good except for the compiling issue of patch 3.
>

OK, i will fix it in version 3.

Thanks

> Thanks
>
>> Thanks,
>> Hailiang
>>
>>
>>> Thanks
>>>
>>>>
>>>> Thanks.
>>>> Hailiang
>>>>
>
> [...]
>
> .
>