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
