Re: [PATCH v3] conf, tcp, udp: Allow specification of interface to bind to

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

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

On Tue, Oct 11, 2022 at 01:32:20AM +0200, Stefano Brivio wrote:
> Since kernel version 5.7, commit c427bfec18f2 ("net: core: enable
> SO_BINDTODEVICE for non-root users"), we can bind sockets to
> interfaces, if they haven't been bound yet (as in bind()).
> 
> Introduce an optional interface specification for forwarded ports,
> prefixed by %, that can be passed together with an address.
> 
> Reported use case: running local services that use ports we want
> to have externally forwarded:
>   https://github.com/containers/podman/issues/14425
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> v3:
>   - escape % characters in usage() formatting
> v2:
>   - fix check on interface name length (spec - ifname, not
>     ifname - buf)
> 
>  conf.c  | 31 +++++++++++++++++++++----------
>  icmp.c  |  4 ++--
>  passt.1 | 12 ++++++++++--
>  tcp.c   | 27 +++++++++++++++------------
>  tcp.h   |  2 +-
>  udp.c   | 35 ++++++++++++++++++-----------------
>  udp.h   |  2 +-
>  util.c  | 19 ++++++++++++++++++-
>  util.h  |  3 ++-
>  9 files changed, 88 insertions(+), 47 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 779371f..93ca0cd 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -180,8 +180,8 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
>  		      struct port_fwd *fwd)
>  {
>  	char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
> +	char buf[BUFSIZ], *spec, *ifname = NULL, *p;
>  	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
> -	char buf[BUFSIZ], *spec, *p;
>  	sa_family_t af = AF_UNSPEC;
>  	bool exclude_only = true;
>  
> @@ -209,9 +209,9 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
>  
>  		for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
>  			if (optname == 't')
> -				tcp_sock_init(c, 0, AF_UNSPEC, NULL, i);
> +				tcp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, i);
>  			else if (optname == 'u')
> -				udp_sock_init(c, 0, AF_UNSPEC, NULL, i);
> +				udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, i);
>  		}
>  
>  		return 0;
> @@ -231,6 +231,14 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
>  		if (optname != 't' && optname != 'u')
>  			goto bad;
>  
> +		if ((ifname = strchr(buf, '%'))) {
> +			if (spec - ifname >= IFNAMSIZ - 1)
> +				goto bad;
> +
> +			*ifname = 0;
> +			ifname++;
> +		}
> +
>  		if (inet_pton(AF_INET, buf, addr))
>  			af = AF_INET;
>  		else if (inet_pton(AF_INET6, buf, addr))
> @@ -278,9 +286,9 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
>  			bitmap_set(fwd->map, i);
>  
>  			if (optname == 't')
> -				tcp_sock_init(c, 0, af, addr, i);
> +				tcp_sock_init(c, 0, af, addr, NULL, i);
>  			else if (optname == 'u')
> -				udp_sock_init(c, 0, af, addr, i);
> +				udp_sock_init(c, 0, af, addr, NULL, i);

AFAICT nothing prevents specifying an interface with the exclude only
case, in which case shouldn't you also be passing ifname here?

Apart from that, LGTM.

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