public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
* [PATCH] fwd: Keep listening on remaining rules if one of them fails to bind
@ 2026-10-18 15:42 Hayato Kiwata
  2026-10-25 14:30 ` Stefano Brivio
  0 siblings, 1 reply; 4+ messages in thread
From: Hayato Kiwata @ 2026-10-18 15:42 UTC (permalink / raw)
  To: passt-dev; +Cc: Hayato Kiwata

fwd_listen_sync_() breaks out of the loop as soon as fwd_sync_one()
fails. So, if a single rule fails to bind(), for example because the
address isn't assigned to the host, no rule after it in the table is
even tried, and those forwards don't work.

Reproducer:

  $ pasta --ns-ifname=tap0 --mtu=65520 --config-net --address=10.0.2.100 \
    --netmask=24 --gateway=10.0.2.2 --dns-forward=10.0.2.3 --no-map-gw \
    --tcp-ns=none --udp-ns=none --ipv4-only --tcp-ports=none \
    --udp-ports=none --conf-path=/tmp/.pasta.sock \
    -- python3 -m http.server 80
  UNIX domain socket bound at /tmp/.pasta.sock
  Serving HTTP on 0.0.0.0 port 80 (http://0.0.0.0:80/) ...

  $ pesto -A --tcp-ports=192.0.2.1/8080:80 /tmp/.pasta.sock
  $ pesto -A --tcp-ports=127.0.0.1/8085:80 /tmp/.pasta.sock
  $ pesto -s /tmp/.pasta.sock
  passt/pasta configuration (/tmp/.pasta.sock)
    HOST
      TCP [192.0.2.1]:8080  =>  80
      TCP [127.0.0.1]:8085  =>  80
    SPLICE
  $ curl -sS -o /dev/null  localhost:8085
  curl: (7) Failed to connect to localhost port 8085 after 0 ms: Could not connect to server

pasta only reports the failure for port 8080, and never tries to
listen on port 8085:

  Listen failed for HOST TCP port 192.0.2.1/8080: Cannot assign requested address
  Couldn't listen on requested ports
  Error switching to new forwarding rules

Keep going through the remaining rules if fwd_sync_one() fails, and
still return -1 if any of them failed, as before.

Signed-off-by: Hayato Kiwata <dev@haytok.jp>
---
 fwd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fwd.c b/fwd.c
index 5679a3d..6c9df01 100644
--- a/fwd.c
+++ b/fwd.c
@@ -456,9 +456,8 @@ static int fwd_listen_sync_(void *arg)
 		ns_enter(a->c);
 
 	for (i = 0; i < a->c->fwd[a->pif]->count; i++) {
-		a->ret = fwd_sync_one(a->c, a->pif, i, a->tcpmap, a->udpmap);
-		if (a->ret < 0)
-			break;
+		if (fwd_sync_one(a->c, a->pif, i, a->tcpmap, a->udpmap) < 0)
+			a->ret = -1;
 	}
 
 	return 0;
-- 
2.50.1 (Apple Git-155)


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] fwd: Keep listening on remaining rules if one of them fails to bind
  2026-10-18 15:42 [PATCH] fwd: Keep listening on remaining rules if one of them fails to bind Hayato Kiwata
@ 2026-10-25 14:30 ` Stefano Brivio
  2026-10-27 17:24   ` [PATCH] " dev
  0 siblings, 1 reply; 4+ messages in thread
From: Stefano Brivio @ 2026-10-25 14:30 UTC (permalink / raw)
  To: Hayato Kiwata, David Gibson; +Cc: passt-dev

Hi Hayato,

On Mon, 19 Oct 2026 00:42:25 +0900
Hayato Kiwata <dev@haytok.jp> wrote:

> fwd_listen_sync_() breaks out of the loop as soon as fwd_sync_one()
> fails. So, if a single rule fails to bind(), for example because the
> address isn't assigned to the host, no rule after it in the table is
> even tried, and those forwards don't work.
> 
> Reproducer:
> 
>   $ pasta --ns-ifname=tap0 --mtu=65520 --config-net --address=10.0.2.100 \
>     --netmask=24 --gateway=10.0.2.2 --dns-forward=10.0.2.3 --no-map-gw \
>     --tcp-ns=none --udp-ns=none --ipv4-only --tcp-ports=none \
>     --udp-ports=none --conf-path=/tmp/.pasta.sock \
>     -- python3 -m http.server 80
>   UNIX domain socket bound at /tmp/.pasta.sock
>   Serving HTTP on 0.0.0.0 port 80 (http://0.0.0.0:80/) ...
> 
>   $ pesto -A --tcp-ports=192.0.2.1/8080:80 /tmp/.pasta.sock
>   $ pesto -A --tcp-ports=127.0.0.1/8085:80 /tmp/.pasta.sock
>   $ pesto -s /tmp/.pasta.sock
>   passt/pasta configuration (/tmp/.pasta.sock)
>     HOST
>       TCP [192.0.2.1]:8080  =>  80
>       TCP [127.0.0.1]:8085  =>  80
>     SPLICE
>   $ curl -sS -o /dev/null  localhost:8085
>   curl: (7) Failed to connect to localhost port 8085 after 0 ms: Could not connect to server
> 
> pasta only reports the failure for port 8080, and never tries to
> listen on port 8085:
> 
>   Listen failed for HOST TCP port 192.0.2.1/8080: Cannot assign requested address
>   Couldn't listen on requested ports
>   Error switching to new forwarding rules

This was actually intended, at some point in the design phase, because
the original idea was to revert to the previous configuration in these
two cases:

1. the new rules are invalid.  This is relatively simple to do:
   conf_recv_rules() detects this and conf_handler() returns early,
   skipping the call to fwd_listen_switch(), so the switch to the new
   tables of forwarding rules doesn't happen at all, before we apply
   any change

2. the new rules are generally valid but not (entirely) working in the
   setup (like in your example).

   Here we call fwd_listen_switch() which closes the listening sockets
   for the old table, swaps the tables, and calls fwd_listen_init(),
   which returns failure, but we don't handle that because this kind of
   rollback feature isn't implemented (yet).

I think we should decide how to handle 2. and make this consistent.  As
you showed, we're not handling this consistently.

We're not sure whether a rollback after 2. would work: forwarding rules
that might have worked earlier might not work now (for example because
host addresses changed).

However, in a general case, I guess that the existing (old) rules are
still more likely to work than not.  That is, given:

a. new forwarding rules don't work because of an unintended
   configuration error

b. old forwarding rules wouldn't work anymore if re-applied because of
   a setup change

I think a. is much more likely, so a rollback would be beneficial and
give the user the chance to look at possible configuration errors
without switching to a possibly unintended state.  What do you think?

If we decide to go ahead and complete the rollback feature, I guess the
missing steps are roughly these:

1. abort on any failure of fwd_listen_init() (maintaining the current
   behaviour of fwd_listen_sync_()), and:

2. close any listening sockets (first part of fwd_listen_switch()) that
   we might have just opened

3. swap the rules again (as you can see, we already preserve the old
   rules, because we planned to complete the rollback implementation at
   some point)

4. call fwd_listen_init() on the old rules

5. make sure we report failure to the client

...given that we need 2., 3. and 4., we could probably generalise
fwd_listen_switch() and use it in reverse once we detect a failure.

About your patch itself:

> Keep going through the remaining rules if fwd_sync_one() fails, and
> still return -1 if any of them failed, as before.
> 
> Signed-off-by: Hayato Kiwata <dev@haytok.jp>
> ---
>  fwd.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fwd.c b/fwd.c
> index 5679a3d..6c9df01 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -456,9 +456,8 @@ static int fwd_listen_sync_(void *arg)
>  		ns_enter(a->c);
>  
>  	for (i = 0; i < a->c->fwd[a->pif]->count; i++) {
> -		a->ret = fwd_sync_one(a->c, a->pif, i, a->tcpmap, a->udpmap);
> -		if (a->ret < 0)
> -			break;
> +		if (fwd_sync_one(a->c, a->pif, i, a->tcpmap, a->udpmap) < 0)
> +			a->ret = -1;

...this looks reasonable to me and better than the existing behaviour,
but it's pointless to do this if we want to finish things up and add
the rollback for this case.

>  	}
>  
>  	return 0;

-- 
Stefano


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Keep listening on remaining rules if one of them fails to bind
  2026-10-25 14:30 ` Stefano Brivio
