Re: [PATCH 3/4] pif: Record originating pif in listening socket refs

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

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

On Fri, Nov 03, 2023 at 10:47:36AM +0100, Stefano Brivio wrote:
> On Mon,  9 Oct 2023 19:30:12 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > For certain socket types, we record in the epoll ref whether they're
> > sockets in the namespace, or on the host.  We now have the notion of "pif"
> > to indicate what "place" a socket is associated with, so generalise the
> > simple one-bit 'ns' to a pif id.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  passt.h      |  1 +
> >  tcp.c        |  5 +++--
> >  tcp.h        |  4 ++--
> >  tcp_splice.c | 10 ++++++----
> >  udp.c        | 23 ++++++++++++-----------
> >  udp.h        |  8 ++++----
> >  6 files changed, 28 insertions(+), 23 deletions(-)
> > 
> > diff --git a/passt.h b/passt.h
> > index 53defa4..cac720a 100644
> > --- a/passt.h
> > +++ b/passt.h
> > @@ -35,6 +35,7 @@ union epoll_ref;
> >  #include <assert.h>
> >  #include <sys/epoll.h>
> >  
> > +#include "pif.h"
> >  #include "packet.h"
> >  #include "icmp.h"
> >  #include "port_fwd.h"
> > diff --git a/tcp.c b/tcp.c
> > index c13b7de..bad8c38 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -2964,6 +2964,7 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
> >  {
> >  	union tcp_listen_epoll_ref tref = {
> >  		.port = port + c->tcp.fwd_in.delta[port],
> > +		.pif = PIF_HOST,
> >  	};
> >  	int s;
> >  
> > @@ -3025,7 +3026,7 @@ static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port)
> >  {
> >  	union tcp_listen_epoll_ref tref = {
> >  		.port = port + c->tcp.fwd_out.delta[port],
> > -		.ns = true,
> > +		.pif = PIF_SPLICE,
> >  	};
> >  	struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
> >  	int s;
> > @@ -3051,7 +3052,7 @@ static void tcp_ns_sock_init6(const struct ctx *c, in_port_t port)
> >  {
> >  	union tcp_listen_epoll_ref tref = {
> >  		.port = port + c->tcp.fwd_out.delta[port],
> > -		.ns = true,
> > +		.pif = PIF_SPLICE,
> >  	};
> >  	int s;
> >  
> > diff --git a/tcp.h b/tcp.h
> > index 6444d6a..3f6937c 100644
> > --- a/tcp.h
> > +++ b/tcp.h
> > @@ -41,13 +41,13 @@ union tcp_epoll_ref {
> >  /**
> >   * union tcp_listen_epoll_ref - epoll reference portion for TCP listening
> >   * @port:	Port number we're forwarding *to* (listening port plus delta)
> > - * @ns:		True if listening within the pasta namespace
> > + * @pif:	Pif in which the socket is listening
> >   * @u32:	Opaque u32 value of reference
> >   */
> >  union tcp_listen_epoll_ref {
> >  	struct {
> >  		in_port_t	port;
> > -		bool		ns;
> > +		pif_id		pif;
> >  	};
> >  	uint32_t u32;
> >  };
> > diff --git a/tcp_splice.c b/tcp_splice.c
> > index 54fc317..19f5406 100644
> > --- a/tcp_splice.c
> > +++ b/tcp_splice.c
> > @@ -411,12 +411,12 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn,
> >   * @c:		Execution context
> >   * @conn:	Connection pointer
> >   * @port:	Destination port, host order
> > - * @outbound:	Connection request coming from namespace
> > + * @pif:	Originating pif of the splice
> >   *
> >   * Return: return code from connect()
> >   */
> >  static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn,
> > -			  in_port_t port, int outbound)
> > +			  in_port_t port, pif_id pif)
> >  {
> >  	int s = -1;
> >  
> > @@ -427,7 +427,7 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn,
> >  	 * entering the ns anyway, so we might as well refill the
> >  	 * pool.
> >  	 */
> > -	if (outbound) {
> > +	if (pif == PIF_SPLICE) {
> >  		int *p = CONN_V6(conn) ? init_sock_pool6 : init_sock_pool4;
> >  		int af = CONN_V6(conn) ? AF_INET6 : AF_INET;
> >  
> > @@ -437,6 +437,8 @@ static int tcp_splice_new(const struct ctx *c, struct tcp_splice_conn *conn,
> >  	} else {
> >  		int *p = CONN_V6(conn) ? ns_sock_pool6 : ns_sock_pool4;
> >  
> > +		ASSERT(pif == PIF_HOST);
> > +
> >  		/* If pool is empty, refill it first */
> >  		if (p[TCP_SOCK_POOL_SIZE-1] < 0)
> >  			NS_CALL(tcp_sock_refill_ns, c);
> > @@ -516,7 +518,7 @@ bool tcp_splice_conn_from_sock(const struct ctx *c,
> >  	conn->c.spliced = true;
> >  	conn->a = s;
> >  
> > -	if (tcp_splice_new(c, conn, ref.port, ref.ns))
> > +	if (tcp_splice_new(c, conn, ref.port, ref.pif))
> >  		conn_flag(c, conn, CLOSING);
> >  
> >  	return true;
> > diff --git a/udp.c b/udp.c
> > index db35859..ce02979 100644
> > --- a/udp.c
> > +++ b/udp.c
> > @@ -365,7 +365,7 @@ static void udp_sock6_iov_init(const struct ctx *c)
> >   * @c:		Execution context
> >   * @v6:		Set for IPv6 sockets
> >   * @src:	Source port of original connection, host order
> > - * @splice:	UDP_BACK_TO_INIT from init, UDP_BACK_TO_NS from namespace
> > + * @ns:		Does the splice originate in the ns or not
> >   *
> >   * Return: prepared socket, negative error code on failure
> >   *
> > @@ -375,16 +375,17 @@ int udp_splice_new(const struct ctx *c, int v6, in_port_t src, bool ns)
> >  {
> >  	struct epoll_event ev = { .events = EPOLLIN | EPOLLRDHUP | EPOLLHUP };
> >  	union epoll_ref ref = { .type = EPOLL_TYPE_UDP,
> > -				.udp = { .splice = true, .ns = ns,
> > -					 .v6 = v6, .port = src }
> > +				.udp = { .splice = true, .v6 = v6, .port = src }
> >  			      };
> >  	struct udp_splice_port *sp;
> >  	int act, s;
> >  
> >  	if (ns) {
> > +		ref.udp.pif = PIF_SPLICE;
> >  		sp = &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];
> >  		act = UDP_ACT_SPLICE_INIT;
> >  	}
> > @@ -495,15 +496,15 @@ static int udp_mmh_splice_port(bool v6, const struct mmsghdr *mmh)
> >   * @n:		Number of datagrams to send
> >   * @src:	Datagrams will be sent from this port (on origin side)
> >   * @dst:	Datagrams will be send to this port (on destination side)
> > + * @from_pif:	Pif from which the packet originated
> 
> "pif" isn't a word and not a proper acronym either... should we keep
> this simpler and always spell it lowercase (we would do the same with,
> say, "uint8_t") even at the beginning of a sentence? Not a strong
> preference.

Makes sense to me, changed throughout.

> >   * @v6:		Send as IPv6?
> > - * @from_ns:	If true send from pasta ns to init, otherwise reverse
> >   * @allow_new:	If true create sending socket if needed, if false discard
> >   *              if no sending socket is available
> >   * @now:	Timestamp
> >   */
> >  static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n,
> > -				in_port_t src, in_port_t dst,
> > -				bool v6, bool from_ns, bool allow_new,
> > +				in_port_t src, in_port_t dst, pif_id from_pif,
> > +				bool v6, bool allow_new,
> >  				const struct timespec *now)
> >  {
> >  	struct mmsghdr *mmh_recv, *mmh_send;
> > @@ -518,7 +519,7 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n,
> >  		mmh_send = udp4_mh_splice;
> >  	}
> >  
> > -	if (from_ns) {
> > +	if (from_pif == PIF_SPLICE) {
> >  		src += c->udp.fwd_in.rdelta[src];
> >  		s = udp_splice_init[v6][src].sock;
> >  		if (!s && allow_new)
> > @@ -530,6 +531,7 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n,
> >  		udp_splice_ns[v6][dst].ts = now->tv_sec;
> >  		udp_splice_init[v6][src].ts = now->tv_sec;
> >  	} else {
> > +		ASSERT(from_pif == PIF_HOST);
> >  		src += c->udp.fwd_out.rdelta[src];
> >  		s = udp_splice_ns[v6][src].sock;
> >  		if (!s && allow_new) {
> > @@ -776,7 +778,7 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events,
> >  
> >  		if (splicefrom >= 0)
> >  			udp_splice_sendfrom(c, i, m, splicefrom, dstport,
> > -					    v6, ref.udp.ns, ref.udp.orig, now);
> > +					    ref.udp.pif, v6, ref.udp.orig, now);
> >  		else
> >  			udp_tap_send(c, i, m, dstport, v6, now);
> >  	}
> > @@ -974,8 +976,10 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> >  	int s, r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1;
> >  
> >  	if (ns) {
> > +		uref.pif = PIF_SPLICE;
> >  		uref.port = (in_port_t)(port + c->udp.fwd_out.f.delta[port]);
> >  	} else {
> > +		uref.pif = PIF_HOST;
> >  		uref.port = (in_port_t)(port + c->udp.fwd_in.f.delta[port]);
> >  	}
> >  
> > @@ -990,7 +994,6 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> >  			udp_splice_init[V4][port].sock = s < 0 ? -1 : s;
> >  		} else {
> >  			struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
> > -			uref.ns = true;
> >  
> >  			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
> >  					 ifname, port, uref.u32);
> > @@ -1008,8 +1011,6 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
> >  			udp_tap_map[V6][uref.port].sock = s < 0 ? -1 : s;
> >  			udp_splice_init[V6][port].sock = s < 0 ? -1 : s;
> >  		} else {
> > -			uref.ns = true;
> > -
> >  			r6 = s = sock_l4(c, AF_INET6, IPPROTO_UDP,
> >  					 &in6addr_loopback,
> >  					 ifname, port, uref.u32);
> > diff --git a/udp.h b/udp.h
> > index 7837134..122a3d9 100644
> > --- a/udp.h
> > +++ b/udp.h
> > @@ -20,21 +20,21 @@ void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s);
> >  
> >  /**
> >   * union udp_epoll_ref - epoll reference portion for TCP connections
> > + * @port:		Source port for connected sockets, bound port otherwise
> > + * @pif:		Pif for this socket
> >   * @bound:		Set if this file descriptor is a bound socket
> >   * @splice:		Set if descriptor packets to be "spliced"
> >   * @orig:		Set if a spliced socket which can originate "connections"
> > - * @ns:			Set if this is a socket in the pasta network namespace
> >   * @v6:			Set for IPv6 sockets or connections
> > - * @port:		Source port for connected sockets, bound port otherwise
> >   * @u32:		Opaque u32 value of reference
> >   */
> >  union udp_epoll_ref {
> >  	struct {
> > +		in_port_t	port;
> > +		pif_id		pif;
> >  		bool		splice:1,
> >  				orig:1,
> > -				ns:1,
> >  				v6:1;
> > -		uint32_t	port:16;
> 
> This is rather a comment to 2/4, but its application is more obvious
> here: having 'uint8_t pif' would show clearly where it needs to be
> placed in these types of struct, instead of hiding its type behind the
> typedef.

Oof.  I have mixed thoughts about this.  In this instance, you're
right, that the typedef obscures what's going in here, in a packed
bitfield where it's pretty relevant.  In other places the type makes
it clearer what's valid information in that slot.  So I'm not really
sure which way to go.

> The rest of the series looks good to me.
> 

-- 
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 --]