Re: [PATCH] flow, repair: Wait for a short while for passt-repair to connect

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

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

On Fri, Mar 07, 2025 at 07:14:45AM +0100, Stefano Brivio wrote:
> On Fri, 7 Mar 2025 13:59:08 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Mar 06, 2025 at 08:00:51PM +0100, Stefano Brivio wrote:
> > > ...and time out after that. This will be needed because of an upcoming
> > > change to passt-repair enabling it to start before passt is started,
> > > on both source and target, by means of an inotify watch.
> > > 
> > > Once the inotify watch triggers, passt-repair will connect right away,
> > > but we have no guarantees that the connection completes before we
> > > start the migration process, so wait for it (for a reasonable amount
> > > of time).
> > > 
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> > 
> > Hmm.  So I'm pretty confident this patch does what it sets out to do.
> > I'm increasingly dubious about this direction.
> > 
> > > ---
> > > I'm still working on the passt-repair change mentioned above. It's
> > > turning out to be a bit trickier than expected because to support
> > > typical libvirt migration usage, on the target, we also need to be
> > > able to wait for creation of a directory and, separately, for a socket
> > > file being added inside it.  
> > 
> > Oof.  This significantly amplifies my concerns about the security
> > model: one of the most obvious ways to to bound who could potentially
> > convince passt-repair to connect is to make sure the directory exists
> > before hand: if you don't have permissions to create the socket in
> > that directory, passt-repair won't connect.
> > 
> > If we're also waiting for directories to appear, it makes this even
> > harder to analyse in terms of whether someone who's not supposed to
> > could convince passt-repair to connect to them.
> 
> I don't think your concern is particularly founded because in a general
> case we have LSM policies preventing other processes from creating that
> file in that directory, and in KubeVirt's case we simply have no other
> processes that can access the pod mount namespace and do that.

I mean, LSM properties should generally be a additional layer of
protection, things should be basically ok with the vanilla Unix
permission model.

> But that part of the change is not strictly necessary, surely not for a
> proper libvirt integration (libvirt knows the path) and also not for
> the integration for KubeVirt (KubeVirt can build the path).
> 
> So, to dodge the obstructionism here, I'll happily drop that part. It
> also saves me time.
> 
> > Given the complexity you're hitting here, I'm really starting to think
> > maybe we should reverse our model: have passt connect to passt-repair.
> > The security model is easier to understand: permission to the socket
> > and its directory is permission to use TCP_REPAIR.  There's no awkward
> > waiting on either side - I think it might actually be less total code
> > chyurn.
> 
> Of course it's a bit less code, but it turns a potential concern into
> the actually expected situation: you have something permanently around
> that allows any process (because you have no practical way to restrict

A (semi-)permanentaly running daemon was not what I had in mind.  What
Istead I was thinking that you start passt-repair shortly before the
migration (as we already planned), it does exactly one accept(), then
terminates once its single client is done.

> it, not in distributions and not in KubeVirt) to set/clear repair mode
> on sockets. Unless you start introducing authentication, that is.

Well, the plan was the authentication was via the Unix permissions on
the socket.

> In the current model it's still a one-off operation, even with an
> additional wait. You start passt-repair once to do its job, and once it
> does the job, it terminates.

Right, also what I was intending... but of course, I realised a few
hours later that I'd forgotten that our filesystem isolation means we
can't connect() to a Unix socket in the wider world at migration time.
There are ways to work around that, of course, but I have yet to spot
one that seems better than the status quo.  So,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

I guess.


> > Judging by the early queries from the Kubevirt guys it seems
> > to be the more obvious model people would expect.
> 
> Note that you talked to people taking care of (almost exclusively)
> networking there. Knowing myself a fair bit of code, architecture, and
> history of security weaknesses and discussions around them, I'm pretty
> sure you can't do the opposite.
> 
> The long-term solutions are anyway having libvirt provide a phase or
> blocking event where this can be done, and, at least in KubeVirt's
> case, eventually handle this with seitan.

Right.  Probably not really valuable while it still needs
CAP_NET_ADMIN, but if we do manage to change that to CAP_NET_RAW, it
would make sense for passt to only use the helper if it can't do the
TCP_REPAIR itself.  That makes setup simpler for environments that
don't have counterproductive "security" policies.

