Re: [PATCH] conf, pasta: Make -g and -a skip route/addresses copy for matching IP version only

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

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

On Tue, Aug 06, 2024 at 08:38:22PM +0200, Stefano Brivio wrote:
> Paul reports that setting IPv4 address and gateway manually, using
> --address and --gateway, causes pasta to fail inserting IPv6 routes
> in a setup where multiple, inter-dependent IPv6 routes are present
> on the host.
> 
> That's because, currently, any -g option implies --no-copy-routes
> altogether, and any -a implies --no-copy-addrs.
> 
> Limit this implication to the matching IP version, instead, by having
> two copies of no_copy_routes and no_copy_addrs in the context
> structure, separately for IPv4 and IPv6.
> 
> While at it, change them to 'bool': we had them as 'int' because
> getopt_long() used to set them directly, but it hasn't been the case
> for a while already.
> 
> Reported-by: Paul Holzinger <pholzing@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  conf.c  | 32 ++++++++++++++++++++------------
>  passt.1 |  4 ++--
>  passt.h | 14 ++++++++++----
>  pasta.c |  8 ++++----
>  4 files changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 46fcd91..14d8ece 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1379,14 +1379,16 @@ void conf(struct ctx *c, int argc, char **argv)
>  				die("--no-copy-routes is for pasta mode only");
>  
>  			warn("--no-copy-routes will be dropped soon");
> -			c->no_copy_routes = copy_routes_opt = true;
> +			c->ip4.no_copy_routes = c->ip6.no_copy_routes = true;
> +			copy_routes_opt = true;
>  			break;
>  		case 19:
>  			if (c->mode != MODE_PASTA)
>  				die("--no-copy-addrs is for pasta mode only");
>  
>  			warn("--no-copy-addrs will be dropped soon");
> -			c->no_copy_addrs = copy_addrs_opt = true;
> +			c->ip4.no_copy_addrs = c->ip6.no_copy_addrs = true;
> +			copy_addrs_opt = true;
>  			break;
>  		case 20:
>  			if (c->mode != MODE_PASTA)
> @@ -1465,23 +1467,26 @@ void conf(struct ctx *c, int argc, char **argv)
>  
>  			break;
>  		case 'a':
> -			if (c->mode == MODE_PASTA)
> -				c->no_copy_addrs = 1;
> -
>  			if (inet_pton(AF_INET6, optarg, &c->ip6.addr)	&&
>  			    !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr)	&&
>  			    !IN6_IS_ADDR_LOOPBACK(&c->ip6.addr)		&&
>  			    !IN6_IS_ADDR_V4MAPPED(&c->ip6.addr)		&&
>  			    !IN6_IS_ADDR_V4COMPAT(&c->ip6.addr)		&&
> -			    !IN6_IS_ADDR_MULTICAST(&c->ip6.addr))
> +			    !IN6_IS_ADDR_MULTICAST(&c->ip6.addr)) {
> +				if (c->mode == MODE_PASTA)
> +					c->ip6.no_copy_addrs = true;
>  				break;
> +			}
>  
>  			if (inet_pton(AF_INET, optarg, &c->ip4.addr)	&&
>  			    !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr)	&&
>  			    !IN4_IS_ADDR_BROADCAST(&c->ip4.addr)	&&
>  			    !IN4_IS_ADDR_LOOPBACK(&c->ip4.addr)		&&
> -			    !IN4_IS_ADDR_MULTICAST(&c->ip4.addr))
> +			    !IN4_IS_ADDR_MULTICAST(&c->ip4.addr)) {
> +				if (c->mode == MODE_PASTA)
> +					c->ip4.no_copy_addrs = true;
>  				break;
> +			}
>  
>  			die("Invalid address: %s", optarg);
>  			break;
> @@ -1495,19 +1500,22 @@ void conf(struct ctx *c, int argc, char **argv)
>  			parse_mac(c->mac, optarg);
>  			break;
>  		case 'g':
> -			if (c->mode == MODE_PASTA)
> -				c->no_copy_routes = 1;
> -
>  			if (inet_pton(AF_INET6, optarg, &c->ip6.gw)     &&
>  			    !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.gw)	&&
> -			    !IN6_IS_ADDR_LOOPBACK(&c->ip6.gw))
> +			    !IN6_IS_ADDR_LOOPBACK(&c->ip6.gw)) {
> +				if (c->mode == MODE_PASTA)
> +					c->ip6.no_copy_routes = true;
>  				break;
> +			}
>  
>  			if (inet_pton(AF_INET, optarg, &c->ip4.gw)	&&
>  			    !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.gw)	&&
>  			    !IN4_IS_ADDR_BROADCAST(&c->ip4.gw)		&&
> -			    !IN4_IS_ADDR_LOOPBACK(&c->ip4.gw))
> +			    !IN4_IS_ADDR_LOOPBACK(&c->ip4.gw)) {
> +				if (c->mode == MODE_PASTA)
> +					c->ip4.no_copy_routes = true;
>  				break;
> +			}
>  
>  			die("Invalid gateway address: %s", optarg);
>  			break;
> diff --git a/passt.1 b/passt.1
> index 81789cc..3062b71 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -589,7 +589,7 @@ or sourced from the host, and bring up the tap interface.
>  .BR \-\-no-copy-routes " " (DEPRECATED)
>  With \-\-config-net, do not copy all the routes associated to the interface we
>  derive addresses and routes from: set up only the default gateway. Implied by
> --g, \-\-gateway.
> +-g, \-\-gateway, for the corresponding IP version only.
>  
>  Default is to copy all the routing entries from the interface in the outer
>  namespace to the target namespace, translating the output interface attribute to
> @@ -604,7 +604,7 @@ below.
>  .BR \-\-no-copy-addrs " " (DEPRECATED)
>  With \-\-config-net, do not copy all the addresses associated to the interface
>  we derive addresses and routes from: set up a single one. Implied by \-a,
> -\-\-address.
> +\-\-address, for the corresponding IP version only.
>  
>  Default is to copy all the addresses, except for link-local ones, from the
>  interface from the outer namespace to the target namespace.
> diff --git a/passt.h b/passt.h
> index d0f31a2..12ae1b9 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -100,6 +100,8 @@ enum passt_modes {
>   * @dns_host:		Use this DNS on the host for forwarding
>   * @addr_out:		Optional source address for outbound traffic
>   * @ifname_out:		Optional interface name to bind outbound sockets to
> + * @no_copy_routes:	Don't copy all routes when configuring target namespace
> + * @no_copy_addrs:	Don't copy all addresses when configuring namespace
>   */
>  struct ip4_ctx {
>  	struct in_addr addr;
> @@ -112,6 +114,9 @@ struct ip4_ctx {
>  
>  	struct in_addr addr_out;
>  	char ifname_out[IFNAMSIZ];
> +
> +	bool no_copy_routes;
> +	bool no_copy_addrs;
>  };
>  
>  /**
> @@ -126,6 +131,8 @@ struct ip4_ctx {
>   * @dns_host:		Use this DNS on the host for forwarding
>   * @addr_out:		Optional source address for outbound traffic
>   * @ifname_out:		Optional interface name to bind outbound sockets to
> + * @no_copy_routes:	Don't copy all routes when configuring target namespace
> + * @no_copy_addrs:	Don't copy all addresses when configuring namespace
>   */
>  struct ip6_ctx {
>  	struct in6_addr addr;
> @@ -139,6 +146,9 @@ struct ip6_ctx {
>  
>  	struct in6_addr addr_out;
>  	char ifname_out[IFNAMSIZ];
> +
> +	bool no_copy_routes;
> +	bool no_copy_addrs;
>  };
>  
>  #include <netinet/if_ether.h>
> @@ -173,8 +183,6 @@ struct ip6_ctx {
>   * @pasta_ifn:		Name of namespace interface for pasta
>   * @pasta_ifi:		Index of namespace interface for pasta
>   * @pasta_conf_ns:	Configure namespace after creating it
> - * @no_copy_routes:	Don't copy all routes when configuring target namespace
> - * @no_copy_addrs:	Don't copy all addresses when configuring namespace
>   * @no_tcp:		Disable TCP operation
>   * @tcp:		Context for TCP protocol handler
>   * @no_tcp:		Disable UDP operation
> @@ -233,8 +241,6 @@ struct ctx {
>  	char pasta_ifn[IF_NAMESIZE];
>  	unsigned int pasta_ifi;
>  	int pasta_conf_ns;
> -	int no_copy_routes;
> -	int no_copy_addrs;
>  
>  	int no_tcp;
>  	struct tcp_ctx tcp;
> diff --git a/pasta.c b/pasta.c
> index b4a3d99..615ff7b 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -306,7 +306,7 @@ void pasta_ns_conf(struct ctx *c)
>  		nl_link_up(nl_sock_ns, c->pasta_ifi, c->mtu);
>  
>  		if (c->ifi4) {
> -			if (c->no_copy_addrs) {
> +			if (c->ip4.no_copy_addrs) {
>  				rc = nl_addr_set(nl_sock_ns, c->pasta_ifi,
>  						 AF_INET,
>  						 &c->ip4.addr,
> @@ -322,7 +322,7 @@ void pasta_ns_conf(struct ctx *c)
>  				    strerror(-rc));
>  			}
>  
> -			if (c->no_copy_routes) {
> +			if (c->ip4.no_copy_routes) {
>  				rc = nl_route_set_def(nl_sock_ns, c->pasta_ifi,
>  						      AF_INET, &c->ip4.gw);
>  			} else {
> @@ -337,7 +337,7 @@ void pasta_ns_conf(struct ctx *c)
>  		}
>  
>  		if (c->ifi6) {
> -			if (c->no_copy_addrs) {
> +			if (c->ip6.no_copy_addrs) {
>  				rc = nl_addr_set(nl_sock_ns, c->pasta_ifi,
>  						 AF_INET6, &c->ip6.addr, 64);
>  			} else {
> @@ -351,7 +351,7 @@ void pasta_ns_conf(struct ctx *c)
>  				    strerror(-rc));
>  			}
>  
> -			if (c->no_copy_routes) {
> +			if (c->ip6.no_copy_routes) {
>  				rc = nl_route_set_def(nl_sock_ns, c->pasta_ifi,
>  						      AF_INET6, &c->ip6.gw);
>  			} else {

-- 
David Gibson (he or they)	| 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 --]