Re: [PATCH v2 10/10] tcp_splice: Fill out flowside information for spliced connections

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 28 Aug 2023 15:41:46 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Every flow in the flow table now has space for the the addresses as seen by
> both the host and guest side.  We fill that information in for regular
> "tap" TCP connections, but not for spliced connections.
> 
> Fill in that information for spliced connections too, so it's now uniformly
> available for all flow types (that we've implemented so far).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tcp.c        | 46 +++++++++++++++++++---------------------------
>  tcp_splice.c | 40 ++++++++++++++++++++++++++--------------
>  tcp_splice.h |  3 +--
>  3 files changed, 46 insertions(+), 43 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 297134f..7459fc2 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2639,37 +2639,25 @@ static void tcp_snat_inbound(const struct ctx *c, union inany_addr *addr)
>   * tcp_tap_conn_from_sock() - Initialize state for non-spliced connection
>   * @c:		Execution context
>   * @ref:	epoll reference of listening socket
> - * @conn:	connection structure to initialize
> + * @conn:	connection structure (with TAPSIDE(@conn) completed)
>   * @s:		Accepted socket
> - * @sa:		Peer socket address (from accept())
>   * @now:	Current timestamp
> - *
> - * Return: true if able to create a tap connection, false otherwise
>   */
> -static bool tcp_tap_conn_from_sock(struct ctx *c,
> +static void tcp_tap_conn_from_sock(struct ctx *c,
>  				   union tcp_listen_epoll_ref ref,
>  				   struct tcp_tap_conn *conn, int s,
> -				   struct sockaddr *sa,
>  				   const struct timespec *now)
>  {
>  	char fsstr[FLOWSIDE_STRLEN];
>  
> +	ASSERT(flowside_complete(SOCKSIDE(conn)));
> +
>  	conn->f.type = FLOW_TCP;
>  	conn->sock = s;
>  	conn->timer = -1;
>  	conn->ws_to_tap = conn->ws_from_tap = 0;
>  	conn_event(c, conn, SOCK_ACCEPTED);
>  
> -	if (flowside_getsockname(SOCKSIDE(conn), s) < 0) {
> -		err("tcp: Failed to get local name, connection dropped");
> -		return false;
> -	}
> -	inany_from_sockaddr(&SOCKSIDE(conn)->eaddr, &SOCKSIDE(conn)->eport, sa);
> -
> -	ASSERT(flowside_complete(SOCKSIDE(conn)));
> -	debug("TCP: index %li, new connection from socket, %s", FLOW_IDX(conn),
> -	      flowside_fmt(SOCKSIDE(conn), fsstr, sizeof(fsstr)));
> -
>  	TAPSIDE(conn)->faddr = SOCKSIDE(conn)->eaddr;
>  	TAPSIDE(conn)->fport = SOCKSIDE(conn)->eport;
>  	tcp_snat_inbound(c, &TAPSIDE(conn)->faddr);
> @@ -2699,8 +2687,6 @@ static bool tcp_tap_conn_from_sock(struct ctx *c,
>  	conn_flag(c, conn, ACK_FROM_TAP_DUE);
>  
>  	tcp_get_sndbuf(conn);
> -
> -	return true;
>  }
>  
>  /**
> @@ -2712,6 +2698,7 @@ static bool tcp_tap_conn_from_sock(struct ctx *c,
>  void tcp_listen_handler(struct ctx *c, union epoll_ref ref,
>  			const struct timespec *now)
>  {
> +	char fsstr[FLOWSIDE_STRLEN];
>  	struct sockaddr_storage sa;
>  	union flow *flow;
>  	socklen_t sl;
> @@ -2730,20 +2717,25 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref,
>  	if (s < 0)
>  		return;
>  
> -	flow = flowtab + c->flow_count++;
> +	flow = flowtab + c->flow_count;
>  
> -	if (c->mode == MODE_PASTA &&
> -	    tcp_splice_conn_from_sock(c, ref.tcp_listen, &flow->tcp_splice,
> -				      s, (struct sockaddr *)&sa))
> +	if (flowside_getsockname(&flow->f.side[0], s) < 0) {
> +		err("tcp: Failed to get local name, connection dropped");
> +		close(s);
>  		return;
> +	}
> +	inany_from_sockaddr(&flow->f.side[0].eaddr, &flow->f.side[0].eport,
> +			    &sa);
> +	c->flow_count++;
>  
> -	if (tcp_tap_conn_from_sock(c, ref.tcp_listen, &flow->tcp, s,
> -				   (struct sockaddr *)&sa, now))
> +	debug("TCP: index %li, new connection from socket, %s", FLOW_IDX(flow),
> +	      flowside_fmt(&flow->f.side[0], fsstr, sizeof(fsstr)));
> +
> +	if (c->mode == MODE_PASTA &&
> +	    tcp_splice_conn_from_sock(c, ref.tcp_listen, &flow->tcp_splice, s))
>  		return;
>  
> -	/* Failed to create the connection */
> -	close(s);
> -	c->flow_count--;
> +	tcp_tap_conn_from_sock(c, ref.tcp_listen, &flow->tcp, s, now);
>  }
>  
>  /**
> diff --git a/tcp_splice.c b/tcp_splice.c
> index 676e7e8..018d095 100644
> --- a/tcp_splice.c
> +++ b/tcp_splice.c
> @@ -73,6 +73,9 @@ static int ns_sock_pool6	[TCP_SOCK_POOL_SIZE];
>  /* Pool of pre-opened pipes */
>  static int splice_pipe_pool		[TCP_SPLICE_PIPE_POOL_SIZE][2][2];
>  
> +#define ASIDE(conn)			(&(conn)->f.side[0])
> +#define BSIDE(conn)			(&(conn)->f.side[1])
> +
>  #define CONN_V6(x)			(x->flags & SPLICE_V6)
>  #define CONN_V4(x)			(!CONN_V6(x))
>  #define CONN_HAS(conn, set)		((conn->events & (set)) == (set))
> @@ -310,7 +313,16 @@ void tcp_splice_destroy(struct ctx *c, union flow *flow)
>  static int tcp_splice_connect_finish(const struct ctx *c,
>  				     struct tcp_splice_conn *conn)
>  {
> -	int i;
> +	char fsstr[FLOWSIDE_STRLEN];
> +	int i, rc;
> +
> +	rc = flowside_getsockname(BSIDE(conn), conn->b);
> +	if (rc)
> +		return rc;
> +
> +	ASSERT(flowside_complete(BSIDE(conn)));
> +	debug("TCP (splice): index %li, connection forwarded, %s", FLOW_IDX(conn),
> +	      flowside_fmt(BSIDE(conn), fsstr, sizeof(fsstr)));
>  
>  	conn->pipe_a_b[0] = conn->pipe_b_a[0] = -1;
>  	conn->pipe_a_b[1] = conn->pipe_b_a[1] = -1;
> @@ -386,10 +398,13 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn,
>  	if (CONN_V6(conn)) {
>  		sa = (struct sockaddr *)&addr6;
>  		sl = sizeof(addr6);
> +		inany_from_af(&BSIDE(conn)->eaddr, AF_INET6, &addr6.sin6_addr);
>  	} else {
>  		sa = (struct sockaddr *)&addr4;
>  		sl = sizeof(addr4);
> +		inany_from_af(&BSIDE(conn)->eaddr, AF_INET, &addr4.sin_addr);
>  	}
> +	BSIDE(conn)->eport = port;
>  
>  	if (connect(conn->b, sa, sl)) {
>  		if (errno != EINPROGRESS) {
> @@ -480,33 +495,30 @@ static void tcp_splice_dir(struct tcp_splice_conn *conn, int ref_sock,
>   * tcp_splice_conn_from_sock() - Attempt to init state for a spliced connection
>   * @c:		Execution context
>   * @ref:	epoll reference of listening socket
> - * @conn:	connection structure to initialize
> + * @conn:	connection structure (with ASIDE(@conn) completed)
>   * @s:		Accepted socket
> - * @sa:		Peer address of connection
>   *
>   * Return: true if able to create a spliced connection, false otherwise
>   * #syscalls:pasta setsockopt
>   */
>  bool tcp_splice_conn_from_sock(struct ctx *c, union tcp_listen_epoll_ref ref,
> -			       struct tcp_splice_conn *conn, int s,
> -			       const struct sockaddr *sa)
> +			       struct tcp_splice_conn *conn, int s)
>  {
> -	const struct in_addr *a4;
> -	union inany_addr aany;
> -	in_port_t port;
> +	const struct in_addr *e4 = inany_v4(&ASIDE(conn)->eaddr);
> +	const struct in_addr *f4 = inany_v4(&ASIDE(conn)->faddr);
>  
>  	ASSERT(c->mode == MODE_PASTA);
> +	ASSERT(flowside_complete(ASIDE(conn)));
>  
> -	inany_from_sockaddr(&aany, &port, sa);
> -	a4 = inany_v4(&aany);
> -
> -	if (a4) {
> -		if (!IN4_IS_ADDR_LOOPBACK(a4))
> +	if (e4) {
> +		if (!IN4_IS_ADDR_LOOPBACK(e4))
>  			return false;
> +		ASSERT(f4 && IN4_IS_ADDR_LOOPBACK(f4));

I can't follow this: the test you're replacing is actually (still) a
test used by tcp_listen_handler() unless I'm missing something.

Returning false here should simply mean we can't use a spliced
connection, not that something is wrong.

>  		conn->flags = 0;
>  	} else {
> -		if (!IN6_IS_ADDR_LOOPBACK(&aany.a6))
> +		if (!IN6_IS_ADDR_LOOPBACK(&ASIDE(conn)->eaddr.a6))
>  			return false;
> +		ASSERT(IN6_IS_ADDR_LOOPBACK(&ASIDE(conn)->faddr.a6));

...same here.

Everything else in the series looks good to me! It looks simpler (so
far) than I thought it would be.

-- 
Stefano