Re: [PATCH v2 02/16] udp: Separate tracking of inbound and outbound packet flows

[Stefano Brivio <sbrivio@redhat.com>]

Just two nits here:

On Thu, 24 Nov 2022 12:16:45 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Each entry udp_splice_map[v6][N] keeps information about two essentially
> unrelated packet flows. @ns_conn_sock, @ns_conn_ts and @init_bound_sock
> track a packet flow from port N in the host init namespace to some other
> port in the pasta namespace (the one @ns_conn_sock is connected to).
> @init_conn_sock, @init_conn_ts and @ns_bound_sock track packet flow from
> port N in the pasta namespace to some other port in the host init namespace
> (the one @init_conn_sock is connected to).
> 
> Split udp_splice_map[][] into two separate tables for the two directions.
> Each entry in each table is a 'struct udp_splice_flow' with @orig_sock
> (previously the bound socket), @target_sock (previously the connected
> socket) and @ts (the timeout for the target socket).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  udp.c | 111 +++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 56 insertions(+), 55 deletions(-)
> 
> diff --git a/udp.c b/udp.c
> index a025a48..4caf73e 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -47,44 +47,44 @@
>   *

This comment still references struct udp_splice_port, it should now say
"see struct udp_spliced_flow" instead.

>   *   - forward direction: 127.0.0.1:5000 -> 127.0.0.1:80 in init from bound
>   *     socket s, with epoll reference: index = 80, splice = UDP_TO_NS
> - *     - if udp_splice_map[V4][5000].ns_conn_sock:
> - *       - send packet to udp4_splice_map[5000].ns_conn_sock
> + *     - if udp_splice_to_ns[V4][5000].target_sock:
> + *       - send packet to udp_splice_to_ns[V4][5000].target_sock
>   *     - otherwise:
> - *       - create new socket udp_splice_map[V4][5000].ns_conn_sock
> + *       - create new socket udp_splice_to_ns[V4][5000].target_sock
>   *       - bind in namespace to 127.0.0.1:5000
>   *       - connect in namespace to 127.0.0.1:80 (note: this destination port
>   *         might be remapped to another port instead)
>   *       - add to epoll with reference: index = 5000, splice: UDP_BACK_TO_INIT
> - *       - set udp_splice_map[V4][5000].init_bound_sock to s
> - *   - update udp_splice_map[V4][5000].ns_conn_ts with current time
> + *       - set udp_splice_to_ns[V4][5000].orig_sock to s
> + *     - update udp_splice_to_ns[V4][5000].ts with current time
>   *
>   *   - reverse direction: 127.0.0.1:80 -> 127.0.0.1:5000 in namespace from
>   *     connected socket s, having epoll reference: index = 5000,
>   *     splice = UDP_BACK_TO_INIT
> - *     - if udp_splice_map[V4][5000].init_bound_sock:
> - *       - send to udp_splice_map[V4][5000].init_bound_sock, with destination
> - *         port 5000
> + *     - if udp_splice_to_ns[V4][5000].orig_sock:
> + *       - send to udp_splice_to_ns[V4][5000].orig_sock, with destination port
> + *         5000
>   *     - otherwise, discard
>   *
>   * - from namespace to init:
>   *
>   *   - forward direction: 127.0.0.1:2000 -> 127.0.0.1:22 in namespace from bound
>   *     socket s, with epoll reference: index = 22, splice = UDP_TO_INIT
> - *     - if udp4_splice_map[V4][2000].init_conn_sock:
> - *       - send packet to udp4_splice_map[2000].init_conn_sock
> + *     - if udp4_splice_to_init[V4][2000].target_sock:
> + *       - send packet to udp_splice_to_init[V4][2000].target_sock
>   *     - otherwise:
> - *       - create new socket udp_splice_map[V4][2000].init_conn_sock
> + *       - create new socket udp_splice_to_init[V4][2000].target_sock
>   *       - bind in init to 127.0.0.1:2000
>   *       - connect in init to 127.0.0.1:22 (note: this destination port
>   *         might be remapped to another port instead)
>   *       - add to epoll with reference: index = 2000, splice = UDP_BACK_TO_NS
> - *       - set udp_splice_map[V4][2000].ns_bound_sock to s
> - *     - update udp_splice_map[V4][2000].init_conn_ts with current time
> + *       - set udp_splice_to_init[V4][2000].orig_sock to s
> + *     - update udp_splice_to_init[V4][2000].ts with current time
>   *
>   *   - reverse direction: 127.0.0.1:22 -> 127.0.0.1:2000 in init from connected
>   *     socket s, having epoll reference: index = 2000, splice = UDP_BACK_TO_NS
> - *   - if udp_splice_map[V4][2000].ns_bound_sock:
> - *     - send to udp_splice_map[V4][2000].ns_bound_sock, with destination port
> + *   - if udp_splice_to_init[V4][2000].orig_sock:
> + *     - send to udp_splice_to_init[V4][2000].orig_sock, with destination port
>   *       2000
>   *   - otherwise, discard
>   */
> @@ -138,28 +138,26 @@ struct udp_tap_port {
>  };
>  
>  /**
> - * struct udp_splice_port - Source port tracking for traffic between namespaces
> - * @ns_conn_sock:	Socket connected in namespace for init source port
> - * @init_conn_sock:	Socket connected in init for namespace source port
> - * @ns_conn_ts:		Timestamp of activity for socket connected in namespace
> - * @init_conn_ts:	Timestamp of activity for socket connceted in init
> - * @ns_bound_sock:	Bound socket in namespace for this source port in init
> - * @init_bound_sock:	Bound socket in init for this source port in namespace
> + * struct udp_splice_flow - Spliced "connection"
> + * @orig_sock:		Originating socket, bound to dest port in source ns of
> + *			originating datagram
> + * @target_sock:	Target socket, bound to source port of originating
> + *			datagram in dest ns, connected to dest port of
> + *			originating datagram in dest ns
> + * @ts:			Activity timestamp
>   */
> -struct udp_splice_port {
> -	int ns_conn_sock;
> -	int init_conn_sock;
> -
> -	time_t ns_conn_ts;
> -	time_t init_conn_ts;
> -
> -	int ns_bound_sock;
> -	int init_bound_sock;
> +struct udp_splice_flow {
> +	int orig_sock;
> +	int target_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];
> -static struct udp_splice_port	udp_splice_map	[IP_VERSIONS][NUM_PORTS];
> +
> +/* Spliced "connections" indexed by originating source port (host order) */
> +static struct udp_splice_flow udp_splice_to_ns  [IP_VERSIONS][NUM_PORTS];
> +static struct udp_splice_flow udp_splice_to_init[IP_VERSIONS][NUM_PORTS];
>  
>  enum udp_act_type {
>  	UDP_ACT_TAP,
> @@ -421,8 +419,17 @@ int udp_splice_connect(const struct ctx *c, int v6, int bound_sock,
>  				.r.p.udp.udp = { .splice = splice, .v6 = v6,
>  						 .port = src }
>  			      };
> -	struct udp_splice_port *sp = &udp_splice_map[v6 ? V6 : V4][src];
> +	struct udp_splice_flow *flow;
>  	int s;
> +	int act;

...and this should go before 'int s;'.

-- 
Stefano