[PATCH] tap: Drop frames if no client connected

[PATCH] tap: Drop frames if no client connected

[Yumei Huang]

If no client is attached, discard outgoing frames and report them as
sent. This mimics the behavior of a physical host with its network
cable unplugged.

Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Yumei Huang <yuhuang@redhat.com>
---
 tap.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tap.c b/tap.c
index 7ba6399..e01219d 100644
--- a/tap.c
+++ b/tap.c
@@ -507,13 +507,17 @@ static size_t tap_send_frames_passt(const struct ctx *c,
  * @iov must have total length @bufs_per_frame * @nframes, with each set of
  * @bufs_per_frame contiguous buffers representing a single frame.
  *
- * Return: number of frames actually sent
+ * Return: number of frames actually sent, or accounted as sent
  */
 size_t tap_send_frames(const struct ctx *c, const struct iovec *iov,
 		       size_t bufs_per_frame, size_t nframes)
 {
 	size_t m;
 
+	if (c->fd_tap == -1)
+		/* If no client connected, account the frames have been sent */
+		return nframes;
+
 	if (!nframes)
 		return 0;
 
-- 
2.47.0

Re: [PATCH] tap: Drop frames if no client connected

[Stefano Brivio]

On Thu, 11 Sep 2025 16:55:19 +0800
Yumei Huang <yuhuang@redhat.com> wrote:

> If no client is attached, discard outgoing frames and report them as
> sent. This mimics the behavior of a physical host with its network
> cable unplugged.
> 
> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Yumei Huang <yuhuang@redhat.com>

Thanks, the fix itself obviously makes sense, but I have a few questions
and comments:

- first off, what happens if we don't return early in tap_send_frames()?
  Commit messages for fixes (assuming this is a fix) should always say
  what concrete problem we had, what is going to be fixed, or if we're
  not aware of any real issue but things are just fragile / wrong

- until a while ago, this couldn't happen at all. We were just blocking
  the whole execution as long as the tap / guest / container interface
  wasn't up and running.

  I wonder when this changed and if it makes sense to go back to the
  previous behaviour. I had just a quick look and I wonder if I
  accidentally broke this in c9b241346569 ("conf, passt, tap: Open
  socket and PID files before switching UID/GID").

  Before that, main() would call tap_sock_init(), which would call
  tap_sock_unix_open(), a blocking function.

  Should we make the whole thing blocking again? If not, is there
  anything else that's breaking with that? Timers, other inputs, etc.

  I didn't really have time to investigate until now, I can try to
  have another look soon though, unless you find out more meanwhile.

> ---
>  tap.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tap.c b/tap.c
> index 7ba6399..e01219d 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -507,13 +507,17 @@ static size_t tap_send_frames_passt(const struct ctx *c,
>   * @iov must have total length @bufs_per_frame * @nframes, with each set of
>   * @bufs_per_frame contiguous buffers representing a single frame.
>   *
> - * Return: number of frames actually sent
> + * Return: number of frames actually sent, or accounted as sent
>   */
>  size_t tap_send_frames(const struct ctx *c, const struct iovec *iov,
>  		       size_t bufs_per_frame, size_t nframes)
>  {
>  	size_t m;
>  
> +	if (c->fd_tap == -1)
> +		/* If no client connected, account the frames have been sent */

I think the comment is redundant because, well, if c->fd_tap is -1
(obvious, documented), we return 'nframes' (also documented).

If it's not redundant, for any reason, "to account" in this sense
isn't transitive. You could say: "consider that the frames have been
sent" but not "account that the frames have been sent".

You can pick a different meaning of "to account" and say "account the
frames as sent", though.

> +		return nframes;
> +
>  	if (!nframes)
>  		return 0;
>  

-- 
Stefano

Re: [PATCH] tap: Drop frames if no client connected

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5463 bytes --]

On Thu, Sep 11, 2025 at 11:54:25AM +0200, Stefano Brivio wrote:
> On Thu, 11 Sep 2025 16:55:19 +0800
> Yumei Huang <yuhuang@redhat.com> wrote:
> 
> > If no client is attached, discard outgoing frames and report them as
> > sent. This mimics the behavior of a physical host with its network
> > cable unplugged.
> > 
> > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Yumei Huang <yuhuang@redhat.com>
> 
> Thanks, the fix itself obviously makes sense, but I have a few questions
> and comments:
> 
> - first off, what happens if we don't return early in tap_send_frames()?
>   Commit messages for fixes (assuming this is a fix) should always say
>   what concrete problem we had, what is going to be fixed, or if we're
>   not aware of any real issue but things are just fragile / wrong

Without this we will get an EBADF in either writev() (pasta) or
sendmsg() (passt).  That's basically harmless, but a bit ugly.
Explicitly catching this case results in behaviour that's probably a
bit clearer to debug if we hit it.

Putting that context in the commit message would be useful.

> - until a while ago, this couldn't happen at all. We were just blocking
>   the whole execution as long as the tap / guest / container interface
>   wasn't up and running.
> 
>   I wonder when this changed and if it makes sense to go back to the
>   previous behaviour. I had just a quick look and I wonder if I
>   accidentally broke this in c9b241346569 ("conf, passt, tap: Open
>   socket and PID files before switching UID/GID").
> 
>   Before that, main() would call tap_sock_init(), which would call
>   tap_sock_unix_open(), a blocking function.
> 
>   Should we make the whole thing blocking again? If not, is there
>   anything else that's breaking with that? Timers, other inputs, etc.

I don't think we can quite do that.  I'm not sure if it's the only
reason, but for vhost-user I believe we need the epoll loop up and
running before we have the tap connection fully set up, because we
need it to process the vhost-user control messages.  Laurent, can you
verify?

There are several different approaches we can take here.  I discussed
some with Yumei and suggested she take this one.  Here's some
reasoning (maybe this would also be useful in the commit message,
though it's rather bulky)

# Don't listen() until the tap connection is ready

 - It's not clear that the host rejecting the connection is better
   than the host accepting, then the connection stalling until the 
   guest is ready.
 - Would require substantial rework because we currently listen() as
   we parse the command line and don't store the information we'd need
   to do it later.

# Don't accept() until the tap connection is ready

 - To the peer, will behave basically the same as this patch - the
   host will complete the TCP handshake, then the connection will stall
   until the guest is ready.
 - More work to implement, because essentially every sock-side handler
   has to check fd_tap and abort early

# Drop packets in tap_send_frames(), but return 0

 - To the peer, would behave basically the same
 - Would make the TCP code do a bunch of busy work attempting to
   resend, probably to no avail
 - Handling of errors returned by tap_send_frames() is on the basis
   that it's probably a transient fault (buffer full) and we want to
   resend very soon.  That approach doesn't make sense for a missing
   guest.

>   I didn't really have time to investigate until now, I can try to
>   have another look soon though, unless you find out more meanwhile.
> 
> > ---
> >  tap.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tap.c b/tap.c
> > index 7ba6399..e01219d 100644
> > --- a/tap.c
> > +++ b/tap.c
> > @@ -507,13 +507,17 @@ static size_t tap_send_frames_passt(const struct ctx *c,
> >   * @iov must have total length @bufs_per_frame * @nframes, with each set of
> >   * @bufs_per_frame contiguous buffers representing a single frame.
> >   *
> > - * Return: number of frames actually sent
> > + * Return: number of frames actually sent, or accounted as sent
> >   */
> >  size_t tap_send_frames(const struct ctx *c, const struct iovec *iov,
> >  		       size_t bufs_per_frame, size_t nframes)
> >  {
> >  	size_t m;
> >  
> > +	if (c->fd_tap == -1)
> > +		/* If no client connected, account the frames have been sent */
> 
> I think the comment is redundant because, well, if c->fd_tap is -1
> (obvious, documented), we return 'nframes' (also documented).
> 
> If it's not redundant, for any reason, "to account" in this sense
> isn't transitive. You could say: "consider that the frames have been
> sent" but not "account that the frames have been sent".
> 
> You can pick a different meaning of "to account" and say "account the
> frames as sent", though.

It's an amusing truth of the passt project that you'll get more
English usage notes from the Italian living in Germany than the native
English speaker living in an English speaking country :).

Fwiw, I agree that the comment can probably just be dropped.  If kept,
I'd suggest:
	If no client is connected, silently drop the frames

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tap: Drop frames if no client connected

[Yumei Huang]

Thank you David for helping explain all this!

On Fri, Sep 12, 2025 at 10:02 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Thu, Sep 11, 2025 at 11:54:25AM +0200, Stefano Brivio wrote:
> > On Thu, 11 Sep 2025 16:55:19 +0800
> > Yumei Huang <yuhuang@redhat.com> wrote:
> >
> > > If no client is attached, discard outgoing frames and report them as
> > > sent. This mimics the behavior of a physical host with its network
> > > cable unplugged.
> > >
> > > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > > Signed-off-by: Yumei Huang <yuhuang@redhat.com>
> >
> > Thanks, the fix itself obviously makes sense, but I have a few questions
> > and comments:
> >
> > - first off, what happens if we don't return early in tap_send_frames()?
> >   Commit messages for fixes (assuming this is a fix) should always say
> >   what concrete problem we had, what is going to be fixed, or if we're
> >   not aware of any real issue but things are just fragile / wrong
>
> Without this we will get an EBADF in either writev() (pasta) or
> sendmsg() (passt).  That's basically harmless, but a bit ugly.
> Explicitly catching this case results in behaviour that's probably a
> bit clearer to debug if we hit it.
>
> Putting that context in the commit message would be useful.
>
> > - until a while ago, this couldn't happen at all. We were just blocking
> >   the whole execution as long as the tap / guest / container interface
> >   wasn't up and running.
> >
> >   I wonder when this changed and if it makes sense to go back to the
> >   previous behaviour. I had just a quick look and I wonder if I
> >   accidentally broke this in c9b241346569 ("conf, passt, tap: Open
> >   socket and PID files before switching UID/GID").
BTW, I read the commit c9b241346569, it didn't change the behaviour.
Before this, tap_sock_init() will call tap_sock_unix_init() without
checking if c->fd_tap.
> >
> >   Before that, main() would call tap_sock_init(), which would call
> >   tap_sock_unix_open(), a blocking function.
> >
> >   Should we make the whole thing blocking again? If not, is there
> >   anything else that's breaking with that? Timers, other inputs, etc.
>
> I don't think we can quite do that.  I'm not sure if it's the only
> reason, but for vhost-user I believe we need the epoll loop up and
> running before we have the tap connection fully set up, because we
> need it to process the vhost-user control messages.  Laurent, can you
> verify?
>
> There are several different approaches we can take here.  I discussed
> some with Yumei and suggested she take this one.  Here's some
> reasoning (maybe this would also be useful in the commit message,
> though it's rather bulky)
>
> # Don't listen() until the tap connection is ready
>
>  - It's not clear that the host rejecting the connection is better
>    than the host accepting, then the connection stalling until the
>    guest is ready.
>  - Would require substantial rework because we currently listen() as
>    we parse the command line and don't store the information we'd need
>    to do it later.
>
> # Don't accept() until the tap connection is ready
>
>  - To the peer, will behave basically the same as this patch - the
>    host will complete the TCP handshake, then the connection will stall
>    until the guest is ready.
>  - More work to implement, because essentially every sock-side handler
>    has to check fd_tap and abort early
>
> # Drop packets in tap_send_frames(), but return 0
>
>  - To the peer, would behave basically the same
>  - Would make the TCP code do a bunch of busy work attempting to
>    resend, probably to no avail
>  - Handling of errors returned by tap_send_frames() is on the basis
>    that it's probably a transient fault (buffer full) and we want to
>    resend very soon.  That approach doesn't make sense for a missing
>    guest.
>
> >   I didn't really have time to investigate until now, I can try to
> >   have another look soon though, unless you find out more meanwhile.
> >
> > > ---
> > >  tap.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tap.c b/tap.c
> > > index 7ba6399..e01219d 100644
> > > --- a/tap.c
> > > +++ b/tap.c
> > > @@ -507,13 +507,17 @@ static size_t tap_send_frames_passt(const struct ctx *c,
> > >   * @iov must have total length @bufs_per_frame * @nframes, with each set of
> > >   * @bufs_per_frame contiguous buffers representing a single frame.
> > >   *
> > > - * Return: number of frames actually sent
> > > + * Return: number of frames actually sent, or accounted as sent
> > >   */
> > >  size_t tap_send_frames(const struct ctx *c, const struct iovec *iov,
> > >                    size_t bufs_per_frame, size_t nframes)
> > >  {
> > >     size_t m;
> > >
> > > +   if (c->fd_tap == -1)
> > > +           /* If no client connected, account the frames have been sent */
> >
> > I think the comment is redundant because, well, if c->fd_tap is -1
> > (obvious, documented), we return 'nframes' (also documented).
Agree, I will drop it in v2.
> >
> > If it's not redundant, for any reason, "to account" in this sense
> > isn't transitive. You could say: "consider that the frames have been
> > sent" but not "account that the frames have been sent".
> >
> > You can pick a different meaning of "to account" and say "account the
> > frames as sent", though.
>
> It's an amusing truth of the passt project that you'll get more
> English usage notes from the Italian living in Germany than the native
> English speaker living in an English speaking country :).
Haha, true! I’m still improving my English,  so the suggestions are
very helpful and much appreciated:)
>
> Fwiw, I agree that the comment can probably just be dropped.  If kept,
> I'd suggest:
>         If no client is connected, silently drop the frames
>
> --
> 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


-- 
Thanks,

Yumei Huang

Re: [PATCH] tap: Drop frames if no client connected

[Stefano Brivio]

On Fri, 12 Sep 2025 10:45:30 +0800
Yumei Huang <yuhuang@redhat.com> wrote:

> > On Thu, Sep 11, 2025 at 11:54:25AM +0200, Stefano Brivio wrote:  
> >
> > [...]
> >
> > > - until a while ago, this couldn't happen at all. We were just blocking
> > >   the whole execution as long as the tap / guest / container interface
> > >   wasn't up and running.
> > >
> > >   I wonder when this changed and if it makes sense to go back to the
> > >   previous behaviour. I had just a quick look and I wonder if I
> > >   accidentally broke this in c9b241346569 ("conf, passt, tap: Open
> > >   socket and PID files before switching UID/GID").  
> BTW, I read the commit c9b241346569, it didn't change the behaviour.
> Before this, tap_sock_init() will call tap_sock_unix_init() without
> checking if c->fd_tap.

Sure, it didn't check c->fd_tap, but was it blocking the whole
execution?

I'm not sure, I just know that at some point we were blocking
everything. As David mentioned it's not possible with vhost-user
anyway, so it doesn't matter so much.

-- 
Stefano

Re: [PATCH] tap: Drop frames if no client connected

[Stefano Brivio]

On Fri, 12 Sep 2025 12:01:37 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Sep 11, 2025 at 11:54:25AM +0200, Stefano Brivio wrote:
> > On Thu, 11 Sep 2025 16:55:19 +0800
> > Yumei Huang <yuhuang@redhat.com> wrote:
> >   
> > > If no client is attached, discard outgoing frames and report them as
> > > sent. This mimics the behavior of a physical host with its network
> > > cable unplugged.
> > > 
> > > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > > Signed-off-by: Yumei Huang <yuhuang@redhat.com>  
> > 
> > Thanks, the fix itself obviously makes sense, but I have a few questions
> > and comments:
> > 
> > - first off, what happens if we don't return early in tap_send_frames()?
> >   Commit messages for fixes (assuming this is a fix) should always say
> >   what concrete problem we had, what is going to be fixed, or if we're
> >   not aware of any real issue but things are just fragile / wrong  
> 
> Without this we will get an EBADF in either writev() (pasta) or
> sendmsg() (passt).  That's basically harmless, but a bit ugly.
> Explicitly catching this case results in behaviour that's probably a
> bit clearer to debug if we hit it.
> 
> Putting that context in the commit message would be useful.
> 
> > - until a while ago, this couldn't happen at all. We were just blocking
> >   the whole execution as long as the tap / guest / container interface
> >   wasn't up and running.
> > 
> >   I wonder when this changed and if it makes sense to go back to the
> >   previous behaviour. I had just a quick look and I wonder if I
> >   accidentally broke this in c9b241346569 ("conf, passt, tap: Open
> >   socket and PID files before switching UID/GID").
> > 
> >   Before that, main() would call tap_sock_init(), which would call
> >   tap_sock_unix_open(), a blocking function.
> > 
> >   Should we make the whole thing blocking again? If not, is there
> >   anything else that's breaking with that? Timers, other inputs, etc.  
> 
> I don't think we can quite do that.  I'm not sure if it's the only
> reason, but for vhost-user I believe we need the epoll loop up and
> running before we have the tap connection fully set up, because we
> need it to process the vhost-user control messages.  Laurent, can you
> verify?

We discussed this in the past, before realising that the execution
continues for whatever reason, and probably before I broke the
assumption that guest connection was blocking.

Yes, in the vhost-user case, the epoll loop needs to run before we have
a working connection to the guest, but:

- we can anyway block until the control socket is set up (we used to do
  that)

- the vhost-user implementation autonomously throws data away received
  before that point

Now, I don't think we necessarily need to stick to that approach, it
was the obvious choice when passt was much simpler, and it keeps things
simple in the sense that we don't need to care about cases like the
ones this patch is addressing.

On the other hand, if we want to switch to a different model, we need
to have a look at other possible breakages, I guess.

> There are several different approaches we can take here.  I discussed
> some with Yumei and suggested she take this one.  Here's some
> reasoning (maybe this would also be useful in the commit message,
> though it's rather bulky)
> 
> # Don't listen() until the tap connection is ready
> 
>  - It's not clear that the host rejecting the connection is better
>    than the host accepting, then the connection stalling until the 
>    guest is ready.
>  - Would require substantial rework because we currently listen() as
>    we parse the command line and don't store the information we'd need
>    to do it later.

Right, that looks like a lot of effort for nothing.

> # Don't accept() until the tap connection is ready
> 
>  - To the peer, will behave basically the same as this patch - the
>    host will complete the TCP handshake, then the connection will stall
>    until the guest is ready.

Same here.

>  - More work to implement, because essentially every sock-side handler
>    has to check fd_tap and abort early

There's one substantial issue at TCP level, though, that we're keeping
with the current approach and with this patch: we'll accept inbound
connections and silently stall them.

We could mitigate that by making the TCP handler aware of this, and by
resetting the connection if the guest isn't there. This would at least
be consistent with the case where the guest isn't listening on the port
(we accept(), fail to connect to it, eventually call tcp_rst()).

If we don't do this, I think we should at least check what happens in
terms of race conditions between passt starting and the guest appearing
and accepting the connection. I guess we'll retry for a bit, which is
desirable, but we should check that the whole retrying thing actually
works.

That's because the current approach just happened by accident.

> # Drop packets in tap_send_frames(), but return 0
> 
>  - To the peer, would behave basically the same
>  - Would make the TCP code do a bunch of busy work attempting to
>    resend, probably to no avail

Right, that's something we certainly want to avoid.

>  - Handling of errors returned by tap_send_frames() is on the basis
>    that it's probably a transient fault (buffer full) and we want to
>    resend very soon.  That approach doesn't make sense for a missing
>    guest.
> 
> >   I didn't really have time to investigate until now, I can try to
> >   have another look soon though, unless you find out more meanwhile.
> >   
> > > ---
> > >  tap.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tap.c b/tap.c
> > > index 7ba6399..e01219d 100644
> > > --- a/tap.c
> > > +++ b/tap.c
> > > @@ -507,13 +507,17 @@ static size_t tap_send_frames_passt(const struct ctx *c,
> > >   * @iov must have total length @bufs_per_frame * @nframes, with each set of
> > >   * @bufs_per_frame contiguous buffers representing a single frame.
> > >   *
> > > - * Return: number of frames actually sent
> > > + * Return: number of frames actually sent, or accounted as sent
> > >   */
> > >  size_t tap_send_frames(const struct ctx *c, const struct iovec *iov,
> > >  		       size_t bufs_per_frame, size_t nframes)
> > >  {
> > >  	size_t m;
> > >  
> > > +	if (c->fd_tap == -1)
> > > +		/* If no client connected, account the frames have been sent */  
> > 
> > I think the comment is redundant because, well, if c->fd_tap is -1
> > (obvious, documented), we return 'nframes' (also documented).
> > 
> > If it's not redundant, for any reason, "to account" in this sense
> > isn't transitive. You could say: "consider that the frames have been
> > sent" but not "account that the frames have been sent".
> > 
> > You can pick a different meaning of "to account" and say "account the
> > frames as sent", though.  
> 
> It's an amusing truth of the passt project that you'll get more
> English usage notes from the Italian living in Germany than the native
> English speaker living in an English speaking country :).

I'd rather call that pedantry than usage note, and it's so bad that we
ask gcc to align with that:

  https://passt.top/passt/tree/Makefile#n34

:) ...this comment itself, is, of course, -pedantic.

> Fwiw, I agree that the comment can probably just be dropped.  If kept,
> I'd suggest:
> 	If no client is connected, silently drop the frames

-- 
Stefano

Re: [PATCH] tap: Drop frames if no client connected

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8586 bytes --]

On Mon, Sep 15, 2025 at 08:13:19AM +0200, Stefano Brivio wrote:
> On Fri, 12 Sep 2025 12:01:37 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Sep 11, 2025 at 11:54:25AM +0200, Stefano Brivio wrote:
> > > On Thu, 11 Sep 2025 16:55:19 +0800
> > > Yumei Huang <yuhuang@redhat.com> wrote:
> > >   
> > > > If no client is attached, discard outgoing frames and report them as
> > > > sent. This mimics the behavior of a physical host with its network
> > > > cable unplugged.
> > > > 
> > > > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > > > Signed-off-by: Yumei Huang <yuhuang@redhat.com>  
> > > 
> > > Thanks, the fix itself obviously makes sense, but I have a few questions
> > > and comments:
> > > 
> > > - first off, what happens if we don't return early in tap_send_frames()?
> > >   Commit messages for fixes (assuming this is a fix) should always say
> > >   what concrete problem we had, what is going to be fixed, or if we're
> > >   not aware of any real issue but things are just fragile / wrong  
> > 
> > Without this we will get an EBADF in either writev() (pasta) or
> > sendmsg() (passt).  That's basically harmless, but a bit ugly.
> > Explicitly catching this case results in behaviour that's probably a
> > bit clearer to debug if we hit it.
> > 
> > Putting that context in the commit message would be useful.
> > 
> > > - until a while ago, this couldn't happen at all. We were just blocking
> > >   the whole execution as long as the tap / guest / container interface
> > >   wasn't up and running.
> > > 
> > >   I wonder when this changed and if it makes sense to go back to the
> > >   previous behaviour. I had just a quick look and I wonder if I
> > >   accidentally broke this in c9b241346569 ("conf, passt, tap: Open
> > >   socket and PID files before switching UID/GID").
> > > 
> > >   Before that, main() would call tap_sock_init(), which would call
> > >   tap_sock_unix_open(), a blocking function.
> > > 
> > >   Should we make the whole thing blocking again? If not, is there
> > >   anything else that's breaking with that? Timers, other inputs, etc.  
> > 
> > I don't think we can quite do that.  I'm not sure if it's the only
> > reason, but for vhost-user I believe we need the epoll loop up and
> > running before we have the tap connection fully set up, because we
> > need it to process the vhost-user control messages.  Laurent, can you
> > verify?
> 
> We discussed this in the past, before realising that the execution
> continues for whatever reason, and probably before I broke the
> assumption that guest connection was blocking.
> 
> Yes, in the vhost-user case, the epoll loop needs to run before we have
> a working connection to the guest, but:
> 
> - we can anyway block until the control socket is set up (we used to do
>   that)

The vhost-user control socket?  I'm not entirely sure what you mean by
"block" here.  Since we need the epoll loop up, I don't see how we can
block in the conventional sense.

> - the vhost-user implementation autonomously throws data away received
>   before that point

Right.  It doesn't have anywhere to put it, so it doesn't have much
choice.

> Now, I don't think we necessarily need to stick to that approach, it
> was the obvious choice when passt was much simpler, and it keeps things
> simple in the sense that we don't need to care about cases like the
> ones this patch is addressing.
> 
> On the other hand, if we want to switch to a different model, we need
> to have a look at other possible breakages, I guess.
> 
> > There are several different approaches we can take here.  I discussed
> > some with Yumei and suggested she take this one.  Here's some
> > reasoning (maybe this would also be useful in the commit message,
> > though it's rather bulky)
> > 
> > # Don't listen() until the tap connection is ready
> > 
> >  - It's not clear that the host rejecting the connection is better
> >    than the host accepting, then the connection stalling until the 
> >    guest is ready.
> >  - Would require substantial rework because we currently listen() as
> >    we parse the command line and don't store the information we'd need
> >    to do it later.
> 
> Right, that looks like a lot of effort for nothing.
> 
> > # Don't accept() until the tap connection is ready
> > 
> >  - To the peer, will behave basically the same as this patch - the
> >    host will complete the TCP handshake, then the connection will stall
> >    until the guest is ready.
> 
> Same here.
> 
> >  - More work to implement, because essentially every sock-side handler
> >    has to check fd_tap and abort early
> 
> There's one substantial issue at TCP level, though, that we're keeping
> with the current approach and with this patch: we'll accept inbound
> connections and silently stall them.
> 
> We could mitigate that by making the TCP handler aware of this, and by
> resetting the connection if the guest isn't there. This would at least
> be consistent with the case where the guest isn't listening on the port
> (we accept(), fail to connect to it, eventually call tcp_rst()).

True.  Arguably less consistent with a non-passt-connected peer that's
not there though.  Plus with the silently stall approach we have a
chance that the TCP connection will recover if the guest attaches
reasonably soon.

> If we don't do this, I think we should at least check what happens in
> terms of race conditions between passt starting and the guest appearing
> and accepting the connection. I guess we'll retry for a bit, which is
> desirable, but we should check that the whole retrying thing actually
> works.
> 
> That's because the current approach just happened by accident.

Right.  I'm not entirely sure what concrete action you're suggesting
at this point, though.

> > # Drop packets in tap_send_frames(), but return 0
> > 
> >  - To the peer, would behave basically the same
> >  - Would make the TCP code do a bunch of busy work attempting to
> >    resend, probably to no avail
> 
> Right, that's something we certainly want to avoid.
> 
> >  - Handling of errors returned by tap_send_frames() is on the basis
> >    that it's probably a transient fault (buffer full) and we want to
> >    resend very soon.  That approach doesn't make sense for a missing
> >    guest.
> > 
> > >   I didn't really have time to investigate until now, I can try to
> > >   have another look soon though, unless you find out more meanwhile.
> > >   
> > > > ---
> > > >  tap.c | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tap.c b/tap.c
> > > > index 7ba6399..e01219d 100644
> > > > --- a/tap.c
> > > > +++ b/tap.c
> > > > @@ -507,13 +507,17 @@ static size_t tap_send_frames_passt(const struct ctx *c,
> > > >   * @iov must have total length @bufs_per_frame * @nframes, with each set of
> > > >   * @bufs_per_frame contiguous buffers representing a single frame.
> > > >   *
> > > > - * Return: number of frames actually sent
> > > > + * Return: number of frames actually sent, or accounted as sent
> > > >   */
> > > >  size_t tap_send_frames(const struct ctx *c, const struct iovec *iov,
> > > >  		       size_t bufs_per_frame, size_t nframes)
> > > >  {
> > > >  	size_t m;
> > > >  
> > > > +	if (c->fd_tap == -1)
> > > > +		/* If no client connected, account the frames have been sent */  
> > > 
> > > I think the comment is redundant because, well, if c->fd_tap is -1
> > > (obvious, documented), we return 'nframes' (also documented).
> > > 
> > > If it's not redundant, for any reason, "to account" in this sense
> > > isn't transitive. You could say: "consider that the frames have been
> > > sent" but not "account that the frames have been sent".
> > > 
> > > You can pick a different meaning of "to account" and say "account the
> > > frames as sent", though.  
> > 
> > It's an amusing truth of the passt project that you'll get more
> > English usage notes from the Italian living in Germany than the native
> > English speaker living in an English speaking country :).
> 
> I'd rather call that pedantry than usage note, and it's so bad that we
> ask gcc to align with that:

So would I, but I was being polite :-p.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tap: Drop frames if no client connected

[Stefano Brivio]

On Thu, 18 Sep 2025 14:28:37 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Sep 15, 2025 at 08:13:19AM +0200, Stefano Brivio wrote:
> > On Fri, 12 Sep 2025 12:01:37 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Thu, Sep 11, 2025 at 11:54:25AM +0200, Stefano Brivio wrote:  
> > > > On Thu, 11 Sep 2025 16:55:19 +0800
> > > > Yumei Huang <yuhuang@redhat.com> wrote:
> > > >     
> > > > > If no client is attached, discard outgoing frames and report them as
> > > > > sent. This mimics the behavior of a physical host with its network
> > > > > cable unplugged.
> > > > > 
> > > > > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > Signed-off-by: Yumei Huang <yuhuang@redhat.com>    
> > > > 
> > > > Thanks, the fix itself obviously makes sense, but I have a few questions
> > > > and comments:
> > > > 
> > > > - first off, what happens if we don't return early in tap_send_frames()?
> > > >   Commit messages for fixes (assuming this is a fix) should always say
> > > >   what concrete problem we had, what is going to be fixed, or if we're
> > > >   not aware of any real issue but things are just fragile / wrong    
> > > 
> > > Without this we will get an EBADF in either writev() (pasta) or
> > > sendmsg() (passt).  That's basically harmless, but a bit ugly.
> > > Explicitly catching this case results in behaviour that's probably a
> > > bit clearer to debug if we hit it.
> > > 
> > > Putting that context in the commit message would be useful.
> > >   
> > > > - until a while ago, this couldn't happen at all. We were just blocking
> > > >   the whole execution as long as the tap / guest / container interface
> > > >   wasn't up and running.
> > > > 
> > > >   I wonder when this changed and if it makes sense to go back to the
> > > >   previous behaviour. I had just a quick look and I wonder if I
> > > >   accidentally broke this in c9b241346569 ("conf, passt, tap: Open
> > > >   socket and PID files before switching UID/GID").
> > > > 
> > > >   Before that, main() would call tap_sock_init(), which would call
> > > >   tap_sock_unix_open(), a blocking function.
> > > > 
> > > >   Should we make the whole thing blocking again? If not, is there
> > > >   anything else that's breaking with that? Timers, other inputs, etc.    
> > > 
> > > I don't think we can quite do that.  I'm not sure if it's the only
> > > reason, but for vhost-user I believe we need the epoll loop up and
> > > running before we have the tap connection fully set up, because we
> > > need it to process the vhost-user control messages.  Laurent, can you
> > > verify?  
> > 
> > We discussed this in the past, before realising that the execution
> > continues for whatever reason, and probably before I broke the
> > assumption that guest connection was blocking.
> > 
> > Yes, in the vhost-user case, the epoll loop needs to run before we have
> > a working connection to the guest, but:
> > 
> > - we can anyway block until the control socket is set up (we used to do
> >   that)  
> 
> The vhost-user control socket?  I'm not entirely sure what you mean by
> "block" here.  Since we need the epoll loop up, I don't see how we can
> block in the conventional sense.

Let's rather say "until the data setup is complete".

And by "block", I mean we would ignore any other event, obviously we
have to listen to the control socket (in the main loop or in a separate
dedicated loop).

I'm not suggesting we do this though (see below). It's just a
possibility.

> 
> > - the vhost-user implementation autonomously throws data away received
> >   before that point  
> 
> Right.  It doesn't have anywhere to put it, so it doesn't have much
> choice.
> 
> > Now, I don't think we necessarily need to stick to that approach, it
> > was the obvious choice when passt was much simpler, and it keeps things
> > simple in the sense that we don't need to care about cases like the
> > ones this patch is addressing.
> > 
> > On the other hand, if we want to switch to a different model, we need
> > to have a look at other possible breakages, I guess.
> >   
> > > There are several different approaches we can take here.  I discussed
> > > some with Yumei and suggested she take this one.  Here's some
> > > reasoning (maybe this would also be useful in the commit message,
> > > though it's rather bulky)
> > > 
> > > # Don't listen() until the tap connection is ready
> > > 
> > >  - It's not clear that the host rejecting the connection is better
> > >    than the host accepting, then the connection stalling until the 
> > >    guest is ready.
> > >  - Would require substantial rework because we currently listen() as
> > >    we parse the command line and don't store the information we'd need
> > >    to do it later.  
> > 
> > Right, that looks like a lot of effort for nothing.
> >   
> > > # Don't accept() until the tap connection is ready
> > > 
> > >  - To the peer, will behave basically the same as this patch - the
> > >    host will complete the TCP handshake, then the connection will stall
> > >    until the guest is ready.  
> > 
> > Same here.
> >   
> > >  - More work to implement, because essentially every sock-side handler
> > >    has to check fd_tap and abort early  
> > 
> > There's one substantial issue at TCP level, though, that we're keeping
> > with the current approach and with this patch: we'll accept inbound
> > connections and silently stall them.
> > 
> > We could mitigate that by making the TCP handler aware of this, and by
> > resetting the connection if the guest isn't there. This would at least
> > be consistent with the case where the guest isn't listening on the port
> > (we accept(), fail to connect to it, eventually call tcp_rst()).  
> 
> True.  Arguably less consistent with a non-passt-connected peer that's
> not there though.  Plus with the silently stall approach we have a
> chance that the TCP connection will recover if the guest attaches
> reasonably soon.
> 
> > If we don't do this, I think we should at least check what happens in
> > terms of race conditions between passt starting and the guest appearing
> > and accepting the connection. I guess we'll retry for a bit, which is
> > desirable, but we should check that the whole retrying thing actually
> > works.
> > 
> > That's because the current approach just happened by accident.  
> 
> Right.  I'm not entirely sure what concrete action you're suggesting
> at this point, though.

What I suggested in Monday's call and seemed to be all agreed upon, and
also mentioned above: *check what happens*.

Try that case, with this patch.

Does it work to cover situations where users might start passt a bit
before the guest connects, and try to connect to services right away?

I suggested using ssh which should have a quite long timeout and retry
connecting for a while. You mentioned you would assist Yumei in testing
this if needed.

-- 
Stefano

Re: [PATCH] tap: Drop frames if no client connected

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8429 bytes --]

On Thu, Sep 18, 2025 at 09:17:14AM +0200, Stefano Brivio wrote:
> On Thu, 18 Sep 2025 14:28:37 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Sep 15, 2025 at 08:13:19AM +0200, Stefano Brivio wrote:
> > > On Fri, 12 Sep 2025 12:01:37 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Thu, Sep 11, 2025 at 11:54:25AM +0200, Stefano Brivio wrote:  
> > > > > On Thu, 11 Sep 2025 16:55:19 +0800
> > > > > Yumei Huang <yuhuang@redhat.com> wrote:
> > > > >     
> > > > > > If no client is attached, discard outgoing frames and report them as
> > > > > > sent. This mimics the behavior of a physical host with its network
> > > > > > cable unplugged.
> > > > > > 
> > > > > > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > > Signed-off-by: Yumei Huang <yuhuang@redhat.com>    
> > > > > 
> > > > > Thanks, the fix itself obviously makes sense, but I have a few questions
> > > > > and comments:
> > > > > 
> > > > > - first off, what happens if we don't return early in tap_send_frames()?
> > > > >   Commit messages for fixes (assuming this is a fix) should always say
> > > > >   what concrete problem we had, what is going to be fixed, or if we're
> > > > >   not aware of any real issue but things are just fragile / wrong    
> > > > 
> > > > Without this we will get an EBADF in either writev() (pasta) or
> > > > sendmsg() (passt).  That's basically harmless, but a bit ugly.
> > > > Explicitly catching this case results in behaviour that's probably a
> > > > bit clearer to debug if we hit it.
> > > > 
> > > > Putting that context in the commit message would be useful.
> > > >   
> > > > > - until a while ago, this couldn't happen at all. We were just blocking
> > > > >   the whole execution as long as the tap / guest / container interface
> > > > >   wasn't up and running.
> > > > > 
> > > > >   I wonder when this changed and if it makes sense to go back to the
> > > > >   previous behaviour. I had just a quick look and I wonder if I
> > > > >   accidentally broke this in c9b241346569 ("conf, passt, tap: Open
> > > > >   socket and PID files before switching UID/GID").
> > > > > 
> > > > >   Before that, main() would call tap_sock_init(), which would call
> > > > >   tap_sock_unix_open(), a blocking function.
> > > > > 
> > > > >   Should we make the whole thing blocking again? If not, is there
> > > > >   anything else that's breaking with that? Timers, other inputs, etc.    
> > > > 
> > > > I don't think we can quite do that.  I'm not sure if it's the only
> > > > reason, but for vhost-user I believe we need the epoll loop up and
> > > > running before we have the tap connection fully set up, because we
> > > > need it to process the vhost-user control messages.  Laurent, can you
> > > > verify?  
> > > 
> > > We discussed this in the past, before realising that the execution
> > > continues for whatever reason, and probably before I broke the
> > > assumption that guest connection was blocking.
> > > 
> > > Yes, in the vhost-user case, the epoll loop needs to run before we have
> > > a working connection to the guest, but:
> > > 
> > > - we can anyway block until the control socket is set up (we used to do
> > >   that)  
> > 
> > The vhost-user control socket?  I'm not entirely sure what you mean by
> > "block" here.  Since we need the epoll loop up, I don't see how we can
> > block in the conventional sense.
> 
> Let's rather say "until the data setup is complete".
> 
> And by "block", I mean we would ignore any other event, obviously we
> have to listen to the control socket (in the main loop or in a separate
> dedicated loop).

Ok.  We could do that.  I don't think the peer visible behaviour would
really be different from what we get now silently dropping frames to
tap.  I'm not convinced it's really simpler than the current approach
either:

 * For now, we could just skip all epoll handling if the event type
   isn't the control socket, but we'd need to be finer grained about
   this if we add anything else that needs handling before guest
   connection (e.g. dynamic configuration update mechanism and/or
   netlink monitor)

 * Ignoring events in that way could lead to us busy-looping on epoll,
   because we might not clear events.  So we're back to having to
   consider every event type, at least to some extent.

> I'm not suggesting we do this though (see below). It's just a
> possibility.
> 
> > 
> > > - the vhost-user implementation autonomously throws data away received
> > >   before that point  
> > 
> > Right.  It doesn't have anywhere to put it, so it doesn't have much
> > choice.
> > 
> > > Now, I don't think we necessarily need to stick to that approach, it
> > > was the obvious choice when passt was much simpler, and it keeps things
> > > simple in the sense that we don't need to care about cases like the
> > > ones this patch is addressing.
> > > 
> > > On the other hand, if we want to switch to a different model, we need
> > > to have a look at other possible breakages, I guess.
> > >   
> > > > There are several different approaches we can take here.  I discussed
> > > > some with Yumei and suggested she take this one.  Here's some
> > > > reasoning (maybe this would also be useful in the commit message,
> > > > though it's rather bulky)
> > > > 
> > > > # Don't listen() until the tap connection is ready
> > > > 
> > > >  - It's not clear that the host rejecting the connection is better
> > > >    than the host accepting, then the connection stalling until the 
> > > >    guest is ready.
> > > >  - Would require substantial rework because we currently listen() as
> > > >    we parse the command line and don't store the information we'd need
> > > >    to do it later.  
> > > 
> > > Right, that looks like a lot of effort for nothing.
> > >   
> > > > # Don't accept() until the tap connection is ready
> > > > 
> > > >  - To the peer, will behave basically the same as this patch - the
> > > >    host will complete the TCP handshake, then the connection will stall
> > > >    until the guest is ready.  
> > > 
> > > Same here.
> > >   
> > > >  - More work to implement, because essentially every sock-side handler
> > > >    has to check fd_tap and abort early  
> > > 
> > > There's one substantial issue at TCP level, though, that we're keeping
> > > with the current approach and with this patch: we'll accept inbound
> > > connections and silently stall them.
> > > 
> > > We could mitigate that by making the TCP handler aware of this, and by
> > > resetting the connection if the guest isn't there. This would at least
> > > be consistent with the case where the guest isn't listening on the port
> > > (we accept(), fail to connect to it, eventually call tcp_rst()).  
> > 
> > True.  Arguably less consistent with a non-passt-connected peer that's
> > not there though.  Plus with the silently stall approach we have a
> > chance that the TCP connection will recover if the guest attaches
> > reasonably soon.
> > 
> > > If we don't do this, I think we should at least check what happens in
> > > terms of race conditions between passt starting and the guest appearing
> > > and accepting the connection. I guess we'll retry for a bit, which is
> > > desirable, but we should check that the whole retrying thing actually
> > > works.
> > > 
> > > That's because the current approach just happened by accident.  
> > 
> > Right.  I'm not entirely sure what concrete action you're suggesting
> > at this point, though.
> 
> What I suggested in Monday's call and seemed to be all agreed upon, and
> also mentioned above: *check what happens*.
> 
> Try that case, with this patch.
> 
> Does it work to cover situations where users might start passt a bit
> before the guest connects, and try to connect to services right away?
> 
> I suggested using ssh which should have a quite long timeout and retry
> connecting for a while. You mentioned you would assist Yumei in testing
> this if needed.

Ah, yes, you're right and I'd forgotten that.  Following up today.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tap: Drop frames if no client connected

[Yumei Huang]

On Fri, Sep 19, 2025 at 9:38 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Thu, Sep 18, 2025 at 09:17:14AM +0200, Stefano Brivio wrote:
> > On Thu, 18 Sep 2025 14:28:37 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > On Mon, Sep 15, 2025 at 08:13:19AM +0200, Stefano Brivio wrote:
> > > > On Fri, 12 Sep 2025 12:01:37 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >
> > > > > On Thu, Sep 11, 2025 at 11:54:25AM +0200, Stefano Brivio wrote:
> > > > > > On Thu, 11 Sep 2025 16:55:19 +0800
> > > > > > Yumei Huang <yuhuang@redhat.com> wrote:
> > > > > >
> > > > > > > If no client is attached, discard outgoing frames and report them as
> > > > > > > sent. This mimics the behavior of a physical host with its network
> > > > > > > cable unplugged.
> > > > > > >
> > > > > > > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > > > Signed-off-by: Yumei Huang <yuhuang@redhat.com>
> > > > > >
> > > > > > Thanks, the fix itself obviously makes sense, but I have a few questions
> > > > > > and comments:
> > > > > >
> > > > > > - first off, what happens if we don't return early in tap_send_frames()?
> > > > > >   Commit messages for fixes (assuming this is a fix) should always say
> > > > > >   what concrete problem we had, what is going to be fixed, or if we're
> > > > > >   not aware of any real issue but things are just fragile / wrong
> > > > >
> > > > > Without this we will get an EBADF in either writev() (pasta) or
> > > > > sendmsg() (passt).  That's basically harmless, but a bit ugly.
> > > > > Explicitly catching this case results in behaviour that's probably a
> > > > > bit clearer to debug if we hit it.
> > > > >
> > > > > Putting that context in the commit message would be useful.
> > > > >
> > > > > > - until a while ago, this couldn't happen at all. We were just blocking
> > > > > >   the whole execution as long as the tap / guest / container interface
> > > > > >   wasn't up and running.
> > > > > >
> > > > > >   I wonder when this changed and if it makes sense to go back to the
> > > > > >   previous behaviour. I had just a quick look and I wonder if I
> > > > > >   accidentally broke this in c9b241346569 ("conf, passt, tap: Open
> > > > > >   socket and PID files before switching UID/GID").
> > > > > >
> > > > > >   Before that, main() would call tap_sock_init(), which would call
> > > > > >   tap_sock_unix_open(), a blocking function.
> > > > > >
> > > > > >   Should we make the whole thing blocking again? If not, is there
> > > > > >   anything else that's breaking with that? Timers, other inputs, etc.
> > > > >
> > > > > I don't think we can quite do that.  I'm not sure if it's the only
> > > > > reason, but for vhost-user I believe we need the epoll loop up and
> > > > > running before we have the tap connection fully set up, because we
> > > > > need it to process the vhost-user control messages.  Laurent, can you
> > > > > verify?
> > > >
> > > > We discussed this in the past, before realising that the execution
> > > > continues for whatever reason, and probably before I broke the
> > > > assumption that guest connection was blocking.
> > > >
> > > > Yes, in the vhost-user case, the epoll loop needs to run before we have
> > > > a working connection to the guest, but:
> > > >
> > > > - we can anyway block until the control socket is set up (we used to do
> > > >   that)
> > >
> > > The vhost-user control socket?  I'm not entirely sure what you mean by
> > > "block" here.  Since we need the epoll loop up, I don't see how we can
> > > block in the conventional sense.
> >
> > Let's rather say "until the data setup is complete".
> >
> > And by "block", I mean we would ignore any other event, obviously we
> > have to listen to the control socket (in the main loop or in a separate
> > dedicated loop).
>
> Ok.  We could do that.  I don't think the peer visible behaviour would
> really be different from what we get now silently dropping frames to
> tap.  I'm not convinced it's really simpler than the current approach
> either:
>
>  * For now, we could just skip all epoll handling if the event type
>    isn't the control socket, but we'd need to be finer grained about
>    this if we add anything else that needs handling before guest
>    connection (e.g. dynamic configuration update mechanism and/or
>    netlink monitor)
>
>  * Ignoring events in that way could lead to us busy-looping on epoll,
>    because we might not clear events.  So we're back to having to
>    consider every event type, at least to some extent.
>
> > I'm not suggesting we do this though (see below). It's just a
> > possibility.
> >
> > >
> > > > - the vhost-user implementation autonomously throws data away received
> > > >   before that point
> > >
> > > Right.  It doesn't have anywhere to put it, so it doesn't have much
> > > choice.
> > >
> > > > Now, I don't think we necessarily need to stick to that approach, it
> > > > was the obvious choice when passt was much simpler, and it keeps things
> > > > simple in the sense that we don't need to care about cases like the
> > > > ones this patch is addressing.
> > > >
> > > > On the other hand, if we want to switch to a different model, we need
> > > > to have a look at other possible breakages, I guess.
> > > >
> > > > > There are several different approaches we can take here.  I discussed
> > > > > some with Yumei and suggested she take this one.  Here's some
> > > > > reasoning (maybe this would also be useful in the commit message,
> > > > > though it's rather bulky)
> > > > >
> > > > > # Don't listen() until the tap connection is ready
> > > > >
> > > > >  - It's not clear that the host rejecting the connection is better
> > > > >    than the host accepting, then the connection stalling until the
> > > > >    guest is ready.
> > > > >  - Would require substantial rework because we currently listen() as
> > > > >    we parse the command line and don't store the information we'd need
> > > > >    to do it later.
> > > >
> > > > Right, that looks like a lot of effort for nothing.
> > > >
> > > > > # Don't accept() until the tap connection is ready
> > > > >
> > > > >  - To the peer, will behave basically the same as this patch - the
> > > > >    host will complete the TCP handshake, then the connection will stall
> > > > >    until the guest is ready.
> > > >
> > > > Same here.
> > > >
> > > > >  - More work to implement, because essentially every sock-side handler
> > > > >    has to check fd_tap and abort early
> > > >
> > > > There's one substantial issue at TCP level, though, that we're keeping
> > > > with the current approach and with this patch: we'll accept inbound
> > > > connections and silently stall them.
> > > >
> > > > We could mitigate that by making the TCP handler aware of this, and by
> > > > resetting the connection if the guest isn't there. This would at least
> > > > be consistent with the case where the guest isn't listening on the port
> > > > (we accept(), fail to connect to it, eventually call tcp_rst()).
> > >
> > > True.  Arguably less consistent with a non-passt-connected peer that's
> > > not there though.  Plus with the silently stall approach we have a
> > > chance that the TCP connection will recover if the guest attaches
> > > reasonably soon.
> > >
> > > > If we don't do this, I think we should at least check what happens in
> > > > terms of race conditions between passt starting and the guest appearing
> > > > and accepting the connection. I guess we'll retry for a bit, which is
> > > > desirable, but we should check that the whole retrying thing actually
> > > > works.
> > > >
> > > > That's because the current approach just happened by accident.
> > >
> > > Right.  I'm not entirely sure what concrete action you're suggesting
> > > at this point, though.
> >
> > What I suggested in Monday's call and seemed to be all agreed upon, and
> > also mentioned above: *check what happens*.
> >
> > Try that case, with this patch.
> >
> > Does it work to cover situations where users might start passt a bit
> > before the guest connects, and try to connect to services right away?
> >
> > I suggested using ssh which should have a quite long timeout and retry
> > connecting for a while. You mentioned you would assist Yumei in testing
> > this if needed.
>
> Ah, yes, you're right and I'd forgotten that.  Following up today.

I tried both 'ssh' and 'socat'(writing a big file) before a guest
connects, they get a 'Connection reset' after 10s, even if the guest
connects in ~2s.
It's because, when start ssh or socat, passt would try to finish the
tcp handshake with the guest. It sends SYN to the guest immediately
and waits for SYN-ACK. However, the SYN frame is dropped/lost due to
no guest connected. So though the guest connects in seconds, the tcp
handshake would timeout, and returns rst via tcp_rst().
Either with or without this patch, they got the same 'connection reset'.
Maybe it's something to fix?
>
> --
> 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



-- 
Thanks,

Yumei Huang

Re: [PATCH] tap: Drop frames if no client connected

[Stefano Brivio]

On Mon, 22 Sep 2025 15:17:12 +0800
Yumei Huang <yuhuang@redhat.com> wrote:

> On Fri, Sep 19, 2025 at 9:38 AM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> >
> > On Thu, Sep 18, 2025 at 09:17:14AM +0200, Stefano Brivio wrote:  
> > > On Thu, 18 Sep 2025 14:28:37 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >  
> > > > On Mon, Sep 15, 2025 at 08:13:19AM +0200, Stefano Brivio wrote:  
> > > > > On Fri, 12 Sep 2025 12:01:37 +1000
> > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > >  
> > > > > > On Thu, Sep 11, 2025 at 11:54:25AM +0200, Stefano Brivio wrote:  
> > > > > > > On Thu, 11 Sep 2025 16:55:19 +0800
> > > > > > > Yumei Huang <yuhuang@redhat.com> wrote:
> > > > > > >  
> > > > > > > > If no client is attached, discard outgoing frames and report them as
> > > > > > > > sent. This mimics the behavior of a physical host with its network
> > > > > > > > cable unplugged.
> > > > > > > >
> > > > > > > > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > > > > Signed-off-by: Yumei Huang <yuhuang@redhat.com>  
> > > > > > >
> > > > > > > Thanks, the fix itself obviously makes sense, but I have a few questions
> > > > > > > and comments:
> > > > > > >
> > > > > > > - first off, what happens if we don't return early in tap_send_frames()?
> > > > > > >   Commit messages for fixes (assuming this is a fix) should always say
> > > > > > >   what concrete problem we had, what is going to be fixed, or if we're
> > > > > > >   not aware of any real issue but things are just fragile / wrong  
> > > > > >
> > > > > > Without this we will get an EBADF in either writev() (pasta) or
> > > > > > sendmsg() (passt).  That's basically harmless, but a bit ugly.
> > > > > > Explicitly catching this case results in behaviour that's probably a
> > > > > > bit clearer to debug if we hit it.
> > > > > >
> > > > > > Putting that context in the commit message would be useful.
> > > > > >  
> > > > > > > - until a while ago, this couldn't happen at all. We were just blocking
> > > > > > >   the whole execution as long as the tap / guest / container interface
> > > > > > >   wasn't up and running.
> > > > > > >
> > > > > > >   I wonder when this changed and if it makes sense to go back to the
> > > > > > >   previous behaviour. I had just a quick look and I wonder if I
> > > > > > >   accidentally broke this in c9b241346569 ("conf, passt, tap: Open
> > > > > > >   socket and PID files before switching UID/GID").
> > > > > > >
> > > > > > >   Before that, main() would call tap_sock_init(), which would call
> > > > > > >   tap_sock_unix_open(), a blocking function.
> > > > > > >
> > > > > > >   Should we make the whole thing blocking again? If not, is there
> > > > > > >   anything else that's breaking with that? Timers, other inputs, etc.  
> > > > > >
> > > > > > I don't think we can quite do that.  I'm not sure if it's the only
> > > > > > reason, but for vhost-user I believe we need the epoll loop up and
> > > > > > running before we have the tap connection fully set up, because we
> > > > > > need it to process the vhost-user control messages.  Laurent, can you
> > > > > > verify?  
> > > > >
> > > > > We discussed this in the past, before realising that the execution
> > > > > continues for whatever reason, and probably before I broke the
> > > > > assumption that guest connection was blocking.
> > > > >
> > > > > Yes, in the vhost-user case, the epoll loop needs to run before we have
> > > > > a working connection to the guest, but:
> > > > >
> > > > > - we can anyway block until the control socket is set up (we used to do
> > > > >   that)  
> > > >
> > > > The vhost-user control socket?  I'm not entirely sure what you mean by
> > > > "block" here.  Since we need the epoll loop up, I don't see how we can
> > > > block in the conventional sense.  
> > >
> > > Let's rather say "until the data setup is complete".
> > >
> > > And by "block", I mean we would ignore any other event, obviously we
> > > have to listen to the control socket (in the main loop or in a separate
> > > dedicated loop).  
> >
> > Ok.  We could do that.  I don't think the peer visible behaviour would
> > really be different from what we get now silently dropping frames to
> > tap.  I'm not convinced it's really simpler than the current approach
> > either:
> >
> >  * For now, we could just skip all epoll handling if the event type
> >    isn't the control socket, but we'd need to be finer grained about
> >    this if we add anything else that needs handling before guest
> >    connection (e.g. dynamic configuration update mechanism and/or
> >    netlink monitor)
> >
> >  * Ignoring events in that way could lead to us busy-looping on epoll,
> >    because we might not clear events.  So we're back to having to
> >    consider every event type, at least to some extent.
> >  
> > > I'm not suggesting we do this though (see below). It's just a
> > > possibility.
> > >  
> > > >  
> > > > > - the vhost-user implementation autonomously throws data away received
> > > > >   before that point  
> > > >
> > > > Right.  It doesn't have anywhere to put it, so it doesn't have much
> > > > choice.
> > > >  
> > > > > Now, I don't think we necessarily need to stick to that approach, it
> > > > > was the obvious choice when passt was much simpler, and it keeps things
> > > > > simple in the sense that we don't need to care about cases like the
> > > > > ones this patch is addressing.
> > > > >
> > > > > On the other hand, if we want to switch to a different model, we need
> > > > > to have a look at other possible breakages, I guess.
> > > > >  
> > > > > > There are several different approaches we can take here.  I discussed
> > > > > > some with Yumei and suggested she take this one.  Here's some
> > > > > > reasoning (maybe this would also be useful in the commit message,
> > > > > > though it's rather bulky)
> > > > > >
> > > > > > # Don't listen() until the tap connection is ready
> > > > > >
> > > > > >  - It's not clear that the host rejecting the connection is better
> > > > > >    than the host accepting, then the connection stalling until the
> > > > > >    guest is ready.
> > > > > >  - Would require substantial rework because we currently listen() as
> > > > > >    we parse the command line and don't store the information we'd need
> > > > > >    to do it later.  
> > > > >
> > > > > Right, that looks like a lot of effort for nothing.
> > > > >  
> > > > > > # Don't accept() until the tap connection is ready
> > > > > >
> > > > > >  - To the peer, will behave basically the same as this patch - the
> > > > > >    host will complete the TCP handshake, then the connection will stall
> > > > > >    until the guest is ready.  
> > > > >
> > > > > Same here.
> > > > >  
> > > > > >  - More work to implement, because essentially every sock-side handler
> > > > > >    has to check fd_tap and abort early  
> > > > >
> > > > > There's one substantial issue at TCP level, though, that we're keeping
> > > > > with the current approach and with this patch: we'll accept inbound
> > > > > connections and silently stall them.
> > > > >
> > > > > We could mitigate that by making the TCP handler aware of this, and by
> > > > > resetting the connection if the guest isn't there. This would at least
> > > > > be consistent with the case where the guest isn't listening on the port
> > > > > (we accept(), fail to connect to it, eventually call tcp_rst()).  
> > > >
> > > > True.  Arguably less consistent with a non-passt-connected peer that's
> > > > not there though.  Plus with the silently stall approach we have a
> > > > chance that the TCP connection will recover if the guest attaches
> > > > reasonably soon.
> > > >  
> > > > > If we don't do this, I think we should at least check what happens in
> > > > > terms of race conditions between passt starting and the guest appearing
> > > > > and accepting the connection. I guess we'll retry for a bit, which is
> > > > > desirable, but we should check that the whole retrying thing actually
> > > > > works.
> > > > >
> > > > > That's because the current approach just happened by accident.  
> > > >
> > > > Right.  I'm not entirely sure what concrete action you're suggesting
> > > > at this point, though.  
> > >
> > > What I suggested in Monday's call and seemed to be all agreed upon, and
> > > also mentioned above: *check what happens*.
> > >
> > > Try that case, with this patch.
> > >
> > > Does it work to cover situations where users might start passt a bit
> > > before the guest connects, and try to connect to services right away?
> > >
> > > I suggested using ssh which should have a quite long timeout and retry
> > > connecting for a while. You mentioned you would assist Yumei in testing
> > > this if needed.  
> >
> > Ah, yes, you're right and I'd forgotten that.  Following up today.  
> 
> I tried both 'ssh' and 'socat'(writing a big file) before a guest
> connects, they get a 'Connection reset' after 10s, even if the guest
> connects in ~2s.
> It's because, when start ssh or socat, passt would try to finish the
> tcp handshake with the guest. It sends SYN to the guest immediately
> and waits for SYN-ACK. However, the SYN frame is dropped/lost due to
> no guest connected. So though the guest connects in seconds, the tcp
> handshake would timeout, and returns rst via tcp_rst().

Ah, right. We won't try to resend the SYN, that's simply not
implemented.

The timeout you see is SYN_TIMEOUT, timer set by tcp_timer_ctl() and
handled by tcp_timer_handler().

> Either with or without this patch, they got the same 'connection reset'.
> Maybe it's something to fix?

First off, this shows that the current patch is harmless, so I would go
ahead and apply it (but see 2. below).

Strictly speaking, I don't think we really *need* to fix anything, but
for sure the behaviour isn't ideal. I see two alternatives:

1. we implement a periodic retry for the SYN segment. This would *seem*
   to give the best behaviour in this case, but:

   a. it's quite complicated (we need to calculate some delays for the
      timers, etc.), and not really transparent (which is in general a
      goal of passt)

   b. if the guest never appears, we're just wasting client's time. See
      db2c91ae86c7 ("tcp: Set ACK flag on *all* RST segments, even for
      client in SYN-SENT state") for an example where it's important to
      fail fast

   c. if the guest appears but isn't listening to the port, see b.

2. reset right away as I was suggesting in
   https://archives.passt.top/passt-dev/20250915081319.00e72e53@elisabeth/:

   > We could mitigate that by making the TCP handler aware of this, and by
   > resetting the connection if the guest isn't there. This would at least
   > be consistent with the case where the guest isn't listening on the port
   > (we accept(), fail to connect to it, eventually call tcp_rst()).

   and let the client retry as appropriate (if implemented). Those retries
   can be quite fast, see this report (from IRC) for 722d347c1932 ("tcp:
   Don't reset outbound connection on SYN retries"):

3.3223:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
3.3223:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
3.3224:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
3.3224:          Flow 0 (NEW): FREE -> NEW
3.3224:          Flow 0 (INI): NEW -> INI
3.3224:          Flow 0 (INI): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => ?
3.3224:          Flow 0 (TGT): INI -> TGT
3.3224:          Flow 0 (TGT): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
3.3224:          Flow 0 (TCP connection): TGT -> TYPED
3.3224:          Flow 0 (TCP connection): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
3.3224:          Flow 0 (TCP connection): event at tcp_conn_from_tap:1489
3.3224:          Flow 0 (TCP connection): TAP_SYN_RCVD: CLOSED -> SYN_SENT
3.3224:          Flow 0 (TCP connection): failed to set TCP_MAXSEG on socket 21
3.3224:          Flow 0 (TCP connection): Side 0 hash table insert: bucket: 294539
3.3225:          Flow 0 (TCP connection): TYPED -> ACTIVE
3.3225:          Flow 0 (TCP connection): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
4.0027:          pasta: epoll event on namespace timer watch 17 (events: 0x00000001)
4.3612:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
4.3613:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
4.3613:          Flow 0 (TCP connection): packet length 40 from tap
4.3613:          Flow 0 (TCP connection): TCP reset at tcp_tap_handler:1989
4.3613:          Flow 0 (TCP connection): flag at tcp_prepare_flags:1163
4.3613:          Flow 0 (TCP connection): event at tcp_rst_do:1206
4.3613:          Flow 0 (TCP connection): CLOSED: SYN_SENT -> CLOSED
4.3614:          Flow 0 (TCP connection): Side 0 hash table remove: bucket: 294539
4.3614:          Flow 0 (FREE): ACTIVE -> FREE
4.3614:          Flow 0 (FREE): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80

   ...the retry happened within one second. This is a container, so Linux
   kernel, and the client was wget.

So, in the end, I would suggest going with 2.: check if the guest /
container is connected in the TCP handler (tcp_data_from_sock()) and
reset the connection if it's not.

I would suggest checking that together with this patch. They would
still be two different patches, but I think it would be good to
check / test what happens with both of them.

-- 
Stefano

Re: [PATCH] tap: Drop frames if no client connected

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7809 bytes --]

On Mon, Sep 22, 2025 at 10:03:30PM +0200, Stefano Brivio wrote:
> On Mon, 22 Sep 2025 15:17:12 +0800
> Yumei Huang <yuhuang@redhat.com> wrote:
> > On Fri, Sep 19, 2025 at 9:38 AM David Gibson
> > <david@gibson.dropbear.id.au> wrote:
> > > On Thu, Sep 18, 2025 at 09:17:14AM +0200, Stefano Brivio wrote:  
> > > > On Thu, 18 Sep 2025 14:28:37 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
[snip]
> > > > Does it work to cover situations where users might start passt a bit
> > > > before the guest connects, and try to connect to services right away?
> > > >
> > > > I suggested using ssh which should have a quite long timeout and retry
> > > > connecting for a while. You mentioned you would assist Yumei in testing
> > > > this if needed.  
> > >
> > > Ah, yes, you're right and I'd forgotten that.  Following up today.  
> > 
> > I tried both 'ssh' and 'socat'(writing a big file) before a guest
> > connects, they get a 'Connection reset' after 10s, even if the guest
> > connects in ~2s.
> > It's because, when start ssh or socat, passt would try to finish the
> > tcp handshake with the guest. It sends SYN to the guest immediately
> > and waits for SYN-ACK. However, the SYN frame is dropped/lost due to
> > no guest connected. So though the guest connects in seconds, the tcp
> > handshake would timeout, and returns rst via tcp_rst().
> 
> Ah, right. We won't try to resend the SYN, that's simply not
> implemented.
> 
> The timeout you see is SYN_TIMEOUT, timer set by tcp_timer_ctl() and
> handled by tcp_timer_handler().
> 
> > Either with or without this patch, they got the same 'connection reset'.
> > Maybe it's something to fix?
> 
> First off, this shows that the current patch is harmless, so I would go
> ahead and apply it (but see 2. below).
> 
> Strictly speaking, I don't think we really *need* to fix anything, but
> for sure the behaviour isn't ideal. I see two alternatives:
> 
> 1. we implement a periodic retry for the SYN segment. This would *seem*
>    to give the best behaviour in this case, but:
> 
>    a. it's quite complicated (we need to calculate some delays for the
>       timers, etc.), and not really transparent (which is in general a
>       goal of passt)

I'm not really sure why you say it's not transparent, or at least what
other option you're comparing it to.  The peer has initiated a
connection to us in the normal way (which may include resending SYNs).
Now we're initiating a connection to the guest in the normal way
(which may include resending SYNs).

>    b. if the guest never appears, we're just wasting client's time. See
>       db2c91ae86c7 ("tcp: Set ACK flag on *all* RST segments, even for
>       client in SYN-SENT state") for an example where it's important to
>       fail fast

Sure.  I'd say RSTing here would be *less* transparent, but it might
still be worth it to make the peer fail fast.

>    c. if the guest appears but isn't listening to the port, see b.
> 
> 2. reset right away as I was suggesting in
>    https://archives.passt.top/passt-dev/20250915081319.00e72e53@elisabeth/:
> 
>    > We could mitigate that by making the TCP handler aware of this, and by
>    > resetting the connection if the guest isn't there. This would at least
>    > be consistent with the case where the guest isn't listening on the port
>    > (we accept(), fail to connect to it, eventually call tcp_rst()).
> 
>    and let the client retry as appropriate (if implemented). Those retries
>    can be quite fast, see this report (from IRC) for 722d347c1932 ("tcp:
>    Don't reset outbound connection on SYN retries"):

I don't see how that commit is relevant to this situation.  That's
talking about SYN retries.  We can see those in the case of outbound
connections bot we'll never see them for the case of inbound
connections, because the host kernel has already completed the
handshake.  For inbound we essentially have two options:

 a) Retry SYNs ourselves, emulating what the peer would do if it was
    talking directly to an absent guest.
 b) Reject SYNs quickly, trusting that the guest will have some sort of
    application level retry.  That will depend on the client.  I guess
    my fear here is that a client seeing a completed handshake + RST
    might assume that the guest server is permanently broken, rather
    than just temporarily missing as it might if there's no response at
    all.

I suggested Yumei's approach here to aim for (a) on the basis of
transparency - it's as close as I think we can get to a bridged guest
that's just missing.  I'm not necessarily opposed to (b), but I think
it's less transparent, so we need an argument that it will lead to
better outcomes regardless.

> 3.3223:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> 3.3223:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> 3.3224:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> 3.3224:          Flow 0 (NEW): FREE -> NEW
> 3.3224:          Flow 0 (INI): NEW -> INI
> 3.3224:          Flow 0 (INI): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => ?
> 3.3224:          Flow 0 (TGT): INI -> TGT
> 3.3224:          Flow 0 (TGT): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> 3.3224:          Flow 0 (TCP connection): TGT -> TYPED
> 3.3224:          Flow 0 (TCP connection): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> 3.3224:          Flow 0 (TCP connection): event at tcp_conn_from_tap:1489
> 3.3224:          Flow 0 (TCP connection): TAP_SYN_RCVD: CLOSED -> SYN_SENT
> 3.3224:          Flow 0 (TCP connection): failed to set TCP_MAXSEG on socket 21
> 3.3224:          Flow 0 (TCP connection): Side 0 hash table insert: bucket: 294539
> 3.3225:          Flow 0 (TCP connection): TYPED -> ACTIVE
> 3.3225:          Flow 0 (TCP connection): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> 4.0027:          pasta: epoll event on namespace timer watch 17 (events: 0x00000001)
> 4.3612:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> 4.3613:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> 4.3613:          Flow 0 (TCP connection): packet length 40 from tap
> 4.3613:          Flow 0 (TCP connection): TCP reset at tcp_tap_handler:1989
> 4.3613:          Flow 0 (TCP connection): flag at tcp_prepare_flags:1163
> 4.3613:          Flow 0 (TCP connection): event at tcp_rst_do:1206
> 4.3613:          Flow 0 (TCP connection): CLOSED: SYN_SENT -> CLOSED
> 4.3614:          Flow 0 (TCP connection): Side 0 hash table remove: bucket: 294539
> 4.3614:          Flow 0 (FREE): ACTIVE -> FREE
> 4.3614:          Flow 0 (FREE): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> 
>    ...the retry happened within one second. This is a container, so Linux
>    kernel, and the client was wget.

I'm not seeing a retry at all in this log, plus it's an outbound
connection, which is not the case we're dealing with here.

> So, in the end, I would suggest going with 2.: check if the guest /
> container is connected in the TCP handler (tcp_data_from_sock()) and
> reset the connection if it's not.
> 
> I would suggest checking that together with this patch. They would
> still be two different patches, but I think it would be good to
> check / test what happens with both of 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tap: Drop frames if no client connected

[Stefano Brivio]

On Tue, 23 Sep 2025 17:53:09 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Sep 22, 2025 at 10:03:30PM +0200, Stefano Brivio wrote:
> > On Mon, 22 Sep 2025 15:17:12 +0800
> > Yumei Huang <yuhuang@redhat.com> wrote:  
> > > On Fri, Sep 19, 2025 at 9:38 AM David Gibson
> > > <david@gibson.dropbear.id.au> wrote:  
> > > > On Thu, Sep 18, 2025 at 09:17:14AM +0200, Stefano Brivio wrote:    
> > > > > On Thu, 18 Sep 2025 14:28:37 +1000
> > > > > David Gibson <david@gibson.dropbear.id.au> wrote:  
> [snip]
> > > > > Does it work to cover situations where users might start passt a bit
> > > > > before the guest connects, and try to connect to services right away?
> > > > >
> > > > > I suggested using ssh which should have a quite long timeout and retry
> > > > > connecting for a while. You mentioned you would assist Yumei in testing
> > > > > this if needed.    
> > > >
> > > > Ah, yes, you're right and I'd forgotten that.  Following up today.    
> > > 
> > > I tried both 'ssh' and 'socat'(writing a big file) before a guest
> > > connects, they get a 'Connection reset' after 10s, even if the guest
> > > connects in ~2s.
> > > It's because, when start ssh or socat, passt would try to finish the
> > > tcp handshake with the guest. It sends SYN to the guest immediately
> > > and waits for SYN-ACK. However, the SYN frame is dropped/lost due to
> > > no guest connected. So though the guest connects in seconds, the tcp
> > > handshake would timeout, and returns rst via tcp_rst().  
> > 
> > Ah, right. We won't try to resend the SYN, that's simply not
> > implemented.
> > 
> > The timeout you see is SYN_TIMEOUT, timer set by tcp_timer_ctl() and
> > handled by tcp_timer_handler().
> >   
> > > Either with or without this patch, they got the same 'connection reset'.
> > > Maybe it's something to fix?  
> > 
> > First off, this shows that the current patch is harmless, so I would go
> > ahead and apply it (but see 2. below).
> > 
> > Strictly speaking, I don't think we really *need* to fix anything, but
> > for sure the behaviour isn't ideal. I see two alternatives:
> > 
> > 1. we implement a periodic retry for the SYN segment. This would *seem*
> >    to give the best behaviour in this case, but:
> > 
> >    a. it's quite complicated (we need to calculate some delays for the
> >       timers, etc.), and not really transparent (which is in general a
> >       goal of passt)  
> 
> I'm not really sure why you say it's not transparent, or at least what
> other option you're comparing it to.  The peer has initiated a
> connection to us in the normal way (which may include resending SYNs).
> Now we're initiating a connection to the guest in the normal way
> (which may include resending SYNs).

I was comparing this to b. or to doing nothing.

But, actually, you're right, the kernel wouldn't tell us about a
repeated SYN, it would still be the same socket returned from accept(),
so it's not necessarily less transparent.

I was thinking that we know when the guest connects, so we could just
delay the SYN segment until then, by introducing a separate TAP_SYN_SENT
event (right now it's implicit in SOCK_ACCEPTED). But when the guest
connects, services are typically not up yet. You would typically get a
RST while the guest is booting.

> >    b. if the guest never appears, we're just wasting client's time. See
> >       db2c91ae86c7 ("tcp: Set ACK flag on *all* RST segments, even for
> >       client in SYN-SENT state") for an example where it's important to
> >       fail fast  
> 
> Sure.  I'd say RSTing here would be *less* transparent, but it might
> still be worth it to make the peer fail fast.

But that's what happens naturally (with Linux) if nobody is listening,
and in RFC 9293 terms, I'd say we should approximate a CLOSED state,
3.10.7.1:

  If the state is CLOSED (i.e., TCB does not exist), then [...] [a]n
  incoming segment not containing a RST causes a RST to be sent in response.

rather than a LISTEN state (3.10.7.2). However, see below.

> > 2. reset right away as I was suggesting in
> >    https://archives.passt.top/passt-dev/20250915081319.00e72e53@elisabeth/:
> >   
> >    > We could mitigate that by making the TCP handler aware of this, and by
> >    > resetting the connection if the guest isn't there. This would at least
> >    > be consistent with the case where the guest isn't listening on the port
> >    > (we accept(), fail to connect to it, eventually call tcp_rst()).  
> > 
> >    and let the client retry as appropriate (if implemented). Those retries
> >    can be quite fast, see this report (from IRC) for 722d347c1932 ("tcp:
> >    Don't reset outbound connection on SYN retries"):  
> 
> I don't see how that commit is relevant to this situation.  That's
> talking about SYN retries.

That's just an example about how SYN segments are retried. It's not
otherwise relevant for this situation.

> We can see those in the case of outbound
> connections bot we'll never see them for the case of inbound
> connections, because the host kernel has already completed the
> handshake.  For inbound we essentially have two options:
> 
>  a) Retry SYNs ourselves, emulating what the peer would do if it was
>     talking directly to an absent guest.
>  b) Reject SYNs quickly, trusting that the guest will have some sort of
>     application level retry.  That will depend on the client.  I guess
>     my fear here is that a client seeing a completed handshake + RST
>     might assume that the guest server is permanently broken, rather
>     than just temporarily missing as it might if there's no response at
>     all.

Oops, that's a detail I forgot: we complete the handshake and then
reset... which brings us to https://bugs.passt.top/show_bug.cgi?id=131.

Once that's implemented, perhaps it will be low effort to not listen()
at all in that case. Right now, I'm not sure anymore.

On the other hand, with just this patch, we will reset the connection
after 10 seconds (no matter what happens), which is just like this, but
delayed.

> I suggested Yumei's approach here to aim for (a) on the basis of
> transparency - it's as close as I think we can get to a bridged guest
> that's just missing.  I'm not necessarily opposed to (b), but I think
> it's less transparent, so we need an argument that it will lead to
> better outcomes regardless.

Given the problem above, maybe we should really look into a) (but this
patch doesn't do it).

Well, let me merge this, and other than that I would suggest looking
into a) if time allows.

b) looks still slightly better than the current situation, because right
now we'll accept and RST after 10 seconds. So if time doesn't allow,
let's settle for b) for the moment being?

> > 3.3223:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> > 3.3223:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> > 3.3224:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> > 3.3224:          Flow 0 (NEW): FREE -> NEW
> > 3.3224:          Flow 0 (INI): NEW -> INI
> > 3.3224:          Flow 0 (INI): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => ?
> > 3.3224:          Flow 0 (TGT): INI -> TGT
> > 3.3224:          Flow 0 (TGT): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > 3.3224:          Flow 0 (TCP connection): TGT -> TYPED
> > 3.3224:          Flow 0 (TCP connection): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > 3.3224:          Flow 0 (TCP connection): event at tcp_conn_from_tap:1489
> > 3.3224:          Flow 0 (TCP connection): TAP_SYN_RCVD: CLOSED -> SYN_SENT
> > 3.3224:          Flow 0 (TCP connection): failed to set TCP_MAXSEG on socket 21
> > 3.3224:          Flow 0 (TCP connection): Side 0 hash table insert: bucket: 294539
> > 3.3225:          Flow 0 (TCP connection): TYPED -> ACTIVE
> > 3.3225:          Flow 0 (TCP connection): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > 4.0027:          pasta: epoll event on namespace timer watch 17 (events: 0x00000001)
> > 4.3612:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> > 4.3613:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> > 4.3613:          Flow 0 (TCP connection): packet length 40 from tap
> > 4.3613:          Flow 0 (TCP connection): TCP reset at tcp_tap_handler:1989
> > 4.3613:          Flow 0 (TCP connection): flag at tcp_prepare_flags:1163
> > 4.3613:          Flow 0 (TCP connection): event at tcp_rst_do:1206
> > 4.3613:          Flow 0 (TCP connection): CLOSED: SYN_SENT -> CLOSED
> > 4.3614:          Flow 0 (TCP connection): Side 0 hash table remove: bucket: 294539
> > 4.3614:          Flow 0 (FREE): ACTIVE -> FREE
> > 4.3614:          Flow 0 (FREE): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > 
> >    ...the retry happened within one second. This is a container, so Linux
> >    kernel, and the client was wget.  
> 
> I'm not seeing a retry at all in this log, plus it's an outbound
> connection, which is not the case we're dealing with here.

It's two SYN segments from a guest (yes, an outbound connection):

3.3224:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)

4.3613:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)

that's a retry and that's all I wanted to show: the typical timing you
get from Linux.

-- 
Stefano

Re: [PATCH] tap: Drop frames if no client connected

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 12440 bytes --]

On Tue, Sep 23, 2025 at 01:00:39PM +0200, Stefano Brivio wrote:
> On Tue, 23 Sep 2025 17:53:09 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Sep 22, 2025 at 10:03:30PM +0200, Stefano Brivio wrote:
> > > On Mon, 22 Sep 2025 15:17:12 +0800
> > > Yumei Huang <yuhuang@redhat.com> wrote:  
> > > > On Fri, Sep 19, 2025 at 9:38 AM David Gibson
> > > > <david@gibson.dropbear.id.au> wrote:  
> > > > > On Thu, Sep 18, 2025 at 09:17:14AM +0200, Stefano Brivio wrote:    
> > > > > > On Thu, 18 Sep 2025 14:28:37 +1000
> > > > > > David Gibson <david@gibson.dropbear.id.au> wrote:  
> > [snip]
> > > > > > Does it work to cover situations where users might start passt a bit
> > > > > > before the guest connects, and try to connect to services right away?
> > > > > >
> > > > > > I suggested using ssh which should have a quite long timeout and retry
> > > > > > connecting for a while. You mentioned you would assist Yumei in testing
> > > > > > this if needed.    
> > > > >
> > > > > Ah, yes, you're right and I'd forgotten that.  Following up today.    
> > > > 
> > > > I tried both 'ssh' and 'socat'(writing a big file) before a guest
> > > > connects, they get a 'Connection reset' after 10s, even if the guest
> > > > connects in ~2s.
> > > > It's because, when start ssh or socat, passt would try to finish the
> > > > tcp handshake with the guest. It sends SYN to the guest immediately
> > > > and waits for SYN-ACK. However, the SYN frame is dropped/lost due to
> > > > no guest connected. So though the guest connects in seconds, the tcp
> > > > handshake would timeout, and returns rst via tcp_rst().  
> > > 
> > > Ah, right. We won't try to resend the SYN, that's simply not
> > > implemented.
> > > 
> > > The timeout you see is SYN_TIMEOUT, timer set by tcp_timer_ctl() and
> > > handled by tcp_timer_handler().
> > >   
> > > > Either with or without this patch, they got the same 'connection reset'.
> > > > Maybe it's something to fix?  
> > > 
> > > First off, this shows that the current patch is harmless, so I would go
> > > ahead and apply it (but see 2. below).
> > > 
> > > Strictly speaking, I don't think we really *need* to fix anything, but
> > > for sure the behaviour isn't ideal. I see two alternatives:
> > > 
> > > 1. we implement a periodic retry for the SYN segment. This would *seem*
> > >    to give the best behaviour in this case, but:
> > > 
> > >    a. it's quite complicated (we need to calculate some delays for the
> > >       timers, etc.), and not really transparent (which is in general a
> > >       goal of passt)  
> > 
> > I'm not really sure why you say it's not transparent, or at least what
> > other option you're comparing it to.  The peer has initiated a
> > connection to us in the normal way (which may include resending SYNs).
> > Now we're initiating a connection to the guest in the normal way
> > (which may include resending SYNs).
> 
> I was comparing this to b. or to doing nothing.
> 
> But, actually, you're right, the kernel wouldn't tell us about a
> repeated SYN, it would still be the same socket returned from accept(),
> so it's not necessarily less transparent.

Not only can't it tell us about a repeated SYN, but there won't *be* a
repeated SYN, unless the host kernel's SYN-ACK gets lost.

> I was thinking that we know when the guest connects, so we could just
> delay the SYN segment until then, by introducing a separate TAP_SYN_SENT
> event (right now it's implicit in SOCK_ACCEPTED). But when the guest
> connects, services are typically not up yet. You would typically get a
> RST while the guest is booting.

We could; I would have thought the timescale over which we expect a
guest to be attached would mean that resending SYNs on a timer would
achieve the same thing more simply (and handle other cases, too).

Getting an RST during boot would be typical, but as we've seen in
Volker's case, this may not be a newly booting guest, but a
reconnecting guest (or pasta is backing a NIC being hotplugged into
the guest).

> > >    b. if the guest never appears, we're just wasting client's time. See
> > >       db2c91ae86c7 ("tcp: Set ACK flag on *all* RST segments, even for
> > >       client in SYN-SENT state") for an example where it's important to
> > >       fail fast  
> > 
> > Sure.  I'd say RSTing here would be *less* transparent, but it might
> > still be worth it to make the peer fail fast.
> 
> But that's what happens naturally (with Linux) if nobody is listening,
> and in RFC 9293 terms, I'd say we should approximate a CLOSED state,
> 3.10.7.1:
> 
>   If the state is CLOSED (i.e., TCB does not exist), then [...] [a]n
>   incoming segment not containing a RST causes a RST to be sent in response.
> 
> rather than a LISTEN state (3.10.7.2). However, see below.

Well, it depends on what physical model we're trying to emulate
here. My assumption was that we were trying to make this look like the
guest was off, or had its network cable unplugged.  In which case we
want to just discard packets to the extent we can.

We could model an unconnected guest as a host with no listens, in
which case, yes, we should RST ASAP.  That seems less natural to me.

> > > 2. reset right away as I was suggesting in
> > >    https://archives.passt.top/passt-dev/20250915081319.00e72e53@elisabeth/:
> > >   
> > >    > We could mitigate that by making the TCP handler aware of this, and by
> > >    > resetting the connection if the guest isn't there. This would at least
> > >    > be consistent with the case where the guest isn't listening on the port
> > >    > (we accept(), fail to connect to it, eventually call tcp_rst()).  
> > > 
> > >    and let the client retry as appropriate (if implemented). Those retries
> > >    can be quite fast, see this report (from IRC) for 722d347c1932 ("tcp:
> > >    Don't reset outbound connection on SYN retries"):  
> > 
> > I don't see how that commit is relevant to this situation.  That's
> > talking about SYN retries.
> 
> That's just an example about how SYN segments are retried. It's not
> otherwise relevant for this situation.

Sure, but again, SYNs *won't* be retried in this case, because the
host completes the handshake.

Well.. there could be retried SYNs, but the host kernel either won't
tell us about it (lost SYN-ACK) or doesn't know itself (lost SYN).

> > We can see those in the case of outbound
> > connections bot we'll never see them for the case of inbound
> > connections, because the host kernel has already completed the
> > handshake.  For inbound we essentially have two options:
> > 
> >  a) Retry SYNs ourselves, emulating what the peer would do if it was
> >     talking directly to an absent guest.
> >  b) Reject SYNs quickly, trusting that the guest will have some sort of
> >     application level retry.  That will depend on the client.  I guess
> >     my fear here is that a client seeing a completed handshake + RST
> >     might assume that the guest server is permanently broken, rather
> >     than just temporarily missing as it might if there's no response at
> >     all.

Fwiw, these two options basically come down to whether we're trying to
make a missing guest look like a machine that's off, or a machine
that's not listening.

> Oops, that's a detail I forgot: we complete the handshake and then
> reset... which brings us to https://bugs.passt.top/show_bug.cgi?id=131.
> 
> Once that's implemented, perhaps it will be low effort to not listen()
> at all in that case. Right now, I'm not sure anymore.

Delaying the listen() would make us more closely (pretty much
exactly?) resemble the case where we're pretending the guest is a
machine that's not listening.  It doesn't get us closer to treating
the guest as a machine that's off or physicall disconnected.  It would
just mean we get SYN -> RST instead of SYN -> SYN-ACK -> ACK -> RST.

> On the other hand, with just this patch, we will reset the connection
> after 10 seconds (no matter what happens), which is just like this, but
> delayed.
> 
> > I suggested Yumei's approach here to aim for (a) on the basis of
> > transparency - it's as close as I think we can get to a bridged guest
> > that's just missing.  I'm not necessarily opposed to (b), but I think
> > it's less transparent, so we need an argument that it will lead to
> > better outcomes regardless.
> 
> Given the problem above, maybe we should really look into a) (but this
> patch doesn't do it).

It doesn't, because we don't retry SYNs (which I hadn't realised when
I suggested it). Arguably we should do that anyway.  It would be
extremely rare, but it's not impossible for our SYN to be really truly
lost on its way to the guest due to, say, a full buffer in the tap
device, or in the guest itself.

> Well, let me merge this, and other than that I would suggest looking
> into a) if time allows.
> 
> b) looks still slightly better than the current situation, because right
> now we'll accept and RST after 10 seconds. So if time doesn't allow,
> let's settle for b) for the moment being?
> 
> > > 3.3223:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> > > 3.3223:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> > > 3.3224:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> > > 3.3224:          Flow 0 (NEW): FREE -> NEW
> > > 3.3224:          Flow 0 (INI): NEW -> INI
> > > 3.3224:          Flow 0 (INI): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => ?
> > > 3.3224:          Flow 0 (TGT): INI -> TGT
> > > 3.3224:          Flow 0 (TGT): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > > 3.3224:          Flow 0 (TCP connection): TGT -> TYPED
> > > 3.3224:          Flow 0 (TCP connection): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > > 3.3224:          Flow 0 (TCP connection): event at tcp_conn_from_tap:1489
> > > 3.3224:          Flow 0 (TCP connection): TAP_SYN_RCVD: CLOSED -> SYN_SENT
> > > 3.3224:          Flow 0 (TCP connection): failed to set TCP_MAXSEG on socket 21
> > > 3.3224:          Flow 0 (TCP connection): Side 0 hash table insert: bucket: 294539
> > > 3.3225:          Flow 0 (TCP connection): TYPED -> ACTIVE
> > > 3.3225:          Flow 0 (TCP connection): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > > 4.0027:          pasta: epoll event on namespace timer watch 17 (events: 0x00000001)
> > > 4.3612:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> > > 4.3613:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> > > 4.3613:          Flow 0 (TCP connection): packet length 40 from tap
> > > 4.3613:          Flow 0 (TCP connection): TCP reset at tcp_tap_handler:1989
> > > 4.3613:          Flow 0 (TCP connection): flag at tcp_prepare_flags:1163
> > > 4.3613:          Flow 0 (TCP connection): event at tcp_rst_do:1206
> > > 4.3613:          Flow 0 (TCP connection): CLOSED: SYN_SENT -> CLOSED
> > > 4.3614:          Flow 0 (TCP connection): Side 0 hash table remove: bucket: 294539
> > > 4.3614:          Flow 0 (FREE): ACTIVE -> FREE
> > > 4.3614:          Flow 0 (FREE): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > > 
> > >    ...the retry happened within one second. This is a container, so Linux
> > >    kernel, and the client was wget.  
> > 
> > I'm not seeing a retry at all in this log, plus it's an outbound
> > connection, which is not the case we're dealing with here.
> 
> It's two SYN segments from a guest (yes, an outbound connection):
> 
> 3.3224:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> 
> 4.3613:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> 
> that's a retry and that's all I wanted to show: the typical timing you
> get from Linux.

Ah, right, I see.  Right, but again, irrelevant to this case since we
don't get / can't see the repeated SYNs.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tap: Drop frames if no client connected

[Stefano Brivio]

On Tue, 23 Sep 2025 21:26:24 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Sep 23, 2025 at 01:00:39PM +0200, Stefano Brivio wrote:
> > On Tue, 23 Sep 2025 17:53:09 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Mon, Sep 22, 2025 at 10:03:30PM +0200, Stefano Brivio wrote:  
> > > > On Mon, 22 Sep 2025 15:17:12 +0800
> > > > Yumei Huang <yuhuang@redhat.com> wrote:    
> > > > > On Fri, Sep 19, 2025 at 9:38 AM David Gibson
> > > > > <david@gibson.dropbear.id.au> wrote:    
> > > > > > On Thu, Sep 18, 2025 at 09:17:14AM +0200, Stefano Brivio wrote:      
> > > > > > > On Thu, 18 Sep 2025 14:28:37 +1000
> > > > > > > David Gibson <david@gibson.dropbear.id.au> wrote:    
> > > [snip]  
> > > > > > > Does it work to cover situations where users might start passt a bit
> > > > > > > before the guest connects, and try to connect to services right away?
> > > > > > >
> > > > > > > I suggested using ssh which should have a quite long timeout and retry
> > > > > > > connecting for a while. You mentioned you would assist Yumei in testing
> > > > > > > this if needed.      
> > > > > >
> > > > > > Ah, yes, you're right and I'd forgotten that.  Following up today.      
> > > > > 
> > > > > I tried both 'ssh' and 'socat'(writing a big file) before a guest
> > > > > connects, they get a 'Connection reset' after 10s, even if the guest
> > > > > connects in ~2s.
> > > > > It's because, when start ssh or socat, passt would try to finish the
> > > > > tcp handshake with the guest. It sends SYN to the guest immediately
> > > > > and waits for SYN-ACK. However, the SYN frame is dropped/lost due to
> > > > > no guest connected. So though the guest connects in seconds, the tcp
> > > > > handshake would timeout, and returns rst via tcp_rst().    
> > > > 
> > > > Ah, right. We won't try to resend the SYN, that's simply not
> > > > implemented.
> > > > 
> > > > The timeout you see is SYN_TIMEOUT, timer set by tcp_timer_ctl() and
> > > > handled by tcp_timer_handler().
> > > >     
> > > > > Either with or without this patch, they got the same 'connection reset'.
> > > > > Maybe it's something to fix?    
> > > > 
> > > > First off, this shows that the current patch is harmless, so I would go
> > > > ahead and apply it (but see 2. below).
> > > > 
> > > > Strictly speaking, I don't think we really *need* to fix anything, but
> > > > for sure the behaviour isn't ideal. I see two alternatives:
> > > > 
> > > > 1. we implement a periodic retry for the SYN segment. This would *seem*
> > > >    to give the best behaviour in this case, but:
> > > > 
> > > >    a. it's quite complicated (we need to calculate some delays for the
> > > >       timers, etc.), and not really transparent (which is in general a
> > > >       goal of passt)    
> > > 
> > > I'm not really sure why you say it's not transparent, or at least what
> > > other option you're comparing it to.  The peer has initiated a
> > > connection to us in the normal way (which may include resending SYNs).
> > > Now we're initiating a connection to the guest in the normal way
> > > (which may include resending SYNs).  
> > 
> > I was comparing this to b. or to doing nothing.
> > 
> > But, actually, you're right, the kernel wouldn't tell us about a
> > repeated SYN, it would still be the same socket returned from accept(),
> > so it's not necessarily less transparent.  
> 
> Not only can't it tell us about a repeated SYN, but there won't *be* a
> repeated SYN, unless the host kernel's SYN-ACK gets lost.

Hmm, wait, that was actually one of my original points about
transparency, which I forgot later on: that means one SYN. So it would
be more transparent to send one SYN. But that's irrelevant, see below.

> > I was thinking that we know when the guest connects, so we could just
> > delay the SYN segment until then, by introducing a separate TAP_SYN_SENT
> > event (right now it's implicit in SOCK_ACCEPTED). But when the guest
> > connects, services are typically not up yet. You would typically get a
> > RST while the guest is booting.  
> 
> We could; I would have thought the timescale over which we expect a
> guest to be attached would mean that resending SYNs on a timer would
> achieve the same thing more simply (and handle other cases, too).

Right, we would need a timer anyway, fair point.

> Getting an RST during boot would be typical, but as we've seen in
> Volker's case, this may not be a newly booting guest, but a
> reconnecting guest (or pasta is backing a NIC being hotplugged into
> the guest).

In practice we would even be able to distinguish the two cases,
because in case of a reconnection we'll get an ARP reply for the
request Volker added recently, and otherwise we won't.

> > > >    b. if the guest never appears, we're just wasting client's time. See
> > > >       db2c91ae86c7 ("tcp: Set ACK flag on *all* RST segments, even for
> > > >       client in SYN-SENT state") for an example where it's important to
> > > >       fail fast    
> > > 
> > > Sure.  I'd say RSTing here would be *less* transparent, but it might
> > > still be worth it to make the peer fail fast.  
> > 
> > But that's what happens naturally (with Linux) if nobody is listening,
> > and in RFC 9293 terms, I'd say we should approximate a CLOSED state,
> > 3.10.7.1:
> > 
> >   If the state is CLOSED (i.e., TCB does not exist), then [...] [a]n
> >   incoming segment not containing a RST causes a RST to be sent in response.
> > 
> > rather than a LISTEN state (3.10.7.2). However, see below.  
> 
> Well, it depends on what physical model we're trying to emulate
> here. My assumption was that we were trying to make this look like the
> guest was off, or had its network cable unplugged.  In which case we
> want to just discard packets to the extent we can.

Considering all the implications above, I'm not sure if this is more
natural, but it start looking simpler and more compatible.

> We could model an unconnected guest as a host with no listens, in
> which case, yes, we should RST ASAP.  That seems less natural to me.
> 
> > > > 2. reset right away as I was suggesting in
> > > >    https://archives.passt.top/passt-dev/20250915081319.00e72e53@elisabeth/:
> > > >     
> > > >    > We could mitigate that by making the TCP handler aware of this, and by
> > > >    > resetting the connection if the guest isn't there. This would at least
> > > >    > be consistent with the case where the guest isn't listening on the port
> > > >    > (we accept(), fail to connect to it, eventually call tcp_rst()).    
> > > > 
> > > >    and let the client retry as appropriate (if implemented). Those retries
> > > >    can be quite fast, see this report (from IRC) for 722d347c1932 ("tcp:
> > > >    Don't reset outbound connection on SYN retries"):    
> > > 
> > > I don't see how that commit is relevant to this situation.  That's
> > > talking about SYN retries.  
> > 
> > That's just an example about how SYN segments are retried. It's not
> > otherwise relevant for this situation.  
> 
> Sure, but again, SYNs *won't* be retried in this case, because the
> host completes the handshake.
> 
> Well.. there could be retried SYNs, but the host kernel either won't
> tell us about it (lost SYN-ACK) or doesn't know itself (lost SYN).
> 
> > > We can see those in the case of outbound
> > > connections bot we'll never see them for the case of inbound
> > > connections, because the host kernel has already completed the
> > > handshake.  For inbound we essentially have two options:
> > > 
> > >  a) Retry SYNs ourselves, emulating what the peer would do if it was
> > >     talking directly to an absent guest.
> > >  b) Reject SYNs quickly, trusting that the guest will have some sort of
> > >     application level retry.  That will depend on the client.  I guess
> > >     my fear here is that a client seeing a completed handshake + RST
> > >     might assume that the guest server is permanently broken, rather
> > >     than just temporarily missing as it might if there's no response at
> > >     all.  
> 
> Fwiw, these two options basically come down to whether we're trying to
> make a missing guest look like a machine that's off, or a machine
> that's not listening.
> 
> > Oops, that's a detail I forgot: we complete the handshake and then
> > reset... which brings us to https://bugs.passt.top/show_bug.cgi?id=131.
> > 
> > Once that's implemented, perhaps it will be low effort to not listen()
> > at all in that case. Right now, I'm not sure anymore.  
> 
> Delaying the listen() would make us more closely (pretty much
> exactly?) resemble the case where we're pretending the guest is a
> machine that's not listening.  It doesn't get us closer to treating
> the guest as a machine that's off or physicall disconnected.  It would
> just mean we get SYN -> RST instead of SYN -> SYN-ACK -> ACK -> RST.

Which is much better. Well, I start thinking that SYN -> (nothing)
would be even better in this case, but we can't, because at some point
we have to close that socket.

Right now we have SYN -> 10 seconds -> RST, as Yumei pointed out, with
or without this patch.

So I guess the best way forward would be...:

> > On the other hand, with just this patch, we will reset the connection
> > after 10 seconds (no matter what happens), which is just like this, but
> > delayed.
> >   
> > > I suggested Yumei's approach here to aim for (a) on the basis of
> > > transparency - it's as close as I think we can get to a bridged guest
> > > that's just missing.  I'm not necessarily opposed to (b), but I think
> > > it's less transparent, so we need an argument that it will lead to
> > > better outcomes regardless.  
> > 
> > Given the problem above, maybe we should really look into a) (but this
> > patch doesn't do it).  
> 
> It doesn't, because we don't retry SYNs (which I hadn't realised when
> I suggested it). Arguably we should do that anyway.  It would be
> extremely rare, but it's not impossible for our SYN to be really truly
> lost on its way to the guest due to, say, a full buffer in the tap
> device, or in the guest itself.

...retrying SYNs. We won't get to (host-side) SYN -> (nothing), but
reasonably close to it, that is, SYN -> (long delay) -> RST.

And if the guest appears meanwhile, it's fine anyway.

> > Well, let me merge this, and other than that I would suggest looking
> > into a) if time allows.
> > 
> > b) looks still slightly better than the current situation, because right
> > now we'll accept and RST after 10 seconds. So if time doesn't allow,
> > let's settle for b) for the moment being?
> >   
> > > > 3.3223:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> > > > 3.3223:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> > > > 3.3224:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> > > > 3.3224:          Flow 0 (NEW): FREE -> NEW
> > > > 3.3224:          Flow 0 (INI): NEW -> INI
> > > > 3.3224:          Flow 0 (INI): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => ?
> > > > 3.3224:          Flow 0 (TGT): INI -> TGT
> > > > 3.3224:          Flow 0 (TGT): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > > > 3.3224:          Flow 0 (TCP connection): TGT -> TYPED
> > > > 3.3224:          Flow 0 (TCP connection): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > > > 3.3224:          Flow 0 (TCP connection): event at tcp_conn_from_tap:1489
> > > > 3.3224:          Flow 0 (TCP connection): TAP_SYN_RCVD: CLOSED -> SYN_SENT
> > > > 3.3224:          Flow 0 (TCP connection): failed to set TCP_MAXSEG on socket 21
> > > > 3.3224:          Flow 0 (TCP connection): Side 0 hash table insert: bucket: 294539
> > > > 3.3225:          Flow 0 (TCP connection): TYPED -> ACTIVE
> > > > 3.3225:          Flow 0 (TCP connection): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > > > 4.0027:          pasta: epoll event on namespace timer watch 17 (events: 0x00000001)
> > > > 4.3612:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> > > > 4.3613:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> > > > 4.3613:          Flow 0 (TCP connection): packet length 40 from tap
> > > > 4.3613:          Flow 0 (TCP connection): TCP reset at tcp_tap_handler:1989
> > > > 4.3613:          Flow 0 (TCP connection): flag at tcp_prepare_flags:1163
> > > > 4.3613:          Flow 0 (TCP connection): event at tcp_rst_do:1206
> > > > 4.3613:          Flow 0 (TCP connection): CLOSED: SYN_SENT -> CLOSED
> > > > 4.3614:          Flow 0 (TCP connection): Side 0 hash table remove: bucket: 294539
> > > > 4.3614:          Flow 0 (FREE): ACTIVE -> FREE
> > > > 4.3614:          Flow 0 (FREE): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > > > 
> > > >    ...the retry happened within one second. This is a container, so Linux
> > > >    kernel, and the client was wget.    
> > > 
> > > I'm not seeing a retry at all in this log, plus it's an outbound
> > > connection, which is not the case we're dealing with here.  
> > 
> > It's two SYN segments from a guest (yes, an outbound connection):
> > 
> > 3.3224:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> > 
> > 4.3613:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> > 
> > that's a retry and that's all I wanted to show: the typical timing you
> > get from Linux.  
> 
> Ah, right, I see.  Right, but again, irrelevant to this case since we
> don't get / can't see the repeated SYNs.

Well, again, we should do something like that (it also seems to be what
you're suggesting).

-- 
Stefano

Re: [PATCH] tap: Drop frames if no client connected

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 17537 bytes --]

On Wed, Sep 24, 2025 at 01:56:09AM +0200, Stefano Brivio wrote:
> On Tue, 23 Sep 2025 21:26:24 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Sep 23, 2025 at 01:00:39PM +0200, Stefano Brivio wrote:
> > > On Tue, 23 Sep 2025 17:53:09 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Mon, Sep 22, 2025 at 10:03:30PM +0200, Stefano Brivio wrote:  
> > > > > On Mon, 22 Sep 2025 15:17:12 +0800
> > > > > Yumei Huang <yuhuang@redhat.com> wrote:    
> > > > > > On Fri, Sep 19, 2025 at 9:38 AM David Gibson
> > > > > > <david@gibson.dropbear.id.au> wrote:    
> > > > > > > On Thu, Sep 18, 2025 at 09:17:14AM +0200, Stefano Brivio wrote:      
> > > > > > > > On Thu, 18 Sep 2025 14:28:37 +1000
> > > > > > > > David Gibson <david@gibson.dropbear.id.au> wrote:    
> > > > [snip]  
> > > > > > > > Does it work to cover situations where users might start passt a bit
> > > > > > > > before the guest connects, and try to connect to services right away?
> > > > > > > >
> > > > > > > > I suggested using ssh which should have a quite long timeout and retry
> > > > > > > > connecting for a while. You mentioned you would assist Yumei in testing
> > > > > > > > this if needed.      
> > > > > > >
> > > > > > > Ah, yes, you're right and I'd forgotten that.  Following up today.      
> > > > > > 
> > > > > > I tried both 'ssh' and 'socat'(writing a big file) before a guest
> > > > > > connects, they get a 'Connection reset' after 10s, even if the guest
> > > > > > connects in ~2s.
> > > > > > It's because, when start ssh or socat, passt would try to finish the
> > > > > > tcp handshake with the guest. It sends SYN to the guest immediately
> > > > > > and waits for SYN-ACK. However, the SYN frame is dropped/lost due to
> > > > > > no guest connected. So though the guest connects in seconds, the tcp
> > > > > > handshake would timeout, and returns rst via tcp_rst().    
> > > > > 
> > > > > Ah, right. We won't try to resend the SYN, that's simply not
> > > > > implemented.
> > > > > 
> > > > > The timeout you see is SYN_TIMEOUT, timer set by tcp_timer_ctl() and
> > > > > handled by tcp_timer_handler().
> > > > >     
> > > > > > Either with or without this patch, they got the same 'connection reset'.
> > > > > > Maybe it's something to fix?    
> > > > > 
> > > > > First off, this shows that the current patch is harmless, so I would go
> > > > > ahead and apply it (but see 2. below).
> > > > > 
> > > > > Strictly speaking, I don't think we really *need* to fix anything, but
> > > > > for sure the behaviour isn't ideal. I see two alternatives:
> > > > > 
> > > > > 1. we implement a periodic retry for the SYN segment. This would *seem*
> > > > >    to give the best behaviour in this case, but:
> > > > > 
> > > > >    a. it's quite complicated (we need to calculate some delays for the
> > > > >       timers, etc.), and not really transparent (which is in general a
> > > > >       goal of passt)    
> > > > 
> > > > I'm not really sure why you say it's not transparent, or at least what
> > > > other option you're comparing it to.  The peer has initiated a
> > > > connection to us in the normal way (which may include resending SYNs).
> > > > Now we're initiating a connection to the guest in the normal way
> > > > (which may include resending SYNs).  
> > > 
> > > I was comparing this to b. or to doing nothing.
> > > 
> > > But, actually, you're right, the kernel wouldn't tell us about a
> > > repeated SYN, it would still be the same socket returned from accept(),
> > > so it's not necessarily less transparent.  
> > 
> > Not only can't it tell us about a repeated SYN, but there won't *be* a
> > repeated SYN, unless the host kernel's SYN-ACK gets lost.
> 
> Hmm, wait, that was actually one of my original points about
> transparency, which I forgot later on: that means one SYN. So it would
> be more transparent to send one SYN. But that's irrelevant, see below.
> 
> > > I was thinking that we know when the guest connects, so we could just
> > > delay the SYN segment until then, by introducing a separate TAP_SYN_SENT
> > > event (right now it's implicit in SOCK_ACCEPTED). But when the guest
> > > connects, services are typically not up yet. You would typically get a
> > > RST while the guest is booting.  
> > 
> > We could; I would have thought the timescale over which we expect a
> > guest to be attached would mean that resending SYNs on a timer would
> > achieve the same thing more simply (and handle other cases, too).
> 
> Right, we would need a timer anyway, fair point.
> 
> > Getting an RST during boot would be typical, but as we've seen in
> > Volker's case, this may not be a newly booting guest, but a
> > reconnecting guest (or pasta is backing a NIC being hotplugged into
> > the guest).
> 
> In practice we would even be able to distinguish the two cases,
> because in case of a reconnection we'll get an ARP reply for the
> request Volker added recently, and otherwise we won't.

Probably.  I'm not sure that would be entirely reliable.

> > > > >    b. if the guest never appears, we're just wasting client's time. See
> > > > >       db2c91ae86c7 ("tcp: Set ACK flag on *all* RST segments, even for
> > > > >       client in SYN-SENT state") for an example where it's important to
> > > > >       fail fast    
> > > > 
> > > > Sure.  I'd say RSTing here would be *less* transparent, but it might
> > > > still be worth it to make the peer fail fast.  
> > > 
> > > But that's what happens naturally (with Linux) if nobody is listening,
> > > and in RFC 9293 terms, I'd say we should approximate a CLOSED state,
> > > 3.10.7.1:
> > > 
> > >   If the state is CLOSED (i.e., TCB does not exist), then [...] [a]n
> > >   incoming segment not containing a RST causes a RST to be sent in response.
> > > 
> > > rather than a LISTEN state (3.10.7.2). However, see below.  
> > 
> > Well, it depends on what physical model we're trying to emulate
> > here. My assumption was that we were trying to make this look like the
> > guest was off, or had its network cable unplugged.  In which case we
> > want to just discard packets to the extent we can.
> 
> Considering all the implications above, I'm not sure if this is more
> natural, but it start looking simpler and more compatible.
> 
> > We could model an unconnected guest as a host with no listens, in
> > which case, yes, we should RST ASAP.  That seems less natural to me.
> > 
> > > > > 2. reset right away as I was suggesting in
> > > > >    https://archives.passt.top/passt-dev/20250915081319.00e72e53@elisabeth/:
> > > > >     
> > > > >    > We could mitigate that by making the TCP handler aware of this, and by
> > > > >    > resetting the connection if the guest isn't there. This would at least
> > > > >    > be consistent with the case where the guest isn't listening on the port
> > > > >    > (we accept(), fail to connect to it, eventually call tcp_rst()).    
> > > > > 
> > > > >    and let the client retry as appropriate (if implemented). Those retries
> > > > >    can be quite fast, see this report (from IRC) for 722d347c1932 ("tcp:
> > > > >    Don't reset outbound connection on SYN retries"):    
> > > > 
> > > > I don't see how that commit is relevant to this situation.  That's
> > > > talking about SYN retries.  
> > > 
> > > That's just an example about how SYN segments are retried. It's not
> > > otherwise relevant for this situation.  
> > 
> > Sure, but again, SYNs *won't* be retried in this case, because the
> > host completes the handshake.
> > 
> > Well.. there could be retried SYNs, but the host kernel either won't
> > tell us about it (lost SYN-ACK) or doesn't know itself (lost SYN).
> > 
> > > > We can see those in the case of outbound
> > > > connections bot we'll never see them for the case of inbound
> > > > connections, because the host kernel has already completed the
> > > > handshake.  For inbound we essentially have two options:
> > > > 
> > > >  a) Retry SYNs ourselves, emulating what the peer would do if it was
> > > >     talking directly to an absent guest.
> > > >  b) Reject SYNs quickly, trusting that the guest will have some sort of
> > > >     application level retry.  That will depend on the client.  I guess
> > > >     my fear here is that a client seeing a completed handshake + RST
> > > >     might assume that the guest server is permanently broken, rather
> > > >     than just temporarily missing as it might if there's no response at
> > > >     all.  
> > 
> > Fwiw, these two options basically come down to whether we're trying to
> > make a missing guest look like a machine that's off, or a machine
> > that's not listening.
> > 
> > > Oops, that's a detail I forgot: we complete the handshake and then
> > > reset... which brings us to https://bugs.passt.top/show_bug.cgi?id=131.
> > > 
> > > Once that's implemented, perhaps it will be low effort to not listen()
> > > at all in that case. Right now, I'm not sure anymore.  
> > 
> > Delaying the listen() would make us more closely (pretty much
> > exactly?) resemble the case where we're pretending the guest is a
> > machine that's not listening.  It doesn't get us closer to treating
> > the guest as a machine that's off or physicall disconnected.  It would
> > just mean we get SYN -> RST instead of SYN -> SYN-ACK -> ACK -> RST.
> 
> Which is much better. Well, I start thinking that SYN -> (nothing)
> would be even better in this case, but we can't, because at some point
> we have to close that socket.

Right.  SYN -> (nothing) would be ideal (IMO), but we can't do it.  At
least, not without something like the prelisten sockets concept I've
toyed with from time to time.

> Right now we have SYN -> 10 seconds -> RST, as Yumei pointed out, with
> or without this patch.

> So I guess the best way forward would be...:
> 
> > > On the other hand, with just this patch, we will reset the connection
> > > after 10 seconds (no matter what happens), which is just like this, but
> > > delayed.
> > >   
> > > > I suggested Yumei's approach here to aim for (a) on the basis of
> > > > transparency - it's as close as I think we can get to a bridged guest
> > > > that's just missing.  I'm not necessarily opposed to (b), but I think
> > > > it's less transparent, so we need an argument that it will lead to
> > > > better outcomes regardless.  
> > > 
> > > Given the problem above, maybe we should really look into a) (but this
> > > patch doesn't do it).  
> > 
> > It doesn't, because we don't retry SYNs (which I hadn't realised when
> > I suggested it). Arguably we should do that anyway.  It would be
> > extremely rare, but it's not impossible for our SYN to be really truly
> > lost on its way to the guest due to, say, a full buffer in the tap
> > device, or in the guest itself.
> 
> ...retrying SYNs. We won't get to (host-side) SYN -> (nothing), but
> reasonably close to it, that is, SYN -> (long delay) -> RST.
> 
> And if the guest appears meanwhile, it's fine anyway.
> 
> > > Well, let me merge this, and other than that I would suggest looking
> > > into a) if time allows.
> > > 
> > > b) looks still slightly better than the current situation, because right
> > > now we'll accept and RST after 10 seconds. So if time doesn't allow,
> > > let's settle for b) for the moment being?
> > >   
> > > > > 3.3223:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> > > > > 3.3223:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> > > > > 3.3224:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> > > > > 3.3224:          Flow 0 (NEW): FREE -> NEW
> > > > > 3.3224:          Flow 0 (INI): NEW -> INI
> > > > > 3.3224:          Flow 0 (INI): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => ?
> > > > > 3.3224:          Flow 0 (TGT): INI -> TGT
> > > > > 3.3224:          Flow 0 (TGT): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > > > > 3.3224:          Flow 0 (TCP connection): TGT -> TYPED
> > > > > 3.3224:          Flow 0 (TCP connection): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > > > > 3.3224:          Flow 0 (TCP connection): event at tcp_conn_from_tap:1489
> > > > > 3.3224:          Flow 0 (TCP connection): TAP_SYN_RCVD: CLOSED -> SYN_SENT
> > > > > 3.3224:          Flow 0 (TCP connection): failed to set TCP_MAXSEG on socket 21
> > > > > 3.3224:          Flow 0 (TCP connection): Side 0 hash table insert: bucket: 294539
> > > > > 3.3225:          Flow 0 (TCP connection): TYPED -> ACTIVE
> > > > > 3.3225:          Flow 0 (TCP connection): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > > > > 4.0027:          pasta: epoll event on namespace timer watch 17 (events: 0x00000001)
> > > > > 4.3612:          pasta: epoll event on /dev/net/tun device 18 (events: 0x00000001)
> > > > > 4.3613:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> > > > > 4.3613:          Flow 0 (TCP connection): packet length 40 from tap
> > > > > 4.3613:          Flow 0 (TCP connection): TCP reset at tcp_tap_handler:1989
> > > > > 4.3613:          Flow 0 (TCP connection): flag at tcp_prepare_flags:1163
> > > > > 4.3613:          Flow 0 (TCP connection): event at tcp_rst_do:1206
> > > > > 4.3613:          Flow 0 (TCP connection): CLOSED: SYN_SENT -> CLOSED
> > > > > 4.3614:          Flow 0 (TCP connection): Side 0 hash table remove: bucket: 294539
> > > > > 4.3614:          Flow 0 (FREE): ACTIVE -> FREE
> > > > > 4.3614:          Flow 0 (FREE): TAP [192.168.122.14]:55532 -> [192.0.0.1]:80 => HOST [0.0.0.0]:0 -> [192.0.0.1]:80
> > > > > 
> > > > >    ...the retry happened within one second. This is a container, so Linux
> > > > >    kernel, and the client was wget.    
> > > > 
> > > > I'm not seeing a retry at all in this log, plus it's an outbound
> > > > connection, which is not the case we're dealing with here.  
> > > 
> > > It's two SYN segments from a guest (yes, an outbound connection):
> > > 
> > > 3.3224:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> > > 
> > > 4.3613:          tap: protocol 6, 192.168.122.14:55532 -> 192.0.0.1:80 (1 packet)
> > > 
> > > that's a retry and that's all I wanted to show: the typical timing you
> > > get from Linux.  
> > 
> > Ah, right, I see.  Right, but again, irrelevant to this case since we
> > don't get / can't see the repeated SYNs.
> 
> Well, again, we should do something like that (it also seems to be what
> you're suggesting).

Ok, yes, I see.  I was meaning irrelevant in the sense that we can't
use the peer's SYN resends to trigger anything for us.


So... summarising.  As I see it, we have two main cases to consider:
the one where the guest comes online pretty soon, and the one where it
doesn't.  Here's what I think the behaviour would be for these two
cases with a variety of ways of handling it.  This is more-or-less
from the peer's perspective.

(0) Physicaly disconnected guest (bridged network, no passt involved)

    (0a) Guest online never
        SYN ... SYN ... SYN ... <peer times out>

    (0b) Guest online soonish
        SYN ... SYN ... SYN-ACK, ACK <working connection>

(1) Status quo

Passt doesn't resend SYNs, and will time out the connection after 10s.

    (1a) Guest online never
        SYN, SYN-ACK, ACK ... ... ... ... <passt times out> RST

    (0b) Guest online soonish
        SYN, SYN-ACK, ACK ... ... ... ... <passt times out> RST

(2) Yumei's patch

As (1), but without EBADFs

(3) passt resends SYNs

    (3a) Guest online never
        SYN, SYN-ACK, ACK ... ... ... ... ... <passt times out> RST

    (3b) Guest online soonish
        SYN, SYN-ACK, ACK ... ... ... ... <working connection>

(4) Passt resends SYNs + Yumei's patch

As (3), but without EBADFs

(5) passt explicitly resets when guest is not present

    (6a) Guest online never
        SYN, SYN-ACK, ACK, RST

    (6b) Guest online soonish
        SYN, SYN-ACK, ACK, RST

(6) Delayed listen()

    (6a) Guest online never
        SYN, RST

    (6b) Guest online soonish
        SYN, RST

(99) Bridged guest isn't listening (no passt)

    (99a) Guest online never
        SYN, RST

    (99b) Guest online soonish
        SYN, RST

=====

So, if (99) is our model, we can match it pretty exactly with delayed
listen().  But if (0) is our model, the closest we can get is (3) or
(4), which I think will look fairly similar to peer application, even
though it looks different to the peer TCP stack.

I think (0) is a better model, because it means we won't reset
connections if they happen to land when a still running guest has its
connection to passt temporarily interrupted.

Which brings me, I think, to the same conclusion you had: we should
resend SYNs.

Suggested next steps:
 - Apply Yumei's patch, it doesn't change behaviour and removes the
   odd EBADFs
 - Yumei investigates implementing SYN resends

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tap: Drop frames if no client connected

[Stefano Brivio]

On Wed, 24 Sep 2025 11:49:28 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> So... summarising.  As I see it, we have two main cases to consider:
> the one where the guest comes online pretty soon, and the one where it
> doesn't.  Here's what I think the behaviour would be for these two
> cases with a variety of ways of handling it.  This is more-or-less
> from the peer's perspective.
> 
> (0) Physicaly disconnected guest (bridged network, no passt involved)
> 
>     (0a) Guest online never
>         SYN ... SYN ... SYN ... <peer times out>
> 
>     (0b) Guest online soonish
>         SYN ... SYN ... SYN-ACK, ACK <working connection>
> 
> (1) Status quo
> 
> Passt doesn't resend SYNs, and will time out the connection after 10s.
> 
>     (1a) Guest online never
>         SYN, SYN-ACK, ACK ... ... ... ... <passt times out> RST
> 
>     (0b) Guest online soonish
>         SYN, SYN-ACK, ACK ... ... ... ... <passt times out> RST
> 
> (2) Yumei's patch
> 
> As (1), but without EBADFs
> 
> (3) passt resends SYNs
> 
>     (3a) Guest online never
>         SYN, SYN-ACK, ACK ... ... ... ... ... <passt times out> RST
> 
>     (3b) Guest online soonish
>         SYN, SYN-ACK, ACK ... ... ... ... <working connection>
> 
> (4) Passt resends SYNs + Yumei's patch
> 
> As (3), but without EBADFs
> 
> (5) passt explicitly resets when guest is not present
> 
>     (6a) Guest online never
>         SYN, SYN-ACK, ACK, RST
> 
>     (6b) Guest online soonish
>         SYN, SYN-ACK, ACK, RST
> 
> (6) Delayed listen()
> 
>     (6a) Guest online never
>         SYN, RST
> 
>     (6b) Guest online soonish
>         SYN, RST
> 
> (99) Bridged guest isn't listening (no passt)
> 
>     (99a) Guest online never
>         SYN, RST
> 
>     (99b) Guest online soonish
>         SYN, RST
> 
> =====

It all makes sense, thanks for summarising those.

> So, if (99) is our model, we can match it pretty exactly with delayed
> listen().  But if (0) is our model, the closest we can get is (3) or
> (4), which I think will look fairly similar to peer application, even
> though it looks different to the peer TCP stack.
> 
> I think (0) is a better model, because it means we won't reset
> connections if they happen to land when a still running guest has its
> connection to passt temporarily interrupted.
> 
> Which brings me, I think, to the same conclusion you had: we should
> resend SYNs.
> 
> Suggested next steps:
>  - Apply Yumei's patch, it doesn't change behaviour and removes the
>    odd EBADFs
>  - Yumei investigates implementing SYN resends

Right, that also makes sense to me.

For the second part, we could probably reuse a mechanism similar to
what we do for re-transmits, and perhaps rename 'retrans' in struct
tcp_tap_conn to 'retries', so that we can use it for both (we're a bit
tight on space there).

-- 
Stefano

Re: [PATCH] tap: Drop frames if no client connected

[Yumei Huang]

On Wed, Sep 24, 2025 at 5:56 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Wed, 24 Sep 2025 11:49:28 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > So... summarising.  As I see it, we have two main cases to consider:
> > the one where the guest comes online pretty soon, and the one where it
> > doesn't.  Here's what I think the behaviour would be for these two
> > cases with a variety of ways of handling it.  This is more-or-less
> > from the peer's perspective.
> >
> > (0) Physicaly disconnected guest (bridged network, no passt involved)
> >
> >     (0a) Guest online never
> >         SYN ... SYN ... SYN ... <peer times out>
> >
> >     (0b) Guest online soonish
> >         SYN ... SYN ... SYN-ACK, ACK <working connection>
> >
> > (1) Status quo
> >
> > Passt doesn't resend SYNs, and will time out the connection after 10s.
> >
> >     (1a) Guest online never
> >         SYN, SYN-ACK, ACK ... ... ... ... <passt times out> RST
> >
> >     (0b) Guest online soonish
> >         SYN, SYN-ACK, ACK ... ... ... ... <passt times out> RST
> >
> > (2) Yumei's patch
> >
> > As (1), but without EBADFs
> >
> > (3) passt resends SYNs
> >
> >     (3a) Guest online never
> >         SYN, SYN-ACK, ACK ... ... ... ... ... <passt times out> RST
> >
> >     (3b) Guest online soonish
> >         SYN, SYN-ACK, ACK ... ... ... ... <working connection>
> >
> > (4) Passt resends SYNs + Yumei's patch
> >
> > As (3), but without EBADFs
> >
> > (5) passt explicitly resets when guest is not present
> >
> >     (6a) Guest online never
> >         SYN, SYN-ACK, ACK, RST
> >
> >     (6b) Guest online soonish
> >         SYN, SYN-ACK, ACK, RST
> >
> > (6) Delayed listen()
> >
> >     (6a) Guest online never
> >         SYN, RST
> >
> >     (6b) Guest online soonish
> >         SYN, RST
> >
> > (99) Bridged guest isn't listening (no passt)
> >
> >     (99a) Guest online never
> >         SYN, RST
> >
> >     (99b) Guest online soonish
> >         SYN, RST
> >
> > =====
>
> It all makes sense, thanks for summarising those.
>
> > So, if (99) is our model, we can match it pretty exactly with delayed
> > listen().  But if (0) is our model, the closest we can get is (3) or
> > (4), which I think will look fairly similar to peer application, even
> > though it looks different to the peer TCP stack.
> >
> > I think (0) is a better model, because it means we won't reset
> > connections if they happen to land when a still running guest has its
> > connection to passt temporarily interrupted.
> >
> > Which brings me, I think, to the same conclusion you had: we should
> > resend SYNs.
> >
> > Suggested next steps:
> >  - Apply Yumei's patch, it doesn't change behaviour and removes the
> >    odd EBADFs
> >  - Yumei investigates implementing SYN resends
>
> Right, that also makes sense to me.

Glad we reached an agreement here. BTW, in case you missed it, the v2
patch was sent as
https://archives.passt.top/passt-dev/20250912081705.20796-1-yuhuang@redhat.com/T/#u.

>
> For the second part, we could probably reuse a mechanism similar to
> what we do for re-transmits, and perhaps rename 'retrans' in struct
> tcp_tap_conn to 'retries', so that we can use it for both (we're a bit
> tight on space there).

I got an initial thought about calling tcp_send_flag() in
tcp_flow_defer(). But it seems not working. Trying to figure that
out..

>
> --
> Stefano
>


-- 
Thanks,

Yumei Huang

Re: [PATCH] tap: Drop frames if no client connected

[Stefano Brivio]

On Thu, 25 Sep 2025 13:08:35 +0800
Yumei Huang <yuhuang@redhat.com> wrote:

> On Wed, Sep 24, 2025 at 5:56 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > On Wed, 24 Sep 2025 11:49:28 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >  
> > > So... summarising.  As I see it, we have two main cases to consider:
> > > the one where the guest comes online pretty soon, and the one where it
> > > doesn't.  Here's what I think the behaviour would be for these two
> > > cases with a variety of ways of handling it.  This is more-or-less
> > > from the peer's perspective.
> > >
> > > (0) Physicaly disconnected guest (bridged network, no passt involved)
> > >
> > >     (0a) Guest online never
> > >         SYN ... SYN ... SYN ... <peer times out>
> > >
> > >     (0b) Guest online soonish
> > >         SYN ... SYN ... SYN-ACK, ACK <working connection>
> > >
> > > (1) Status quo
> > >
> > > Passt doesn't resend SYNs, and will time out the connection after 10s.
> > >
> > >     (1a) Guest online never
> > >         SYN, SYN-ACK, ACK ... ... ... ... <passt times out> RST
> > >
> > >     (0b) Guest online soonish
> > >         SYN, SYN-ACK, ACK ... ... ... ... <passt times out> RST
> > >
> > > (2) Yumei's patch
> > >
> > > As (1), but without EBADFs
> > >
> > > (3) passt resends SYNs
> > >
> > >     (3a) Guest online never
> > >         SYN, SYN-ACK, ACK ... ... ... ... ... <passt times out> RST
> > >
> > >     (3b) Guest online soonish
> > >         SYN, SYN-ACK, ACK ... ... ... ... <working connection>
> > >
> > > (4) Passt resends SYNs + Yumei's patch
> > >
> > > As (3), but without EBADFs
> > >
> > > (5) passt explicitly resets when guest is not present
> > >
> > >     (6a) Guest online never
> > >         SYN, SYN-ACK, ACK, RST
> > >
> > >     (6b) Guest online soonish
> > >         SYN, SYN-ACK, ACK, RST
> > >
> > > (6) Delayed listen()
> > >
> > >     (6a) Guest online never
> > >         SYN, RST
> > >
> > >     (6b) Guest online soonish
> > >         SYN, RST
> > >
> > > (99) Bridged guest isn't listening (no passt)
> > >
> > >     (99a) Guest online never
> > >         SYN, RST
> > >
> > >     (99b) Guest online soonish
> > >         SYN, RST
> > >
> > > =====  
> >
> > It all makes sense, thanks for summarising those.
> >  
> > > So, if (99) is our model, we can match it pretty exactly with delayed
> > > listen().  But if (0) is our model, the closest we can get is (3) or
> > > (4), which I think will look fairly similar to peer application, even
> > > though it looks different to the peer TCP stack.
> > >
> > > I think (0) is a better model, because it means we won't reset
> > > connections if they happen to land when a still running guest has its
> > > connection to passt temporarily interrupted.
> > >
> > > Which brings me, I think, to the same conclusion you had: we should
> > > resend SYNs.
> > >
> > > Suggested next steps:
> > >  - Apply Yumei's patch, it doesn't change behaviour and removes the
> > >    odd EBADFs
> > >  - Yumei investigates implementing SYN resends  
> >
> > Right, that also makes sense to me.  
> 
> Glad we reached an agreement here. BTW, in case you missed it, the v2
> patch was sent as
> https://archives.passt.top/passt-dev/20250912081705.20796-1-yuhuang@redhat.com/T/#u.

I never miss patches. :) No worries, I just got a few interruptions in
a row but I plan to apply it soon.

> > For the second part, we could probably reuse a mechanism similar to
> > what we do for re-transmits, and perhaps rename 'retrans' in struct
> > tcp_tap_conn to 'retries', so that we can use it for both (we're a bit
> > tight on space there).  
> 
> I got an initial thought about calling tcp_send_flag() in
> tcp_flow_defer(). But it seems not working. Trying to figure that
> out..

That might work, even though, I guess, the most natural alternative
would be to change the handling of an expired SYN_TIMEOUT in
tcp_timer_handler(). Look at this case:

	} else if (conn->flags & ACK_FROM_TAP_DUE) {
		if (!(conn->events & ESTABLISHED)) {
			flow_dbg(conn, "handshake timeout");

...it should become a bit more like this one:

		} else {
			flow_dbg(conn, "ACK timeout, retry");
			conn->retrans++;
			...

where we retry for a few times, before resetting the connection.

With timers, you already have timed triggers, as opposed to trying
things out periodically from tcp_flow_defer().

-- 
Stefano