[PATCH 0/6] Fix race condition while closing spliced connections
Fix bug 202, where a race condition could cause connections to be incorrectly reset in certain circumstances. Patch 2/6 is the bug fix proper. 1/6 improves error reporting and debugging messages in the vicinity. Patches 3..6/6 are some cleanups I noticed in the area while working on the fix. Link: https://bugs.passt.top/show_bug.cgi?id=202 David Gibson (6): tcp_splice: Improve error reporting tcp_splice: Avoid missing EOF recognition while forwarding tcp_splice: Clean up flow control path for splice forwarding tcp_splice: Simplify tracking of read/written bytes tcp_splice: Simplify EPOLLRDHUP / eof / FIN handling tcp_splice: Simplify shutdown(2) handling tcp_conn.h | 6 +- tcp_splice.c | 180 +++++++++++++++++++++++++++------------------------ 2 files changed, 97 insertions(+), 89 deletions(-) -- 2.54.0
For each each direction of each spliced connection, we keep track of how
many bytes we've read from one socket and written to the other. However,
we never actually care about the absolute values of these, only the
difference between them, which represents how much data is currently "in
flight" in the splicing pipe.
Simplify the handling by having a single variable tracking the number of
bytes in the pipe.
As a bonus, the new scheme makes it clearer that we don't need to worry
about overflows: pending can never become larger than the maximum pipe
bufffer size, well within 32-bits.
I _think_ the old scheme was safe in the case of overflow - again under
the assumption that read/written can never be further apart than the pipe
buffer size. However, it's much harder to reason about this case. It's
certainly plausible that an overflow could occur - sending 4GiB through
a local socket is entirely achievable.
Signed-off-by: David Gibson
tcp_splice_sock_handler() has an optimised path for the common case where
the amount we splice(2) into the pipe is exactly the same as the amount we
splice(2) out again. If the pipe is empty at that point, we stop
forwarding until we get another epoll event.
However, via a subtle chain of events, this can cause a bug for a
half-closed connection. Suppose the connection is already half-closed in
the other direction - that is, we've already called shutdown(SHUT_WR) on
the socket for which we're getting the event. In this event we're getting
the last batch of data in the other direction, and also a FIN. This can
result in EPOLLIN, EPOLLRDHUP and EPOLLHUP events simultaneously.
We read the last data from the socket and successfully splice it to the
other side. Since there is no data in the pipe, we exit the forwarding
loop. However, because we did read data, we don't set the eof flag.
Because we don't set eof, we don't (yet) propagate the FIN to the other
side, or set FIN_SENT_(!fromsidei). Therefore we don't (yet) recognize
this as a clean termination and set the CLOSING flag. We would correct
this when we get our next event, however before we can do so we process
the EPOLLHUP event. Because we haven't recognized this as a clean close
we assume it is an abrupt close and send an RST to the other side.
To avoid this, don't stop attempting to forward data on this path.
Continue for at least one more loop. If we're at EOF, we'll recognize it
on the next splice(2). If not it gives us an opportunity to forward more
data without returning to the mail epoll loop.
Link: https://bugs.passt.top/show_bug.cgi?id=202
Signed-off-by: David Gibson
There are two ways we can tell one of our sockets has received a FIN. We
can either see an EPOLLRDHUP epoll event, or we can get a zero-length read
(EOF) on the socket. We currently use both, in a mildly confusing way:
we only set the FIN_RCVD() flag based on the EPOLLRDHUP event, but then
some other close out logic is based on seeing an EOF.
Simplify this by setting the flag based on only the EOF. To make sure we
don't miss an event if we get an EPOLLRDHUP with no data, we trigger the
forwarding path for EPOLLRDHUP as well as EPOLLIN.
Signed-off-by: David Gibson
At the end of tcp_splice_forward(), we check for half-closed connections
and propagate the FIN to the other side with a shutdown(2). Currently we
check for a half closed connection in either direction. That's unnecessary
here, because tcp_splice_forward() will already be called for each
direction if there are any relevant events.
Signed-off-by: David Gibson
On Wed, 20 May 2026 23:08:47 +1000
David Gibson
tcp_splice_sock_handler() has an optimised path for the common case where the amount we splice(2) into the pipe is exactly the same as the amount we splice(2) out again. If the pipe is empty at that point, we stop forwarding until we get another epoll event.
However, via a subtle chain of events, this can cause a bug for a half-closed connection. Suppose the connection is already half-closed in the other direction - that is, we've already called shutdown(SHUT_WR) on the socket for which we're getting the event. In this event we're getting the last batch of data in the other direction, and also a FIN. This can result in EPOLLIN, EPOLLRDHUP and EPOLLHUP events simultaneously.
We read the last data from the socket and successfully splice it to the other side. Since there is no data in the pipe, we exit the forwarding loop. However, because we did read data, we don't set the eof flag.
Because we don't set eof, we don't (yet) propagate the FIN to the other side, or set FIN_SENT_(!fromsidei). Therefore we don't (yet) recognize this as a clean termination and set the CLOSING flag. We would correct this when we get our next event, however before we can do so we process the EPOLLHUP event. Because we haven't recognized this as a clean close we assume it is an abrupt close and send an RST to the other side.
To avoid this, don't stop attempting to forward data on this path. Continue for at least one more loop. If we're at EOF, we'll recognize it on the next splice(2). If not it gives us an opportunity to forward more data without returning to the mail epoll loop.
Oops. The fix looks correct to me, but I wonder: is it clear to you why the issue only started occurring in this release? This code had "always" been there. I see a few possible directions but I'm not quite sure. Not that important anyway, if you could reproduce the issue and this fixes it. Just one nit:
Link: https://bugs.passt.top/show_bug.cgi?id=202 Signed-off-by: David Gibson
Reported-by: Paul Holzinger
--- tcp_splice.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tcp_splice.c b/tcp_splice.c index 1359d6b8..34ffea73 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -605,7 +605,7 @@ retry: } }
- break; + continue; }
conn->read[fromsidei] += readlen > 0 ? readlen : 0;
-- Stefano
On Wed, 20 May 2026 23:08:49 +1000
David Gibson
For each each direction of each spliced connection, we keep track of how many bytes we've read from one socket and written to the other. However, we never actually care about the absolute values of these, only the difference between them, which represents how much data is currently "in flight" in the splicing pipe.
Simplify the handling by having a single variable tracking the number of bytes in the pipe.
For me it actually looks slightly more complicated to think about it this way, I added explicit 'read' and 'written' after being bitten by some issue I introduced with a previous 'pending' concept, but I have to admit it slightly simplifies the overflow topic.
As a bonus, the new scheme makes it clearer that we don't need to worry about overflows: pending can never become larger than the maximum pipe bufffer size, well within 32-bits.
I _think_ the old scheme was safe in the case of overflow - again under the assumption that read/written can never be further apart than the pipe buffer size. However, it's much harder to reason about this case. It's certainly plausible that an overflow could occur - sending 4GiB through a local socket is entirely achievable.
For me it looked pretty simple: you can overflow 32 bits (at 100 Gbps, but without hitting the "optimised" case, it would take about five minutes), but all the operations between the two counters are between two uint32_t, so they would happen in uint32_t, hence modulo 32 bits, similar to TCP sequences. Anyway, overall, I think it's an improvement over the original. One nit here:
Signed-off-by: David Gibson
--- tcp_conn.h | 6 ++---- tcp_splice.c | 18 +++++++++--------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/tcp_conn.h b/tcp_conn.h index 9f5bee03..c8381aa7 100644 --- a/tcp_conn.h +++ b/tcp_conn.h @@ -206,8 +206,7 @@ struct tcp_tap_transfer_ext { * @f: Generic flow information * @s: File descriptor for sockets * @pipe: File descriptors for pipes - * @read: Bytes read (not fully written to other side in one shot) - * @written: Bytes written (not fully written from one other side read) + * @pending: Bytes currently in each pipe * @events: Events observed/actions performed on connection * @flags: Connection flags (attributes, not events) */ @@ -218,8 +217,7 @@ struct tcp_splice_conn { int s[SIDES]; int pipe[SIDES][2];
- uint32_t read[SIDES]; - uint32_t written[SIDES]; + uint32_t pending[SIDES];
uint8_t events; #define SPLICE_CLOSED 0 diff --git a/tcp_splice.c b/tcp_splice.c index 18e8b303..8fbd490f 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -292,7 +292,7 @@ bool tcp_splice_flow_defer(struct tcp_splice_conn *conn) conn->s[sidei] = -1; }
- conn->read[sidei] = conn->written[sidei] = 0; + conn->pending[sidei] = 0; }
conn->events = SPLICE_CLOSED; @@ -490,7 +490,7 @@ static int tcp_splice_forward(struct ctx *c, struct int eof = 0;
while (1) { - ssize_t readlen, written, pending; + ssize_t readlen, written; int more = 0;
retry: @@ -537,7 +537,7 @@ retry: flow_trace(conn, "%zi from write-side call (passed %zi)", written, c->tcp.pipe_size);
- /* Most common case: skip updating counters. */ + /* Most common case: skip updating pending. */
"pending" isn't a noun (even though the variable name is, but it's not quite obvious that you're referring to it). I think that: /* Most common case: skip updating count of pending bytes */ would be slightly clearer (and also omit the '.' because it's not a complete sentence, as we usually do on single-line comments, similarly to most occurrences in the kernel).
if (readlen > 0 && readlen == written) { if (readlen >= (long)c->tcp.pipe_size * 10 / 100) continue; @@ -561,11 +561,11 @@ retry: continue; }
- conn->read[fromsidei] += readlen > 0 ? readlen : 0; - conn->written[fromsidei] += written > 0 ? written : 0; + conn->pending[fromsidei] += readlen > 0 ? readlen : 0; + conn->pending[fromsidei] -= written > 0 ? written : 0;
if (written < 0) { - if (conn->read[fromsidei] == conn->written[fromsidei]) + if (!conn->pending[fromsidei]) break;
conn_event(conn, OUT_WAIT(!fromsidei)); @@ -575,15 +575,15 @@ retry: if (never_read && written == (long)(c->tcp.pipe_size)) goto retry;
- pending = conn->read[fromsidei] - conn->written[fromsidei]; - if (!never_read && written > 0 && written < pending) + if (!never_read && written > 0 && + written < conn->pending[fromsidei]) goto retry;
if (eof) break; }
- if (conn->read[fromsidei] == conn->written[fromsidei] && eof) { + if (!conn->pending[fromsidei] && eof) { unsigned sidei;
flow_foreach_sidei(sidei) {
-- Stefano
On Wed, 20 May 2026 23:08:50 +1000
David Gibson
There are two ways we can tell one of our sockets has received a FIN. We can either see an EPOLLRDHUP epoll event, or we can get a zero-length read (EOF) on the socket. We currently use both, in a mildly confusing way: we only set the FIN_RCVD() flag based on the EPOLLRDHUP event, but then some other close out logic is based on seeing an EOF.
Simplify this by setting the flag based on only the EOF. To make sure we don't miss an event if we get an EPOLLRDHUP with no data, we trigger the forwarding path for EPOLLRDHUP as well as EPOLLIN.
Signed-off-by: David Gibson
--- tcp_splice.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index 8fbd490f..b45f0060 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -487,7 +487,6 @@ static int tcp_splice_forward(struct ctx *c, struct uint8_t lowat_set_flag = RCVLOWAT_SET(fromsidei); uint8_t lowat_act_flag = RCVLOWAT_ACT(fromsidei); int never_read = 1; - int eof = 0;
while (1) { ssize_t readlen, written; @@ -510,7 +509,7 @@ retry: flow_trace(conn, "%zi from read-side call", readlen);
if (!readlen) { - eof = 1; + conn_event(conn, FIN_RCVD(fromsidei));
I'm not sure if I really found a concrete issue with this, but it looks a bit scary, because it changes the semantics of FIN_RCVD, which used to mean that we infer we received a FIN, regardless of whether we're done processing all data from that half of the connection. Now FIN_RCVD is only set if we actually processed all the data and we hit the end of file. The (potential) issue I see here is that we get EPOLLRDHUP, splice() returns -1 with EAGAIN in errno because we had no room in the pipe, and it would have returned 0 instead. Will we ever get our zero-sized "read" later? If not, we might have missed EPOLLRDHUP *and* the end of file. I'm not entirely sure we have guarantees in that sense from splice(). The existing implementation distinguishes between end-of-file we hit in a given iteration, and EPOLLRDHUP we might have seen at any time. That was actually intended. -- Stefano
On Wed, 20 May 2026 23:08:51 +1000
David Gibson
At the end of tcp_splice_forward(), we check for half-closed connections and propagate the FIN to the other side with a shutdown(2). Currently we check for a half closed connection in either direction. That's unnecessary here, because tcp_splice_forward() will already be called for each direction if there are any relevant events.
True, but do we have the guarantee that tcp_splice_forward() will also be called once all relevant FIN_RCVD / FIN_SENT flags have been sent? The reason why we check both sides here is that we might have updated flags for one side, and now we need to double check if it's time to call shutdown() as a consequence. Maybe we never have to, but I think it's not really obvious to prove.
Signed-off-by: David Gibson
--- tcp_splice.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index b45f0060..e5018f2e 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -582,21 +582,15 @@ retry: break; }
- if (!conn->pending[fromsidei] && - conn->events & FIN_RCVD(fromsidei)) { - unsigned sidei; - - flow_foreach_sidei(sidei) { - if ((conn->events & FIN_RCVD(sidei)) && - !(conn->events & FIN_SENT(!sidei))) { - if (shutdown(conn->s[!sidei], SHUT_WR) < 0) { - flow_perror(conn, "shutdown() on %s", - pif_name(conn->f.pif[!sidei])); - return -1; - } - conn_event(conn, FIN_SENT(!sidei)); - } + if ((conn->events & FIN_RCVD(fromsidei)) && + !(conn->events & FIN_SENT(!fromsidei)) && + !conn->pending[fromsidei]) { + if (shutdown(conn->s[!fromsidei], SHUT_WR) < 0) { + flow_perror(conn, "shutdown() on %s", + pif_name(conn->f.pif[!fromsidei])); + return -1; } + conn_event(conn, FIN_SENT(!fromsidei)); }
return 0;
-- Stefano
On Wed, May 20, 2026 at 10:30:04PM +0200, Stefano Brivio wrote:
On Wed, 20 May 2026 23:08:50 +1000 David Gibson
wrote: There are two ways we can tell one of our sockets has received a FIN. We can either see an EPOLLRDHUP epoll event, or we can get a zero-length read (EOF) on the socket. We currently use both, in a mildly confusing way: we only set the FIN_RCVD() flag based on the EPOLLRDHUP event, but then some other close out logic is based on seeing an EOF.
Simplify this by setting the flag based on only the EOF. To make sure we don't miss an event if we get an EPOLLRDHUP with no data, we trigger the forwarding path for EPOLLRDHUP as well as EPOLLIN.
Signed-off-by: David Gibson
--- tcp_splice.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index 8fbd490f..b45f0060 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -487,7 +487,6 @@ static int tcp_splice_forward(struct ctx *c, struct uint8_t lowat_set_flag = RCVLOWAT_SET(fromsidei); uint8_t lowat_act_flag = RCVLOWAT_ACT(fromsidei); int never_read = 1; - int eof = 0;
while (1) { ssize_t readlen, written; @@ -510,7 +509,7 @@ retry: flow_trace(conn, "%zi from read-side call", readlen);
if (!readlen) { - eof = 1; + conn_event(conn, FIN_RCVD(fromsidei));
I'm not sure if I really found a concrete issue with this, but it looks a bit scary, because it changes the semantics of FIN_RCVD, which used to mean that we infer we received a FIN, regardless of whether we're done processing all data from that half of the connection.
Now FIN_RCVD is only set if we actually processed all the data and we hit the end of file.
True. But the only place that tested FIN_RCVD was at the end of tcp_splice_forward(), conditional on 'eof' anyway. In a sense, this was the cause of bug202 - we had FIN_RCVD set, but we didn't process it and shutdown() on the other side, because we didn't have eof.
The (potential) issue I see here is that we get EPOLLRDHUP, splice() returns -1 with EAGAIN in errno because we had no room in the pipe, and it would have returned 0 instead.
Will we ever get our zero-sized "read" later? If not, we might have missed EPOLLRDHUP *and* the end of file. I'm not entirely sure we have guarantees in that sense from splice().
It's not really about guarantees from splice. I'm pretty sure this is ok, reasoning as follows. Consider all the exit points from the loop body: - Each return is a return -1, so we kill the connection anyway. They don't matter - Each continue, goto retry and the end of the body will do the read side splice() again, so get another chance to see the EOF - That leaves just the breaks Consider each break (there are three, since patch 2 of this series) if (written < 0) { if (!conn->pending[fromsidei]) break; (1) The pipe is empty and the write-splice returned EAGAIN, so it didn't remove data from the pipe. Therefore, the pipe must have been empty before the write-splice. Which means the read-splice can't have blocked on a full pipe. conn_event(conn, OUT_WAIT(!fromsidei)); break; } (2) The pipe is non-empty and the write-splice returned EAGAIN, so it must have blocked on the output socket. We've set OUT_WAIT(), so we'll get an EPOLLOUT at some point which will cause us to read-splice again, meaning we get another chance to see the EOF. [...] if (conn->events & FIN_RCVD(fromsidei)) break; (3) By the new semantics of FIN_RCVD, we *have* seen the EOF.
The existing implementation distinguishes between end-of-file we hit in a given iteration, and EPOLLRDHUP we might have seen at any time. That was actually intended.
It might be intended, but I can't see that we did anything with that information. That said the conditions on which we exit / retry this loop are pretty darn confusing. I'll see if I can improve them. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
On Wed, May 20, 2026 at 10:28:36PM +0200, Stefano Brivio wrote:
On Wed, 20 May 2026 23:08:47 +1000 David Gibson
wrote: tcp_splice_sock_handler() has an optimised path for the common case where the amount we splice(2) into the pipe is exactly the same as the amount we splice(2) out again. If the pipe is empty at that point, we stop forwarding until we get another epoll event.
However, via a subtle chain of events, this can cause a bug for a half-closed connection. Suppose the connection is already half-closed in the other direction - that is, we've already called shutdown(SHUT_WR) on the socket for which we're getting the event. In this event we're getting the last batch of data in the other direction, and also a FIN. This can result in EPOLLIN, EPOLLRDHUP and EPOLLHUP events simultaneously.
We read the last data from the socket and successfully splice it to the other side. Since there is no data in the pipe, we exit the forwarding loop. However, because we did read data, we don't set the eof flag.
Because we don't set eof, we don't (yet) propagate the FIN to the other side, or set FIN_SENT_(!fromsidei). Therefore we don't (yet) recognize this as a clean termination and set the CLOSING flag. We would correct this when we get our next event, however before we can do so we process the EPOLLHUP event. Because we haven't recognized this as a clean close we assume it is an abrupt close and send an RST to the other side.
To avoid this, don't stop attempting to forward data on this path. Continue for at least one more loop. If we're at EOF, we'll recognize it on the next splice(2). If not it gives us an opportunity to forward more data without returning to the mail epoll loop.
Oops. The fix looks correct to me, but I wonder: is it clear to you why the issue only started occurring in this release? This code had "always" been there.
Because we didn't used to force resets on abnormal connection terminations, so it still worked by accident.
I see a few possible directions but I'm not quite sure. Not that important anyway, if you could reproduce the issue and this fixes it.
Ah, actually, I do still need to test with the original reproducer. It fixes it for my reproducer which I'm maybe 90% confident is exercising the same bug.
Just one nit:
Link: https://bugs.passt.top/show_bug.cgi?id=202 Signed-off-by: David Gibson
Reported-by: Paul Holzinger
Good point, fixed.
--- tcp_splice.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tcp_splice.c b/tcp_splice.c index 1359d6b8..34ffea73 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -605,7 +605,7 @@ retry: } }
- break; + continue; }
conn->read[fromsidei] += readlen > 0 ? readlen : 0;
-- Stefano
-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
On Wed, May 20, 2026 at 10:29:12PM +0200, Stefano Brivio wrote:
On Wed, 20 May 2026 23:08:49 +1000 David Gibson
wrote: For each each direction of each spliced connection, we keep track of how many bytes we've read from one socket and written to the other. However, we never actually care about the absolute values of these, only the difference between them, which represents how much data is currently "in flight" in the splicing pipe.
Simplify the handling by having a single variable tracking the number of bytes in the pipe.
For me it actually looks slightly more complicated to think about it this way, I added explicit 'read' and 'written' after being bitten by some issue I introduced with a previous 'pending' concept, but I have to admit it slightly simplifies the overflow topic.
As a bonus, the new scheme makes it clearer that we don't need to worry about overflows: pending can never become larger than the maximum pipe bufffer size, well within 32-bits.
I _think_ the old scheme was safe in the case of overflow - again under the assumption that read/written can never be further apart than the pipe buffer size. However, it's much harder to reason about this case. It's certainly plausible that an overflow could occur - sending 4GiB through a local socket is entirely achievable.
For me it looked pretty simple: you can overflow 32 bits (at 100 Gbps, but without hitting the "optimised" case, it would take about five minutes), but all the operations between the two counters are between two uint32_t, so they would happen in uint32_t, hence modulo 32 bits, similar to TCP sequences.
Plus it's only equality comparisons, so we don't need SEQ_GT or the like. Yeah, that's the reasoning, but to me that's still a lot more than "can't exceed pipe size".
Anyway, overall, I think it's an improvement over the original. One nit here:
Signed-off-by: David Gibson
--- tcp_conn.h | 6 ++---- tcp_splice.c | 18 +++++++++--------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/tcp_conn.h b/tcp_conn.h index 9f5bee03..c8381aa7 100644 --- a/tcp_conn.h +++ b/tcp_conn.h @@ -206,8 +206,7 @@ struct tcp_tap_transfer_ext { * @f: Generic flow information * @s: File descriptor for sockets * @pipe: File descriptors for pipes - * @read: Bytes read (not fully written to other side in one shot) - * @written: Bytes written (not fully written from one other side read) + * @pending: Bytes currently in each pipe * @events: Events observed/actions performed on connection * @flags: Connection flags (attributes, not events) */ @@ -218,8 +217,7 @@ struct tcp_splice_conn { int s[SIDES]; int pipe[SIDES][2];
- uint32_t read[SIDES]; - uint32_t written[SIDES]; + uint32_t pending[SIDES];
uint8_t events; #define SPLICE_CLOSED 0 diff --git a/tcp_splice.c b/tcp_splice.c index 18e8b303..8fbd490f 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -292,7 +292,7 @@ bool tcp_splice_flow_defer(struct tcp_splice_conn *conn) conn->s[sidei] = -1; }
- conn->read[sidei] = conn->written[sidei] = 0; + conn->pending[sidei] = 0; }
conn->events = SPLICE_CLOSED; @@ -490,7 +490,7 @@ static int tcp_splice_forward(struct ctx *c, struct int eof = 0;
while (1) { - ssize_t readlen, written, pending; + ssize_t readlen, written; int more = 0;
retry: @@ -537,7 +537,7 @@ retry: flow_trace(conn, "%zi from write-side call (passed %zi)", written, c->tcp.pipe_size);
- /* Most common case: skip updating counters. */ + /* Most common case: skip updating pending. */
"pending" isn't a noun (even though the variable name is, but it's not quite obvious that you're referring to it). I think that:
/* Most common case: skip updating count of pending bytes */
would be slightly clearer (and also omit the '.' because it's not a complete sentence, as we usually do on single-line comments, similarly to most occurrences in the kernel).
Done.
if (readlen > 0 && readlen == written) { if (readlen >= (long)c->tcp.pipe_size * 10 / 100) continue; @@ -561,11 +561,11 @@ retry: continue; }
- conn->read[fromsidei] += readlen > 0 ? readlen : 0; - conn->written[fromsidei] += written > 0 ? written : 0; + conn->pending[fromsidei] += readlen > 0 ? readlen : 0; + conn->pending[fromsidei] -= written > 0 ? written : 0;
if (written < 0) { - if (conn->read[fromsidei] == conn->written[fromsidei]) + if (!conn->pending[fromsidei]) break;
conn_event(conn, OUT_WAIT(!fromsidei)); @@ -575,15 +575,15 @@ retry: if (never_read && written == (long)(c->tcp.pipe_size)) goto retry;
- pending = conn->read[fromsidei] - conn->written[fromsidei]; - if (!never_read && written > 0 && written < pending) + if (!never_read && written > 0 && + written < conn->pending[fromsidei]) goto retry;
if (eof) break; }
- if (conn->read[fromsidei] == conn->written[fromsidei] && eof) { + if (!conn->pending[fromsidei] && eof) { unsigned sidei;
flow_foreach_sidei(sidei) {
-- Stefano
-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
On Wed, May 20, 2026 at 10:30:23PM +0200, Stefano Brivio wrote:
On Wed, 20 May 2026 23:08:51 +1000 David Gibson
wrote: At the end of tcp_splice_forward(), we check for half-closed connections and propagate the FIN to the other side with a shutdown(2). Currently we check for a half closed connection in either direction. That's unnecessary here, because tcp_splice_forward() will already be called for each direction if there are any relevant events.
True, but do we have the guarantee that tcp_splice_forward() will also be called once all relevant FIN_RCVD / FIN_SENT flags have been sent?
Yes, because tcp_splice_forward() is (now) the only place that *sets* FIN_RCVD (or FIN_SENT).
The reason why we check both sides here is that we might have updated flags for one side, and now we need to double check if it's time to call shutdown() as a consequence.
Maybe we never have to, but I think it's not really obvious to prove.
tcp_splice_forward() only touches FIN_RCVD(fromsidei). So, we only need to examine FIN_RCVD(fromsidei). If FIN_RCVD is set for the other side, it must be in another call to tcp_splice_forward() which will also examine that other flag and shutdown() as necessary.
Signed-off-by: David Gibson
--- tcp_splice.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index b45f0060..e5018f2e 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -582,21 +582,15 @@ retry: break; }
- if (!conn->pending[fromsidei] && - conn->events & FIN_RCVD(fromsidei)) { - unsigned sidei; - - flow_foreach_sidei(sidei) { - if ((conn->events & FIN_RCVD(sidei)) && - !(conn->events & FIN_SENT(!sidei))) { - if (shutdown(conn->s[!sidei], SHUT_WR) < 0) { - flow_perror(conn, "shutdown() on %s", - pif_name(conn->f.pif[!sidei])); - return -1; - } - conn_event(conn, FIN_SENT(!sidei)); - } + if ((conn->events & FIN_RCVD(fromsidei)) && + !(conn->events & FIN_SENT(!fromsidei)) && + !conn->pending[fromsidei]) { + if (shutdown(conn->s[!fromsidei], SHUT_WR) < 0) { + flow_perror(conn, "shutdown() on %s", + pif_name(conn->f.pif[!fromsidei])); + return -1; } + conn_event(conn, FIN_SENT(!fromsidei)); }
return 0;
-- Stefano
-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
On Thu, 21 May 2026 12:11:49 +1000
David Gibson
On Wed, May 20, 2026 at 10:30:23PM +0200, Stefano Brivio wrote:
On Wed, 20 May 2026 23:08:51 +1000 David Gibson
wrote: At the end of tcp_splice_forward(), we check for half-closed connections and propagate the FIN to the other side with a shutdown(2). Currently we check for a half closed connection in either direction. That's unnecessary here, because tcp_splice_forward() will already be called for each direction if there are any relevant events.
True, but do we have the guarantee that tcp_splice_forward() will also be called once all relevant FIN_RCVD / FIN_SENT flags have been sent?
Yes, because tcp_splice_forward() is (now) the only place that *sets* FIN_RCVD (or FIN_SENT).
The reason why we check both sides here is that we might have updated flags for one side, and now we need to double check if it's time to call shutdown() as a consequence.
Maybe we never have to, but I think it's not really obvious to prove.
tcp_splice_forward() only touches FIN_RCVD(fromsidei). So, we only need to examine FIN_RCVD(fromsidei). If FIN_RCVD is set for the other side, it must be in another call to tcp_splice_forward() which will also examine that other flag and shutdown() as necessary.
Okay, fair. That's what I call "not really obvious" but it's probably obvious enough.
Signed-off-by: David Gibson
--- tcp_splice.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index b45f0060..e5018f2e 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -582,21 +582,15 @@ retry: break; }
- if (!conn->pending[fromsidei] && - conn->events & FIN_RCVD(fromsidei)) { - unsigned sidei; - - flow_foreach_sidei(sidei) { - if ((conn->events & FIN_RCVD(sidei)) && - !(conn->events & FIN_SENT(!sidei))) { - if (shutdown(conn->s[!sidei], SHUT_WR) < 0) { - flow_perror(conn, "shutdown() on %s", - pif_name(conn->f.pif[!sidei])); - return -1; - } - conn_event(conn, FIN_SENT(!sidei)); - } + if ((conn->events & FIN_RCVD(fromsidei)) && + !(conn->events & FIN_SENT(!fromsidei)) && + !conn->pending[fromsidei]) { + if (shutdown(conn->s[!fromsidei], SHUT_WR) < 0) { + flow_perror(conn, "shutdown() on %s", + pif_name(conn->f.pif[!fromsidei])); + return -1; } + conn_event(conn, FIN_SENT(!fromsidei)); }
return 0;
-- Stefano
On Thu, 21 May 2026 12:03:33 +1000
David Gibson
On Wed, May 20, 2026 at 10:30:04PM +0200, Stefano Brivio wrote:
On Wed, 20 May 2026 23:08:50 +1000 David Gibson
wrote: There are two ways we can tell one of our sockets has received a FIN. We can either see an EPOLLRDHUP epoll event, or we can get a zero-length read (EOF) on the socket. We currently use both, in a mildly confusing way: we only set the FIN_RCVD() flag based on the EPOLLRDHUP event, but then some other close out logic is based on seeing an EOF.
Simplify this by setting the flag based on only the EOF. To make sure we don't miss an event if we get an EPOLLRDHUP with no data, we trigger the forwarding path for EPOLLRDHUP as well as EPOLLIN.
Signed-off-by: David Gibson
--- tcp_splice.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index 8fbd490f..b45f0060 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -487,7 +487,6 @@ static int tcp_splice_forward(struct ctx *c, struct uint8_t lowat_set_flag = RCVLOWAT_SET(fromsidei); uint8_t lowat_act_flag = RCVLOWAT_ACT(fromsidei); int never_read = 1; - int eof = 0;
while (1) { ssize_t readlen, written; @@ -510,7 +509,7 @@ retry: flow_trace(conn, "%zi from read-side call", readlen);
if (!readlen) { - eof = 1; + conn_event(conn, FIN_RCVD(fromsidei));
I'm not sure if I really found a concrete issue with this, but it looks a bit scary, because it changes the semantics of FIN_RCVD, which used to mean that we infer we received a FIN, regardless of whether we're done processing all data from that half of the connection.
Now FIN_RCVD is only set if we actually processed all the data and we hit the end of file.
True. But the only place that tested FIN_RCVD was at the end of tcp_splice_forward(), conditional on 'eof' anyway. In a sense, this was the cause of bug202 - we had FIN_RCVD set, but we didn't process it and shutdown() on the other side, because we didn't have eof.
That sounds like a good motivation to clean this up, just two concerns below:
The (potential) issue I see here is that we get EPOLLRDHUP, splice() returns -1 with EAGAIN in errno because we had no room in the pipe, and it would have returned 0 instead.
Will we ever get our zero-sized "read" later? If not, we might have missed EPOLLRDHUP *and* the end of file. I'm not entirely sure we have guarantees in that sense from splice().
It's not really about guarantees from splice. I'm pretty sure this is ok, reasoning as follows.
Consider all the exit points from the loop body: - Each return is a return -1, so we kill the connection anyway. They don't matter - Each continue, goto retry and the end of the body will do the read side splice() again, so get another chance to see the EOF - That leaves just the breaks
Consider each break (there are three, since patch 2 of this series) if (written < 0) { if (!conn->pending[fromsidei]) break;
(1) The pipe is empty and the write-splice returned EAGAIN, so it didn't remove data from the pipe.
You're assuming that !conn->pending[fromsidei] means that the pipe is empty. From what we see of it, it is. What the kernel can do with it, though, is different. It might return EAGAIN even if we think we should have space, because it's resizing it under memory pressure or anything like that. Or it delays freeing up space or accounting for whatever reason. So it would be nice to make this part robust to that. I thought setting FIN_RCVD on EPOLLRDHUP was a good way to achieve that.
Therefore, the pipe must have been empty before the write-splice. Which means the read-splice can't have blocked on a full pipe. conn_event(conn, OUT_WAIT(!fromsidei)); break; }
(2) The pipe is non-empty and the write-splice returned EAGAIN, so it must have blocked on the output socket. We've set OUT_WAIT(), so we'll get an EPOLLOUT at some point which will cause us to read-splice again, meaning we get another chance to see the EOF.
...later. But what if we don't get a zero-sized read *at all*? I'm not sure if splice() guarantees we do get one if we reach end-of-file. That's something valid and very well established for read() and recv(), but splice() is a bit weird. The documentation says: A return value of 0 means end of input. but I wouldn't assume we'll *always* get at least one in case of EOF.
[...] if (conn->events & FIN_RCVD(fromsidei)) break; (3) By the new semantics of FIN_RCVD, we *have* seen the EOF.
The existing implementation distinguishes between end-of-file we hit in a given iteration, and EPOLLRDHUP we might have seen at any time. That was actually intended.
It might be intended, but I can't see that we did anything with that information.
We always set FIN_RCVD on it. You're right, if we only checked that on 'eof', that didn't solve much, but that wasn't necessarily intended. My original intention was to make setting of FIN_RCVD (or whatever it was originally) robust.
That said the conditions on which we exit / retry this loop are pretty darn confusing. I'll see if I can improve them.
-- Stefano
On Thu, May 21, 2026 at 07:40:31AM +0200, Stefano Brivio wrote:
On Thu, 21 May 2026 12:03:33 +1000 David Gibson
wrote: On Wed, May 20, 2026 at 10:30:04PM +0200, Stefano Brivio wrote:
On Wed, 20 May 2026 23:08:50 +1000 David Gibson
wrote: There are two ways we can tell one of our sockets has received a FIN. We can either see an EPOLLRDHUP epoll event, or we can get a zero-length read (EOF) on the socket. We currently use both, in a mildly confusing way: we only set the FIN_RCVD() flag based on the EPOLLRDHUP event, but then some other close out logic is based on seeing an EOF.
Simplify this by setting the flag based on only the EOF. To make sure we don't miss an event if we get an EPOLLRDHUP with no data, we trigger the forwarding path for EPOLLRDHUP as well as EPOLLIN.
Signed-off-by: David Gibson
--- tcp_splice.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index 8fbd490f..b45f0060 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -487,7 +487,6 @@ static int tcp_splice_forward(struct ctx *c, struct uint8_t lowat_set_flag = RCVLOWAT_SET(fromsidei); uint8_t lowat_act_flag = RCVLOWAT_ACT(fromsidei); int never_read = 1; - int eof = 0;
while (1) { ssize_t readlen, written; @@ -510,7 +509,7 @@ retry: flow_trace(conn, "%zi from read-side call", readlen);
if (!readlen) { - eof = 1; + conn_event(conn, FIN_RCVD(fromsidei));
I'm not sure if I really found a concrete issue with this, but it looks a bit scary, because it changes the semantics of FIN_RCVD, which used to mean that we infer we received a FIN, regardless of whether we're done processing all data from that half of the connection.
Now FIN_RCVD is only set if we actually processed all the data and we hit the end of file.
True. But the only place that tested FIN_RCVD was at the end of tcp_splice_forward(), conditional on 'eof' anyway. In a sense, this was the cause of bug202 - we had FIN_RCVD set, but we didn't process it and shutdown() on the other side, because we didn't have eof.
That sounds like a good motivation to clean this up, just two concerns below:
The (potential) issue I see here is that we get EPOLLRDHUP, splice() returns -1 with EAGAIN in errno because we had no room in the pipe, and it would have returned 0 instead.
Will we ever get our zero-sized "read" later? If not, we might have missed EPOLLRDHUP *and* the end of file. I'm not entirely sure we have guarantees in that sense from splice().
It's not really about guarantees from splice. I'm pretty sure this is ok, reasoning as follows.
Consider all the exit points from the loop body: - Each return is a return -1, so we kill the connection anyway. They don't matter - Each continue, goto retry and the end of the body will do the read side splice() again, so get another chance to see the EOF - That leaves just the breaks
Consider each break (there are three, since patch 2 of this series) if (written < 0) { if (!conn->pending[fromsidei]) break;
(1) The pipe is empty and the write-splice returned EAGAIN, so it didn't remove data from the pipe.
You're assuming that !conn->pending[fromsidei] means that the pipe is empty. From what we see of it, it is.
It does mean the pipe is empty. Everything we put in, we've taken out. There cannot be anything in there.
What the kernel can do with it, though, is different. It might return EAGAIN even if we think we should have space, because it's resizing it under memory pressure or anything like that. Or it delays freeing up space or accounting for whatever reason.
Theoretically, I suppose. But !pending doesn't just mean the pipe is not full it means it's completely completely empty. Not being able to put any bytes at all into an empty pipe would be *very* surprising. So much so that if it happened in practice, I suspect we wouldn't be safe not having epoll events on the pipe ends, so that we can be notified when it deigns to accept some data.
So it would be nice to make this part robust to that. I thought setting FIN_RCVD on EPOLLRDHUP was a good way to achieve that.
Therefore, the pipe must have been empty before the write-splice. Which means the read-splice can't have blocked on a full pipe. conn_event(conn, OUT_WAIT(!fromsidei)); break; }
(2) The pipe is non-empty and the write-splice returned EAGAIN, so it must have blocked on the output socket. We've set OUT_WAIT(), so we'll get an EPOLLOUT at some point which will cause us to read-splice again, meaning we get another chance to see the EOF.
...later. But what if we don't get a zero-sized read *at all*? I'm not sure if splice() guarantees we do get one if we reach end-of-file.
That's something valid and very well established for read() and recv(), but splice() is a bit weird. The documentation says:
A return value of 0 means end of input.
but I wouldn't assume we'll *always* get at least one in case of EOF.
What else could we plausibly get?
[...] if (conn->events & FIN_RCVD(fromsidei)) break; (3) By the new semantics of FIN_RCVD, we *have* seen the EOF.
The existing implementation distinguishes between end-of-file we hit in a given iteration, and EPOLLRDHUP we might have seen at any time. That was actually intended.
It might be intended, but I can't see that we did anything with that information.
We always set FIN_RCVD on it. You're right, if we only checked that on 'eof', that didn't solve much, but that wasn't necessarily intended. My original intention was to make setting of FIN_RCVD (or whatever it was originally) robust.
Ok, well. I've spotted other changes to make in the vicinity that I think will make some of this easier to reason about anyway. So I'll consider your points as I rework this and other patches.
That said the conditions on which we exit / retry this loop are pretty darn confusing. I'll see if I can improve them.
-- Stefano
-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
On Thu, 21 May 2026 16:56:43 +1000
David Gibson
On Thu, May 21, 2026 at 07:40:31AM +0200, Stefano Brivio wrote:
On Thu, 21 May 2026 12:03:33 +1000 David Gibson
wrote: On Wed, May 20, 2026 at 10:30:04PM +0200, Stefano Brivio wrote:
On Wed, 20 May 2026 23:08:50 +1000 David Gibson
wrote: There are two ways we can tell one of our sockets has received a FIN. We can either see an EPOLLRDHUP epoll event, or we can get a zero-length read (EOF) on the socket. We currently use both, in a mildly confusing way: we only set the FIN_RCVD() flag based on the EPOLLRDHUP event, but then some other close out logic is based on seeing an EOF.
Simplify this by setting the flag based on only the EOF. To make sure we don't miss an event if we get an EPOLLRDHUP with no data, we trigger the forwarding path for EPOLLRDHUP as well as EPOLLIN.
Signed-off-by: David Gibson
--- tcp_splice.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index 8fbd490f..b45f0060 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -487,7 +487,6 @@ static int tcp_splice_forward(struct ctx *c, struct uint8_t lowat_set_flag = RCVLOWAT_SET(fromsidei); uint8_t lowat_act_flag = RCVLOWAT_ACT(fromsidei); int never_read = 1; - int eof = 0;
while (1) { ssize_t readlen, written; @@ -510,7 +509,7 @@ retry: flow_trace(conn, "%zi from read-side call", readlen);
if (!readlen) { - eof = 1; + conn_event(conn, FIN_RCVD(fromsidei));
I'm not sure if I really found a concrete issue with this, but it looks a bit scary, because it changes the semantics of FIN_RCVD, which used to mean that we infer we received a FIN, regardless of whether we're done processing all data from that half of the connection.
Now FIN_RCVD is only set if we actually processed all the data and we hit the end of file.
True. But the only place that tested FIN_RCVD was at the end of tcp_splice_forward(), conditional on 'eof' anyway. In a sense, this was the cause of bug202 - we had FIN_RCVD set, but we didn't process it and shutdown() on the other side, because we didn't have eof.
That sounds like a good motivation to clean this up, just two concerns below:
The (potential) issue I see here is that we get EPOLLRDHUP, splice() returns -1 with EAGAIN in errno because we had no room in the pipe, and it would have returned 0 instead.
Will we ever get our zero-sized "read" later? If not, we might have missed EPOLLRDHUP *and* the end of file. I'm not entirely sure we have guarantees in that sense from splice().
It's not really about guarantees from splice. I'm pretty sure this is ok, reasoning as follows.
Consider all the exit points from the loop body: - Each return is a return -1, so we kill the connection anyway. They don't matter - Each continue, goto retry and the end of the body will do the read side splice() again, so get another chance to see the EOF - That leaves just the breaks
Consider each break (there are three, since patch 2 of this series) if (written < 0) { if (!conn->pending[fromsidei]) break;
(1) The pipe is empty and the write-splice returned EAGAIN, so it didn't remove data from the pipe.
You're assuming that !conn->pending[fromsidei] means that the pipe is empty. From what we see of it, it is.
It does mean the pipe is empty. Everything we put in, we've taken out. There cannot be anything in there.
What the kernel can do with it, though, is different. It might return EAGAIN even if we think we should have space, because it's resizing it under memory pressure or anything like that. Or it delays freeing up space or accounting for whatever reason.
Theoretically, I suppose. But !pending doesn't just mean the pipe is not full it means it's completely completely empty. Not being able to put any bytes at all into an empty pipe would be *very* surprising. So much so that if it happened in practice, I suspect we wouldn't be safe not having epoll events on the pipe ends, so that we can be notified when it deigns to accept some data.
We can get 512-byte pipes, I actually saw that happening in practice with either: - people setting low values for ulimits - the user (or just pasta itself) having a lot of pipes open and if I recall correctly that's where I saw the case of a supposedly empty pipe giving us EAGAIN. That was years ago though and I didn't specifically fix that. We currently probe the size based on the value we can have for 32 pipes (TCP_SPLICE_PIPE_POOL_SIZE). By making that 4096 or so you should get rather small pipes. Things might already be broken with them, I haven't checked the behaviour in a long while. I think 512 bytes was the lower bound I hit.
So it would be nice to make this part robust to that. I thought setting FIN_RCVD on EPOLLRDHUP was a good way to achieve that.
Therefore, the pipe must have been empty before the write-splice. Which means the read-splice can't have blocked on a full pipe. conn_event(conn, OUT_WAIT(!fromsidei)); break; }
(2) The pipe is non-empty and the write-splice returned EAGAIN, so it must have blocked on the output socket. We've set OUT_WAIT(), so we'll get an EPOLLOUT at some point which will cause us to read-splice again, meaning we get another chance to see the EOF.
...later. But what if we don't get a zero-sized read *at all*? I'm not sure if splice() guarantees we do get one if we reach end-of-file.
That's something valid and very well established for read() and recv(), but splice() is a bit weird. The documentation says:
A return value of 0 means end of input.
but I wouldn't assume we'll *always* get at least one in case of EOF.
What else could we plausibly get?
-1 with EBADF, probably with EPOLLERR, because something timed out? But I guess you're right, as long as we're not in the EPOLLERR category of things, we should consistently get 0, even if we read multiple times. I had in mind some kernel behaviour where we get 0 once, and then -1 (EAGAIN?) because... go figure. But no, it can't happen.
[...] if (conn->events & FIN_RCVD(fromsidei)) break; (3) By the new semantics of FIN_RCVD, we *have* seen the EOF.
The existing implementation distinguishes between end-of-file we hit in a given iteration, and EPOLLRDHUP we might have seen at any time. That was actually intended.
It might be intended, but I can't see that we did anything with that information.
We always set FIN_RCVD on it. You're right, if we only checked that on 'eof', that didn't solve much, but that wasn't necessarily intended. My original intention was to make setting of FIN_RCVD (or whatever it was originally) robust.
Ok, well. I've spotted other changes to make in the vicinity that I think will make some of this easier to reason about anyway. So I'll consider your points as I rework this and other patches.
That said the conditions on which we exit / retry this loop are pretty darn confusing. I'll see if I can improve them.
-- Stefano
-- Stefano
participants (2)
-
David Gibson
-
Stefano Brivio