nbd/server.c | 45 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 10 deletions(-)
During NBD_OPT_GO / NBD_OPT_INFO negotiation the server runs in a
coroutine that performs blocking I/O with the client. The handlers call
nbd_export_find(), which returns a raw NBDExport * with no reference
taken (it is a plain QTAILQ lookup), and then dereference that pointer
across multiple yields (nbd_write / nbd_negotiate_send_info etc.).
A client that is still negotiating is not yet on exp->clients, so it
holds no reference. If the management layer runs block-export-del
(mode hard) while such a client is parked in I/O,
nbd_export_request_shutdown() removes the export from the exports list
and the BlockExport is freed; when the negotiating coroutine resumes it
dereferences a dangling NBDExport * -> use-after-free of host memory.
Take a reference right after the lookup succeeds in
nbd_negotiate_handle_export_name() and nbd_negotiate_handle_info(), and
drop it on every path that leaves the handler without having handed the
export to the client (the client takes its own reference when it is
inserted into exp->clients). The success paths are unchanged so the
client-owned reference is not double-counted.
Known remaining spot, deliberately not changed here:
nbd_export_meta_context() stores nbd_export_find()'s result into
meta->exp (client->contexts.exp) which is expected to stay consistent
with client->exp (see the assert in nbd_co_block_status_payload_read()).
The reference semantics of contexts.exp deserve maintainer input before
changing them; this patch only covers the two handlers that run before
the client owns a reference.
This is an RFC: the block/export reference-counting semantics should be
confirmed with the NBD maintainers (notably whether a negotiating client
should be tracked so that block-export-del SAFE mode reports the export
as busy instead of racing with a hard delete).
Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
---
nbd/server.c | 45 +++++++++++++++++++++++++++++++++++----------
1 file changed, 35 insertions(+), 10 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index e6c47f8c2e..8754236a9e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -517,6 +517,14 @@ nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
error_setg(errp, "export not found");
return -EINVAL;
}
+ /*
+ * The export can be deleted by the management layer while we are
+ * blocked in the writes below (before the client is inserted into
+ * exp->clients and takes its own reference). Hold a reference for
+ * the whole negotiation so a concurrent block-export-del cannot
+ * free the export under us.
+ */
+ blk_exp_ref(&client->exp->common);
nbd_check_meta_export(client, client->exp);
myflags = client->exp->nbdflags;
@@ -533,11 +541,15 @@ nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
ret = nbd_write(client->ioc, buf, len, errp);
if (ret < 0) {
error_prepend(errp, "write failed: ");
+ blk_exp_unref(&client->exp->common);
+ client->exp = NULL;
return ret;
}
QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
blk_exp_ref(&client->exp->common);
+ /* Drop the negotiation reference; the client owns one now. */
+ blk_exp_unref(&client->exp->common);
return 0;
}
@@ -659,6 +671,13 @@ nbd_negotiate_handle_info(NBDClient *client, Error **errp)
errp, "export '%s' not present",
sane_name);
}
+ /*
+ * Hold a reference across the whole info exchange: the export can
+ * be deleted by the management layer (block-export-del) while we
+ * are blocked sending replies below and before the client is
+ * inserted into exp->clients for NBD_OPT_GO.
+ */
+ blk_exp_ref(&exp->common);
if (client->opt == NBD_OPT_GO) {
nbd_check_meta_export(client, exp);
}
@@ -668,7 +687,7 @@ nbd_negotiate_handle_info(NBDClient *client, Error **errp)
rc = nbd_negotiate_send_info(client, NBD_INFO_NAME, namelen, name,
errp);
if (rc < 0) {
- return rc;
+ goto out;
}
}
@@ -681,7 +700,7 @@ nbd_negotiate_handle_info(NBDClient *client, Error **errp)
rc = nbd_negotiate_send_info(client, NBD_INFO_DESCRIPTION,
len, exp->description, errp);
if (rc < 0) {
- return rc;
+ goto out;
}
}
@@ -707,7 +726,7 @@ nbd_negotiate_handle_info(NBDClient *client, Error **errp)
rc = nbd_negotiate_send_info(client, NBD_INFO_BLOCK_SIZE,
sizeof(sizes), sizes, errp);
if (rc < 0) {
- return rc;
+ goto out;
}
/* Send NBD_INFO_EXPORT always */
@@ -725,7 +744,7 @@ nbd_negotiate_handle_info(NBDClient *client, Error **errp)
rc = nbd_negotiate_send_info(client, NBD_INFO_EXPORT,
sizeof(buf), buf, errp);
if (rc < 0) {
- return rc;
+ goto out;
}
/*
@@ -736,17 +755,18 @@ nbd_negotiate_handle_info(NBDClient *client, Error **errp)
*/
if (client->opt == NBD_OPT_INFO && !blocksize &&
blk_get_request_alignment(exp->common.blk) > 1) {
- return nbd_negotiate_send_rep_err(client,
- NBD_REP_ERR_BLOCK_SIZE_REQD,
- errp,
- "request NBD_INFO_BLOCK_SIZE to "
- "use this export");
+ rc = nbd_negotiate_send_rep_err(client,
+ NBD_REP_ERR_BLOCK_SIZE_REQD,
+ errp,
+ "request NBD_INFO_BLOCK_SIZE to "
+ "use this export");
+ goto out;
}
/* Final reply */
rc = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
if (rc < 0) {
- return rc;
+ goto out;
}
if (client->opt == NBD_OPT_GO) {
@@ -756,6 +776,11 @@ nbd_negotiate_handle_info(NBDClient *client, Error **errp)
blk_exp_ref(&client->exp->common);
rc = 1;
}
+ blk_exp_unref(&exp->common);
+ return rc;
+
+out:
+ blk_exp_unref(&exp->common);
return rc;
}
--
2.50.1.windows.1
© 2016 - 2026 Red Hat, Inc.