[PATCH 0/3] Fix regression in auto port forwarding

[PATCH 0/3] Fix regression in auto port forwarding

[David Gibson]

My recently merged series cleaning up the auto-port-forward scanning
contained a series bug: automatic forwards would only appear on
alternating seconds.

This turned out to be due to a fundamentally broken premise in my
thinking.  I was thinking that for consistency we wanted the most
recent port map information throughout the process.  But that's not
really true: for the purposes of exclusion what we really need to know
is which of the listening sockets we scan are ours.  That's given by
the *prior* state of the forward maps, not the updated one based on a
new scan.

The series also had a number of worthwhile changes though.  This
series fixes it up, by reverting the most misguided of the patches and
correcting behaviour of another one while preserving the accompanying
code re-orgs.

Link: https://bugs.passt.top/show_bug.cgi?id=176

David Gibson (3):
  Revert "fwd: Update all port maps before applying exclusions"
  fwd: Exclude ports based on prior mapping state
  fwd: Don't explicitly exclude reverse-direction TCP ports for UDP

 fwd.c | 44 ++++++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

-- 
2.51.1

[PATCH 1/3] Revert "fwd: Update all port maps before applying exclusions"

[David Gibson]

This reverts commit 81942a2417357ff10b02ccc8275cde2d4d6fbfbe.

That commit was based on the premise of trying to make all the exclusions
use the "latest" scan data.  That was a fundamentally wrong approach: what
we need to exclude is listening ports that pasta itself has created.  That
is, we need to exclude ports that we were _already_ listening on, not ones
that we intend to listen once we rebind - we *want* the old data.

Reverting this reduces the cases in which bug 176 occurs, but it's not a
complete fix.

Link: https://bugs.passt.top/show_bug.cgi?id=176

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd.c | 47 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/fwd.c b/fwd.c
index 68bb1166..7b6c40fb 100644
--- a/fwd.c
+++ b/fwd.c
@@ -358,8 +358,10 @@ static void procfs_scan_listen(int fd, unsigned int lstate, uint8_t *map)
 /**
  * fwd_scan_ports_tcp() - Scan /proc to update TCP forwarding map
  * @fwd:	Forwarding information to update
+ * @rev:	Forwarding information for the reverse direction
  */
