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

[David Gibson <david@gibson.dropbear.id.au>]

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