slirp: deliver received TCP RSTs to the guest
cover slirp: deliver received TCP RSTs to the guest
QEMU's user-mode networking does not currently pass received TCP RSTs to guests, meaning that applications in guests hang if the remote server rejects their network connections. This is particularly noticeable when IPv6 is enabled, the guest is configured to prefer IPv6 and the remote server rejects IPv6 connections (segment-data.zqtk.net is one example), but the bug appears to be longstanding and affects TCP over IPv4 as well.
There are three short patches in this series. The first fixes a crash which would be exposed by the last patch in the series. The second, which fixes delivery of an RST interrupting an already-established TCP connection, was submitted by Edgar Iglesias in 2008 and appears to have been missed then. The last patch fixes the case where the remote end sends RST in reply to our SYN (rejects our incoming connection attempt).
Lightly tested on a Linux host with Linux and Windows 7 guests.
Edgar E. Iglesias (1):
slirp: Propagate host TCP RST to the guest.
Steven Luo (2):
slirp: don't crash when tcp_sockclosed() is called with a NULL tp
slirp: handle deferred ECONNREFUSED on non-blocking TCP sockets
slirp/socket.c | 17 ++++++++++++++++- slirp/tcp_input.c | 6 ++++++ slirp/tcp_subr.c | 7 +++++-- 3 files changed, 27 insertions(+), 3 deletions(-)
Signature · 1 line
2.1.4
PATCH 1/3 slirp: don't crash when tcp_sockclosed() is called with a NULL tp
---
This prevents a crash that would be exposed by a later patch in this series. The removed check for non-null is clearly wrong, as it comes after the pointer has already been dereferenced in this function.
slirp/tcp_subr.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
slirp/tcp_subr.c +5 -2
@@ -356,6 +356,10 @@ tcp_sockclosed(struct tcpcb *tp)DEBUG_CALL ("tcp_sockclosed" );DEBUG_ARG ("tp = %p" , tp); +if (!tp) { +return ; + } +switch (tp->t_state) {case TCPS_CLOSED :
@@ -374,8 +378,7 @@ tcp_sockclosed(struct tcpcb *tp) tp->t_state =TCPS_LAST_ACK ;break ; } -if (tp) -tcp_output (tp); +tcp_output (tp); }/*
Signature · 1 line
2.1.4
PATCH 2/3 slirp: Propagate host TCP RST to the guest.
In reply to [email protected]When the host aborts (RST) it's side of a TCP connection we need to propagate that RST to the guest. The current code can leave such guest connections dangling forever. Spotted by Jason Wessel.
---
Edgar proposed this patch many years ago:
https://lists.gnu.org/archive/html/qemu-devel/2008-06/msg00383.html
It doesn't appear that it was ever merged. (It's the top Google result for "QEMU slirp RST".) I've been unable to test the specific case it addresses (an established connection interrupted by RST), but the discussion from 2008 seems to imply it worked for the person reporting the problem then, and my next patch builds on this one.
As this patch isn't my work and did not come with a Signed-off-by line, I'm not entirely clear on how I should handle that.
slirp/socket.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
slirp/socket.c +16 -1
@@ -176,9 +176,24 @@ soread(struct socket *so)if (nn <0 && (errno ==EINTR || errno ==EAGAIN ))return 0 ;else { +int err; +socklen_t slen =sizeof err; + + err = errno; +if (nn ==0 ) { +getsockopt (so->s,SOL_SOCKET ,SO_ERROR , + &err, &slen); + } +DEBUG_MISC ((dfd," --- soread() disconnected, nn = %d, errno = %d-%s\n" , nn, errno,strerror (errno)));sofcantrcvmore (so); -tcp_sockclosed (sototcpcb (so)); + +if (err ==ECONNRESET + || err ==ENOTCONN || err ==EPIPE ) { +tcp_drop (sototcpcb (so), err); + }else { +tcp_sockclosed (sototcpcb (so)); + }return -1 ; } }
Signature · 1 line
2.1.4
Re: [Qemu-devel] [PATCH 2/3] slirp: Propagate host TCP RST to the guest.
Quoted 2 lines · On 06/04/2016 02:14, [email protected] wrote:
As this patch isn't my work and did not come with a Signed-off-by line, I'm not entirely clear on how I should handle that.
Signed-off-by lines weren't in consistent use at the time, so I think it's okay to leave it out.
Paolo
Re: [Qemu-devel] [PATCH 2/3] slirp: Propagate host TCP RST to the guest.
In reply to [email protected]Quoted 11 lines · On Tue, Apr 05, 2016 at 05:14:31PM -0700, [email protected] wrote:
When the host aborts (RST) it's side of a TCP connection we need to propagate that RST to the guest. The current code can leave such guest connections dangling forever. Spotted by Jason Wessel.
[[email protected]: coding style adjustments]Signed-off-by Steven Luo <[email protected]>---
Edgar proposed this patch many years ago:https://lists.gnu.org/archive/html/qemu-devel/2008-06/msg00383.html
Quoted 8 lines
It doesn't appear that it was ever merged. (It's the top Google result for "QEMU slirp RST".) I've been unable to test the specific case it addresses (an established connection interrupted by RST), but the discussion from 2008 seems to imply it worked for the person reporting the problem then, and my next patch builds on this one.
As this patch isn't my work and did not come with a Signed-off-by line, I'm not entirely clear on how I should handle that.
Hi Steven,
I don't mind to leave it as is but you could also use the --author argument to git commit to keep the authorship as [email protected]
BTW, I had totally forgotten about this... thanks for picking it up!
Best regards,
Edgar
Quoted 37 lines
slirp/socket.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)slirp/socket.c +16
-1@@ -176,9 +176,24 @@ soread(struct socket *so)if (nn <0 && (errno ==EINTR || errno ==EAGAIN ))return 0 ;else { +int err; +socklen_t slen =sizeof err; + + err = errno; +if (nn ==0 ) { +getsockopt (so->s,SOL_SOCKET ,SO_ERROR , + &err, &slen); + } +DEBUG_MISC ((dfd," --- soread() disconnected, nn = %d, errno = %d-%s\n" , nn, errno,strerror (errno)));sofcantrcvmore (so); -tcp_sockclosed (sototcpcb (so)); + +if (err ==ECONNRESET + || err ==ENOTCONN || err ==EPIPE ) { +tcp_drop (sototcpcb (so), err); + }else { +tcp_sockclosed (sototcpcb (so)); + }return -1 ; } }Signature · 1 line
2.1.4
Re: [Qemu-devel] [PATCH 2/3] slirp: Propagate host TCP RST to the guest.
Quoted 5 lines · Edgar E. Iglesias
Hi Steven,
I don't mind to leave it as is but you could also use the --author argument to git commit to keep the authorship as [email protected]
It looks like an intermediate mail server mangled the From addresses somewhere along the way -- I have it as [email protected] in my local tree. My apologies -- I'll fix and resend later.
-Steven Luo
Hello,
Thanks for this!
Quoted 5 lines · [email protected], on Tue 05 Apr 2016 17:13:58 -0700, wrote:
The second,
which fixes delivery of an RST interrupting an already-established TCP connection, was submitted by Edgar Iglesias in 2008 and appears to have been missed then. The last patch fixes the case where the remote end sends RST in reply to our SYN (rejects our incoming connection attempt).
It seems I'm getting another crash with these: sowrite would be called too for the reseted socket, while the socket has been freed and is not even on the polling list any more, I had to additionally do the patch below, could you review it so I can push the whole series?
Quoted 2 lines
I've been unable to test the specific case it addresses (an established connection interrupted by RST),
It's actually quite easy: just reboot the server :) The new instance of the server will send a RST whenever the client sends more data.
Samuel
slirp/slirp.c +5 -0
@@ -553,6 +553,11 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)if (ret >0 ) {tcp_output (sototcpcb (so)); } +if (ret <0 ) { +/* Socket error and thus removed, do not try to do +* anything more with it. */ +continue ; + } }/*
Quoted 11 lines · Samuel Thibault
Quoted 5 lines · [email protected], on Tue 05 Apr 2016 17:13:58 -0700, wrote:
The second,
which fixes delivery of an RST interrupting an already-established TCP connection, was submitted by Edgar Iglesias in 2008 and appears to have been missed then. The last patch fixes the case where the remote end sends RST in reply to our SYN (rejects our incoming connection attempt).It seems I'm getting another crash with these: sowrite would be called too for the reseted socket, while the socket has been freed and is not even on the polling list any more, I had to additionally do the patch below, could you review it so I can push the whole series?
I can't reproduce the crash, but the !(so->so_state & SS_NOFDREF) test immediately below would seem to be a use-after-free in this case, so I figure we do need something like this. That said, sorecvoob() also calls soread(), so I'd guess we need to deal with the possibility that soread() frees the socket in that case as well? (I can't find any other callers of soread(), but if they exist, they probably need to be fixed too.)
I could take care of this when I resend this patch series, if you prefer.
Quoted 2 lines
It's actually quite easy: just reboot the server :) The new instance of the server will send a RST whenever the client sends more data.
Thanks for the hint -- I've verified that case works as well now.
slirp/slirp.c void slirp_pollfds_poll(GArray *pollfds, int select_error) · line 558
Quoted 11 lines
slirp/slirp.c +3
-0@@ -553,6 +553,11 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)if (ret >0 ) {tcp_output (sototcpcb (so)); } +if (ret <0 ) { +/* Socket error and thus removed, do not try to do +* anything more with it. */
I think this should be "might have been removed"? tcp_sockclosed() doesn't seem to call tcp_close() in every case, so we can get -1 from soread() without the socket being freed.
slirp/slirp.c void slirp_pollfds_poll(GArray *pollfds, int select_error) · line 563
Quoted 5 lines
slirp/slirp.c +2
-0+continue ; + } }/*
-Steven Luo
Hello,
Quoted 3 lines · Steven Luo, on Wed 06 Apr 2016 17:00:50 -0700, wrote:
That said,
sorecvoob()also callssoread(), so I'd guess we need to deal with the possibility thatsoread()frees the socket in that case as well?
Indeed, then sorecvoob() needs to return that information, so slirp_pollfds_poll can avoid doing anything else with this socket.
Quoted 2 lines
I could take care of this when I resend this patch series, if you prefer.
If you like, please do :)
Quoted 3 lines
I think this should be "might have been removed"?
tcp_sockclosed()doesn't seem to calltcp_close()in every case, so we can get -1 fromsoread()without the socket being freed.
Right.
Samuel
PATCH 3/3 slirp: handle deferred ECONNREFUSED on non-blocking TCP sockets
In reply to [email protected]slirp currently only handles ECONNREFUSED in the case where connect() returns immediately with that error; since we use non-blocking sockets, most of the time we won't receive the error until we later try to read from the socket. Ensure that we deliver the appropriate RST to the guest in this case.
--- slirp/socket.c | 2 +- slirp/tcp_input.c | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-)
slirp/socket.c +1 -1
@@ -188,7 +188,7 @@ soread(struct socket *so)DEBUG_MISC ((dfd," --- soread() disconnected, nn = %d, errno = %d-%s\n" , nn, errno,strerror (errno)));sofcantrcvmore (so); -if (err ==ECONNRESET +if (err ==ECONNRESET || err ==ECONNREFUSED || err ==ENOTCONN || err ==EPIPE ) {tcp_drop (sototcpcb (so), err); }else {
slirp/tcp_input.c +6 -0
@@ -725,6 +725,12 @@ findso: so->so_ti = ti; tp->t_timer[TCPT_KEEP ] =TCPTV_KEEP_INIT ; tp->t_state =TCPS_SYN_RECEIVED ; +/* +* Initialize receive sequence numbers now so that we can send a +* valid RST if the remote end rejects our connection. +*/ + tp->irs = ti->ti_seq; +tcp_rcvseqinit (tp);tcp_template (tp); }return ;
Signature · 1 line
2.1.4
Quoted 20 lines · On 06.04.2016 02:13, [email protected] wrote:
QEMU's user-mode networking does not currently pass received TCP RSTs to guests, meaning that applications in guests hang if the remote server rejects their network connections. This is particularly noticeable when IPv6 is enabled, the guest is configured to prefer IPv6 and the remote server rejects IPv6 connections (segment-data.zqtk.net is one example), but the bug appears to be longstanding and affects TCP over IPv4 as well.
There are three short patches in this series. The first fixes a crash which would be exposed by the last patch in the series. The second, which fixes delivery of an RST interrupting an already-established TCP connection, was submitted by Edgar Iglesias in 2008 and appears to have been missed then. The last patch fixes the case where the remote end sends RST in reply to our SYN (rejects our incoming connection attempt).
Lightly tested on a Linux host with Linux and Windows 7 guests.
Edgar E. Iglesias (1):
slirp: Propagate host TCP RST to the guest.
Quoted 8 lines
Steven Luo (2):
slirp: don't crash whentcp_sockclosed()is called with a NULL tp
slirp: handle deferred ECONNREFUSED on non-blocking TCP socketsslirp/socket.c | 17 ++++++++++++++++- slirp/tcp_input.c | 6 ++++++ slirp/tcp_subr.c | 7 +++++-- 3 files changed, 27 insertions(+), 3 deletions(-)
FWIW, please make sure to CC: Samuel Thibault when posting slirp patches
- he's the new (co-)maintainer of the slirp code now.
Thanks,
Thomas
Quoted 8 lines · On Tue, Apr 05, 2016 at 05:13:58PM -0700, [email protected] wrote:
QEMU's user-mode networking does not currently pass received TCP RSTs to guests, meaning that applications in guests hang if the remote server rejects their network connections. This is particularly noticeable when IPv6 is enabled, the guest is configured to prefer IPv6 and the remote server rejects IPv6 connections (segment-data.zqtk.net is one example), but the bug appears to be longstanding and affects TCP over IPv4 as well.
Quoted 8 lines
There are three short patches in this series. The first fixes a crash which would be exposed by the last patch in the series. The second, which fixes delivery of an RST interrupting an already-established TCP connection, was submitted by Edgar Iglesias in 2008 and appears to have been missed then. The last patch fixes the case where the remote end sends RST in reply to our SYN (rejects our incoming connection attempt).
Lightly tested on a Linux host with Linux and Windows 7 guests.
Thanks, the series looks good to me.
Cheers,
Edgar
Quoted 16 lines
Edgar E. Iglesias (1):
slirp: Propagate host TCP RST to the guest.Steven Luo (2):
slirp: don't crash whentcp_sockclosed()is called with a NULL tp
slirp: handle deferred ECONNREFUSED on non-blocking TCP socketsslirp/socket.c | 17 ++++++++++++++++- slirp/tcp_input.c | 6 ++++++ slirp/tcp_subr.c | 7 +++++-- 3 files changed, 27 insertions(+), 3 deletions(-)Signature · 1 line
2.1.4