Re: [PATCH 3/3] fwd, conf: Probe host's ephemeral ports

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

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

On Wed, Aug 28, 2024 at 12:22:18PM +0200, Laurent Vivier wrote:
> On 28/08/2024 07:56, David Gibson wrote:
> > When we forward "all" ports (-t all or -u all), or use an exclude-only
> > range, we don't actually forward *all* ports - that wouln't leave local
> > ports to use for outgoing connections.  Rather we forward all non-ephemeral
> > ports - those that won't be used for outgoing connections or datagrams.
> > 
> > Currently we assume the range of ephemeral ports is that recommended by
> > RFC 6335, 49152-65535.  However, that's not the range used by default on
> > Linux, 32768-60999 but configurable with the net.ipv4.ip_local_port_range
> > sysctl.
> > 
> > We can't really know what range the guest will consider ephemeral, but if
> > it differs too much from the host it's likely to cause problems we can't
> > avoid anyway.  So, using the host's ephemeral range is a better guess than
> > using the RFC 6335 range.
> > 
> > Therefore, add logic to probe the host's ephemeral range, falling back to
> > the RFC 6335 range if that fails.  This has the bonus advantage of
> > reducing the number of ports bound by -t all, -u all on most Linux machines
> > thereby reducing kernel memory usage.  Specifically this reduces kernel
> > memory usage with -t all, -u all from ~380MiB to ~289MiB.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >   conf.c |  1 +
> >   fwd.c  | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >   fwd.h  |  1 +
> >   3 files changed, 57 insertions(+), 2 deletions(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index 3eb117ff..b2758864 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -1721,6 +1721,7 @@ void conf(struct ctx *c, int argc, char **argv)
> >   	/* Inbound port options & DNS can be parsed now (after IPv4/IPv6
> >   	 * settings)
> >   	 */
> > +	fwd_probe_ephemeral();
> >   	udp_portmap_clear();
> >   	optind = 0;
> >   	do {
> > diff --git a/fwd.c b/fwd.c
> > index adf61cb5..40f556e9 100644
> > --- a/fwd.c
> > +++ b/fwd.c
> > @@ -28,8 +28,61 @@
> >   #include "flow_table.h"
> >   /* Empheral port range: values from RFC 6335 */
> > -static const uint16_t fwd_ephemeral_min = (1 << 15) + (1 << 14);
> > -static const uint16_t fwd_ephemeral_max = NUM_PORTS - 1;
> > +static uint16_t fwd_ephemeral_min = (1 << 15) + (1 << 14);
> > +static uint16_t fwd_ephemeral_max = NUM_PORTS - 1;
> > +
> > +#define PORT_RANGE_SYSCTL	"/proc/sys/net/ipv4/ip_local_port_range"
> > +
> > +/** fwd_probe_ephemeral() - Determine what ports this host considers ephemeral
> > + *
> > + * Work out what ports the host thinks are emphemeral and record it for later
> > + * use by fwd_port_is_ephemeral().  If we're unable to probe, assume the range
> > + * recommended by RFC 6335.
> > + */
> > +void fwd_probe_ephemeral(void)
> > +{
> > +	char *line, *tab, *end;
> > +	struct lineread lr;
> > +	long min, max;
> > +	ssize_t len;
> > +	int fd;
> > +
> > +	fd = open(PORT_RANGE_SYSCTL, O_RDONLY | O_CLOEXEC);
> 
> Why O_CLOEXEC?

AIUI current security best practices recommend using O_CLOEXEC
basically always.  clang-tidy complains if it's not there.

> There is no close() in the function, do you rely on it to close the file descriptor?

No, just a very dumb oversight.

> > +	if (fd < 0)
> > +		warn_perror("Unable to open %s", PORT_RANGE_SYSCTL);
> 
> goto parse_error ?

No, this is a different error, but there should be a return.  Added.

> or if you add the close() in parse_error, we need a return.
> 
> > +
> > +	lineread_init(&lr, fd);
> > +	len = lineread_get(&lr, &line);
> > +	if (len < 0)
> > +		goto parse_err;
> > +
> > +	tab = strchr(line, '\t');
> > +	if (!tab)
> > +		goto parse_err;
> > +	*tab = '\0';
> > +
> > +	errno = 0;
> > +	min = strtol(line, &end, 10);
> > +	if (*end || errno)
> > +		goto parse_err;
> > +
> > +	errno = 0;
> > +	max = strtol(tab + 1, &end, 10);
> > +	if (*end || errno)
> > +		goto parse_err;
> 
> As /proc files are well formated, why don't you use fscanf()?
> Something like:
> 
>         FILE *f;
> 
>         f = fopen(PORT_RANGE_SYSCTL, "r");
> 	if (f == NULL) {
> 		warn("Unable to parse %s", PORT_RANGE_SYSCTL);
> 		return;
> 	}
>         ret = fscanf(f, "%d %d", &min, &max);
>         fclose(f);
>         if (ret != 2)
>                 goto parse_error;

Hm, maybe.  I never feel like I know exactly what the parse rules for
scanf() are, so I tend to avoid it.  Stefano, any thoughts?

> 
> Thanks,
> Laurent
> > +
> > +	if (min < 0 || min >= NUM_PORTS ||
> > +	    max < 0 || max >= NUM_PORTS)
> > +		goto parse_err;
> > +
> > +	fwd_ephemeral_min = min;
> > +	fwd_ephemeral_max = max;
> > +
> > +	return;
> > +
> > +parse_err:
> > +	warn("Unable to parse %s", PORT_RANGE_SYSCTL);
> > +}
> >   /**
> >    * fwd_port_is_ephemeral() - Is port number ephemeral?
> > diff --git a/fwd.h b/fwd.h
> > index 42fe57eb..23aac5b2 100644
> > --- a/fwd.h
> > +++ b/fwd.h
> > @@ -12,6 +12,7 @@ struct flowside;
> >   /* Number of ports for both TCP and UDP */
> >   #define	NUM_PORTS	(1U << 16)
> > +void fwd_probe_ephemeral(void);
> >   bool fwd_port_is_ephemeral(uint16_t port);
> >   enum fwd_ports_mode {
> 

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