Re: [PATCH 1/3] fwd_rule: Parse target adddresses for forwarding rules

On Thu, 2 Jul 2026 15:21:29 +1000 David Gibson wrote:

On Thu, Jul 02, 2026 at 06:58:23AM +0200, Stefano Brivio wrote:

...

Nit, in the title: addresses.

Received this moments before I was going to send out a v2 :)

Fixed the title.

...

On Wed, 1 Jul 2026 17:08:09 +1000 David Gibson wrote:

...

Extend the parsing of forwarding rules (-[tu]) to allow the destination address on the target side to be specified. For now just parse them, and give an error if we try to create rules with a specified target address. We'll implement the actual forwarding logic in another patch.

Format (for either command line or pesto): -t 2222:192.0.2.1/2222

This should work along with all the other bits, that is, say: -t 192.0.2.1%eth0/2222-2225:192.0.2.2/22-25

FIXME: Ban for -[TU] for now FIXME: Check interaction with splice handling

Signed-off-by: Stefano Brivio [dwg: Syntax from Stefano's earlier draft, largely rewritten on top of new parsing helpers] Signed-off-by: David Gibson --- fwd_rule.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/fwd_rule.c b/fwd_rule.c index ca409eaf..e8abc884 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -378,14 +378,17 @@ int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new) * @first: First port to forward * @last: Last port to forward * @exclude: Bitmap of ports to exclude (may be NULL) - * @to: Port to translate @first to when forwarding + * @tgt_addr: Destination address on the target side + * @tgt_first: Destination port to use for @first on the traget side

Nit: target.

Fixed.

...

...

* @flags: Flags for forwarding entries */ static void fwd_rule_range_except(struct fwd_table *fwd, bool del, uint8_t proto, const union inany_addr *addr, const char *ifname, uint16_t first, uint16_t last, - const uint8_t *exclude, uint16_t to, + const uint8_t *exclude, + const union inany_addr *tgt_addr, + uint16_t tgt_first, uint8_t flags) { struct fwd_rule rule = { @@ -395,9 +398,17 @@ static void fwd_rule_range_except(struct fwd_table *fwd, bool del, .flags = flags, }; char rulestr[FWD_RULE_STRLEN]; - unsigned delta = to - first; + unsigned delta = tgt_first - first;

Nit: should be moved above now.

Fixed.

...

...

unsigned base, i;

+ if (tgt_addr && !inany_is_unspecified(tgt_addr)) { + char astr[INANY_ADDRSTRLEN]; + + info("Target address: %s", + inany_ntop(tgt_addr, astr, sizeof(astr))); + die("Target address remapping not yet implemented"); + } + if (!addr) rule.flags |= FWD_DUAL_STACK_ANY; if (ifname) { @@ -458,14 +469,17 @@ enum fwd_port_chunk_kind { * @cursor: Parsing point (see parse.c) * @kindp: Updated with kind of chunk we parsed * @lrange: Updated with listening port range (for INCLUDE & EXCLUDE) + * @taddr: Updated with target address (for INCLUDE) * @trange: Updated with target port range (for INCLUDE) */ static bool parse_port_chunk(const char **cursor, enum fwd_port_chunk_kind *kindp, struct port_range *lrange, + union inany_addr *taddr, struct port_range *trange) { struct port_range lr = { 0 }, tr = { 0 }; + union inany_addr taddr_tmp = inany_any6; enum fwd_port_chunk_kind kind; const char *p = *cursor;

@@ -481,6 +495,12 @@ static bool parse_port_chunk(const char **cursor, kind = CHUNK_INCLUDE;

if (parse_literal(&p, ":")) { + const char *tgtspec = p; + + if (!parse_inany(&p, &taddr_tmp) || + !parse_literal(&p, "/"))

This is to support ":/PORT" together with ":ADDR/PORT", right?

No, this is checking for the case where we didn't get a target address at all. De Morgan's law might make it clearer: !(<parsed IP> &&

Right, yes, I understand, but you're doing the check this way because "/PORT" without address would be accepted, right? Otherwise checking for !parse_inany(&p, &taddr_tmp) would have been enough? Anyway, not so important, if the next version looks different anyway.

But I already reworked this to allow port to be omitted with address, and I think the new code is clearer. Or at least unclear in an unrelated way :/.

...

I think it's nice to have for users, but it looks inconsistent here, so maybe you could add a comment before that, say:

/* Accept :/PORT as well as :ADDR/PORT */

...

+ p = tgtspec; /* No target address, backtrack */ + if (!parse_port_range(&p, &tr)) return false; } else { @@ -492,6 +512,8 @@ static bool parse_port_chunk(const char **cursor,

*kindp = kind; *lrange = lr; + if (taddr) + *taddr = taddr_tmp; if (trange) *trange = tr; *cursor = p; @@ -561,7 +583,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, /* Consider excluded ranges and "auto" in the first pass */ p = spec; do { - if (!parse_port_chunk(&p, &kind, &lrange, NULL)) + if (!parse_port_chunk(&p, &kind, &lrange, NULL, NULL)) goto bad;

switch (kind) { @@ -586,8 +608,9 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, p = spec; do { struct port_range trange; + union inany_addr taddr;

- if (!parse_port_chunk(&p, &kind, &lrange, &trange)) + if (!parse_port_chunk(&p, &kind, &lrange, &taddr, &trange)) goto bad;

switch (kind) { @@ -604,7 +627,8 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,

fwd_rule_range_except(fwd, del, proto, addr, ifname, lrange.first, lrange.last, - exclude, trange.first, flags); + exclude, &taddr, trange.first, + flags); break; default: goto bad; @@ -620,7 +644,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,

fwd_rule_range_except(fwd, del, proto, addr, ifname, 1, NUM_PORTS - 1, exclude, - 1, flags | FWD_WEAK); + NULL, 1, flags | FWD_WEAK); } return; bad:

The rest of the series looks good to me, but I didn't test things at all. By the way, man page and usage changes for 3/3 are missing.

Ugh, yeah, I belatedly realised that.

...

I haven't seen anything preventing mapping between IPv4 and IPv6, but I guess I missed something. I actually think it would be a nice feature but I guess it needs some more effort to properly support it.

Oh sod, good point. I forgot to add that check. I think we need it for now, because I'm pretty sure it won't work end to end yet.

I guess probably worth a quick check, I wouldn't be surprised if things actually kind of work for TCP. -- Stefano