@ 2026-10-27 17:24   ` dev
  2026-10-28  7:44     ` Stefano Brivio
  0 siblings, 1 reply; 4+ messages in thread
From: dev @ 2026-10-27 17:24 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: David Gibson, passt-dev

Hi Stefano,

Thanks for the detailed explanation, and for the background on the
original design!

> We're not sure whether a rollback after 2. would work: forwarding rules
> that might have worked earlier might not work now (for example because
> host addresses changed).
>
> However, in a general case, I guess that the existing (old) rules are
> still more likely to work than not.  That is, given:
> 
> a. new forwarding rules don't work because of an unintended
>    configuration error
> 
> b. old forwarding rules wouldn't work anymore if re-applied because of
>    a setup change
>
> I think a. is much more likely, so a rollback would be beneficial and
> give the user the chance to look at possible configuration errors
> without switching to a possibly unintended state.  What do you think?

I strongly agree.

In practice, a. is far more likely. Case b. can happen depending on
what changed on the host, but that's beyond what pasta can handle, so
all we can do is report it and let the user fix the setup.

> 1. abort on any failure of fwd_listen_init() (maintaining the current
>    behaviour of fwd_listen_sync_()), and:
> 
> 2. close any listening sockets (first part of fwd_listen_switch()) that
>    we might have just opened
> 
> 3. swap the rules again (as you can see, we already preserve the old
>    rules, because we planned to complete the rollback implementation at
>    some point)
> 
> 4. call fwd_listen_init() on the old rules

