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

[Stefano Brivio <sbrivio@redhat.com>]

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