-static void fwd_scan_ports_tcp(struct fwd_ports *fwd)
+static void fwd_scan_ports_tcp(struct fwd_ports *fwd,
+			       const struct fwd_ports *rev)
 {
 	if (fwd->mode != FWD_AUTO)
 		return;
@@ -367,15 +369,20 @@ static void fwd_scan_ports_tcp(struct fwd_ports *fwd)
 	memset(fwd->map, 0, PORT_BITMAP_SIZE);
 	procfs_scan_listen(fwd->scan4, TCP_LISTEN, fwd->map);
 	procfs_scan_listen(fwd->scan6, TCP_LISTEN, fwd->map);
+	bitmap_and_not(fwd->map, PORT_BITMAP_SIZE, fwd->map, rev->map);
 }
 
 /**
  * fwd_scan_ports_udp() - Scan /proc to update UDP forwarding map
  * @fwd:	Forwarding information to update
+ * @rev:	Forwarding information for the reverse direction
  * @tcp_fwd:	Corresponding TCP forwarding information
+ * @tcp_rev:	TCP forwarding information for the reverse direction
  */
 static void fwd_scan_ports_udp(struct fwd_ports *fwd,
-			       const struct fwd_ports *tcp_fwd)
+			       const struct fwd_ports *rev,
+			       const struct fwd_ports *tcp_fwd,
+			       const struct fwd_ports *tcp_rev)
 {
 	if (fwd->mode != FWD_AUTO)
 		return;
@@ -391,6 +398,14 @@ static void fwd_scan_ports_udp(struct fwd_ports *fwd,
 	 */
 	procfs_scan_listen(tcp_fwd->scan4, TCP_LISTEN, fwd->map);
 	procfs_scan_listen(tcp_fwd->scan6, TCP_LISTEN, fwd->map);
+
+	/* This means we need to skip numbers of TCP ports bound on the other
+	 * side, too. Otherwise, we would detect corresponding UDP ports as
+	 * bound and try to forward them from the opposite side, but it's
+	 * already us handling them.
+	 */
+	bitmap_and_not(fwd->map, PORT_BITMAP_SIZE, fwd->map, rev->map);
+	bitmap_and_not(fwd->map, PORT_BITMAP_SIZE, fwd->map, tcp_rev->map);
 }
 
 /**
@@ -399,28 +414,12 @@ static void fwd_scan_ports_udp(struct fwd_ports *fwd,
  */
 static void fwd_scan_ports(struct ctx *c)
 {
-	fwd_scan_ports_tcp(&c->tcp.fwd_out);
-	fwd_scan_ports_tcp(&c->tcp.fwd_in);
-	fwd_scan_ports_udp(&c->udp.fwd_out, &c->tcp.fwd_out);
-	fwd_scan_ports_udp(&c->udp.fwd_in, &c->tcp.fwd_in);
-
-	if (c->tcp.fwd_out.mode == FWD_AUTO) {
-		bitmap_and_not(c->tcp.fwd_out.map, PORT_BITMAP_SIZE,
-			       c->tcp.fwd_out.map, c->tcp.fwd_in.map);
-	}
-	if (c->tcp.fwd_in.mode == FWD_AUTO) {
-		bitmap_and_not(c->tcp.fwd_in.map, PORT_BITMAP_SIZE,
-			       c->tcp.fwd_in.map, c->tcp.fwd_out.map);
-	}
-
-	if (c->udp.fwd_out.mode == FWD_AUTO) {
-		bitmap_and_not(c->udp.fwd_out.map, PORT_BITMAP_SIZE,
-			       c->udp.fwd_out.map, c->udp.fwd_in.map);
-	}
-	if (c->udp.fwd_in.mode == FWD_AUTO) {
-		bitmap_and_not(c->udp.fwd_in.map, PORT_BITMAP_SIZE,
-			       c->udp.fwd_in.map, c->udp.fwd_out.map);
-	}
+	fwd_scan_ports_tcp(&c->tcp.fwd_out, &c->tcp.fwd_in);
+	fwd_scan_ports_tcp(&c->tcp.fwd_in, &c->tcp.fwd_out);
+	fwd_scan_ports_udp(&c->udp.fwd_out, &c->udp.fwd_in,
+			   &c->tcp.fwd_out, &c->tcp.fwd_in);
+	fwd_scan_ports_udp(&c->udp.fwd_in, &c->udp.fwd_out,
+			   &c->tcp.fwd_in, &c->tcp.fwd_out);
 }
 
 /**
-- 
2.51.1

[PATCH 2/3] fwd: Exclude ports based on prior mapping state

[David Gibson]

With auto port-forwarding modes we scan for listening ports on the host
and/or guest and create forwardings for them.  To avoid circular forwarding
we need to exclude our own listening ports.  We do this by masking out
the forwarding map for one direction from the other.

Since 1bc7d5485c10, some of our scans take place while the forward maps are
out of sync with what our actual listening ports are though: the map
represents what we intend to forward shortly, rather than what we have
open sockets for right now.

What we have sockets for right now is what matters for the purposes of
excluding from the scan, though, so that was incorrect.  So, restore
correct behaviour by saving the map of ports to exclude before we start
updating any of the forwarding maps with new scans.  This allows us to
keep all the scans separate from all the rebinds, and therefore several
minor cleanups that permitted.

As a bonus, pre-creating the exclusion bitmaps this way should make this
code easier to adapt as we change the forwarding data structures to allow
more flexible configuration.

Fixes: 1bc7d5485c10 ("fwd: Consolidate scans (not rebinds) in fwd.c")
Link: https://bugs.passt.top/show_bug.cgi?id=176

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd.c  | 41 ++++++++++++++++++++---------------------
 util.c |  1 -
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/fwd.c b/fwd.c
index 7b6c40fb..c7a880eb 100644
--- a/fwd.c
+++ b/fwd.c
@@ -358,10 +358,9 @@ static void procfs_scan_listen(int fd, unsigned int lstate, uint8_t *map)
 /**
  * fwd_scan_ports_tcp() - Scan /proc to update TCP forwarding map
  * @fwd:	Forwarding information to update
- * @rev:	Forwarding information for the reverse direction
+ * @exclude:	Ports to _not_ forward
  */
-static void fwd_scan_ports_tcp(struct fwd_ports *fwd,
-			       const struct fwd_ports *rev)
+static void fwd_scan_ports_tcp(struct fwd_ports *fwd, const uint8_t *exclude)
 {
 	if (fwd->mode != FWD_AUTO)
 		return;
@@ -369,20 +368,18 @@ static void fwd_scan_ports_tcp(struct fwd_ports *fwd,
 	memset(fwd->map, 0, PORT_BITMAP_SIZE);
 	procfs_scan_listen(fwd->scan4, TCP_LISTEN, fwd->map);
 	procfs_scan_listen(fwd->scan6, TCP_LISTEN, fwd->map);
-	bitmap_and_not(fwd->map, PORT_BITMAP_SIZE, fwd->map, rev->map);
+	bitmap_and_not(fwd->map, PORT_BITMAP_SIZE, fwd->map, exclude);
 }
 
 /**
  * fwd_scan_ports_udp() - Scan /proc to update UDP forwarding map
  * @fwd:	Forwarding information to update
- * @rev:	Forwarding information for the reverse direction
  * @tcp_fwd:	Corresponding TCP forwarding information
- * @tcp_rev:	TCP forwarding information for the reverse direction
+ * @exclude:	Ports to _not_ forward
  */
 static void fwd_scan_ports_udp(struct fwd_ports *fwd,
-			       const struct fwd_ports *rev,
 			       const struct fwd_ports *tcp_fwd,
-			       const struct fwd_ports *tcp_rev)
+			       const uint8_t *exclude)
 {
 	if (fwd->mode != FWD_AUTO)
 		return;
@@ -399,13 +396,7 @@ static void fwd_scan_ports_udp(struct fwd_ports *fwd,
 	procfs_scan_listen(tcp_fwd->scan4, TCP_LISTEN, fwd->map);
 	procfs_scan_listen(tcp_fwd->scan6, TCP_LISTEN, fwd->map);
 
-	/* This means we need to skip numbers of TCP ports bound on the other
-	 * side, too. Otherwise, we would detect corresponding UDP ports as
-	 * bound and try to forward them from the opposite side, but it's
-	 * already us handling them.
-	 */
-	bitmap_and_not(fwd->map, PORT_BITMAP_SIZE, fwd->map, rev->map);
-	bitmap_and_not(fwd->map, PORT_BITMAP_SIZE, fwd->map, tcp_rev->map);
+	bitmap_and_not(fwd->map, PORT_BITMAP_SIZE, fwd->map, exclude);
 }
 
 /**
@@ -414,12 +405,20 @@ static void fwd_scan_ports_udp(struct fwd_ports *fwd,
  */
 static void fwd_scan_ports(struct ctx *c)
 {
-	fwd_scan_ports_tcp(&c->tcp.fwd_out, &c->tcp.fwd_in);
-	fwd_scan_ports_tcp(&c->tcp.fwd_in, &c->tcp.fwd_out);
-	fwd_scan_ports_udp(&c->udp.fwd_out, &c->udp.fwd_in,
-			   &c->tcp.fwd_out, &c->tcp.fwd_in);
-	fwd_scan_ports_udp(&c->udp.fwd_in, &c->udp.fwd_out,
-			   &c->tcp.fwd_in, &c->tcp.fwd_out);
+	uint8_t excl_tcp_out[PORT_BITMAP_SIZE], excl_udp_out[PORT_BITMAP_SIZE];
+	uint8_t excl_tcp_in[PORT_BITMAP_SIZE], excl_udp_in[PORT_BITMAP_SIZE];
+
+	memcpy(excl_tcp_out, c->tcp.fwd_in.map, sizeof(excl_tcp_out));
+	memcpy(excl_tcp_in, c->tcp.fwd_out.map, sizeof(excl_tcp_in));
+	bitmap_or(excl_udp_out, PORT_BITMAP_SIZE,
+		  c->udp.fwd_in.map, c->tcp.fwd_in.map);
+	bitmap_or(excl_udp_in, PORT_BITMAP_SIZE,
+		  c->udp.fwd_out.map, c->tcp.fwd_out.map);
+
+	fwd_scan_ports_tcp(&c->tcp.fwd_out, excl_tcp_out);
+	fwd_scan_ports_tcp(&c->tcp.fwd_in, excl_tcp_in);
+	fwd_scan_ports_udp(&c->udp.fwd_out, &c->tcp.fwd_out, excl_udp_out);
+	fwd_scan_ports_udp(&c->udp.fwd_in, &c->tcp.fwd_in, excl_udp_in);
 }
 
 /**
diff --git a/util.c b/util.c
index ab23463b..7944a495 100644
--- a/util.c
+++ b/util.c
@@ -338,7 +338,6 @@ bool bitmap_isset(const uint8_t *map, unsigned bit)
  * @a:		First operand
  * @b:		Second operand
  */
-/* cppcheck-suppress unusedFunction */
 void bitmap_or(uint8_t *dst, size_t size, const uint8_t *a, const uint8_t *b)
 {
 	unsigned long *dw = (unsigned long *)dst;
-- 
2.51.1

[PATCH 3/3] fwd: Don't explicitly exclude reverse-direction TCP ports for UDP

[David Gibson]

In auto-forwarding mode, we forward UDP ports for which there isn't (yet)
a listening UDP port on the other side, but where there is a listening
TCP socket for the same port number.  This is useful for certain protocols
such as iperf3.

Correspondinly, when excluding ports from forwarding, we also exclude TCP
ports from the other direction.  That sounds like it makes sense, but is
unnecessary: for the purposes of exclusion, we don't care why we have a
listening UDP socket for that port, just whether we have one.  That is
already incorporated into the UDP bitmap alone.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd.c  | 6 ++----
 util.c | 1 +
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fwd.c b/fwd.c
index c7a880eb..c417e0f5 100644
--- a/fwd.c
+++ b/fwd.c
@@ -410,10 +410,8 @@ static void fwd_scan_ports(struct ctx *c)
 
 	memcpy(excl_tcp_out, c->tcp.fwd_in.map, sizeof(excl_tcp_out));
 	memcpy(excl_tcp_in, c->tcp.fwd_out.map, sizeof(excl_tcp_in));
-	bitmap_or(excl_udp_out, PORT_BITMAP_SIZE,
-		  c->udp.fwd_in.map, c->tcp.fwd_in.map);
-	bitmap_or(excl_udp_in, PORT_BITMAP_SIZE,
-		  c->udp.fwd_out.map, c->tcp.fwd_out.map);
+	memcpy(excl_udp_out, c->udp.fwd_in.map, sizeof(excl_udp_out));
+	memcpy(excl_udp_in, c->udp.fwd_out.map, sizeof(excl_udp_in));
 
 	fwd_scan_ports_tcp(&c->tcp.fwd_out, excl_tcp_out);
 	fwd_scan_ports_tcp(&c->tcp.fwd_in, excl_tcp_in);
diff --git a/util.c b/util.c
index 7944a495..ab23463b 100644
--- a/util.c
+++ b/util.c
@@ -338,6 +338,7 @@ bool bitmap_isset(const uint8_t *map, unsigned bit)
  * @a:		First operand
  * @b:		Second operand
  */
+/* cppcheck-suppress unusedFunction */
 void bitmap_or(uint8_t *dst, size_t size, const uint8_t *a, const uint8_t *b)
 {
 	unsigned long *dw = (unsigned long *)dst;
-- 
2.51.1

Re: [PATCH 0/3] Fix regression in auto port forwarding

[Stefano Brivio]

On Wed, 19 Nov 2025 15:26:31 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> My recently merged series cleaning up the auto-port-forward scanning
> contained a series bug: automatic forwards would only appear on
> alternating seconds.
> 
> This turned out to be due to a fundamentally broken premise in my
> thinking.  I was thinking that for consistency we wanted the most
> recent port map information throughout the process.  But that's not
> really true: for the purposes of exclusion what we really need to know
> is which of the listening sockets we scan are ours.  That's given by
> the *prior* state of the forward maps, not the updated one based on a
> new scan.
> 
> The series also had a number of worthwhile changes though.  This
> series fixes it up, by reverting the most misguided of the patches and
> correcting behaviour of another one while preserving the accompanying
> code re-orgs.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=176
> 
> David Gibson (3):
>   Revert "fwd: Update all port maps before applying exclusions"
>   fwd: Exclude ports based on prior mapping state
>   fwd: Don't explicitly exclude reverse-direction TCP ports for UDP

Applied.

-- 
Stefano