I agree, and that's what my work-in-progress patch does.

> 5. make sure we report failure to the client

Here's how I'm thinking of implementing this. Please let me know if
I got something wrong:

- make fwd_listen_switch() return an int, distinguishing the case
  where we rolled back to the old rules from the case where the
  rollback failed too

- in conf_handler(), write that result to the control socket right
  after fwd_listen_switch(), before conf_close()

- in pesto, read it after send_conf(), and if the update failed,
  print that the old rules were restored and exit non-zero

For the content, I'd start with a status only (success / rolled back /
restore failed). fwd_sync_one() already logs which rule failed on the
pasta side, so returning that to pesto could be added later if needed.

Since this adds a message from the server, we need to bump
PESTO_PROTOCOL_VERSION to 3, and have pesto skip reading the result if
the server version is below 3. Is that OK?

I'm planning to send 5. as a separate patch from the rollback (1. to
4.).

> ...this looks reasonable to me and better than the existing behaviour,
> but it's pointless to do this if we want to finish things up and add
> the rollback for this case.

Agreed, I'll drop this patch.

I was already planning to implement the rollback feature, and I have a
work-in-progress patch covering steps 1. to 4. you listed. I'll send
it once it's ready.

Thanks,
Hayato


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Keep listening on remaining rules if one of them fails to bind
  2026-10-27 17:24   ` [PATCH] " dev
@ 2026-10-28  7:44     ` Stefano Brivio
  0 siblings, 0 replies; 4+ messages in thread
From: Stefano Brivio @ 2026-10-28  7:44 UTC (permalink / raw)
  To: dev; +Cc: David Gibson, passt-dev

On Wed, 28 Oct 2026 02:24:40 +0900
dev@haytok.jp wrote:

