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

[Yumei Huang <yuhuang@redhat.com>]

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