[PATCH 00/16] Cleanups and fixes related to "our" addresses
There are some places where we have addresses that are "ours" in the sense that they're local to passt on at least one interface. But in some cases it wasn't clear which addresses those were or how to use them. Make a number of renames, cleanups and small fixes related to that. ..and also an assortment of slightly related things that I encountered along the way. Note that 1/16 is an important fix for a bug I introduced in the last series I sent. For the rest, apply as many as you're happy with and I'll respin what's left as necessary. David Gibson (16): conf: Don't ignore -t and -u options after -D treewide: Use "our address" instead of "forwarding address" util: Helper for formatting MAC addresses treewide: Rename MAC address fields for clarity treewide: Use struct assignment instead of memcpy() for IP addresses conf: Use array indices rather than pointers for DNS array slots conf: More accurately count entries added in get_dns() conf: Move DNS array bounds checks into add_dns[46] conf: Move adding of a nameserver from resolv.conf into subfunction conf: Correct setting of dns_match address in add_dns6() conf: Treat --dns addresses as guest visible addresses conf: Remove incorrect initialisation of addr_ll_seen util: Correct sock_l4() binding for link local addresses treewide: Change misleading 'addr_ll' name Clarify which addresses in ip[46]_ctx are meaningful where Initialise our_tap_ll to ip6.gw when suitable arp.c | 4 +- conf.c | 181 ++++++++++++++++++++++++++++--------------------- dhcp.c | 5 +- dhcpv6.c | 21 +++--- flow.c | 74 ++++++++++---------- flow.h | 18 ++--- fwd.c | 70 +++++++++---------- icmp.c | 4 +- ndp.c | 9 +-- passt.1 | 14 ++-- passt.c | 2 +- passt.h | 22 +++--- pasta.c | 8 +-- tap.c | 12 ++-- tcp.c | 33 ++++----- tcp_internal.h | 2 +- udp.c | 12 ++-- util.c | 22 +++++- util.h | 3 + 19 files changed, 285 insertions(+), 231 deletions(-) -- 2.46.0
f6d5a5239264 moved handling of -D into a later loop. However as a side
effect it moved this from a switch block to an if block. I left a couple
of 'break' statements that don't make sense in the new context. They
should be 'continue' so that we go onto the next option, rather than
leaving the loop entirely.
Fixes: f6d5a5239264 ("conf: Delay handling -D option until after...")
Signed-off-by: David Gibson
On Wed, 14 Aug 2024 14:30:35 +1000
David Gibson
f6d5a5239264 moved handling of -D into a later loop. However as a side effect it moved this from a switch block to an if block. I left a couple of 'break' statements that don't make sense in the new context. They should be 'continue' so that we go onto the next option, rather than leaving the loop entirely.
Fixes: f6d5a5239264 ("conf: Delay handling -D option until after...")
Signed-off-by: David Gibson
Applied (just this one). -- Stefano
The term "forwarding address" to indicate the local-to-passt address was
well-intentioned, but ends up being kinda confusing. As discussed on a
recent call, let's try "our" instead.
Signed-off-by: David Gibson
There are a couple of places where we somewhat messily open code formatting
an Ethernet like MAC address for display. Add an eth_ntop() helper for
this.
Signed-off-by: David Gibson
c->mac isn't a great name, because it doesn't say whose mac address it is
and it's not necessarily obvious in all the contexts we use it. Since this
is specifically the address that we (passt/pasta) use on the tap interface,
rename it to "our_tap_mac". Rename the "mac_guest" field to "guest_mac"
to be grammatically consistent.
Signed-off-by: David Gibson
We rely on C11 already, so we can use clearer and more type-checkable
struct assignment instead of mempcy() for copying IP addresses around.
This exposes some "pointer could be const" warnings from cppcheck, so
address those too.
Signed-off-by: David Gibson
Currently add_dns[46]() take a somewhat awkward double pointer to the
entry in the c->ip[46].dns array to update. It turns out to be easier to
work with indices into that array instead.
This diff does add some lines, but it's comments, and will allow some
future code reductions.
Signed-off-by: David Gibson
get_dns() counts the number of guest DNS servers it adds, and gives an
error if it couldn't add any. However, this count ignores the fact that
add_dns[46]() may in some cases *not* add an entry. Use the array indices
we're already tracking to get an accurate count.
Signed-off-by: David Gibson
Every time we call add_dns[46] we need to first check if there's space in
the c->ip[46].dns array for the new entry. We might as well make that
check in add_dns[46]() itself.
In fact it looks like the calls in get_dns() had an off by one error, not
allowing the last entry of the array to be filled. So, that bug is also
fixed by the change.
Signed-off-by: David Gibson
get_dns() is already quite deeply nested, and future changes I have in
mind will add more complexity. Prepare for this by splitting out the
adding of a single nameserver to the configuration into its own function.
Signed-off-by: David Gibson
add_dns6() (but not add_dns4()) has a bug setting dns_match: it sets it to
the given address, rather than the gateway address. This is doubly wrong:
- We've just established the given address is a host loopback address
the guest can't access
- We've just set ip6.dns[] to tell the guest to use the gateway address,
so it won't use the dns_match address we're setting
Correct this to use the gateway address, like IPv4.
Signed-off-by: David Gibson
Although it's not 100% explicit in the man page, addresses given to the
--dns option are intended to be addresses as seen by the guest. This
differs from addresses taken from the host's /etc/resolv.conf, which must
be translated to to guest accessible versions in some cases.
Our implementation is currently inconsistent on this: when using
--dns-forward, you must usually also give --dns with the matching address,
which is meaningful only in the guest's address view. However if you give
--dns with a loopback addres, it will be translated like a host view
address.
Move the remapping logic for DNS addresses out of add_dns4() and add_dns6()
into add_dns_resolv() so that it is only applied for host nameserver
addresses, not for nameservers given explicitly with --dns.
Signed-off-by: David Gibson
Despite the names, addr_ll_seen does not relate to addr_ll the same way
addr_see relates to addr. addr_ll_seen is an observed address from the
guest, whereas addr_ll is *our* link-local address for use on the tap link
when we can't use an external endpoint address. It's used both for
passt provided services (DHCPv6, NDP) and in some cases for connections
from addresses the guest can't access.
Signed-off-by: David Gibson
When binding an IPv6 socket in sock_l4() we need to supply a scope id if
the address is link-local. We check for this by comparing the given
address to c->ip6.addr_ll. This is correct only by accident: while
c->ip6.addr_ll is typically set to the hsot interface's link local
address, the actually purpose of it is to provide a link local address
for passt's private use on the tap interface.
Instead set the scope id for any link-local address we're binding to.
We're going to need something and this is what makes sense for sockets
on the host. It doesn't make sense for PIF_SPLICE sockets, but those
should always have loopback, not link-local addresses.
Signed-off-by: David Gibson
c->ip6.addr_ll is not like c->ip6.addr. The latter is an address for the
guest, but the former is an address for our use on the tap link. Rename it
accordingly, to 'our_tap_ll'.
Signed-off-by: David Gibson
Some are guest visible addresses and may not be valid on the host, others
are host visible addresses and may not be valid on the guest.
Signed-off-by: David Gibson
In every place we use our_tap_ll, we only use it as a fallback if the
IPv6 gateway address is not link-local. We can avoid that conditional at
use time by doing it at initialisation of our_tap_ll instead.
Signed-off-by: David Gibson
On Wed, Aug 14, 2024 at 02:30:50PM +1000, David Gibson wrote:
In every place we use our_tap_ll, we only use it as a fallback if the IPv6 gateway address is not link-local. We can avoid that conditional at use time by doing it at initialisation of our_tap_ll instead.
Signed-off-by: David Gibson
Ugh, sorry. Please don't apply this one, I missed a case.
--- conf.c | 3 +++ dhcpv6.c | 5 +---- ndp.c | 5 +---- 3 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/conf.c b/conf.c index f1727ade..30769474 100644 --- a/conf.c +++ b/conf.c @@ -721,6 +721,9 @@ static unsigned int conf_ip6(unsigned int ifi,
ip6->addr_seen = ip6->addr;
+ if (IN6_IS_ADDR_LINKLOCAL(&ip6->gw)) + ip6->our_tap_ll = ip6->gw; + if (MAC_IS_ZERO(mac)) { rc = nl_link_get_mac(nl_sock, ifi, mac); if (rc < 0) { diff --git a/dhcpv6.c b/dhcpv6.c index 44e954e7..69841abc 100644 --- a/dhcpv6.c +++ b/dhcpv6.c @@ -453,10 +453,7 @@ int dhcpv6(struct ctx *c, const struct pool *p,
c->ip6.addr_ll_seen = *saddr;
- if (IN6_IS_ADDR_LINKLOCAL(&c->ip6.gw)) - src = &c->ip6.gw; - else - src = &c->ip6.our_tap_ll; + src = &c->ip6.our_tap_ll;
mh = packet_get(p, 0, sizeof(*uh), sizeof(*mh), NULL); if (!mh) diff --git a/ndp.c b/ndp.c index 3a76b00a..a1ee8349 100644 --- a/ndp.c +++ b/ndp.c @@ -341,10 +341,7 @@ dns_done: else c->ip6.addr_seen = *saddr;
- if (IN6_IS_ADDR_LINKLOCAL(&c->ip6.gw)) - rsaddr = &c->ip6.gw; - else - rsaddr = &c->ip6.our_tap_ll; + rsaddr = &c->ip6.our_tap_ll;
if (ih->icmp6_type == NS) { dlen = sizeof(struct ndp_na);
-- 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 Wed, Aug 14, 2024 at 02:30:34PM +1000, David Gibson wrote:
There are some places where we have addresses that are "ours" in the sense that they're local to passt on at least one interface. But in some cases it wasn't clear which addresses those were or how to use them. Make a number of renames, cleanups and small fixes related to that.
..and also an assortment of slightly related things that I encountered along the way.
Note that 1/16 is an important fix for a bug I introduced in the last series I sent. For the rest, apply as many as you're happy with and I'll respin what's left as necessary.
Ugh. I've found several more problems in the series (though mostly just incorrect comments). Review if you want, but don't apply please. I'll send a new spin soon. -- 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