> Hi Stefano,
> 
> Thanks for the detailed explanation, and for the background on the
> original design!
> 
> > We're not sure whether a rollback after 2. would work: forwarding rules
> > that might have worked earlier might not work now (for example because
> > host addresses changed).
> >
> > However, in a general case, I guess that the existing (old) rules are
> > still more likely to work than not.  That is, given:
> > 
> > a. new forwarding rules don't work because of an unintended
> >    configuration error
> > 
> > b. old forwarding rules wouldn't work anymore if re-applied because of
> >    a setup change
> >
> > I think a. is much more likely, so a rollback would be beneficial and
> > give the user the chance to look at possible configuration errors
> > without switching to a possibly unintended state.  What do you think?  
> 
> I strongly agree.
> 
> In practice, a. is far more likely. Case b. can happen depending on
> what changed on the host, but that's beyond what pasta can handle, so
> all we can do is report it and let the user fix the setup.
> 
> > 1. abort on any failure of fwd_listen_init() (maintaining the current
> >    behaviour of fwd_listen_sync_()), and:
> > 
> > 2. close any listening sockets (first part of fwd_listen_switch()) that
> >    we might have just opened
> > 
> > 3. swap the rules again (as you can see, we already preserve the old
> >    rules, because we planned to complete the rollback implementation at
> >    some point)
> > 
> > 4. call fwd_listen_init() on the old rules  
> 
> I agree, and that's what my work-in-progress patch does.
> 
> > 5. make sure we report failure to the client  
> 
> Here's how I'm thinking of implementing this. Please let me know if
> I got something wrong:
> 
> - make fwd_listen_switch() return an int, distinguishing the case
>   where we rolled back to the old rules from the case where the
>   rollback failed too

It needs to be able to return failure, for sure.  We don't _strictly_
need to distinguish the two types of failures, because if the rollback
fails, there's nothing we can do, and we can ignore that failure.

On the other hand, it might be nice to report it to the user via pesto,
so yes, it would probably be nice to distinguish the types of failure.

By the way of fwd_listen_switch(), it could simply be called a second
time to implement a rollback (the old rules are preserved in
c->fwd_pending), but the error messages wouldn't match anymore.

I see a few possible solutions for that: error messages could be moved
the caller (perhaps my favourite option at the moment), or we could
pass an 'bool rollback' argument, or move most of the logic to a
separate function and introduce an intermediate caller... I'm not sure.
I guess it would be better to try things out and see how they look like.

> - in conf_handler(), write that result to the control socket right
>   after fwd_listen_switch(), before conf_close()
> 
> - in pesto, read it after send_conf(), and if the update failed,
>   print that the old rules were restored and exit non-zero

Both steps make sense to me.

> For the content, I'd start with a status only (success / rolled back /
> restore failed). fwd_sync_one() already logs which rule failed on the
> pasta side, so returning that to pesto could be added later if needed.

Right, yes, given that we distribute client and server together, we
don't have to really plan much in advance or be particularly
future-proof.  We can just bump versions as needed.

The day somebody else starts developing other clients (for example to
embed in container engines or virtual machine managers), then we'll
have to think again about this, but it's not the case for the moment.

> Since this adds a message from the server, we need to bump
> PESTO_PROTOCOL_VERSION to 3, and have pesto skip reading the result if
> the server version is below 3. Is that OK?

Definitely, yes, that's how versioning was meant to be used.

> I'm planning to send 5. as a separate patch from the rollback (1. to
> 4.).

Right, probably easier to review.

> > ...this looks reasonable to me and better than the existing behaviour,
> > but it's pointless to do this if we want to finish things up and add
> > the rollback for this case.  
> 
> Agreed, I'll drop this patch.
> 
> I was already planning to implement the rollback feature, and I have a
> work-in-progress patch covering steps 1. to 4. you listed. I'll send
> it once it's ready.

That sounds like a plan. Thanks for working on this!

-- 
Stefano


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-10-28  7:44 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-18 15:42 [PATCH] fwd: Keep listening on remaining rules if one of them fails to bind Hayato Kiwata
2026-10-25 14:30 ` Stefano Brivio
2026-10-27 17:24   ` [PATCH] " dev
2026-10-28  7:44     ` Stefano Brivio

Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).