From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 78C065A0271 for ; Thu, 12 Oct 2023 03:51:23 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1697075480; bh=qkq5aHwBdjV/8p44z9suYd1YuOeVW9TPtQkvxWpedME=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=QHFuRHQbs3q/LZN/3pbyIBathVX9cKJ6xVceb2icQbcvYg+L36ANIgx8cSjkhCuz+ UwWxUhgM4P2C19TDigAJ2NmRhJZmNovEoF0fYAO9f2Azj0i1Gk/ITB7rR9Hdy56jW5 5KCX7S/eX7++18RUBPvjIJqiyuod7kjkRO6dJLAM= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4S5Xfw0Sgzz4xRn; Thu, 12 Oct 2023 12:51:20 +1100 (AEDT) From: David Gibson To: passt-dev@passt.top, Stefano Brivio Subject: [PATCH 02/11] tcp_splice: Correct error handling in tcp_splice_epoll_ctl() Date: Thu, 12 Oct 2023 12:51:05 +1100 Message-ID: <20231012015114.2612066-3-david@gibson.dropbear.id.au> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20231012015114.2612066-1-david@gibson.dropbear.id.au> References: <20231012015114.2612066-1-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: B72NKFJDZF2FVVIXLL7AJ23J6TTJDU37 X-Message-ID-Hash: B72NKFJDZF2FVVIXLL7AJ23J6TTJDU37 X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: David Gibson X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 --- tcp_splice.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index fd6ce8d..22a854e 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; } /** -- 2.41.0