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 <[email protected]> --- 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
