Hi Steve
1) I tested various combinations of ip_local_port_range and
ip_local_reserved_ports, including edge cases like ip_local_port_range
equal to ip_local_reserved_ports. In all such tests library behaved as
expected. Let me know if I should test anything else.
No, not yet. We wanted to firstly open discussion, but now we can add
this patch into our distribution.
2) You are right, there is no reason why it shouldn't be declared as
static. Should I send updated patch or you will do this minor
modification yourself?
Otto
On Thu, 2022-07-14 at 13:01 -0500, Steve Dickson wrote:
> Hey,
>
> My apologies for taking so long to get to this...
>
> A couple questions:
>
> 1) How well was tested... Is in your distro already?
> 2) Those new functions the patch introduces...
> Don't effect the API? Meaning shouldn't they
> declared as static?
>
> steved.
>
> On 6/9/22 4:26 AM, Otto Hollmann wrote:
> > Read reserved ports from
> > /proc/sys/net/ipv4/ip_local_reserved_ports,
> > store them into bit-wise array and before binding to random port
> > check
> > if port is not reserved.
> >
> >
> > Currently, there is no way how to reserve ports so then will not be
> > used by rpcbind.
> >
> > Random ports are opened by rpcbind because of rmtcalls. There is
> > compile-time flag for disabling them, but in some cases we can not
> > simply disable them.
> >
> > One solution would be run time option --enable-rmtcalls as already
> > discussed, but it was rejected. So if we want to keep rmtcalls
> > enabled
> > and also be able to reserve some ports, there is no other way than
> > filtering available ports. The easiest and clearest way seems to be
> > just respect kernel list of ip_reserved_ports.
> >
> > Unfortunately there is one known disadvantage/side effect - it
> > affects
> > probability of ports which are right after reserved ones. The
> > bigger
> > reserved block is, the higher is probability of selecting following
> > unreserved port. But if there is no reserved port, impact of this
> > patch
> > is minimal/none.
> >
> > Signed-off-by: Otto Hollmann <otto.hollmann@xxxxxxxx>
> > ---
> > src/binddynport.c | 107
> > ++++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 99 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/binddynport.c b/src/binddynport.c
> > index 062629a..6f78ebe 100644
> > --- a/src/binddynport.c
> > +++ b/src/binddynport.c
> > @@ -37,6 +37,7 @@
> > #include <unistd.h>
> > #include <errno.h>
> > #include <string.h>
> > +#include <syslog.h>
> >
> > #include <rpc/rpc.h>
> >
> > @@ -56,6 +57,84 @@ enum {
> > NPORTS = ENDPORT - LOWPORT + 1,
> > };
> >
> > +/*
> > + * This function decodes information about given port from
> > provided array and
> > + * return if port is reserved or not.
> > + *
> > + * @reserved_ports an array of size at least "NPORTS /
> > (8*sizeof(char)) + 1".
> > + * @port port number within range LOWPORT and ENDPORT
> > + *
> > + * Returns 0 if port is not reserved, non-negative if port is
> > reserved.
> > + */
> > +int is_reserved(char *reserved_ports, int port) {
> > + port -= LOWPORT;
> > + if (port < 0 || port >= NPORTS)
> > + return 0;
> > + return reserved_ports[port/(8*sizeof(char))] &
> > 1<<(port%(8*sizeof(char)));
> > +}
> > +
> > +/*
> > + * This function encodes information about given *reserved* port
> > into provided
> > + * array. Don't call this function for ports which are not
> > reserved.
> > + *
> > + * @reserved_ports array TODO .
> > + * @port port number within range LOWPORT and ENDPORT
> > + *
> > + */
> > +void set_reserved(char *reserved_ports, int port) {
> > + port -= LOWPORT;
> > + if (port < 0 || port >= NPORTS)
> > + return;
> > + reserved_ports[port/(8*sizeof(char))] |=
> > 1<<(port%(8*sizeof(char)));
> > +}
> > +
> > +/*
> > + * Parse local reserved ports obtained from
> > + * /proc/sys/net/ipv4/ip_local_reserved_ports into bit array.
> > + *
> > + * @reserved_ports a zeroed array of size at least
> > + * "NPORTS / (8*sizeof(char)) + 1". Will be used for bit-wise
> > encoding of
> > + * reserved ports.
> > + *
> > + * On each call, reserved ports are read from /proc and bit-wise
> > stored into
> > + * provided array
> > + *
> > + * Returns 0 on success, -1 on failure.
> > + */
> > +
> > +int parse_reserved_ports(char *reserved_ports) {
> > + int from, to;
> > + char delimiter = ',';
> > + int res;
> > + FILE * file_ptr =
> > fopen("/proc/sys/net/ipv4/ip_local_reserved_ports","r");
> > + if (file_ptr == NULL) {
> > + (void) syslog(LOG_ERR,
> > + "Unable to open open
> > /proc/sys/net/ipv4/ip_local_reserved_ports.");
> > + return -1;
> > + }
> > + do {
> > + if ((res = fscanf(file_ptr, "%d", &to)) != 1) {
> > + if (res == EOF) break;
> > + goto err;
> > + }
> > + if (delimiter != '-') {
> > + from = to;
> > + }
> > + for (int i = from; i <= to; ++i) {
> > + set_reserved(reserved_ports, i);
> > + }
> > + } while ((res = fscanf(file_ptr, "%c", &delimiter)) == 1);
> > + if (res != EOF)
> > + goto err;
> > + fclose(file_ptr);
> > + return 0;
> > +err:
> > + (void) syslog(LOG_ERR,
> > + "An error occurred while parsing
> > ip_local_reserved_ports.");
> > + fclose(file_ptr);
> > + return -1;
> > +}
> > +
> > /*
> > * Bind a socket to a dynamically-assigned IP port.
> > *
> > @@ -81,7 +160,8 @@ int __binddynport(int fd)
> > in_port_t port, *portp;
> > struct sockaddr *sap;
> > socklen_t salen;
> > - int i, res;
> > + int i, res, array_size;
> > + char *reserved_ports;
> >
> > if (__rpc_sockisbound(fd))
> > return 0;
> > @@ -119,21 +199,32 @@ int __binddynport(int fd)
> > gettimeofday(&tv, NULL);
> > seed = tv.tv_usec * getpid();
> > }
> > + array_size = NPORTS / (8*sizeof(char)) + 1;
> > + reserved_ports = malloc(array_size);
> > + if (!reserved_ports) {
> > + goto out;
> > + }
> > + memset(reserved_ports, 0, array_size);
> > + parse_reserved_ports(reserved_ports);
> > +
> > port = (rand_r(&seed) % NPORTS) + LOWPORT;
> > for (i = 0; i < NPORTS; ++i) {
> > - *portp = htons(port++);
> > - res = bind(fd, sap, salen);
> > - if (res >= 0) {
> > - res = 0;
> > - break;
> > + *portp = htons(port);
> > + if (!is_reserved(reserved_ports, port++)) {
> > + res = bind(fd, sap, salen);
> > + if (res >= 0) {
> > + res = 0;
> > + break;
> > + }
> > + if (errno != EADDRINUSE)
> > + break;
> > }
> > - if (errno != EADDRINUSE)
> > - break;
> > if (port > ENDPORT)
> > port = LOWPORT;
> > }
> >
> > out:
> > + free(reserved_ports);
> > mutex_unlock(&port_lock);
> > return res;
> > }
>
