From 536acab2de70468b3d8e6b5f876576525ce82258 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Tue, 7 Nov 2023 13:42:41 +1100 Subject: tcp_splice: Correct error handling in tcp_splice_epoll_ctl() If we get an error from epoll_ctl() in tcp_splice_epoll_ctl() we goto the 'delete' path where we remove both sockets from the epoll set and return an error. There are several problems with this: - We 'return -errno' after the EPOLL_CTL_DEL operations, which means the deleting epoll_ctl() calls may have overwritten the errno values which actually triggered the failures. - The call from conn_flag_do() occurs when the CLOSING flag is set, in which case we go do the delete path regardless of error. In that case the 'return errno' is meaningless since we don't expect the EPOLL_CTL_DEL operations to fail and we ignore the return code anyway. - All other calls to tcp_splice_epoll_ctl() check the return code and if non-zero immediately call conn_flag(..., CLOSING) which will call tcp_splice_epoll_ctl() again explicitly to remove the sockets from epoll. That means removing them when the error first occurs is redundant. - We never specifically report an error on the epoll_ctl() operations. We just set the connection to CLOSING, more or less silently killing it. This could make debugging difficult in the unlikely even that we get a failure here. Re-organise tcp_splice_epoll_ctl() to just log a message then return in the error case, and only EPOLL_CTL_DEL when explicitly asked to with the CLOSING flag. Signed-off-by: David Gibson Signed-off-by: Stefano Brivio --- tcp_splice.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index 87fbd50..6c4c664 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -182,25 +182,27 @@ static int tcp_splice_epoll_ctl(const struct ctx *c, struct epoll_event ev_b = { .data.u64 = ref_b.u64 }; uint32_t events_a, events_b; - if (conn->flags & CLOSING) - goto delete; + if (conn->flags & CLOSING) { + epoll_ctl(c->epollfd, EPOLL_CTL_DEL, conn->a, &ev_a); + epoll_ctl(c->epollfd, EPOLL_CTL_DEL, conn->b, &ev_b); + return 0; + } tcp_splice_conn_epoll_events(conn->events, &events_a, &events_b); ev_a.events = events_a; ev_b.events = events_b; if (epoll_ctl(c->epollfd, m, conn->a, &ev_a) || - epoll_ctl(c->epollfd, m, conn->b, &ev_b)) - goto delete; + epoll_ctl(c->epollfd, m, conn->b, &ev_b)) { + int ret = -errno; + err("TCP (spliced): index %li, ERROR on epoll_ctl(): %s", + CONN_IDX(conn), strerror(errno)); + return ret; + } conn->in_epoll = true; return 0; - -delete: - epoll_ctl(c->epollfd, EPOLL_CTL_DEL, conn->a, &ev_a); - epoll_ctl(c->epollfd, EPOLL_CTL_DEL, conn->b, &ev_b); - return -errno; } /** -- cgit v1.2.3