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

[Jason Wang]

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

[...]