On Sat, Jul 04, 2026 at 05:27:06PM +0200, Stefano Brivio wrote: > On Thu, 2 Jul 2026 17:32:14 +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 | 90 +++++++++++++++++++++++++++++++++++++++++++++--------- > > passt.1 | 31 +++++++++++++------ > > 2 files changed, 98 insertions(+), 23 deletions(-) > > > > diff --git a/fwd_rule.c b/fwd_rule.c > > index ef35e1b4..bed29ed9 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 target side > > * @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 = { > > @@ -394,10 +397,31 @@ static void fwd_rule_range_except(struct fwd_table *fwd, bool del, > > .proto = proto, > > .flags = flags, > > }; > > + unsigned delta = tgt_first - first; > > char rulestr[FWD_RULE_STRLEN]; > > - unsigned delta = to - first; > > unsigned base, i; > > > > + if (tgt_addr && !inany_is_unspecified(tgt_addr)) { > > + char astr[INANY_ADDRSTRLEN]; > > + > > + if (!inany_is_unicast(tgt_addr)) { > > + die("Target address %s is not unicast", > > + inany_ntop(tgt_addr, astr, sizeof(astr))); > > + } > > Nit: an extra newline here would help readability. Done. > > + if (!addr || !!inany_v4(addr) != !!inany_v4(tgt_addr)) { > > I wonder: if !addr, shouldn't we replace it, for the moment, with > inany_any4 or inany_any6 depending on !!inany_v4(tgt_addr)? > > I see that an empty address already works with -4 or -6, and I > understand that, by doing this, we'll have a change in behaviour once > forwarding between IP versions is implemented. > > But it will take a while before we get there, and, meanwhile, I guess > almost all users will just want to do stuff like -t 8080:192.0.2.1/80, > just to hit: > > Forwarding between IP versions (* => 192.0.2.1) not implemented > > which isn't obvious if you aren't familiar with the implementation. I > can also picture a constant flow of incoming tickets as a result. That's a reasonable concern. I did think about this and came to the tentative conclusion that automatically restricting to one family would be worse, but I'm open to persuasion otherwise. Reasoning as follows. First, it is a little awkward to implement: I think we should still prevent an explicit *:192.0.2.1, which means we'd have to distinguish the explicit * and implicit any cases. Mind you, we probably want something similar when we allow listening addresses for -[TU], so it's not such a big deal. More importly this would become the _only_ place where the target designation affects where and how we listen - in all other ways, that's fully determined by the first part of the spec plus global flags like -[46]. This gets particularly striking when looking at something like: -t 5000,5001:192.0.2.1 With an automatic restriction, we'd end up listening on *:5000, but only on 0.0.0.0:5001, which seems pretty confusing. > The change in behaviour once we implement forwarding between IP > versions, by the way, looks a bit like an extension rather than a real > change that could reasonably cause trouble to anybody. I'd argue that's even more the case with the errors as is: things that weren't possible before become possible. > > + char bstr[INANY_ADDRSTRLEN]; > > + > > + die( > > +"Forwarding between IP versions (%s => %s) not implemented", > > + inany_ntop(addr, bstr, sizeof(bstr)), > > + inany_ntop(tgt_addr, astr, sizeof(astr))); > > + } > > About both validations: shouldn't they live in fwd_rule_add(), where > we already have this kind of stuff? Or at least be duplicated there? Yes, yes they should. Which also means it needs to move to the second patch (or fwd_add_rule() doesn't see the target address - there's nowhere for it in the structure). For the next spin, I've moved this validation to fwd_rule_add() in patch 2. I've also tweaked the messages a bit, which might at least slightly mitigate the concerns above. > Now, I tried dropping those in pesto (#ifndef PESTO ...) to see what > happens, and I couldn't really spot anything really bad, not with > multicast and not with mixing IPv4 and IPv6: > > 14 20.647728 fe80::1 → ::ffff:127.0.0.1 82 TCP 57812 → 5201 [SYN] Seq=0 Win=65535 Len=0 MSS=61440 WS=256 > > but still I wonder if we shouldn't make it a bit more robust. No, definitely. Complete thinko on my behalf. > > + > > + 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,19 +482,31 @@ 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 & ALL) > > * @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; > > > > if (parse_literal(&p, "all")) { > > + const char *tgtspec = p; > > + > > kind = CHUNK_ALL; > > + if (p = tgtspec, > > + parse_literal(&p, ":") && > > + parse_inany(&p, &taddr_tmp)) { > > + /* Target address */ > > + } else { > > + p = tgtspec; > > + } > > } else if (parse_literal(&p, "auto")) { > > kind = CHUNK_AUTO; > > } else if (parse_literal(&p, "~")) { > > @@ -478,12 +514,29 @@ static bool parse_port_chunk(const char **cursor, > > if (!parse_port_range(&p, &lr)) > > return false; > > } else if (parse_port_range(&p, &lr)) { > > - kind = CHUNK_INCLUDE; > > + const char *tgtspec = p; > > > > - if (parse_literal(&p, ":")) { > > - if (!parse_port_range(&p, &tr)) > > - return false; > > + kind = CHUNK_INCLUDE; > > + if (p = tgtspec, > > + parse_literal(&p, ":") && > > + parse_inany(&p, &taddr_tmp) && > > + parse_literal(&p, "/") && > > + parse_port_range(&p, &tr)) { > > + /* Target address & range */ > > + } else if (p = tgtspec, > > + parse_literal(&p, ":") && > > + parse_inany(&p, &taddr_tmp)) { > > + /* Target address only */ > > + tr = lr; > > + } else if (p = tgtspec, > > + parse_literal(&p, ":") && > > + parse_port_range(&p, &tr)) { > > + /* Target range only */ > > + taddr_tmp = inany_any6; > > } else { > > + p = tgtspec; > > + /* No target specification */ > > + taddr_tmp = inany_any6; > > tr = lr; > > } > > } else { > > @@ -492,6 +545,8 @@ static bool parse_port_chunk(const char **cursor, > > > > *kindp = kind; > > *lrange = lr; > > + if (taddr) > > + *taddr = taddr_tmp; > > if (trange) > > *trange = tr; > > *cursor = p; > > @@ -551,6 +606,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, > > const char *spec) > > { > > uint8_t exclude[PORT_BITMAP_SIZE] = { 0 }; > > + union inany_addr all_taddr = inany_any6; > > enum fwd_port_chunk_kind kind; > > struct port_range lrange; > > bool exclude_only = true; > > @@ -561,7 +617,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,14 +642,19 @@ 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) { > > - case CHUNK_AUTO: /* already handled */ > > - case CHUNK_EXCLUDE: /* already handled */ > > - case CHUNK_ALL: /* handled later */ > > + case CHUNK_AUTO: > > + case CHUNK_EXCLUDE: > > + continue; /* already handled */ > > + > > + case CHUNK_ALL: > > + /* Save the address to use later */ > > + all_taddr = taddr; > > continue; > > > > case CHUNK_INCLUDE: > > @@ -604,7 +665,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 +682,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); > > + &all_taddr, 1, flags | FWD_WEAK); > > } > > return; > > bad: > > diff --git a/passt.1 b/passt.1 > > index c3722ef9..9ece0e0c 100644 > > --- a/passt.1 > > +++ b/passt.1 > > @@ -449,12 +449,15 @@ interface name (since Linux 5.7) can be specified. > > > > \fIports\fR is a comma-separated list of entries which may be any of: > > .TP > > -\fIfirst\fR[\fB-\fR\fIlast\fR][\fB:\fR\fItofirst\fR[\fB-\fR\fItolast\fR]] > > +\fIfirst\fR[\fB-\fR\fIlast\fR][\fB:\fR[\fItoaddr\fR\fB/\fR]\fItofirst\fR[\fB-\fR\fItolast\fR]] > > +.TP > > +\fIfirst\fR[\fB-\fR\fIlast\fR][\fB:\fR\fItoaddr\fR] > > Include range. Forward port numbers between \fIfirst\fR and \fIlast\fR > > -(inclusive) to ports between \fItofirst\fR and \fItolast\fR. If > > -\fItofirst\fR and \fItolast\fR are omitted, assume the same as > > -\fIfirst\fR and \fIlast\fR. If \fIlast\fR is omitted, assume the same > > -as \fIfirst\fR. > > +(inclusive) to ports between \fItofirst\fR and \fItolast\fR to address > > +\fItoaddr\fR. If \fItoaddr\fR is omitted, automatically determine the > > +guest or namespace address. If \fItofirst\fR and \fItolast\fR are > > +omitted, assume the same as \fIfirst\fR and \fIlast\fR. If \fIlast\fR > > +is omitted, assume the same as \fIfirst\fR. > > > > .TP > > \fB~\fR\fIfirst\fR[\fB-\fR\fIlast\fR] > > @@ -462,11 +465,13 @@ Exclude range. Don't forward port numbers between \fIfirst\fR and > > \fIlast\fR. This takes precedences over include ranges. > > > > .TP > > -.BR all > > +.BR all\fR[\fB:\fItoaddr\fR] > > Forward all unbound, non-ephemeral ports, not covered by exclude > > -ranges above, as permitted by current capabilities. For low (< 1024) > > -ports, see \fBNOTES\fR. No failures are reported for unavailable > > -ports, unless no ports could be forwarded at all. > > +ranges above, as permitted by current capabilities, to the > > +corresponding ports on address \fItoaddr\fR. If \fItoaddr\fR is > > +omitted, automatically determine the guest or namespace address. For > > +low (< 1024) ports, see \fBNOTES\fR. No failures are reported for > > +unavailable ports, unless no ports could be forwarded at all. > > > > .TP > > .BR auto > > @@ -516,6 +521,14 @@ Forward local port 22, bound to 192.0.2.1 and interface eth0, to port 22 > > -t %eth0/22 > > Forward local port 22, bound to any address on interface eth0, to port 22 > > .TP > > +-t 0.0.0.0/5000:192.0.2.5/6000 > > +Forward local port 5000, bound to any IPv4 address, to port 6000 on address 192.0.2.5. > > +.TP > > +-t 127.0.0.6/all:192.0.2.6 > > +For the local address 127.0.0.6 forward all unbound, non-ephemeral > > I think this is a bit difficult to follow without a comma, it should be: Done. > For the local address 127.0.0.6, forward all unbound, [...] > > also for consistency with the existing: > > For the local address 192.0.2.1, forward ports between 20 and 24 [...] > > > +ports as permitted by current capabilities to the corresponding port > > +on 192.0.2.6. > > +.TP > > -t 2000-5000,~3000-3010 > > Forward local ports between 2000 and 5000, except for those between 3000 and > > 3010 > > the rest and 2/2 look good to me, no further comments. > > -- > Stefano > -- 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