From: Stefano Brivio <sbrivio@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: passt-dev@passt.top
Subject: Re: [PATCH v3 12/14] fwd: Remap ports based directly on forwarding rule
Date: Tue, 13 Jan 2026 23:12:26 +0100 [thread overview]
Message-ID: <20260113231226.3b7fba24@elisabeth> (raw)
In-Reply-To: <20260108022948.2657573-13-david@gibson.dropbear.id.au>
On Thu, 8 Jan 2026 13:29:46 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> Currently we remap port numbers based on the legacy delta[] array, which
> is indexed only by original port number, not the listening address. Now
> that we look up a forwarding rule entry in flow_target(), we can use this
> entry to directly determine the correct remapped port. Implement this,
> and remove the old delta[] array.
>
> Link: https://bugs.passt.top/show_bug.cgi?id=187
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> flow.c | 7 ++++---
> fwd.c | 21 +++++++--------------
> fwd.h | 7 +++----
> 3 files changed, 14 insertions(+), 21 deletions(-)
>
> diff --git a/flow.c b/flow.c
> index 045e9712..38f88732 100644
> --- a/flow.c
> +++ b/flow.c
> @@ -493,6 +493,7 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow,
> struct flow_common *f = &flow->f;
> const struct flowside *ini = &f->side[INISIDE];
> struct flowside *tgt = &f->side[TGTSIDE];
> + const struct fwd_rule *rule = NULL;
Coverity Scan complains about two cases where this NULL rule would be
passed to NAT functions dereferencing it. They should be both false
positives because the rule is always set on pif_is_socket(...), but I
wonder how fragile it is.
Regardless, it would be nice to avoid adding further warnings, if it's
cheap (ASSERT() or check on rule in fwd_nat_from*() functions?). Anyway,
here you go:
/home/sbrivio/passt/flow.c:535:3:
Type: Explicit null dereferenced (FORWARD_NULL)
/home/sbrivio/passt/flow.c:496:2:
1. assign_zero: Assigning: "rule" = "NULL".
/home/sbrivio/passt/flow.c:499:2:
2. path: Condition "flow_new_entry == flow", taking true branch.
/home/sbrivio/passt/flow.c:499:2:
3. path: Condition "f->state == FLOW_STATE_INI", taking true branch.
/home/sbrivio/passt/flow.c:500:2:
4. path: Condition "f->type == FLOW_TYPE_NONE", taking true branch.
/home/sbrivio/passt/flow.c:501:2:
5. path: Condition "f->pif[0] != PIF_NONE", taking true branch.
/home/sbrivio/passt/flow.c:501:2:
6. path: Condition "f->pif[1] == PIF_NONE", taking true branch.
/home/sbrivio/passt/flow.c:502:2:
7. path: Condition "flow->f.state == FLOW_STATE_INI", taking true branch.
/home/sbrivio/passt/flow.c:504:2:
8. path: Condition "pif_is_socket(f->pif[0])", taking false branch.
/home/sbrivio/passt/flow.c:528:2:
9. path: Switch case value "PIF_SPLICE".
/home/sbrivio/passt/flow.c:535:3:
10. var_deref_model: Passing null pointer "rule" to "fwd_nat_from_splice", which dereferences it.
/home/sbrivio/passt/fwd.c:991:2:
10.1. path: Condition "!inany_is_loopback(&ini->eaddr)", taking false branch.
/home/sbrivio/passt/fwd.c:991:2:
10.2. path: Condition "!inany_is_loopback(&ini->oaddr)", taking false branch.
/home/sbrivio/passt/fwd.c:1008:2:
10.3. path: Condition "proto == IPPROTO_UDP", taking true branch.
/home/sbrivio/passt/fwd.c:1012:2:
10.4. dereference: Dereferencing pointer "rule".
/home/sbrivio/passt/flow.c:539:3:
Type: Explicit null dereferenced (FORWARD_NULL)
/home/sbrivio/passt/flow.c:496:2:
1. assign_zero: Assigning: "rule" = "NULL".
/home/sbrivio/passt/flow.c:499:2:
2. path: Condition "flow_new_entry == flow", taking true branch.
/home/sbrivio/passt/flow.c:499:2:
3. path: Condition "f->state == FLOW_STATE_INI", taking true branch.
/home/sbrivio/passt/flow.c:500:2:
4. path: Condition "f->type == FLOW_TYPE_NONE", taking true branch.
/home/sbrivio/passt/flow.c:501:2:
5. path: Condition "f->pif[0] != PIF_NONE", taking true branch.
/home/sbrivio/passt/flow.c:501:2:
6. path: Condition "f->pif[1] == PIF_NONE", taking true branch.
/home/sbrivio/passt/flow.c:502:2:
7. path: Condition "flow->f.state == FLOW_STATE_INI", taking true branch.
/home/sbrivio/passt/flow.c:504:2:
8. path: Condition "pif_is_socket(f->pif[0])", taking false branch.
/home/sbrivio/passt/flow.c:528:2:
9. path: Switch case value "PIF_HOST".
/home/sbrivio/passt/flow.c:539:3:
10. var_deref_model: Passing null pointer "rule" to "fwd_nat_from_host", which dereferences it.
/home/sbrivio/passt/fwd.c:1069:2:
10.1. dereference: Dereferencing pointer "rule".
> uint8_t tgtpif = PIF_NONE;
>
> ASSERT(flow_new_entry == flow && f->state == FLOW_STATE_INI);
> @@ -514,7 +515,7 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow,
> else
> goto nofwd;
>
> - if (!fwd_rule_search(fwd, ini)) {
> + if (!(rule = fwd_rule_search(fwd, ini))) {
> /* This shouldn't happen, because if there's no rule for
> * it we should have no listening socket that would let
> * us get here
> @@ -531,11 +532,11 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow,
> break;
>
> case PIF_SPLICE:
> - tgtpif = fwd_nat_from_splice(c, proto, ini, tgt);
> + tgtpif = fwd_nat_from_splice(rule, proto, ini, tgt);
> break;
>
> case PIF_HOST:
> - tgtpif = fwd_nat_from_host(c, proto, ini, tgt);
> + tgtpif = fwd_nat_from_host(c, rule, proto, ini, tgt);
> fwd_neigh_mac_get(c, &tgt->oaddr, f->tap_omac);
> break;
> default:
> diff --git a/fwd.c b/fwd.c
> index 633ba5db..7c4575ff 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -405,7 +405,6 @@ void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags,
> /* Fill in the legacy forwarding data structures to match the table */
> if (!(new->flags & FWD_SCAN))
> bitmap_set(fwd->map, port);
> - fwd->delta[port] = new->to - new->first;
> }
> }
>
> @@ -978,7 +977,7 @@ uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
>
> /**
> * fwd_nat_from_splice() - Determine to forward a flow from the splice interface
> - * @c: Execution context
> + * @rule: Forwarding rule to apply
> * @proto: Protocol (IP L4 protocol number)
> * @ini: Flow address information of the initiating side
> * @tgt: Flow address information on the target side (updated)
> @@ -986,7 +985,7 @@ uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
> * Return: pif of the target interface to forward the flow to, PIF_NONE if the
> * flow cannot or should not be forwarded at all.
> */
> -uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto,
> +uint8_t fwd_nat_from_splice(const struct fwd_rule *rule, uint8_t proto,
> const struct flowside *ini, struct flowside *tgt)
> {
> if (!inany_is_loopback(&ini->eaddr) ||
> @@ -1010,11 +1009,7 @@ uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto,
> /* But for UDP preserve the source port */
> tgt->oport = ini->eport;
>
> - tgt->eport = ini->oport;
> - if (proto == IPPROTO_TCP)
> - tgt->eport += c->tcp.fwd_out.delta[tgt->eport];
> - else if (proto == IPPROTO_UDP)
> - tgt->eport += c->udp.fwd_out.delta[tgt->eport];
> + tgt->eport = ini->oport - rule->first + rule->to;
This made me notice that the current documentation to struct fwd_rule
doesn't really make it clear that n : 1 port mapping rules are not a
thing, so, maybe, back to 2/14, instead of:
* @to: Port number to forward port @first to.
we could have perhaps:
* @to: Target port for @first, port n maps to @to + (n - @first)
?
By the way, I find this notation a bit complicated to figure out, I
think that:
rule->to + (ini->oport - rule->first)
(redundant parentheses included) is actually clearer. Or maybe even
with a temporary 'delta' variable.
In both cases: the subtraction now happens in in_port_t, so I guess we
should explicitly cast rule->first to int to be strictly correct.
> return PIF_HOST;
> }
> @@ -1058,6 +1053,7 @@ bool nat_inbound(const struct ctx *c, const union inany_addr *addr,
> /**
> * fwd_nat_from_host() - Determine to forward a flow from the host interface
> * @c: Execution context
> + * @rule: Forwarding rule to apply
> * @proto: Protocol (IP L4 protocol number)
> * @ini: Flow address information of the initiating side
> * @tgt: Flow address information on the target side (updated)
> @@ -1065,15 +1061,12 @@ bool nat_inbound(const struct ctx *c, const union inany_addr *addr,
> * Return: pif of the target interface to forward the flow to, PIF_NONE if the
> * flow cannot or should not be forwarded at all.
> */
> -uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto,
> +uint8_t fwd_nat_from_host(const struct ctx *c,
> + const struct fwd_rule *rule, uint8_t proto,
> const struct flowside *ini, struct flowside *tgt)
> {
> /* Common for spliced and non-spliced cases */
> - tgt->eport = ini->oport;
> - if (proto == IPPROTO_TCP)
> - tgt->eport += c->tcp.fwd_in.delta[tgt->eport];
> - else if (proto == IPPROTO_UDP)
> - tgt->eport += c->udp.fwd_in.delta[tgt->eport];
> + tgt->eport = ini->oport - rule->first + rule->to;
Same as above.
>
> if (!c->no_splice && inany_is_loopback(&ini->eaddr) &&
> (proto == IPPROTO_TCP || proto == IPPROTO_UDP)) {
> diff --git a/fwd.h b/fwd.h
> index a10bdbb4..cfe9ed46 100644
> --- a/fwd.h
> +++ b/fwd.h
> @@ -82,7 +82,6 @@ enum fwd_ports_mode {
> * @count: Number of forwarding rules
> * @rules: Array of forwarding rules
> * @map: Bitmap describing which ports are forwarded
> - * @delta: Offset between the original destination and mapped port number
> * @listen_sock_count: Number of entries used in @listen_socks
> * @listen_socks: Listening sockets for forwarding
> */
> @@ -93,7 +92,6 @@ struct fwd_ports {
> unsigned count;
> struct fwd_rule rules[MAX_FWD_RULES];
> uint8_t map[PORT_BITMAP_SIZE];
> - in_port_t delta[NUM_PORTS];
> unsigned listen_sock_count;
> int listen_socks[MAX_LISTEN_SOCKS];
> };
> @@ -117,9 +115,10 @@ bool nat_inbound(const struct ctx *c, const union inany_addr *addr,
> union inany_addr *translated);
> uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
> const struct flowside *ini, struct flowside *tgt);
> -uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto,
> +uint8_t fwd_nat_from_splice(const struct fwd_rule *rule, uint8_t proto,
> const struct flowside *ini, struct flowside *tgt);
> -uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto,
> +uint8_t fwd_nat_from_host(const struct ctx *c,
> + const struct fwd_rule *rule, uint8_t proto,
> const struct flowside *ini, struct flowside *tgt);
> void fwd_neigh_table_update(const struct ctx *c, const union inany_addr *addr,
> const uint8_t *mac, bool permanent);
--
Stefano
next prev parent reply other threads:[~2026-01-13 22:12 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-01-08 2:29 [PATCH v3 00/14] Introduce forwarding table David Gibson
2026-01-08 2:29 ` [PATCH v3 01/14] inany: Extend inany_ntop() to treat NULL as a fully unspecified address David Gibson
2026-01-08 13:16 ` Laurent Vivier
2026-01-08 2:29 ` [PATCH v3 02/14] conf, fwd: Keep a table of our port forwarding configuration David Gibson
2026-01-12 23:26 ` Stefano Brivio
2026-01-13 5:12 ` David Gibson
2026-01-13 22:13 ` Stefano Brivio
2026-01-13 23:53 ` David Gibson
2026-01-08 2:29 ` [PATCH v3 03/14] conf: Accurately record ifname and address for outbound forwards David Gibson
2026-01-08 2:29 ` [PATCH v3 04/14] conf, fwd: Record "auto" port forwards in forwarding table David Gibson
2026-01-12 23:26 ` Stefano Brivio
2026-01-13 5:14 ` David Gibson
2026-01-08 2:29 ` [PATCH v3 05/14] fwd: Make space to store listening sockets in forward table David Gibson
2026-01-12 23:26 ` Stefano Brivio
2026-01-13 5:28 ` David Gibson
2026-01-13 22:13 ` Stefano Brivio
2026-01-13 23:57 ` David Gibson
2026-01-08 2:29 ` [PATCH v3 06/14] ip: Add ipproto_name() function David Gibson
2026-01-08 13:22 ` Laurent Vivier
2026-01-08 23:12 ` David Gibson
2026-01-08 2:29 ` [PATCH v3 07/14] fwd, tcp, udp: Set up listening sockets based on forward table David Gibson
2026-01-12 23:26 ` Stefano Brivio
2026-01-13 5:38 ` David Gibson
2026-01-13 22:13 ` Stefano Brivio
2026-01-13 23:59 ` David Gibson
2026-01-08 2:29 ` [PATCH v3 08/14] tcp, udp: Remove old auto-forwarding socket arrays David Gibson
2026-01-08 2:29 ` [PATCH v3 09/14] conf, fwd: Check forwarding table for conflicting rules David Gibson
2026-01-12 23:26 ` Stefano Brivio
2026-01-13 5:41 ` David Gibson
2026-01-08 2:29 ` [PATCH v3 10/14] fwd: Generate auto-forward exclusions from socket fd tables David Gibson
2026-01-08 2:29 ` [PATCH v3 11/14] flow, fwd: Consult rules table when forwarding a new flow from socket David Gibson
2026-01-13 22:12 ` Stefano Brivio
2026-01-14 0:09 ` David Gibson
2026-01-08 2:29 ` [PATCH v3 12/14] fwd: Remap ports based directly on forwarding rule David Gibson
2026-01-13 22:12 ` Stefano Brivio [this message]
2026-01-14 0:24 ` David Gibson
2026-01-08 2:29 ` [PATCH v3 13/14] fwd, tcp, udp: Add forwarding rule to listening socket epoll references David Gibson
2026-01-13 22:12 ` Stefano Brivio
2026-01-14 0:37 ` David Gibson
2026-01-08 2:29 ` [PATCH v3 14/14] flow, fwd: Optimise forwarding rule lookup using epoll ref when possible David Gibson
2026-01-13 22:13 ` Stefano Brivio
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260113231226.3b7fba24@elisabeth \
--to=sbrivio@redhat.com \
--cc=david@gibson.dropbear.id.au \
--cc=passt-dev@passt.top \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).