Re: [PATCH 4/7] udp: Separate bound sockets from flags

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 15779 bytes --]

On Tue, Feb 27, 2024 at 12:56:59PM +0100, Stefano Brivio wrote:
> On Thu, 22 Feb 2024 10:21:12 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > struct udp_tap_port contains both a socket fd and a set of flags. But,
> > confusingly, these fields are not actually related to each other at all.
> > 
> > udp_tap_map[?][port].sock contains a host UDP socket bound to 'port' (or
> > -1 if we haven't opened such a socket).  That is, it is indexed by the
> > bound port == the host side forwarding port == destination for host->guest
> > packets == source for guest->host packets.
> > 
> > udp_tap_map[?][port].flags, however, contains flags which control how we
> > direct datagrams bound for 'port' from the tap interface.  That is, it is
> > indexed by the destination for guest->host packets == source for
> > host->guest packets == host side endpoint port.
> > 
> > Worse both aspects of this share the ts field used to control expiry.  When
> > we update this because we've used the socket in slot 'port' we could be
> > clobbering the flags timestamp for an unrelated flow of packets that
> > happens to have the same endpoint port as our forwarding port.
> > 
> > Clarify this by moving the flags out into a separate array, with its own
> > timestamp and expiry path.  As a bonus this means that we can now use the
> > same data structure for tracking the socket fds of "tap" and "splice" path
> > sockets.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  udp.c | 132 ++++++++++++++++++++++++++++++----------------------------
> >  1 file changed, 68 insertions(+), 64 deletions(-)
> > 
> > diff --git a/udp.c b/udp.c
> > index 4200f849..97518d92 100644
> > --- a/udp.c
> > +++ b/udp.c
> > @@ -121,40 +121,39 @@
> >  #define UDP_MAX_FRAMES		32  /* max # of frames to receive at once */
> >  
> >  /**
> > - * struct udp_tap_port - Port tracking based on tap-facing source port
> > - * @sock:	Socket bound to source port used as index
> > + * struct udp_port_flags - Translation options by remote Port tracking based on tap-facing source port
> 
> I'm not sure what you mean by this -- perhaps "Address translation
> options for bound source ports"?

Oops, yes.  I think I partially rewrote an existing comment, leaving
an incoherent fusion of two different sentences.

> >   * @flags:	Flags for local bind, loopback address/unicast address as source
> > - * @ts:		Activity timestamp from tap, used for socket aging
> > + * @ts:		Activity timestamp
> >   */
> > -struct udp_tap_port {
> > -	int sock;
> > -	uint8_t flags;
> > +struct udp_port_flags {
> >  #define PORT_LOCAL	BIT(0)
> >  #define PORT_LOOPBACK	BIT(1)
> >  #define PORT_GUA	BIT(2)
> > -
> > +	uint8_t flags;
> >  	time_t ts;
> >  };
> > +static struct udp_port_flags udp_port_flags[IP_VERSIONS][NUM_PORTS];
> >  
> >  /**
> > - * struct udp_splice_port - Bound socket for spliced communication
> > - * @sock:	Socket bound to index port
> > + * struct udp_bound_port - Track bound UDP sockets
> > + * @sock:	Socket bound to index port (or -1)
> >   * @ts:		Activity timestamp
> >   */
> > -struct udp_splice_port {
> > +struct udp_bound_port {
> >  	int sock;
> >  	time_t ts;
> >  };
> >  
> > -/* Port tracking, arrays indexed by packet source port (host order) */
> > -static struct udp_tap_port	udp_tap_map	[IP_VERSIONS][NUM_PORTS];
> > +/* Bound sockets indexed by bound port (host order) */
> > +static struct udp_bound_port	udp_tap_sock	[IP_VERSIONS][NUM_PORTS];
> >  
> > -/* "Spliced" sockets indexed by bound port (host order) */
> > -static struct udp_splice_port udp_splice_ns  [IP_VERSIONS][NUM_PORTS];
> > -static struct udp_splice_port udp_splice_init[IP_VERSIONS][NUM_PORTS];
> > +/* Bound sockets for splicing indexed by bound port (host order) */
> > +static struct udp_bound_port	udp_splice_ns	[IP_VERSIONS][NUM_PORTS];
> > +static struct udp_bound_port	udp_splice_init	[IP_VERSIONS][NUM_PORTS];
> >  
> >  enum udp_act_type {
> > -	UDP_ACT_TAP,
> > +	UDP_ACT_TAP_SOCK,
> > +	UDP_ACT_FLAGS,
> 
> I'm having a hard time reviewing the rest because of this, I think.
> Earlier, UDP_ACT_TAP meant that the activity timestamp was observed on
> a non-spliced port.

Ah!  I'd read those as "ACTions we needed to take" in the expiry
timers, rather than tying them to specific activity.  It amounts to
the same thing pre-patch, but it makes more sense to me with that new
lens.

> Now, we have UDP_ACT_FLAGS which seems to mean that activity was seen
> in the direction of (to) the "tap" side, and UDP_ACT_TAP_SOCK meaning
> that activity was seen in the direction of (to) the socket.
> 
> But that's somehow the opposite of UDP_ACT_SPLICE_NS and
> UDP_ACT_SPLICE_INIT -- even though they don't actually indicate
> activity, rather the fact that activity timestamps refer to sockets that
> originated in (*from*) the target namespace, or in the initial
> namespace.
> 
> Anyway, I'm not quite sure: why do we need two separate flags for "tap"
> (from/to) activity?

We don't, this is an ugly side effect.  The problem is that in
scanning for expiries we only have a single port number.  But the
socket is associated with a particular forwarding port, while the
flags are associated with a particular endpoint port.  Without flow
tracking, we don't have a way to match one to the other.

The compromise here is - or is supposed to be - that we have
independent timestamps for the socket and the flags.  The two
timestamps associated with a single flow are supposed to be updated at
basically the same time - looks like I missed some things to do that
as well.

> I mean, as long as we observe activity on a non-spliced flow, we'll
> update the related timestamp, and we make sure the activity flag is
> set. Can't it simply be UDP_ACT_TAP?
> 
> The rest of the series looks good to me.
> 
> >  	UDP_ACT_SPLICE_NS,
> >  	UDP_ACT_SPLICE_INIT,
> >  	UDP_ACT_TYPE_MAX,
> > @@ -246,7 +245,7 @@ void udp_portmap_clear(void)
> >  	unsigned i;
> >  
> >  	for (i = 0; i < NUM_PORTS; i++) {
> > -		udp_tap_map[V4][i].sock = udp_tap_map[V6][i].sock = -1;
> > +		udp_tap_sock[V4][i].sock = udp_tap_sock[V6][i].sock = -1;
> >  		udp_splice_ns[V4][i].sock = udp_splice_ns[V6][i].sock = -1;
> >  		udp_splice_init[V4][i].sock = udp_splice_init[V6][i].sock = -1;
> >  	}
> > @@ -393,16 +392,16 @@ int udp_splice_new(const struct ctx *c, int v6, in_port_t src, bool ns)
> >  	union epoll_ref ref = { .type = EPOLL_TYPE_UDP,
> >  				.udp = { .splice = true, .v6 = v6, .port = src }
> >  			      };
> > -	struct udp_splice_port *sp;
> > +	struct udp_bound_port *bp;
> >  	int act, s;
> >  
> >  	if (ns) {
> >  		ref.udp.pif = PIF_SPLICE;
> > -		sp = &udp_splice_ns[v6 ? V6 : V4][src];
> > +		bp = &udp_splice_ns[v6 ? V6 : V4][src];
> >  		act = UDP_ACT_SPLICE_NS;
> >  	} else {
> >  		ref.udp.pif = PIF_HOST;
> > -		sp = &udp_splice_init[v6 ? V6 : V4][src];
> > +		bp = &udp_splice_init[v6 ? V6 : V4][src];
> >  		act = UDP_ACT_SPLICE_INIT;
> >  	}
> >  
> > @@ -437,7 +436,7 @@ int udp_splice_new(const struct ctx *c, int v6, in_port_t src, bool ns)
> >  			goto fail;
> >  	}
> >  
> > -	sp->sock = s;
> > +	bp->sock = s;
> >  	bitmap_set(udp_act[v6 ? V6 : V4][act], src);
> >  
> >  	ev.data.u64 = ref.u64;
> > @@ -601,15 +600,15 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
> >  	} else if (IN4_IS_ADDR_LOOPBACK(&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;
> > -		udp_tap_map[V4][src_port].flags |= PORT_LOCAL;
> > +		udp_port_flags[V4][src_port].ts = now->tv_sec;
> > +		udp_port_flags[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;
> > +			udp_port_flags[V4][src_port].flags &= ~PORT_LOOPBACK;
> >  		else
> > -			udp_tap_map[V4][src_port].flags |= PORT_LOOPBACK;
> > +			udp_port_flags[V4][src_port].flags |= PORT_LOOPBACK;
> >  
> > -		bitmap_set(udp_act[V4][UDP_ACT_TAP], src_port);
> > +		bitmap_set(udp_act[V4][UDP_ACT_FLAGS], src_port);
> >  	} else {
> >  		b->iph.saddr = b->s_in.sin_addr.s_addr;
> >  	}
> > @@ -664,20 +663,20 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport,
> >  		else
> >  			b->ip6h.saddr = c->ip6.addr_ll;
> >  
> > -		udp_tap_map[V6][src_port].ts = now->tv_sec;
> > -		udp_tap_map[V6][src_port].flags |= PORT_LOCAL;
> > +		udp_port_flags[V6][src_port].ts = now->tv_sec;
> > +		udp_port_flags[V6][src_port].flags |= PORT_LOCAL;
> >  
> >  		if (IN6_IS_ADDR_LOOPBACK(src))
> > -			udp_tap_map[V6][src_port].flags |= PORT_LOOPBACK;
> > +			udp_port_flags[V6][src_port].flags |= PORT_LOOPBACK;
> >  		else
> > -			udp_tap_map[V6][src_port].flags &= ~PORT_LOOPBACK;
> > +			udp_port_flags[V6][src_port].flags &= ~PORT_LOOPBACK;
> >  
> >  		if (IN6_ARE_ADDR_EQUAL(src, &c->ip6.addr))
> > -			udp_tap_map[V6][src_port].flags |= PORT_GUA;
> > +			udp_port_flags[V6][src_port].flags |= PORT_GUA;
> >  		else
> > -			udp_tap_map[V6][src_port].flags &= ~PORT_GUA;
> > +			udp_port_flags[V6][src_port].flags &= ~PORT_GUA;
> >  
> > -		bitmap_set(udp_act[V6][UDP_ACT_TAP], src_port);
> > +		bitmap_set(udp_act[V6][UDP_ACT_FLAGS], src_port);
> >  	} else {
> >  		b->ip6h.daddr = c->ip6.addr_seen;
> >  		b->ip6h.saddr = b->s_in6.sin6_addr;
> > @@ -857,16 +856,16 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
> >  			s_in.sin_addr = c->ip4.dns_host;
> >  		} else if (IN4_ARE_ADDR_EQUAL(&s_in.sin_addr, &c->ip4.gw) &&
> >  			   !c->no_map_gw) {
> > -			if (!(udp_tap_map[V4][dst].flags & PORT_LOCAL) ||
> > -			    (udp_tap_map[V4][dst].flags & PORT_LOOPBACK))
> > +			if (!(udp_port_flags[V4][dst].flags & PORT_LOCAL) ||
> > +			    (udp_port_flags[V4][dst].flags & PORT_LOOPBACK))
> >  				s_in.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> >  			else
> >  				s_in.sin_addr = c->ip4.addr_seen;
> >  		}
> >  
> >  		debug("UDP from tap src=%hu dst=%hu, s=%d",
> > -		      src, dst, udp_tap_map[V4][src].sock);
> > -		if ((s = udp_tap_map[V4][src].sock) < 0) {
> > +		      src, dst, udp_tap_sock[V4][src].sock);
> > +		if ((s = udp_tap_sock[V4][src].sock) < 0) {
> >  			struct in_addr bind_addr = IN4ADDR_ANY_INIT;
> >  			union udp_epoll_ref uref = {
> >  				.port = src,
> > @@ -886,11 +885,11 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
> >  			if (s < 0)
> >  				return p->count - idx;
> >  
> > -			udp_tap_map[V4][src].sock = s;
> > -			bitmap_set(udp_act[V4][UDP_ACT_TAP], src);
> > +			udp_tap_sock[V4][src].sock = s;
> > +			bitmap_set(udp_act[V4][UDP_ACT_TAP_SOCK], src);
> >  		}
> >  
> > -		udp_tap_map[V4][src].ts = now->tv_sec;
> > +		udp_tap_sock[V4][src].ts = now->tv_sec;
> >  	} else {
> >  		s_in6 = (struct sockaddr_in6) {
> >  			.sin6_family = AF_INET6,
> > @@ -907,10 +906,10 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
> >  			s_in6.sin6_addr = c->ip6.dns_host;
> >  		} else if (IN6_ARE_ADDR_EQUAL(daddr, &c->ip6.gw) &&
> >  			   !c->no_map_gw) {
> > -			if (!(udp_tap_map[V6][dst].flags & PORT_LOCAL) ||
> > -			    (udp_tap_map[V6][dst].flags & PORT_LOOPBACK))
> > +			if (!(udp_port_flags[V6][dst].flags & PORT_LOCAL) ||
> > +			    (udp_port_flags[V6][dst].flags & PORT_LOOPBACK))
> >  				s_in6.sin6_addr = in6addr_loopback;
> > -			else if (udp_tap_map[V6][dst].flags & PORT_GUA)
> > +			else if (udp_port_flags[V6][dst].flags & PORT_GUA)
> >  				s_in6.sin6_addr = c->ip6.addr;
> >  			else
> >  				s_in6.sin6_addr = c->ip6.addr_seen;
> > @@ -918,7 +917,7 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
> >  			bind_addr = &c->ip6.addr_ll;
> >  		}
> >  
> > -		if ((s = udp_tap_map[V6][src].sock) < 0) {
> > +		if ((s = udp_tap_sock[V6][src].sock) < 0) {
> >  			union udp_epoll_ref uref = {
> >  				.v6 = 1,
> >  				.port = src,
> > @@ -939,11 +938,11 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
> >  			if (s < 0)
> >  				return p->count - idx;
> >  
> > -			udp_tap_map[V6][src].sock = s;
> > -			bitmap_set(udp_act[V6][UDP_ACT_TAP], src);
> > +			udp_tap_sock[V6][src].sock = s;
> > +			bitmap_set(udp_act[V6][UDP_ACT_TAP_SOCK], src);
> >  		}
> >  
> > -		udp_tap_map[V6][src].ts = now->tv_sec;
> > +		udp_tap_sock[V6][src].ts = now->tv_sec;
> >  	}
> >  
> >  	for (i = 0; i < (int)p->count - idx; i++) {
> > @@ -1015,7 +1014,7 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> >  			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, addr,
> >  					 ifname, port, uref.u32);
> >  
> > -			udp_tap_map[V4][uref.port].sock = s < 0 ? -1 : s;
> > +			udp_tap_sock[V4][uref.port].sock = s < 0 ? -1 : s;
> >  			udp_splice_init[V4][port].sock = s < 0 ? -1 : s;
> >  		} else {
> >  			struct in_addr loopback = IN4ADDR_LOOPBACK_INIT;
> > @@ -1033,7 +1032,7 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> >  			r6 = s = sock_l4(c, AF_INET6, IPPROTO_UDP, addr,
> >  					 ifname, port, uref.u32);
> >  
> > -			udp_tap_map[V6][uref.port].sock = s < 0 ? -1 : s;
> > +			udp_tap_sock[V6][uref.port].sock = s < 0 ? -1 : s;
> >  			udp_splice_init[V6][port].sock = s < 0 ? -1 : s;
> >  		} else {
> >  			r6 = s = sock_l4(c, AF_INET6, IPPROTO_UDP,
> > @@ -1086,32 +1085,37 @@ static void udp_splice_iov_init(void)
> >  static void udp_timer_one(struct ctx *c, int v6, enum udp_act_type type,
> >  			  in_port_t port, const struct timespec *now)
> >  {
> > -	struct udp_splice_port *sp;
> > -	struct udp_tap_port *tp;
> > +	struct udp_bound_port *bp;
> > +	struct udp_port_flags *pf;
> >  	int *sockp = NULL;
> >  
> >  	switch (type) {
> > -	case UDP_ACT_TAP:
> > -		tp = &udp_tap_map[v6 ? V6 : V4][port];
> > +	case UDP_ACT_TAP_SOCK:
> > +		bp = &udp_tap_sock[v6 ? V6 : V4][port];
> >  
> > -		if (now->tv_sec - tp->ts > UDP_CONN_TIMEOUT) {
> > -			sockp = &tp->sock;
> > -			tp->flags = 0;
> > -		}
> > +		if (now->tv_sec - bp->ts > UDP_CONN_TIMEOUT)
> > +			sockp = &bp->sock;
> > +
> > +		break;
> > +	case UDP_ACT_FLAGS:
> > +		pf = &udp_port_flags[v6 ? V6 : V4][port];
> > +
> > +		if (now->tv_sec - pf->ts > UDP_CONN_TIMEOUT)
> > +			pf->flags = 0;
> >  
> >  		break;
> >  	case UDP_ACT_SPLICE_INIT:
> > -		sp = &udp_splice_init[v6 ? V6 : V4][port];
> > +		bp = &udp_splice_init[v6 ? V6 : V4][port];
> >  
> > -		if (now->tv_sec - sp->ts > UDP_CONN_TIMEOUT)
> > -			sockp = &sp->sock;
> > +		if (now->tv_sec - bp->ts > UDP_CONN_TIMEOUT)
> > +			sockp = &bp->sock;
> >  
> >  		break;
> >  	case UDP_ACT_SPLICE_NS:
> > -		sp = &udp_splice_ns[v6 ? V6 : V4][port];
> > +		bp = &udp_splice_ns[v6 ? V6 : V4][port];
> >  
> > -		if (now->tv_sec - sp->ts > UDP_CONN_TIMEOUT)
> > -			sockp = &sp->sock;
> > +		if (now->tv_sec - bp->ts > UDP_CONN_TIMEOUT)
> > +			sockp = &bp->sock;
> >  
> >  		break;
> >  	default:
> > @@ -1141,7 +1145,7 @@ static void udp_port_rebind(struct ctx *c, bool outbound)
> >  		= outbound ? c->udp.fwd_out.f.map : c->udp.fwd_in.f.map;
> >  	const uint8_t *rmap
> >  		= outbound ? c->udp.fwd_in.f.map : c->udp.fwd_out.f.map;
> > -	struct udp_splice_port (*socks)[NUM_PORTS]
> > +	struct udp_bound_port (*socks)[NUM_PORTS]
> >  		= outbound ? udp_splice_ns : udp_splice_init;
> >  	unsigned port;
> >  
> 

-- 
David Gibson			| 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]