From 996d4f3cdae229fee9b5c8867d87beeb9172b97f Mon Sep 17 00:00:00 2001 From: John Levon Date: Tue, 6 Apr 2021 15:26:19 +0100 Subject: implement short read/write, EOF handling (#415) Report any short reads to callers as ECONNRESET, which is the closest we can meaningfully get right now. This also fixes get_next_command(), which previously wasn't checking for short reads at all. When we fail to send or recv from the socket due to the client disappearing in some manner, call into vfu_reset_ctx() to clean up the connection fd, allowing a subsequent vfu_attach_ctx() to work. If we get 0 bytes from recv[msg](), this is reported by the transport as ENOMSG, and is a normal EOF condition. We can also get ECONNRESET: this can happen when we've written unacknowledged data to the socket, the client side socket is closed, and we try a subsequent read. Finally, we can get a short read or write. Our handling of these still has issues, but for now we'll presume this means the client has gone too. It may in fact be due to a client bug - if it failed to write enough data - but right now, we can't easily tell that. Signed-off-by: John Levon Reviewed-by: Thanos Makatos --- include/libvfio-user.h | 10 ++++-- lib/libvfio-user.c | 91 ++++++++++++++++++++++++++++++++++++++------------ lib/tran_sock.c | 14 ++++---- 3 files changed, 84 insertions(+), 31 deletions(-) diff --git a/include/libvfio-user.h b/include/libvfio-user.h index 1048236..925550a 100644 --- a/include/libvfio-user.h +++ b/include/libvfio-user.h @@ -137,18 +137,22 @@ int vfu_get_poll_fd(vfu_ctx_t *vfu_ctx); /** - * Polls the vfu_ctx and processes the command recieved from client. + * Polls the vfu_ctx and processes the command received from client. * - Blocking vfu_ctx: * Blocks until new request is received from client and continues processing * the requests. Exits only in case of error or if the client disconnects. * - Non-blocking vfu_ctx(LIBVFIO_USER_FLAG_ATTACH_NB): * Processes one request from client if it's available, otherwise it - * immediatelly returns and the caller is responsible for periodically + * immediately returns and the caller is responsible for periodically * calling again. * * @vfu_ctx: The libvfio-user context to poll * - * @returns 0 on success, -1 on error. Sets errno. + * @returns 0 on success, -1 on error, with errno set as follows: + * + * EAGAIN/EWOULDBLOCK: no more commands to process + * ENOTCONN: client closed connection, vfu_attach_ctx() should be called again + * Other errno values are also possible. */ int vfu_run_ctx(vfu_ctx_t *vfu_ctx); diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c index ebd70af..c041694 100644 --- a/lib/libvfio-user.c +++ b/lib/libvfio-user.c @@ -56,6 +56,7 @@ #include "private.h" #include "tran_sock.h" +static void vfu_reset_ctx(vfu_ctx_t *vfu_ctx, const char *reason); void vfu_log(vfu_ctx_t *vfu_ctx, int level, const char *fmt, ...) @@ -698,26 +699,25 @@ MOCK_DEFINE(get_next_command)(vfu_ctx_t *vfu_ctx, struct vfio_user_header *hdr, /* FIXME get request shouldn't set errno, it should return it as -errno */ ret = vfu_ctx->tran->get_request(vfu_ctx, hdr, fds, nr_fds); if (unlikely(ret < 0)) { - if (ret == -EAGAIN || ret == -EWOULDBLOCK) { + switch (-ret) { + case EAGAIN: return 0; - } - if (ret != -EINTR) { + + case ENOMSG: + vfu_reset_ctx(vfu_ctx, "closed"); + return -ENOTCONN; + + case ECONNRESET: + vfu_reset_ctx(vfu_ctx, "reset"); + return -ENOTCONN; + + default: vfu_log(vfu_ctx, LOG_ERR, "failed to receive request: %s", strerror(-ret)); + return ret; } - return ret; - } - if (unlikely(ret == 0)) { - if (errno == EINTR) { - return -EINTR; - } - if (errno == 0) { - vfu_log(vfu_ctx, LOG_INFO, "vfio-user client closed connection"); - } else { - vfu_log(vfu_ctx, LOG_ERR, "end of file: %m"); - } - return -ENOTCONN; } + return ret; } @@ -784,7 +784,13 @@ MOCK_DEFINE(exec_command)(vfu_ctx_t *vfu_ctx, struct vfio_user_header *hdr, if (cmd_data_size > 0) { ret = vfu_ctx->tran->recv_body(vfu_ctx, hdr, &cmd_data); - if (ret < 0) { + if (ret == -ENOMSG) { + vfu_reset_ctx(vfu_ctx, "closed"); + return -ENOTCONN; + } else if (ret == -ECONNRESET) { + vfu_reset_ctx(vfu_ctx, "reset"); + return -ENOTCONN; + } else if (ret < 0) { return ret; } } @@ -943,6 +949,10 @@ MOCK_DEFINE(process_request)(vfu_ctx_t *vfu_ctx) if (ret < 0) { vfu_log(vfu_ctx, LOG_ERR, "msg%#hx: cmd %d failed: %s", hdr.msg_id, hdr.cmd, strerror(-ret)); + + if (ret == -ENOTCONN) { + goto out; + } } else { ret = 0; } @@ -955,11 +965,21 @@ MOCK_DEFINE(process_request)(vfu_ctx_t *vfu_ctx) } else { ret = vfu_ctx->tran->reply(vfu_ctx, hdr.msg_id, iovecs, nr_iovecs, fds_out, nr_fds_out, -ret); - if (unlikely(ret < 0)) { + + if (ret < 0) { vfu_log(vfu_ctx, LOG_ERR, "failed to reply: %s", strerror(-ret)); + + if (ret == -ECONNRESET) { + vfu_reset_ctx(vfu_ctx, "reset"); + ret = -ENOTCONN; + } else if (ret == -ENOMSG) { + vfu_reset_ctx(vfu_ctx, "closed"); + ret = -ENOTCONN; + } } } +out: if (iovecs != NULL) { if (free_iovec_data) { size_t i; @@ -1087,6 +1107,16 @@ free_sparse_mmap_areas(vfu_ctx_t *vfu_ctx) } } +static void +vfu_reset_ctx(vfu_ctx_t *vfu_ctx, const char *reason) +{ + vfu_log(vfu_ctx, LOG_INFO, "%s: %s", __func__, reason); + + if (vfu_ctx->tran->detach != NULL) { + vfu_ctx->tran->detach(vfu_ctx); + } +} + void vfu_destroy_ctx(vfu_ctx_t *vfu_ctx) { @@ -1095,11 +1125,10 @@ vfu_destroy_ctx(vfu_ctx_t *vfu_ctx) return; } + vfu_reset_ctx(vfu_ctx, "destroyed"); + free(vfu_ctx->uuid); free(vfu_ctx->pci.config_space); - if (vfu_ctx->tran->detach != NULL) { - vfu_ctx->tran->detach(vfu_ctx); - } if (vfu_ctx->tran->fini != NULL) { vfu_ctx->tran->fini(vfu_ctx); @@ -1519,7 +1548,18 @@ vfu_dma_read(vfu_ctx_t *vfu_ctx, dma_sg_t *sg, void *data) ret = vfu_ctx->tran->send_msg(vfu_ctx, msg_id, VFIO_USER_DMA_READ, &dma_send, sizeof(dma_send), NULL, dma_recv, recv_size); - memcpy(data, dma_recv->data, sg->length); /* FIXME no need for memcpy */ + + if (ret == -ENOMSG) { + vfu_reset_ctx(vfu_ctx, "closed"); + ret = -ENOTCONN; + } else if (ret == -ECONNRESET) { + vfu_reset_ctx(vfu_ctx, "reset"); + ret = -ENOTCONN; + } else if (ret == 0) { + /* FIXME no need for memcpy */ + memcpy(data, dma_recv->data, sg->length); + } + free(dma_recv); return ret < 0 ? ERROR_INT(-ret) : 0; @@ -1545,6 +1585,15 @@ vfu_dma_write(vfu_ctx_t *vfu_ctx, dma_sg_t *sg, void *data) ret = vfu_ctx->tran->send_msg(vfu_ctx, msg_id, VFIO_USER_DMA_WRITE, dma_send, send_size, NULL, &dma_recv, sizeof(dma_recv)); + + if (ret == -ENOMSG) { + vfu_reset_ctx(vfu_ctx, "closed"); + ret = -ENOTCONN; + } else if (ret == -ECONNRESET) { + vfu_reset_ctx(vfu_ctx, "reset"); + ret = -ENOTCONN; + } + free(dma_send); return ret < 0 ? ERROR_INT(-ret) : 0; diff --git a/lib/tran_sock.c b/lib/tran_sock.c index 9a58570..ea01a68 100644 --- a/lib/tran_sock.c +++ b/lib/tran_sock.c @@ -173,6 +173,8 @@ get_msg(void *data, size_t len, int *fds, size_t *nr_fds, int sock_fd, return -errno; } else if (ret == 0) { return -ENOMSG; + } else if ((size_t)ret < len) { + return -ECONNRESET; } if (msg.msg_flags & MSG_CTRUNC || msg.msg_flags & MSG_TRUNC) { @@ -222,9 +224,6 @@ tran_sock_recv_fds(int sock, struct vfio_user_header *hdr, bool is_reply, if (ret < 0) { return ret; } - if (ret < (int)sizeof(*hdr)) { - return -EINVAL; - } if (is_reply) { if (msg_id != NULL && hdr->msg_id != *msg_id) { @@ -257,11 +256,12 @@ tran_sock_recv_fds(int sock, struct vfio_user_header *hdr, bool is_reply, return -errno; } else if (ret == 0) { return -ENOMSG; - } else if (*len != (size_t)ret) { /* FIXME we should allow receiving less */ - return -EINVAL; + } else if (*len != (size_t)ret) { + return -ECONNRESET; } *len = ret; } + return 0; } @@ -318,7 +318,7 @@ tran_sock_recv_alloc(int sock, struct vfio_user_header *hdr, bool is_reply, return -ENOMSG; } else if (len != (size_t)ret) { free(data); - return -EINVAL; + return -ECONNRESET; } *datap = data; @@ -818,7 +818,7 @@ tran_sock_recv_body(vfu_ctx_t *vfu_ctx, const struct vfio_user_header *hdr, vfu_log(vfu_ctx, LOG_ERR, "msg%#hx: short read: expected=%d, actual=%d", hdr->msg_id, body_size, ret); free(data); - return -EINVAL; + return -ECONNRESET; } *datap = data; -- cgit v1.1