Re: [PATCH v3 2/2] nbd: Clean up clients more efficiently

[Eric Blake]

On Tue, Aug 06, 2024 at 10:32:54AM GMT, Daniel P. Berrangé wrote:
> On Mon, Aug 05, 2024 at 09:21:36PM -0500, Eric Blake wrote:
> > Since an NBD server may be long-living, serving clients that
> > repeatedly connect and disconnect, it can be more efficient to clean
> > up after each client disconnects, rather than storing a list of
> > resources to clean up when the server exits.  Rewrite the list of
> > known clients to be double-linked so that we can get O(1) deletion to
> > keep the list pruned to size as clients exit.  This in turn requires
> > each client to track an opaque pointer of owner information (although
> > qemu-nbd doesn't need to refer to it).
> 
> I tend to feel that this needs to be squashed into the previous
> patch.  The previous patch effectively creates unbounded memory
> usage in the NBD server. ie consider a client that connects and
> immediately disconnects, not sending any data, in a tight loop.
> It will "leak" NBDConn & QIOChanelSocket pointers for each
> iteration of the loop, only to be cleaned up when the NBD Server
> is shutdown.
> 
> Especially given that we tagged the previous commit with a CVE
> and not this commit,  people could be misled into backporting
> only the former commit leaving them open to the leak.

Makes sense.  Will respin v4 along those lines; although I plan to
refactor slightly: have patch 1 pick up the part of this patch that
allows server.c to track a client's owner, then patch 2 be the CVE fix
creating the doubly-linked list of QIOSocketChannel wrappers as
owners.  Also, I realized that nbd_server->connections == 0 is now
effectively redundant with QLIST_EMPTY(&nbd_server->conns), so that's
another cleanup to squash in.

> > @@ -103,14 +112,9 @@ static void nbd_server_free(NBDServerData *server)
> >       */
> >      qio_net_listener_disconnect(server->listener);
> >      object_unref(OBJECT(server->listener));
> > -    while (!QSLIST_EMPTY(&server->conns)) {
> > -        NBDConn *conn = QSLIST_FIRST(&server->conns);
> > -
> > +    QLIST_FOREACH_SAFE(conn, &server->conns, next, tmp) {
> >          qio_channel_shutdown(QIO_CHANNEL(conn->cioc), 
> > QIO_CHANNEL_SHUTDOWN_BOTH,
> >                               NULL);
> > -        object_unref(OBJECT(conn->cioc));
> > -        QSLIST_REMOVE_HEAD(&server->conns, next);
> > -        g_free(conn);
> >      }
> > 
> >      AIO_WAIT_WHILE_UNLOCKED(NULL, server->connections > 0);

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org