[PATCH v4 02/14] conf, fwd: Keep a table of our port forwarding configuration
At present, we set up forwarding as we parse the -t, and -u options, not
keeping a persistent data structure with all the details. We do have
some information in struct fwd_ports, but it doesn't capture all the nuance
that the options do.
As a first step to generalising our forwarding model, add a table of all
the forwarding rules to struct fwd_ports. For now it covers only explicit
forwards, not automatic, and we don't do anything with it other than print
some additional debug information. We'll do more with it in future
patches.
Signed-off-by: David Gibson
On Thu, 15 Jan 2026 19:50:33 +1100
David Gibson
@@ -313,6 +330,90 @@ bool fwd_port_is_ephemeral(in_port_t port) return (port >= fwd_ephemeral_min) && (port <= fwd_ephemeral_max); }
+/** + * fwd_rule_add() - Add a rule to a forwarding table + * @fwd: Table to add to + * @flags: Flags for this entry + * @addr: Our address to forward (NULL for both 0.0.0.0 and ::) + * @ifname: Only forward from this interface name, if non-empty + * @first: First port number to forward + * @last: Last port number to forward + * @to: First port of target port range to map to + */ +void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags, + const union inany_addr *addr, const char *ifname, + in_port_t first, in_port_t last, in_port_t to) +{ + /* Flags which can be set from the caller */ + const uint8_t allowed_flags = FWD_WEAK; + struct fwd_rule *new; + unsigned port; + + ASSERT(!(flags & ~allowed_flags)); + + if (fwd->count >= ARRAY_SIZE(fwd->rules)) + die("Too many port forwarding ranges"); + + new = &fwd->rules[fwd->count++]; + new->flags = flags; + + if (addr) { + new->addr = *addr; + } else { + new->addr = inany_any6; + new->flags |= FWD_DUAL_STACK_ANY; + } + + memset(new->ifname, 0, sizeof(new->ifname)); + if (ifname) { + if (strlen(ifname) + 1 > sizeof(new->ifname)) + die("Interface name %s is too long", ifname); + strncpy(new->ifname, ifname, sizeof(new->ifname)); + }
This looks safe to me now, but: /home/sbrivio/passt/fwd.c:394:3: Type: Buffer not null terminated (BUFFER_SIZE) /home/sbrivio/passt/fwd.c:354:2: 1. path: Condition "!(flags & -7 /* ~allowed_flags */)", taking true branch. /home/sbrivio/passt/fwd.c:356:2: 2. path: Condition "fwd->count >= 255U /* (int)(sizeof (fwd->rules) / sizeof (fwd->rules[0])) */", taking false branch. /home/sbrivio/passt/fwd.c:358:2: 3. path: Condition "fwd->sock_count + num > 196608U /* (int)(sizeof (fwd->socks) / sizeof (fwd->socks[0])) */", taking false branch. /home/sbrivio/passt/fwd.c:362:2: 4. path: Condition "i < fwd->count", taking true branch. /home/sbrivio/passt/fwd.c:366:3: 5. path: Condition "!inany_matches(addr, fwd_rule_addr(rule))", taking false branch. /home/sbrivio/passt/fwd.c:370:3: 6. path: Condition "last < rule->first", taking true branch. /home/sbrivio/passt/fwd.c:372:4: 7. path: Continuing loop. /home/sbrivio/passt/fwd.c:362:2: 8. path: Condition "i < fwd->count", taking true branch. /home/sbrivio/passt/fwd.c:366:3: 9. path: Condition "!inany_matches(addr, fwd_rule_addr(rule))", taking false branch. /home/sbrivio/passt/fwd.c:370:3: 10. path: Condition "last < rule->first", taking true branch. /home/sbrivio/passt/fwd.c:372:4: 11. path: Continuing loop. /home/sbrivio/passt/fwd.c:362:2: 12. path: Condition "i < fwd->count", taking true branch. /home/sbrivio/passt/fwd.c:366:3: 13. path: Condition "!inany_matches(addr, fwd_rule_addr(rule))", taking false branch. /home/sbrivio/passt/fwd.c:370:3: 14. path: Condition "last < rule->first", taking false branch. /home/sbrivio/passt/fwd.c:370:3: 15. path: Condition "rule->last < first", taking true branch. /home/sbrivio/passt/fwd.c:372:4: 16. path: Continuing loop. /home/sbrivio/passt/fwd.c:362:2: 17. path: Condition "i < fwd->count", taking false branch. /home/sbrivio/passt/fwd.c:383:2: 18. path: Condition "addr", taking true branch. /home/sbrivio/passt/fwd.c:385:2: 19. path: Falling through to end of if statement. /home/sbrivio/passt/fwd.c:391:2: 20. path: Condition "ifname", taking true branch. /home/sbrivio/passt/fwd.c:392:3: 21. path: Condition "strlen(ifname) + 1 > 16UL /* sizeof (new->ifname) */", taking false branch. /home/sbrivio/passt/fwd.c:394:3: 22. buffer_size_warning: Calling "strncpy" with a maximum size argument of 16 bytes on destination array "new->ifname" of size 16 bytes might leave the destination string unterminated. /home/sbrivio/passt/fwd.c:397:2: 23. path: Condition "first <= last", taking true branch. /home/sbrivio/passt/fwd.c:406:2: 24. path: Condition "port <= new->last", taking true branch. /home/sbrivio/passt/fwd.c:410:3: 25. path: Condition "!(new->flags & (4UL /* 1UL << 2 */))", taking true branch. /home/sbrivio/passt/fwd.c:412:2: 26. path: Jumping back to the beginning of the loop. /home/sbrivio/passt/fwd.c:406:2: 27. path: Condition "port <= new->last", taking false branch. ...perhaps worth switching to the usual snprintf() approach with return check (see handling of c->ip4.ifname_out in conf()) and be done with it? I'd be slightly more confident if Coverity Scan didn't complain at all (and happier without the noise, too). Other than that, this version looks good to me. I would make a new release just before merging it (with this "fixed") so that we can debug things a bit more conveniently should something go wrong with it. -- Stefano
On Fri, Jan 16, 2026 at 12:01:27AM +0100, Stefano Brivio wrote:
On Thu, 15 Jan 2026 19:50:33 +1100 David Gibson
wrote: @@ -313,6 +330,90 @@ bool fwd_port_is_ephemeral(in_port_t port) return (port >= fwd_ephemeral_min) && (port <= fwd_ephemeral_max); }
+/** + * fwd_rule_add() - Add a rule to a forwarding table + * @fwd: Table to add to + * @flags: Flags for this entry + * @addr: Our address to forward (NULL for both 0.0.0.0 and ::) + * @ifname: Only forward from this interface name, if non-empty + * @first: First port number to forward + * @last: Last port number to forward + * @to: First port of target port range to map to + */ +void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags, + const union inany_addr *addr, const char *ifname, + in_port_t first, in_port_t last, in_port_t to) +{ + /* Flags which can be set from the caller */ + const uint8_t allowed_flags = FWD_WEAK; + struct fwd_rule *new; + unsigned port; + + ASSERT(!(flags & ~allowed_flags)); + + if (fwd->count >= ARRAY_SIZE(fwd->rules)) + die("Too many port forwarding ranges"); + + new = &fwd->rules[fwd->count++]; + new->flags = flags; + + if (addr) { + new->addr = *addr; + } else { + new->addr = inany_any6; + new->flags |= FWD_DUAL_STACK_ANY; + } + + memset(new->ifname, 0, sizeof(new->ifname)); + if (ifname) { + if (strlen(ifname) + 1 > sizeof(new->ifname)) + die("Interface name %s is too long", ifname); + strncpy(new->ifname, ifname, sizeof(new->ifname)); + }
This looks safe to me now, but:
/home/sbrivio/passt/fwd.c:394:3: Type: Buffer not null terminated (BUFFER_SIZE) [snip] ...perhaps worth switching to the usual snprintf() approach with return check (see handling of c->ip4.ifname_out in conf()) and be done with it?
Good idea, not sure why it didn't occur to me earlier. I've done that, and verified it fixes the coverity error (thanks for resending the instructions for that).
I'd be slightly more confident if Coverity Scan didn't complain at all (and happier without the noise, too).
Other than that, this version looks good to me. I would make a new release just before merging it (with this "fixed") so that we can debug things a bit more conveniently should something go wrong with it.
That sounds wise. Do you want a new spin with the coverity fix? Just this patch? Something else? -- 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
On Fri, 16 Jan 2026 11:20:43 +1100
David Gibson
On Fri, Jan 16, 2026 at 12:01:27AM +0100, Stefano Brivio wrote:
On Thu, 15 Jan 2026 19:50:33 +1100 David Gibson
wrote: @@ -313,6 +330,90 @@ bool fwd_port_is_ephemeral(in_port_t port) return (port >= fwd_ephemeral_min) && (port <= fwd_ephemeral_max); }
+/** + * fwd_rule_add() - Add a rule to a forwarding table + * @fwd: Table to add to + * @flags: Flags for this entry + * @addr: Our address to forward (NULL for both 0.0.0.0 and ::) + * @ifname: Only forward from this interface name, if non-empty + * @first: First port number to forward + * @last: Last port number to forward + * @to: First port of target port range to map to + */ +void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags, + const union inany_addr *addr, const char *ifname, + in_port_t first, in_port_t last, in_port_t to) +{ + /* Flags which can be set from the caller */ + const uint8_t allowed_flags = FWD_WEAK; + struct fwd_rule *new; + unsigned port; + + ASSERT(!(flags & ~allowed_flags)); + + if (fwd->count >= ARRAY_SIZE(fwd->rules)) + die("Too many port forwarding ranges"); + + new = &fwd->rules[fwd->count++]; + new->flags = flags; + + if (addr) { + new->addr = *addr; + } else { + new->addr = inany_any6; + new->flags |= FWD_DUAL_STACK_ANY; + } + + memset(new->ifname, 0, sizeof(new->ifname)); + if (ifname) { + if (strlen(ifname) + 1 > sizeof(new->ifname)) + die("Interface name %s is too long", ifname); + strncpy(new->ifname, ifname, sizeof(new->ifname)); + }
This looks safe to me now, but:
/home/sbrivio/passt/fwd.c:394:3: Type: Buffer not null terminated (BUFFER_SIZE) [snip] ...perhaps worth switching to the usual snprintf() approach with return check (see handling of c->ip4.ifname_out in conf()) and be done with it?
Good idea, not sure why it didn't occur to me earlier.
I've done that, and verified it fixes the coverity error (thanks for resending the instructions for that).
I'd be slightly more confident if Coverity Scan didn't complain at all (and happier without the noise, too).
Other than that, this version looks good to me. I would make a new release just before merging it (with this "fixed") so that we can debug things a bit more conveniently should something go wrong with it.
That sounds wise. Do you want a new spin with the coverity fix? Just this patch? Something else?
Yes, thanks, a respin would be nice, so that we have a reasonable "permalink" to it, given it's a quite fundamental feature you're adding and we might want to refer to the series as posted / applied. -- Stefano
On Fri, Jan 16, 2026 at 01:25:54AM +0100, Stefano Brivio wrote:
On Fri, 16 Jan 2026 11:20:43 +1100 David Gibson
wrote: On Fri, Jan 16, 2026 at 12:01:27AM +0100, Stefano Brivio wrote:
On Thu, 15 Jan 2026 19:50:33 +1100 David Gibson
wrote: @@ -313,6 +330,90 @@ bool fwd_port_is_ephemeral(in_port_t port) return (port >= fwd_ephemeral_min) && (port <= fwd_ephemeral_max); }
+/** + * fwd_rule_add() - Add a rule to a forwarding table + * @fwd: Table to add to + * @flags: Flags for this entry + * @addr: Our address to forward (NULL for both 0.0.0.0 and ::) + * @ifname: Only forward from this interface name, if non-empty + * @first: First port number to forward + * @last: Last port number to forward + * @to: First port of target port range to map to + */ +void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags, + const union inany_addr *addr, const char *ifname, + in_port_t first, in_port_t last, in_port_t to) +{ + /* Flags which can be set from the caller */ + const uint8_t allowed_flags = FWD_WEAK; + struct fwd_rule *new; + unsigned port; + + ASSERT(!(flags & ~allowed_flags)); + + if (fwd->count >= ARRAY_SIZE(fwd->rules)) + die("Too many port forwarding ranges"); + + new = &fwd->rules[fwd->count++]; + new->flags = flags; + + if (addr) { + new->addr = *addr; + } else { + new->addr = inany_any6; + new->flags |= FWD_DUAL_STACK_ANY; + } + + memset(new->ifname, 0, sizeof(new->ifname)); + if (ifname) { + if (strlen(ifname) + 1 > sizeof(new->ifname)) + die("Interface name %s is too long", ifname); + strncpy(new->ifname, ifname, sizeof(new->ifname)); + }
This looks safe to me now, but:
/home/sbrivio/passt/fwd.c:394:3: Type: Buffer not null terminated (BUFFER_SIZE) [snip] ...perhaps worth switching to the usual snprintf() approach with return check (see handling of c->ip4.ifname_out in conf()) and be done with it?
Good idea, not sure why it didn't occur to me earlier.
I've done that, and verified it fixes the coverity error (thanks for resending the instructions for that).
I'd be slightly more confident if Coverity Scan didn't complain at all (and happier without the noise, too).
Other than that, this version looks good to me. I would make a new release just before merging it (with this "fixed") so that we can debug things a bit more conveniently should something go wrong with it.
That sounds wise. Do you want a new spin with the coverity fix? Just this patch? Something else?
Yes, thanks, a respin would be nice, so that we have a reasonable "permalink" to it, given it's a quite fundamental feature you're adding and we might want to refer to the series as posted / applied.
Ok. Will do that as soon as this test run completes. -- 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
participants (2)
-
David Gibson
-
Stefano Brivio