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

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

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

On Wed, Oct 02, 2024 at 09:16:52AM +0200, Stefano Brivio wrote:
> 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.

Good point.

> What about:
> 
> 		"  --freebind		Allow any address for forwarding\n"
> 
> ?

Not sure that's notably better, though.  I've gone with "Bind to any
address for forwarding".

> >  		"  --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.
> 
> ?

I've done a bit of rewording of this.

> > +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.

Dangit, thought I'd done that after missing it on the last series.

> >  
> >  	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).

Oops, corrected.

> > +			       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,
> 

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