Re: [PATCH v2] migration/multifd: close TLS channel before socket finalize

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Nov 10, 2020 at 06:45:45PM +0800, Zheng Chuan wrote:
> 
> 
> On 2020/11/10 18:12, Daniel P. Berrangé wrote:
> > On Fri, Nov 06, 2020 at 06:54:54PM +0800, Chuan Zheng wrote:
> >> Since we now support tls multifd, when we cancel migration, the TLS
> >> sockets will be left as CLOSE-WAIT On Src which results in socket
> >> leak.
> >> Fix it by closing TLS channel before socket finalize.
> >>
> >> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> >> ---
> >>  migration/multifd.c | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >>
> >> diff --git a/migration/multifd.c b/migration/multifd.c
> >> index 68b171f..a6838dc 100644
> >> --- a/migration/multifd.c
> >> +++ b/migration/multifd.c
> >> @@ -523,6 +523,19 @@ static void multifd_send_terminate_threads(Error *err)
> >>      }
> >>  }
> >>  
> >> +static void multifd_tls_socket_close(QIOChannel *ioc, Error *err)
> >> +{
> >> +    if (ioc &&
> >> +        object_dynamic_cast(OBJECT(ioc),
> >> +                            TYPE_QIO_CHANNEL_TLS)) {
> >> +        /*
> >> +         * TLS channel is special, we need close it before
> >> +         * socket finalize.
> >> +         */
> >> +        qio_channel_close(ioc, &err);
> >> +    }
> >> +}
> > 
> > This doesn't feel quite right to me.  Calling qio_channel_close will close
> > both the TLS layer, and the underlying QIOChannelSocket. If the latter
> > is safe to do, then we don't need the object_dynamic_cast() check, we can
> > do it unconditionally whether we're using TLS or not.
> > 
> > Having said that, I'm not sure if we actually want to be using
> > qio_channel_close or not ?
> > 
> > I would have expected that there is already code somewhere else in the
> > migration layer that is closing these multifd channels, but I can't
> > actually find where that happens right now.  Assuming that code does
> > exist though, qio_channel_shutdown(ioc, BOTH) feels like the right
> > answer to unblock waiting I/O ops.
> > 
> Hi, Daniel.
> Actually, I have tried to use qio_channel_shutdown at the same place,
> but it seems not work right.
> the socket connection is closed by observing through 'ss' command but
> the socket fds in /proc/$(qemu pid)/fd are still residual.
> 
> The underlying QIOChannelSocket will be closed by
> qio_channel_socket_finalize() through object_unref(QIOChannel) later
> in socket_send_channel_destroy(),
> does that means it is safe to close both of TLS and tcp socket?

Hmm, that makes me even more confused, because the object_unref
should be calling qio_channel_close() already.

eg with your patch we have:

       multifd_tls_socket_close(p->c, NULL);
           -> qio_channel_close(p->c)
	        -> qio_channel_tls_close(p->c)
                     -> qio_channel_close(master)

       socket_send_channel_destroy(p->c)
           -> object_unref(p->c)
	         -> qio_channel_tls_socket_finalize(p->c)
		      -> object_unref(master)
		              -> qio_channel_close(master)

so the multifd_tls_socket_close should not be doing anything
at all *assuming* we releasing the last reference in our
object_unref call.

Given what you describe, I think we are *not* releasing the
last reference. There is an active reference being held
somewhere else, and that is preventing the QIOChannelSocket
from being freed and thus the socket remains open.

If that extra active reference is a bug, then we have a memory
leak of the QIOChannelSocket object, that needs fixing somewhere.

If that extra active reference is intentional, then we do indeed
need to explicitly close the socket. That is possibly better
handled by putting a qio_channel_close() call into the
socket_send_channel_destroy() method.

I wonder if we're leaking a reference to the underlying QIOChannelSocket,
when we create the QIOChannelTLS wrapper ? That could explain a problem
that only happens when using TLS.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|