> > On the other hand,
> > configuring the socket path is still an issue, and being able to tell
> > passt where to connect to through libvirt might become more important
> > than it is now.
> > 
> > > However, this part looks solid and it should do what we need. I
> > > tested it by starting passt-repair after sleep(1) in migrate/* tests,
> > > changing this timeout to a minute, on either source, target, or both.
> > > 
> > >  flow.c   | 20 ++++++++++++++++++++
> > >  repair.c | 31 +++++++++++++++++++++++++++++++
> > >  repair.h |  1 +
> > >  3 files changed, 52 insertions(+)
> > > 
> > > diff --git a/flow.c b/flow.c
> > > index 749c498..5e64b79 100644
> > > --- a/flow.c
> > > +++ b/flow.c
> > > @@ -911,6 +911,21 @@ static int flow_migrate_source_rollback(struct ctx *c, unsigned bound, int ret)
> > >  	return ret;
> > >  }
> > >  
> > > +/**
> > > + * flow_migrate_need_repair() - Do we need to set repair mode for any flow?
> > > + *
> > > + * Return: true if repair mode is needed, false otherwise
> > > + */
> > > +static bool flow_migrate_need_repair(void)
> > > +{
> > > +	union flow *flow;
> > > +
> > > +	foreach_established_tcp_flow(flow)
> > > +		return true;
> > > +
> > > +	return false;
> > > +}
> > > +
> > >  /**
> > >   * flow_migrate_repair_all() - Turn repair mode on or off for all flows
> > >   * @c:		Execution context
> > > @@ -966,6 +981,9 @@ int flow_migrate_source_pre(struct ctx *c, const struct migrate_stage *stage,
> > >  	(void)stage;
> > >  	(void)fd;
> > >  
> > > +	if (flow_migrate_need_repair())
> > > +		repair_wait(c);
> > > +
> > >  	if ((rc = flow_migrate_repair_all(c, true)))
> > >  		return -rc;
> > >  
> > > @@ -1083,6 +1101,8 @@ int flow_migrate_target(struct ctx *c, const struct migrate_stage *stage,
> > >  	if (!count)
> > >  		return 0;
> > >  
> > > +	repair_wait(c);
> > > +
> > >  	if ((rc = flow_migrate_repair_all(c, true)))
> > >  		return -rc;
> > >  
> > > diff --git a/repair.c b/repair.c
> > > index 3ee089f..ce5a4a4 100644
> > > --- a/repair.c
> > > +++ b/repair.c
> > > @@ -27,6 +27,11 @@
> > >  
> > >  #define SCM_MAX_FD 253 /* From Linux kernel (include/net/scm.h), not in UAPI */
> > >  
> > > +/* Wait for a while for TCP_REPAIR helper to connect if it's not there yet */
> > > +#define REPAIR_ACCEPT_TIMEOUT_MS	100  
> > 
> > 100ms is a pretty long time in the context of a stopped guest.  I
> > guess is is a worst case.
> 
> It's actually not a case I can reproduce at all, as expected:
> passt-repair connects quickly enough once the socket is available.

Good to know.  It does mean we're adding 100ms of downtime for the
fallback case where there is no passt-repair, of course.

> > We could possibly mitigate it by doing the
> 
> Mitigate what exactly? The expected mode of operation is with
> passt-repair available. The failure handling is a fallback. I can turn
> it into 1 ms, it just seems to increase risk for no good reason.

> > wait when memory change logging goes on (so migration is starting, bu
> > the guest is not get stopped).  However at that point we can't rely on
> > flow_migrate_need_repair(), because we might have no flows, but one is
> > opened before the guest is stopped.
> 
> Right, that complicates things quite a lot.
> 
> > > +#define REPAIR_ACCEPT_TIMEOUT_US	(REPAIR_ACCEPT_TIMEOUT_MS * 1000)
> > > +static_assert(REPAIR_ACCEPT_TIMEOUT_US < 1000 * 1000);  
> > 
> > In this context the static_assert() seems like a pretty arbitrary
> > bound.  To make the reason for it clearer, I'd suggest moving it to
> > where we construct the timeval.
> 
> Oh, I didn't think of that. Sure, I'll move it.
> 
> > > +
> > >  /* Pending file descriptors for next repair_flush() call, or command change */
> > >  static int repair_fds[SCM_MAX_FD];
> > >  
> > > @@ -138,6 +143,32 @@ void repair_handler(struct ctx *c, uint32_t events)
> > >  	repair_close(c);
> > >  }
> > >  
> > > +/**
> > > + * repair_wait() - Wait (with timeout) for TCP_REPAIR helper to connect
> > > + * @c:		Execution context
> > > + */
> > > +void repair_wait(struct ctx *c)
> > > +{
> > > +	struct timeval tv = { .tv_sec = 0,
> > > +			      .tv_usec = (long)(REPAIR_ACCEPT_TIMEOUT_US) };
> > > +
> > > +	if (c->fd_repair >= 0 || c->fd_repair_listen == -1)
> > > +		return;
> > > +
> > > +	if (setsockopt(c->fd_repair_listen, SOL_SOCKET, SO_RCVTIMEO,
> > > +		       &tv, sizeof(tv))) {
> > > +		err_perror("Set timeout on TCP_REPAIR listening socket");
> > > +		return;
> > > +	}
> > > +
> > > +	repair_listen_handler(c, EPOLLIN);
> > > +
> > > +	tv.tv_usec = 0;
> > > +	if (setsockopt(c->fd_repair_listen, SOL_SOCKET, SO_RCVTIMEO,
> > > +		       &tv, sizeof(tv)))
> > > +		err_perror("Clear timeout on TCP_REPAIR listening socket");
> > > +}
> > > +
> > >  /**
> > >   * repair_flush() - Flush current set of sockets to helper, with current command
> > >   * @c:		Execution context
> > > diff --git a/repair.h b/repair.h
> > > index de279d6..1d37922 100644
> > > --- a/repair.h
> > > +++ b/repair.h
> > > @@ -10,6 +10,7 @@ void repair_sock_init(const struct ctx *c);
> > >  void repair_listen_handler(struct ctx *c, uint32_t events);
> > >  void repair_handler(struct ctx *c, uint32_t events);
> > >  void repair_close(struct ctx *c);
> > > +void repair_wait(struct ctx *c);
> > >  int repair_flush(struct ctx *c);
> > >  int repair_set(struct ctx *c, int s, int cmd);
> 

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