Add a workaround for race condition in the Linux kernel NBD driver

Do all NBD configuration in the child process, after the last fork.
Why? It's needed because there is a race condition in the Linux kernel nbd driver
in nbd_add_socket() - it saves `current` task pointer as `nbd->task_setup` and
then rechecks if the new `current` is the same. Problem is that if that process
is already dead, `current` may be freed and then replaced by another process
with the same pointer value. So the check passes and NBD allows a different process
to set up a device which is already set up. Proper fix would have to be done in the
kernel code, but the workaround is obviously to perform NBD setup from the process
which will then actually call NBD_DO_IT. That process stays alive during the whole
time of NBD device execution and the (nbd->task_setup != current) check always
works correctly, and we don't accidentally break previous NBD devices while setting
up a new device. Forking to check every device is of course rather slow, so we also
do an additional check by calling list_mapped() before searching for a free NBD device.
This commit is contained in:
Vitaliy Filippov
2025-02-21 13:17:37 +03:00
parent 5ee4894fab
commit 97ee400505
+70 -21
View File
@@ -617,36 +617,43 @@ help:
{ {
if (!cfg["dev_num"].is_null()) if (!cfg["dev_num"].is_null())
{ {
if (run_nbd(sockfd, cfg["dev_num"].int64_value(), device_size, NBD_FLAG_SEND_FLUSH, nbd_timeout, bg) < 0) int r;
if ((r = run_nbd(sockfd, cfg["dev_num"].int64_value(), device_size, NBD_FLAG_SEND_FLUSH, nbd_timeout, bg)) != 0)
{ {
perror("run_nbd"); fprintf(stderr, "run_nbd: %s\n", strerror(-r));
exit(1); exit(1);
} }
} }
else else
{ {
// Find an unused device // Find an unused device
auto mapped = list_mapped();
int i = 0; int i = 0;
while (true) while (true)
{ {
if (mapped.find("/dev/nbd"+std::to_string(i)) != mapped.end())
{
i++;
continue;
}
int r = run_nbd(sockfd, i, device_size, NBD_FLAG_SEND_FLUSH, nbd_timeout, bg); int r = run_nbd(sockfd, i, device_size, NBD_FLAG_SEND_FLUSH, nbd_timeout, bg);
if (r == 0) if (r == 0)
{ {
printf("/dev/nbd%d\n", i); printf("/dev/nbd%d\n", i);
break; break;
} }
else if (r == -1 && errno == ENOENT) else if (r == -ENOENT)
{ {
fprintf(stderr, "No free NBD devices found\n"); fprintf(stderr, "No free NBD devices found\n");
exit(1); exit(1);
} }
else if (r == -2 && errno == EBUSY) else if (r == -EBUSY)
{ {
i++; i++;
} }
else else
{ {
perror("run_nbd"); fprintf(stderr, "run_nbd: %s\n", strerror(-r));
exit(1); exit(1);
} }
} }
@@ -702,6 +709,13 @@ help:
ringloop->loop(); ringloop->loop();
ringloop->wait(); ringloop->wait();
} }
destroy_client();
}
void destroy_client()
{
if (cli)
{
cli->flush(); cli->flush();
delete cli; delete cli;
delete epmgr; delete epmgr;
@@ -710,6 +724,7 @@ help:
epmgr = NULL; epmgr = NULL;
ringloop = NULL; ringloop = NULL;
} }
}
void load_module() void load_module()
{ {
@@ -876,11 +891,38 @@ protected:
// Check handle size // Check handle size
assert(sizeof(cur_req.handle) == 8); assert(sizeof(cur_req.handle) == 8);
char path[64] = { 0 }; char path[64] = { 0 };
int notifyfd[2] = { 0 };
if (socketpair(AF_UNIX, SOCK_STREAM, 0, notifyfd) < 0)
{
return -errno;
}
if (!fork())
{
// Do all NBD configuration in the child process, after the last fork.
// Why? It's needed because there is a race condition in the Linux kernel nbd driver
// in nbd_add_socket() - it saves `current` task pointer as `nbd->task_setup` and
// then rechecks if the new `current` is the same. Problem is that if that process
// is already dead, `current` may be freed and then replaced by another process
// with the same pointer value. So the check passes and NBD allows a different process
// to set up a device which is already set up. Proper fix would have to be done in the
// kernel code, but the workaround is obviously to perform NBD setup from the process
// which will then actually call NBD_DO_IT. That process stays alive during the whole
// time of NBD device execution and the (nbd->task_setup != current) check always
// works correctly, and we don't accidentally break previous NBD devices while setting
// up a new device. Forking to check every device is of course rather slow, so we also
// do an additional check by calling list_mapped() before searching for a free NBD device.
destroy_client();
if (bg)
{
daemonize_fork();
}
close(notifyfd[0]);
sprintf(path, "/dev/nbd%d", dev_num); sprintf(path, "/dev/nbd%d", dev_num);
int r, nbd = open(path, O_RDWR), qd_fd; int r, nbd = open(path, O_RDWR), qd_fd;
if (nbd < 0) if (nbd < 0)
{ {
return -1; write(notifyfd[1], &errno, sizeof(errno));
exit(1);
} }
r = ioctl(nbd, NBD_SET_SOCK, sockfd[1]); r = ioctl(nbd, NBD_SET_SOCK, sockfd[1]);
if (r < 0) if (r < 0)
@@ -919,38 +961,45 @@ protected:
fprintf(stderr, "Warning: Failed to configure max_sectors_kb\n"); fprintf(stderr, "Warning: Failed to configure max_sectors_kb\n");
} }
close(qd_fd); close(qd_fd);
if (!fork()) // Notify parent
{ errno = 0;
// Run in child write(notifyfd[1], &errno, sizeof(errno));
close(notifyfd[1]);
close(sockfd[0]); close(sockfd[0]);
if (bg) if (bg)
{ {
daemonize(); daemonize_reopen_stdio();
} }
r = ioctl(nbd, NBD_DO_IT); r = ioctl(nbd, NBD_DO_IT);
if (r < 0) if (r < 0)
{ {
fprintf(stderr, "NBD device terminated with error: %s\n", strerror(errno)); fprintf(stderr, "NBD device /dev/nbd%d terminated with error: %s\n", dev_num, strerror(errno));
} }
close(sockfd[1]); close(sockfd[1]);
ioctl(nbd, NBD_CLEAR_QUE); ioctl(nbd, NBD_CLEAR_QUE);
ioctl(nbd, NBD_CLEAR_SOCK); ioctl(nbd, NBD_CLEAR_SOCK);
exit(0); exit(0);
}
close(sockfd[1]);
close(nbd);
return 0;
end_close: end_close:
r = errno; write(notifyfd[1], &errno, sizeof(errno));
close(nbd); close(nbd);
errno = r; exit(2);
return -2;
end_unmap: end_unmap:
r = errno; write(notifyfd[1], &errno, sizeof(errno));
ioctl(nbd, NBD_CLEAR_SOCK); ioctl(nbd, NBD_CLEAR_SOCK);
close(nbd); close(nbd);
errno = r; exit(3);
return -3; }
// Parent - check status
close(notifyfd[1]);
int child_errno = 0;
int ok = read(notifyfd[0], &child_errno, sizeof(child_errno));
close(notifyfd[0]);
if (ok && !child_errno)
{
close(sockfd[1]);
return 0;
}
return -child_errno;
} }
void submit_send() void submit_send()