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日 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.

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.

Thanks

> Thanks,
> Hailiang
>
>
>> Thanks
>>
>>>
>>> Thanks.
>>> Hailiang
>>>

[...]