[Qemu-devel] [PATCH 0/3] slirp: deliver received TCP RSTs to the guest

[Qemu-devel] [PATCH 0/3] slirp: deliver received TCP RSTs to the guest

[steven]

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(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/3] slirp: don't crash when tcp_sockclosed() is called with a NULL tp

[steven]

Signed-off-by: Steven Luo <steven+qemu@steven676.net>
---
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(-)

diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index dbfd2c6..32ff452 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -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);
 }
 
 /*
-- 
2.1.4

[Qemu-devel] [PATCH 2/3] slirp: Propagate host TCP RST to the guest.

[steven]

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.

[steven@steven676.net: coding style adjustments]
Signed-off-by: Steven Luo <steven+qemu@steven676.net>
---
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(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index b836c42..4372ec2 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -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;
 		}
 	}
-- 
2.1.4

Re: [Qemu-devel] [PATCH 2/3] slirp: Propagate host TCP RST to the guest.

[Paolo Bonzini]

On 06/04/2016 02:14, steven@steven676.net 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.

[Edgar E. Iglesias]

On Tue, Apr 05, 2016 at 05:14:31PM -0700, steven@steven676.net 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.
> 
> [steven@steven676.net: coding style adjustments]
> Signed-off-by: Steven Luo <steven+qemu@steven676.net>
> ---
> 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.

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 edgar.iglesias@gmail.com

BTW, I had totally forgotten about this... thanks for picking it up!

Best regards,
Edgar


> 
>  slirp/socket.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/slirp/socket.c b/slirp/socket.c
> index b836c42..4372ec2 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -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;
>  		}
>  	}
> -- 
> 2.1.4
> 

Re: [Qemu-devel] [PATCH 2/3] slirp: Propagate host TCP RST to the guest.

[Steven Luo]

On April 6, 2016 1:36:01 AM PDT, "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> wrote:
>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
>edgar.iglesias@gmail.com

It looks like an intermediate mail server mangled the From addresses somewhere along the way -- I have it as edgar.iglesias@axis.com in my local tree.  My apologies -- I'll fix and resend later.


-Steven Luo

[Qemu-devel] [PATCH 3/3] slirp: handle deferred ECONNREFUSED on non-blocking TCP sockets

[steven]

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.

Signed-off-by: Steven Luo <steven+qemu@steven676.net>
---
 slirp/socket.c    | 2 +-
 slirp/tcp_input.c | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index 4372ec2..6d22127 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -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 {
diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
index 1fcca30..5433e7f 100644
--- a/slirp/tcp_input.c
+++ b/slirp/tcp_input.c
@@ -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;
-- 
2.1.4

Re: [Qemu-devel] [PATCH 0/3] slirp: deliver received TCP RSTs to the guest

[Thomas Huth]

On 06.04.2016 02:13, steven@steven676.net 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.
> 
> 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(-)

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

Re: [Qemu-devel] [PATCH 0/3] slirp: deliver received TCP RSTs to the guest

[Edgar E. Iglesias]

On Tue, Apr 05, 2016 at 05:13:58PM -0700, steven@steven676.net 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.

Thanks, the series looks good to me.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Cheers,
Edgar



> 
> 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(-)
> 
> -- 
> 2.1.4
> 

Re: [Qemu-devel] [PATCH 0/3] slirp: deliver received TCP RSTs to the guest

[Samuel Thibault]

Hello,

Thanks for this!

steven@steven676.net, 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'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


diff --git a/slirp/slirp.c b/slirp/slirp.c
index fef526c..b13b9af 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -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;
+                    }
                 }
 
                 /*

Re: [Qemu-devel] [PATCH 0/3] slirp: deliver received TCP RSTs to the guest

[Steven Luo]

On Wed, Apr 06, 2016 at 02:57:43PM +0200, Samuel Thibault wrote:
> steven@steven676.net, 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.

> 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.

> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index fef526c..b13b9af 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -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.

> +                        continue;
> +                    }
>                  }
>  
>                  /*

-Steven Luo

Re: [Qemu-devel] [PATCH 0/3] slirp: deliver received TCP RSTs to the guest

[Samuel Thibault]

Hello,

Steven Luo, on Wed 06 Apr 2016 17:00:50 -0700, wrote:
> 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?

Indeed, then sorecvoob() needs to return that information, so
slirp_pollfds_poll can avoid doing anything else with this socket.

> I could take care of this when I resend this patch series, if you
> prefer.

If you like, please do :)

> 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.

Right.

Samuel