Re: [PATCH] conf: Add command line switch to enable IP_FREEBIND socket option

[Stefano Brivio <sbrivio@redhat.com>]

On Wed,  2 Oct 2024 14:47:16 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> In a couple of recent reports, we've seen that it can be useful for pasta
> to forward ports from addresses which are not currently configured on the
> host, but might be in future.  That can be done with the sysctl
> net.ipv4.ip_nonlocal_bind, but that does require CAP_NET_ADMIN to set in
> the first place.  We can allow the same thing on a per-socket basis with
> the IP_FREEBIND (or IPV6_FREEBIND) socket option.
> 
> Add a --freebind command line argument to enable this socket option on
> all listening sockets.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=101
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c  |  2 ++
>  passt.1 |  6 ++++++
>  passt.h |  1 +
>  util.c  | 15 +++++++++++++++
>  4 files changed, 24 insertions(+)
> 
> diff --git a/conf.c b/conf.c
> index 6e62510..84aa89d 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -836,6 +836,7 @@ static void usage(const char *name, FILE *f, int status)
>  		"  --no-ndp		Disable NDP responses\n"
>  		"  --no-dhcpv6		Disable DHCPv6 server\n"
>  		"  --no-ra		Disable router advertisements\n"
> +		"  --freebind		Allow forwarding from any address\n"

I think "from any address" might be a bit ambiguous, because it could
also be read as "Allow forwarding traffic coming from any address",
which is allowed regardless.

What about:

		"  --freebind		Allow any address for forwarding\n"

?

>  		"  --no-map-gw		Don't map gateway address to host\n"
>  		"  -4, --ipv4-only	Enable IPv4 operation only\n"
>  		"  -6, --ipv6-only	Enable IPv6 operation only\n");
> @@ -1255,6 +1256,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  		{"no-dhcpv6",	no_argument,		&c->no_dhcpv6,	1 },
>  		{"no-ndp",	no_argument,		&c->no_ndp,	1 },
>  		{"no-ra",	no_argument,		&c->no_ra,	1 },
> +		{"freebind",	no_argument,		&c->freebind,	1 },
>  		{"no-map-gw",	no_argument,		&no_map_gw,	1 },
>  		{"ipv4-only",	no_argument,		NULL,		'4' },
>  		{"ipv6-only",	no_argument,		NULL,		'6' },
> diff --git a/passt.1 b/passt.1
> index 79d134d..a2547f8 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -327,6 +327,12 @@ namespace will be silently dropped.
>  Disable Router Advertisements. Router Solicitations coming from guest or target
>  namespace will be ignored.
>  
> +.TP
> +.BR \-\-freebind
> +Allow forwarding from addresses which are not configured on the host,

Same here, it could also be read as "Allow forwarding traffic coming
from addresses ...".

Perhaps:

Allow binding to addresses which are not configured on the host (but
might be in the future) for port forwarding.

?

> +but might be in future. This enables the \fBIP_FREEBIND\fR or
> +\fBIPB6_FREEBIND\fR option on listening sockets.
> +
>  .TP
>  .BR \-\-map-host-loopback " " \fIaddr
>  Translate \fIaddr\fR to refer to the host. Packets from the guest to
> diff --git a/passt.h b/passt.h
> index 031c9b6..e00049e 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -284,6 +284,7 @@ struct ctx {
>  	int no_dhcpv6;
>  	int no_ndp;
>  	int no_ra;
> +	int freebind;

Missing update to struct comment.

>  
>  	int low_wmem;
>  	int low_rmem;
> diff --git a/util.c b/util.c
> index ebd93ed..96e3de8 100644
> --- a/util.c
> +++ b/util.c
> @@ -52,6 +52,7 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type,
>  {
>  	sa_family_t af = ((const struct sockaddr *)sa)->sa_family;
>  	union epoll_ref ref = { .type = type, .data = data };
> +	bool freebind = false;
>  	struct epoll_event ev;
>  	int fd, y = 1, ret;
>  	uint8_t proto;
> @@ -61,8 +62,11 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type,
>  	case EPOLL_TYPE_TCP_LISTEN:
>  		proto = IPPROTO_TCP;
>  		socktype = SOCK_STREAM | SOCK_NONBLOCK;
> +		freebind = c->freebind;
>  		break;
>  	case EPOLL_TYPE_UDP_LISTEN:
> +		freebind = c->freebind;
> +		/* fallthrough */
>  	case EPOLL_TYPE_UDP_REPLY:
>  		proto = IPPROTO_UDP;
>  		socktype = SOCK_DGRAM | SOCK_NONBLOCK;
> @@ -127,6 +131,17 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type,
>  		}
>  	}
>  
> +	if (freebind) {
> +		int level = af == AF_INET ? IPPROTO_IP : IPPROTO_IPV6;
> +		int opt = af == AF_INET ? IP_FREEBIND : IPV6_FREEBIND;
> +
> +		if (setsockopt(fd, level, opt, &y, sizeof(y))) {
> +		    err_perror("Failed to set %s on socket %i",

Indentation makes it look like err_perror() is part of the condition
(it should be one tab instead of spaces).

> +			       af == AF_INET ? "IP_FREEBIND" : "IPV6_FREEBIND",
> +			       fd);
> +		}
> +	}
> +
>  	if (bind(fd, sa, sl) < 0) {
>  		/* We'll fail to bind to low ports if we don't have enough
>  		 * capabilities, and we'll fail to bind on already bound ports,

-- 
Stefano