Partly in looking at flow table support, partly for other reasons, I've spotted a number of small fixes that can be made to the UDP code (in addition to the large fixes I'm still working on). These are ready to go now, so here they are. They'll go before the flow related addressing cleanups. Changes since v1: * Removed patch to unconditionally remove ACT flags, which was Just Plain Wrong. * Removed patch to sort out incorrect handling of port flags. While there's a real bug there, it interacts with at least two more bugs that I hadn't spotted, complicating the picture. I'll fix those some other day. David Gibson (5): udp: Don't attempt to translate a 0.0.0.0 source address udp: Set pif in epoll reference for ephemeral host sockets udp: Small streamline to udp_update_hdr4() udp: Fix incorrect usage of IPv6 state in IPv4 path udp: Remove unnecessary test for unspecified addr_out udp.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) -- 2.43.2
If an incoming packet has a source address of 0.0.0.0 we translate that to
the gateway address. This doesn't really make sense, because we have no
way to do a reverse translation for reply packets.
Certain UDP protocols do use an unspecified source address in some
circumstances (e.g. DHCP). These generally either require no reply, a
multicast reply, or provide a suitable reply address by other means.
In none of those cases does translating it in passt/pasta make sense. The
best we can really do here is just leave it as is.
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
udp.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/udp.c b/udp.c
index b19e76db..3d44f81e 100644
--- a/udp.c
+++ b/udp.c
@@ -599,7 +599,6 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t
dstport,
src_port == 53) {
b->iph.saddr = c->ip4.dns_match.s_addr;
} else if (IN4_IS_ADDR_LOOPBACK(&b->s_in.sin_addr) ||
- IN4_IS_ADDR_UNSPECIFIED(&b->s_in.sin_addr)||
IN4_ARE_ADDR_EQUAL(&b->s_in.sin_addr, &c->ip4.addr_seen)) {
b->iph.saddr = c->ip4.gw.s_addr;
udp_tap_map[V4][src_port].ts = now->tv_sec;
--
2.43.2
The udp_epoll_ref contains a field for the pif to which the socket belongs.
We fill this in for permanent sockets created with udp_sock_init() and for
spliced sockets, however, we omit it for ephemeral sockets created for
tap originated flows.
This is a bug, although we currently get away with it, because we don't
consult that field for such flows. Correctly fill it in.
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
udp.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/udp.c b/udp.c
index 3d44f81e..d992acd0 100644
--- a/udp.c
+++ b/udp.c
@@ -868,7 +868,10 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
src, dst, udp_tap_map[V4][src].sock);
if ((s = udp_tap_map[V4][src].sock) < 0) {
struct in_addr bind_addr = IN4ADDR_ANY_INIT;
- union udp_epoll_ref uref = { .port = src };
+ union udp_epoll_ref uref = {
+ .port = src,
+ .pif = PIF_HOST,
+ };
const char *bind_if = NULL;
if (!IN6_IS_ADDR_LOOPBACK(&s_in.sin_addr))
@@ -916,7 +919,11 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
}
if ((s = udp_tap_map[V6][src].sock) < 0) {
- union udp_epoll_ref uref = { .v6 = 1, .port = src };
+ union udp_epoll_ref uref = {
+ .v6 = 1,
+ .port = src,
+ .pif = PIF_HOST,
+ };
const char *bind_if = NULL;
if (!IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr))
--
2.43.2
Streamline the logic here slightly, by introducing a 'src' temporary for
brevity. We also transform the logic for setting/clearing PORT_LOOPBACK.
This makes udp_update_hdr4() more closely match the corresponding logic
from udp_update_udp6().
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
udp.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/udp.c b/udp.c
index d992acd0..561ba604 100644
--- a/udp.c
+++ b/udp.c
@@ -584,6 +584,7 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t
dstport,
const struct timespec *now)
{
struct udp4_l2_buf_t *b = &udp4_l2_buf[n];
+ struct in_addr *src;
in_port_t src_port;
size_t ip_len;
@@ -592,26 +593,26 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t
dstport,
b->iph.tot_len = htons(ip_len);
b->iph.daddr = c->ip4.addr_seen.s_addr;
+ src = &b->s_in.sin_addr;
src_port = ntohs(b->s_in.sin_port);
if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match) &&
- IN4_ARE_ADDR_EQUAL(&b->s_in.sin_addr, &c->ip4.dns_host) &&
- src_port == 53) {
+ IN4_ARE_ADDR_EQUAL(src, &c->ip4.dns_host) && src_port == 53) {
b->iph.saddr = c->ip4.dns_match.s_addr;
- } else if (IN4_IS_ADDR_LOOPBACK(&b->s_in.sin_addr) ||
- IN4_ARE_ADDR_EQUAL(&b->s_in.sin_addr, &c->ip4.addr_seen)) {
+ } else if (IN4_IS_ADDR_LOOPBACK(src) ||
+ IN4_ARE_ADDR_EQUAL(src, &c->ip4.addr_seen)) {
b->iph.saddr = c->ip4.gw.s_addr;
udp_tap_map[V4][src_port].ts = now->tv_sec;
udp_tap_map[V4][src_port].flags |= PORT_LOCAL;
- if (IN4_ARE_ADDR_EQUAL(&b->s_in.sin_addr.s_addr, &c->ip4.addr_seen))
- udp_tap_map[V4][src_port].flags &= ~PORT_LOOPBACK;
- else
+ if (IN4_IS_ADDR_LOOPBACK(src))
udp_tap_map[V4][src_port].flags |= PORT_LOOPBACK;
+ else
+ udp_tap_map[V4][src_port].flags &= ~PORT_LOOPBACK;
bitmap_set(udp_act[V4][UDP_ACT_TAP], src_port);
} else {
- b->iph.saddr = b->s_in.sin_addr.s_addr;
+ b->iph.saddr = src->s_addr;
}
udp_update_check4(b);
--
2.43.2
When forwarding IPv4 packets in udp_tap_handler(), we incorrectly use an IPv6 address test on our IPv4 address (which could cause an out of bounds access), and possibly set our bind interface to the IPv6 interface based on it. Adjust to correctly look at the IPv4 address and IPv4 interface. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- udp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/udp.c b/udp.c index 561ba604..d55a6827 100644 --- a/udp.c +++ b/udp.c @@ -875,8 +875,8 @@ int udp_tap_handler(struct ctx *c, uint8_t pif, }; const char *bind_if = NULL; - if (!IN6_IS_ADDR_LOOPBACK(&s_in.sin_addr)) - bind_if = c->ip6.ifname_out; + if (!IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr)) + bind_if = c->ip4.ifname_out; if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out) && !IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr)) -- 2.43.2
If the configured output address is unspecified, we don't set the bind address to it when creating a new socket in udp_tap_handler(). That sounds sensible, but what we're leaving the bind address as is, exactly, the unspecified address, so this test makes no difference. Remove it. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- udp.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/udp.c b/udp.c index d55a6827..5b7778eb 100644 --- a/udp.c +++ b/udp.c @@ -878,8 +878,7 @@ int udp_tap_handler(struct ctx *c, uint8_t pif, if (!IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr)) bind_if = c->ip4.ifname_out; - if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out) && - !IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr)) + if (!IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr)) bind_addr = c->ip4.addr_out; s = sock_l4(c, AF_INET, IPPROTO_UDP, &bind_addr, @@ -930,8 +929,7 @@ int udp_tap_handler(struct ctx *c, uint8_t pif, if (!IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr)) bind_if = c->ip6.ifname_out; - if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_out) && - !IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr) && + if (!IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr) && !IN6_IS_ADDR_LINKLOCAL(&s_in6.sin6_addr)) bind_addr = &c->ip6.addr_out; -- 2.43.2
On Wed, 28 Feb 2024 16:39:24 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Partly in looking at flow table support, partly for other reasons, I've spotted a number of small fixes that can be made to the UDP code (in addition to the large fixes I'm still working on). These are ready to go now, so here they are. They'll go before the flow related addressing cleanups. Changes since v1: * Removed patch to unconditionally remove ACT flags, which was Just Plain Wrong. * Removed patch to sort out incorrect handling of port flags. While there's a real bug there, it interacts with at least two more bugs that I hadn't spotted, complicating the picture. I'll fix those some other day. David Gibson (5): udp: Don't attempt to translate a 0.0.0.0 source address udp: Set pif in epoll reference for ephemeral host sockets udp: Small streamline to udp_update_hdr4() udp: Fix incorrect usage of IPv6 state in IPv4 path udp: Remove unnecessary test for unspecified addr_outApplied. -- Stefano