From e8c37f83fb92848f328e297ace37536253ee08bc Mon Sep 17 00:00:00 2001 From: Mattias Nissler <122288598+mnissler-rivos@users.noreply.github.com> Date: Tue, 15 Aug 2023 13:38:30 +0200 Subject: [PATCH] Introduce close_safely helper function (#763) The helper function centralizes some extra checks and diligence desired by many/most current code paths but currently inconsistently applied. This includes bypassing the close call when the file descriptor is -1 already, resetting the file descriptor variable to -1 after closing, and preserving errno. All calls to close are replaced by close_safely. Some warning log output is lost over this, but it doesn't seem like this was very useful anyways given that Linux always closes the file descriptor anyways. Signed-off-by: Mattias Nissler --- lib/common.h | 28 ++++++++++++++++++++++++++++ lib/dma.c | 10 ++-------- lib/irq.c | 25 +++---------------------- lib/libvfio-user.c | 23 ++++++++--------------- lib/tran.c | 4 +--- lib/tran_sock.c | 22 +++++++--------------- 6 files changed, 49 insertions(+), 63 deletions(-) diff --git a/lib/common.h b/lib/common.h index 0c3abdaf..07a74a5c 100644 --- a/lib/common.h +++ b/lib/common.h @@ -37,8 +37,10 @@ #ifndef LIB_VFIO_USER_COMMON_H #define LIB_VFIO_USER_COMMON_H +#include #include #include +#include #define UNUSED __attribute__((unused)) #define EXPORT __attribute__((visibility("default"))) @@ -79,6 +81,32 @@ _get_bitmap_size(size_t size, size_t pgsize) return ROUND_UP(nr_pages, sizeof(uint64_t) * CHAR_BIT) / CHAR_BIT; } +/* + * Closes the given file descriptor, and resets the value to -1. Preserves + * errno. Skips closing if *fd is -1. + */ +static inline void +close_safely(int *fd) +{ + int saved_errno = errno; + if (fd != NULL && *fd != -1) { + /* + * POSIX says that close may hit EINTR and leave the file descriptor in + * undefined state. But retrying on EINTR is incorrect, since a + * different thread might have re-opened a file on the same descriptor + * if close actually did free the descriptor. In practice, Linux always + * closes the file descriptor and POSIX has decided to align semantics + * with Linux. Thus, calling close once and ignoring the error is the + * most appropriate course of action. + * + * See also https://www.austingroupbugs.net/view.php?id=529 + */ + (void) close(*fd); + *fd = -1; + } + errno = saved_errno; +} + #ifdef UNIT_TEST #define MOCK_DEFINE(f) \ diff --git a/lib/dma.c b/lib/dma.c index beefeac1..af1495fa 100644 --- a/lib/dma.c +++ b/lib/dma.c @@ -121,10 +121,7 @@ MOCK_DEFINE(dma_controller_unmap_region)(dma_controller_t *dma, assert(region->fd != -1); - if (close(region->fd) == -1) { - vfu_log(dma->vfu_ctx, LOG_WARNING, "failed to close fd %d: %m", - region->fd); - } + close_safely(®ion->fd); } static void @@ -402,10 +399,7 @@ MOCK_DEFINE(dma_controller_add_region)(dma_controller_t *dma, vfu_log(dma->vfu_ctx, LOG_ERR, "failed to memory map DMA region %s: %m", rstr); - if (close(region->fd) == -1) { - vfu_log(dma->vfu_ctx, LOG_WARNING, - "failed to close fd %d: %m", region->fd); - } + close_safely(®ion->fd); free(region->dirty_bitmap); return ERROR_INT(ret); } diff --git a/lib/irq.c b/lib/irq.c index 8a5d200c..183d0718 100644 --- a/lib/irq.c +++ b/lib/irq.c @@ -122,14 +122,7 @@ irqs_disable(vfu_ctx_t *vfu_ctx, uint32_t index, uint32_t start, uint32_t count) } for (i = start; i < count; i++) { - if (efds[i] >= 0) { - if (close(efds[i]) == -1) { - vfu_log(vfu_ctx, LOG_DEBUG, "failed to close IRQ fd %d: %m", - efds[i]); - } - - efds[i] = -1; - } + close_safely(&efds[i]); } } @@ -143,14 +136,7 @@ irqs_reset(vfu_ctx_t *vfu_ctx) irqs_disable(vfu_ctx, VFIO_PCI_ERR_IRQ_INDEX, 0, 0); for (i = 0; i < vfu_ctx->irqs->max_ivs; i++) { - if (efds[i] >= 0) { - if (close(efds[i]) == -1) { - vfu_log(vfu_ctx, LOG_DEBUG, "failed to close IRQ fd %d: %m", - efds[i]); - } - - efds[i] = -1; - } + close_safely(&efds[i]); } } @@ -257,12 +243,7 @@ irqs_set_data_eventfd(vfu_ctx_t *vfu_ctx, struct vfio_irq_set *irq_set, for (i = irq_set->start, j = 0; i < (irq_set->start + irq_set->count); i++, j++) { efd = irqs_get_efd(vfu_ctx, irq_set->index, i); - if (*efd >= 0) { - if (close(*efd) == -1) { - vfu_log(vfu_ctx, LOG_DEBUG, "failed to close IRQ fd %d: %m", *efd); - } - *efd = -1; - } + close_safely(efd); assert(data[j] >= 0); /* * We've already checked in handle_device_set_irqs that diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c index 663d2cd0..48013c28 100644 --- a/lib/libvfio-user.c +++ b/lib/libvfio-user.c @@ -755,12 +755,9 @@ handle_dma_map(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg, dma_map->size, fd, dma_map->offset, prot); if (ret < 0) { - ret = errno; vfu_log(vfu_ctx, LOG_ERR, "failed to add DMA region %s: %m", rstr); - if (fd != -1) { - close(fd); - } - return ERROR_INT(ret); + close_safely(&fd); + return -1; } if (vfu_ctx->dma_register != NULL) { @@ -1096,14 +1093,12 @@ free_msg(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg) free(msg->in.iov.iov_base); for (i = 0; i < msg->in.nr_fds; i++) { - if (msg->in.fds[i] != -1) { - if (msg->processed_cmd) { - vfu_log(vfu_ctx, LOG_DEBUG, - "closing unexpected fd %d (index %zu) from cmd %u", - msg->in.fds[i], i, msg->hdr.cmd); - } - close(msg->in.fds[i]); + if (msg->in.fds[i] != -1 && msg->processed_cmd) { + vfu_log(vfu_ctx, LOG_DEBUG, + "closing unexpected fd %d (index %zu) from cmd %u", + msg->in.fds[i], i, msg->hdr.cmd); } + close_safely(&msg->in.fds[i]); } free(msg->in.fds); @@ -1276,11 +1271,9 @@ get_request_header(vfu_ctx_t *vfu_ctx, vfu_msg_t **msgp) *msgp = alloc_msg(&hdr, fds, nr_fds); if (*msgp == NULL) { - int saved_errno = errno; for (i = 0; i < nr_fds; i++) { - close(fds[i]); + close_safely(&fds[i]); } - errno = saved_errno; return -1; } diff --git a/lib/tran.c b/lib/tran.c index a183877f..dff47eff 100644 --- a/lib/tran.c +++ b/lib/tran.c @@ -252,9 +252,7 @@ recv_version(vfu_ctx_t *vfu_ctx, uint16_t *msg_idp, (void) vfu_ctx->tran->reply(vfu_ctx, &rmsg, ret); for (i = 0; i < msg.in.nr_fds; i++) { - if (msg.in.fds[i] != -1) { - close(msg.in.fds[i]); - } + close_safely(&msg.in.fds[i]); } free(msg.in.iov.iov_base); diff --git a/lib/tran_sock.c b/lib/tran_sock.c index 7c4e30b4..fb0ccbb6 100644 --- a/lib/tran_sock.c +++ b/lib/tran_sock.c @@ -419,8 +419,8 @@ tran_sock_init(vfu_ctx_t *vfu_ctx) out: if (ret != 0) { - if (ts != NULL && ts->listen_fd != -1) { - close(ts->listen_fd); + if (ts != NULL) { + close_safely(&ts->listen_fd); } free(ts); return ERROR_INT(ret); @@ -466,10 +466,8 @@ tran_sock_attach(vfu_ctx_t *vfu_ctx) ret = tran_negotiate(vfu_ctx); if (ret < 0) { - ret = errno; - close(ts->conn_fd); - ts->conn_fd = -1; - return ERROR_INT(ret); + close_safely(&ts->conn_fd); + return -1; } return 0; @@ -636,10 +634,8 @@ tran_sock_detach(vfu_ctx_t *vfu_ctx) ts = vfu_ctx->tran_data; - if (ts != NULL && ts->conn_fd != -1) { - // FIXME: handle EINTR - (void) close(ts->conn_fd); - ts->conn_fd = -1; + if (ts != NULL) { + close_safely(&ts->conn_fd); } } @@ -654,11 +650,7 @@ tran_sock_fini(vfu_ctx_t *vfu_ctx) if (ts != NULL) { (void) unlink(vfu_ctx->uuid); - if (ts->listen_fd != -1) { - // FIXME: handle EINTR - (void) close(ts->listen_fd); - ts->listen_fd = -1; - } + close_safely(&ts->listen_fd); } free(vfu_ctx->tran_data);