[PATCH 0/3 libtirpc] Support abstract addresses for rpcbind in libtirpc

[PATCH 0/3 libtirpc] Support abstract addresses for rpcbind in libtirpc

[NeilBrown]

I posted these patches in May 2023 at the same time as I posted patches
for the kernel to connect to an abstract address.
The kernel patches landed and I forgot to follow-up with these.
Thanks Petr for the reminder.

NeilBrown


 [PATCH 1/3] Allow working with abstract AF_UNIX addresses.
 [PATCH 2/3] Change local_rpcb() to take a targaddr pointer.
 [PATCH 3/3] Try using a new abstract address when connecting rpcbind

[PATCH 1/3] Allow working with abstract AF_UNIX addresses.

[NeilBrown]

Linux supports abstract addresses for AF_UNIX.
These have .sun_path starting with '\0'.
When presented in human-readable form they have a leading '@' instead.
The length of the sockaddr must not include any trailing
zeroes after the abstract name, as they will treated as part of the
name and cause address matching to fail.

This patch makes various changes to code that works with sun_path to
ensure that abstract addresses work correctly.

In particular it fixes a bug in __rpc_sockisbound() which incorrectly
determines that a socket bound to an abstract address is in fact not
bound.  This prevents sockets with abstract addresses being used even
when created outside of the library.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 src/rpc_com.h     |  6 ++++++
 src/rpc_generic.c | 18 ++++++++++++------
 src/rpc_soc.c     |  6 +++++-
 3 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/src/rpc_com.h b/src/rpc_com.h
index 76badefcfe90..ded72d1a647e 100644
--- a/src/rpc_com.h
+++ b/src/rpc_com.h
@@ -60,6 +60,12 @@ bool_t __xdrrec_getrec(XDR *, enum xprt_stat *, bool_t);
 void __xprt_unregister_unlocked(SVCXPRT *);
 void __xprt_set_raddr(SVCXPRT *, const struct sockaddr_storage *);
 
+/* Evaluate to actual length of the `sockaddr_un' structure, whether
+ * abstract or not.
+ */
+#include <stddef.h>
+#define SUN_LEN_A(ptr) (offsetof(struct sockaddr_un, sun_path)	\
+			+ 1 + strlen((ptr)->sun_path + 1))
 
 extern int __svc_maxrec;
 
diff --git a/src/rpc_generic.c b/src/rpc_generic.c
index aabbe4be896c..e649c87198a3 100644
--- a/src/rpc_generic.c
+++ b/src/rpc_generic.c
@@ -650,7 +650,8 @@ __rpc_taddr2uaddr_af(int af, const struct netbuf *nbuf)
 		if (path_len < 0)
 			return NULL;
 
-		if (asprintf(&ret, "%.*s", path_len, sun->sun_path) < 0)
+		if (asprintf(&ret, "%c%.*s", sun->sun_path[0] ?: '\0',
+			     path_len - 1, sun->sun_path + 1) < 0)
 			return (NULL);
 		break;
 	default:
@@ -682,9 +683,10 @@ __rpc_uaddr2taddr_af(int af, const char *uaddr)
 
 	/*
 	 * AF_LOCAL addresses are expected to be absolute
-	 * pathnames, anything else will be AF_INET or AF_INET6.
+	 * pathnames or abstract names, anything else will be
+	 * AF_INET or AF_INET6.
 	 */
-	if (*addrstr != '/') {
+	if (*addrstr != '/' && *addrstr != '@') {
 		p = strrchr(addrstr, '.');
 		if (p == NULL)
 			goto out;
@@ -747,6 +749,9 @@ __rpc_uaddr2taddr_af(int af, const char *uaddr)
 		strncpy(sun->sun_path, addrstr, sizeof(sun->sun_path) - 1);
 		ret->len = SUN_LEN(sun);
 		ret->maxlen = sizeof(struct sockaddr_un);
+		if (sun->sun_path[0] == '@')
+			/* Abstract address */
+			sun->sun_path[0] = '\0';
 		ret->buf = sun;
 		break;
 	default:
@@ -834,6 +839,7 @@ __rpc_sockisbound(int fd)
 		struct sockaddr_un  usin;
 	} u_addr;
 	socklen_t slen;
+	int path_len;
 
 	slen = sizeof (struct sockaddr_storage);
 	if (getsockname(fd, (struct sockaddr *)(void *)&ss, &slen) < 0)
@@ -849,9 +855,9 @@ __rpc_sockisbound(int fd)
 			return (u_addr.sin6.sin6_port != 0);
 #endif
 		case AF_LOCAL:
-			/* XXX check this */
-			memcpy(&u_addr.usin, &ss, sizeof(u_addr.usin)); 
-			return (u_addr.usin.sun_path[0] != 0);
+			memcpy(&u_addr.usin, &ss, sizeof(u_addr.usin));
+			path_len = slen - offsetof(struct sockaddr_un, sun_path);
+			return path_len > 0;
 		default:
 			break;
 	}
diff --git a/src/rpc_soc.c b/src/rpc_soc.c
index fde121db75cf..c6c93b50337d 100644
--- a/src/rpc_soc.c
+++ b/src/rpc_soc.c
@@ -701,7 +701,11 @@ svcunix_create(sock, sendsize, recvsize, path)
 	memset(&sun, 0, sizeof sun);
 	sun.sun_family = AF_LOCAL;
 	strncpy(sun.sun_path, path, (sizeof(sun.sun_path)-1));
-	addrlen = sizeof(struct sockaddr_un);
+	if (sun.sun_path[0] == '@')
+		/* abstract address */
+		sun.sun_path[0] = '\0';
+
+	addrlen = SUN_LEN_A(&sun);
 	sa = (struct sockaddr *)&sun;
 
 	if (bind(sock, sa, addrlen) < 0)
-- 
2.43.0

[PATCH 2/3] Change local_rpcb() to take a targaddr pointer.

[NeilBrown]

Two callers of local_rpcb() want the target-addr, and local_rcpb() has
easy access to it.  So accept a pointer and fill it in if not NULL.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 src/rpcb_clnt.c | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/src/rpcb_clnt.c b/src/rpcb_clnt.c
index 68fe69a320ff..f587580228ab 100644
--- a/src/rpcb_clnt.c
+++ b/src/rpcb_clnt.c
@@ -89,7 +89,7 @@ static struct address_cache *copy_of_cached(const char *, char *);
 static void delete_cache(struct netbuf *);
 static void add_cache(const char *, const char *, struct netbuf *, char *);
 static CLIENT *getclnthandle(const char *, const struct netconfig *, char **);
-static CLIENT *local_rpcb(void);
+static CLIENT *local_rpcb(char **targaddr);
 #ifdef NOTUSED
 static struct netbuf *got_entry(rpcb_entry_list_ptr, const struct netconfig *);
 #endif
@@ -430,19 +430,12 @@ getclnthandle(host, nconf, targaddr)
 	    nconf->nc_netid, si.si_af, si.si_proto, si.si_socktype));
 
 	if (nconf->nc_protofmly != NULL && strcmp(nconf->nc_protofmly, NC_LOOPBACK) == 0) {
-		client = local_rpcb();
+		client = local_rpcb(targaddr);
 		if (! client) {
 			LIBTIRPC_DEBUG(1, ("getclnthandle: %s", 
 				clnt_spcreateerror("local_rpcb failed")));
 			goto out_err;
 		} else {
-			struct sockaddr_un sun;
-
-			if (targaddr) {
-				*targaddr = malloc(sizeof(sun.sun_path));
-				strncpy(*targaddr, _PATH_RPCBINDSOCK,
-				    sizeof(sun.sun_path));
-			}
 			return (client);
 		}
 	} else {
@@ -541,7 +534,8 @@ getpmaphandle(nconf, hostname, tgtaddr)
  * rpcbind. Returns NULL on error and free's everything.
  */
 static CLIENT *
-local_rpcb()
+local_rpcb(targaddr)
+	char **targaddr;
 {
 	CLIENT *client;
 	static struct netconfig *loopnconf;
@@ -574,6 +568,8 @@ local_rpcb()
 	if (client != NULL) {
 		/* Mark the socket to be closed in destructor */
 		(void) CLNT_CONTROL(client, CLSET_FD_CLOSE, NULL);
+		if (targaddr)
+			*targaddr = strdup(sun.sun_path);
 		return client;
 	}
 
@@ -632,7 +628,7 @@ try_nconf:
 		endnetconfig(nc_handle);
 	}
 	mutex_unlock(&loopnconf_lock);
-	client = getclnthandle(hostname, loopnconf, NULL);
+	client = getclnthandle(hostname, loopnconf, targaddr);
 	return (client);
 }
 
@@ -661,20 +657,11 @@ rpcb_set(program, version, nconf, address)
 		rpc_createerr.cf_stat = RPC_UNKNOWNADDR;
 		return (FALSE);
 	}
-	client = local_rpcb();
+	client = local_rpcb(&parms.r_addr);
 	if (! client) {
 		return (FALSE);
 	}
 
-	/* convert to universal */
-	/*LINTED const castaway*/
-	parms.r_addr = taddr2uaddr((struct netconfig *) nconf,
-				   (struct netbuf *)address);
-	if (!parms.r_addr) {
-		CLNT_DESTROY(client);
-		rpc_createerr.cf_stat = RPC_N2AXLATEFAILURE;
-		return (FALSE); /* no universal address */
-	}
 	parms.r_prog = program;
 	parms.r_vers = version;
 	parms.r_netid = nconf->nc_netid;
@@ -712,7 +699,7 @@ rpcb_unset(program, version, nconf)
 	RPCB parms;
 	char uidbuf[32];
 
-	client = local_rpcb();
+	client = local_rpcb(NULL);
 	if (! client) {
 		return (FALSE);
 	}
@@ -1342,7 +1329,7 @@ rpcb_taddr2uaddr(nconf, taddr)
 		rpc_createerr.cf_stat = RPC_UNKNOWNADDR;
 		return (NULL);
 	}
-	client = local_rpcb();
+	client = local_rpcb(NULL);
 	if (! client) {
 		return (NULL);
 	}
@@ -1376,7 +1363,7 @@ rpcb_uaddr2taddr(nconf, uaddr)
 		rpc_createerr.cf_stat = RPC_UNKNOWNADDR;
 		return (NULL);
 	}
-	client = local_rpcb();
+	client = local_rpcb(NULL);
 	if (! client) {
 		return (NULL);
 	}
-- 
2.43.0

[PATCH 3/3] Try using a new abstract address when connecting rpcbind

[NeilBrown]

As RPC services are network services, it can make sense to localise
them in a network namespace on Linux.  Unfortunately the use of a path
name - /var/run/rpcbind.sock - to contact rpcbind makes that difficult
and requires a mount namespace to be created as well.

Linux supports abstract addresses for AF_UNIX sockets.  These start with
a nul byte and (by convention) no other nul bytes with the length
specified by the addrlen.  Abstract addresses are matched by byte
comparison without reference to the filesystem, and are local to the
network namespace in which are used.  Using an abstract address for
contacting rpcbind removes the need for a mount namespace.

Back comparability is assured by attempting to connect to the existing
well known address (/var/run/rpcbind.sock) if the abstract address
cannot be reached.

Choosing the name needs some care as the same address will be configured
for rpcbind, and needs to be built in to libtirpc for this enhancement
to be fully successful.  There is no formal standard for choosing
abstract addresses.  The defacto standard appears to be to use a path
name similar to what would be used for a filesystem AF_UNIX address -
but with a leading nul.

In that case
   "\0/var/run/rpcbind.sock"
seems like the best choice.  However at this time /var/run is deprecated
in favour of /run, so
   "\0/run/rpcbind.sock"
might be better.
Though as we are deliberately moving away from using the filesystem it
might seem more sensible to explicitly break the connection and just
have
   "\0rpcbind.socket"
using the same name as the systemd unit file..

The linux kernel already attempts to connect to the second option,
"\0/run/rpcbind.sock" since Linux v6.5 so this patch chooses that
option.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 src/rpcb_clnt.c       | 81 +++++++++++++++++++++++++++----------------
 tirpc/rpc/rpcb_prot.h |  1 +
 tirpc/rpc/rpcb_prot.x |  1 +
 3 files changed, 53 insertions(+), 30 deletions(-)

diff --git a/src/rpcb_clnt.c b/src/rpcb_clnt.c
index f587580228ab..5ddd81a08863 100644
--- a/src/rpcb_clnt.c
+++ b/src/rpcb_clnt.c
@@ -545,36 +545,50 @@ local_rpcb(targaddr)
 	size_t tsize;
 	struct netbuf nbuf;
 	struct sockaddr_un sun;
+	int i;
 
 	/*
 	 * Try connecting to the local rpcbind through a local socket
-	 * first. If this doesn't work, try all transports defined in
-	 * the netconfig file.
+	 * first - trying both addresses. If this doesn't work, try all
+	 * non-local transports defined in the netconfig file.
 	 */
-	memset(&sun, 0, sizeof sun);
-	sock = socket(AF_LOCAL, SOCK_STREAM, 0);
-	if (sock < 0)
-		goto try_nconf;
-	sun.sun_family = AF_LOCAL;
-	strcpy(sun.sun_path, _PATH_RPCBINDSOCK);
-	nbuf.len = SUN_LEN(&sun);
-	nbuf.maxlen = sizeof (struct sockaddr_un);
-	nbuf.buf = &sun;
-
-	tsize = __rpc_get_t_size(AF_LOCAL, 0, 0);
-	client = clnt_vc_create(sock, &nbuf, (rpcprog_t)RPCBPROG,
-	    (rpcvers_t)RPCBVERS, tsize, tsize);
-
-	if (client != NULL) {
-		/* Mark the socket to be closed in destructor */
-		(void) CLNT_CONTROL(client, CLSET_FD_CLOSE, NULL);
-		if (targaddr)
-			*targaddr = strdup(sun.sun_path);
-		return client;
-	}
+	for (i = 0; i < 2; i++) {
+		memset(&sun, 0, sizeof sun);
+		sock = socket(AF_LOCAL, SOCK_STREAM, 0);
+		if (sock < 0)
+			goto try_nconf;
+		sun.sun_family = AF_LOCAL;
+		switch (i) {
+		case 0:
+			memcpy(sun.sun_path, _PATH_RPCBINDSOCK_ABSTRACT,
+			       sizeof(_PATH_RPCBINDSOCK_ABSTRACT));
+			break;
+		case 1:
+			strcpy(sun.sun_path, _PATH_RPCBINDSOCK);
+			break;
+		}
+		nbuf.len = SUN_LEN_A(&sun);
+		nbuf.maxlen = sizeof (struct sockaddr_un);
+		nbuf.buf = &sun;
+
+		tsize = __rpc_get_t_size(AF_LOCAL, 0, 0);
+		client = clnt_vc_create(sock, &nbuf, (rpcprog_t)RPCBPROG,
+					(rpcvers_t)RPCBVERS, tsize, tsize);
+
+		if (client != NULL) {
+			/* Mark the socket to be closed in destructor */
+			(void) CLNT_CONTROL(client, CLSET_FD_CLOSE, NULL);
+			if (targaddr) {
+				if (sun.sun_path[0] == 0)
+					sun.sun_path[0] = '@';
+				*targaddr = strdup(sun.sun_path);
+			}
+			return client;
+		}
 
-	/* Nobody needs this socket anymore; free the descriptor. */
-	close(sock);
+		/* Nobody needs this socket anymore; free the descriptor. */
+		close(sock);
+	}
 
 try_nconf:
 
@@ -755,7 +769,7 @@ got_entry(relp, nconf)
 
 /*
  * Quick check to see if rpcbind is up.  Tries to connect over
- * local transport.
+ * local transport - first abstract, then regular.
  */
 bool_t
 __rpcbind_is_up()
@@ -782,15 +796,22 @@ __rpcbind_is_up()
 	if (sock < 0)
 		return (FALSE);
 	sun.sun_family = AF_LOCAL;
-	strncpy(sun.sun_path, _PATH_RPCBINDSOCK, sizeof(sun.sun_path));
 
-	if (connect(sock, (struct sockaddr *)&sun, sizeof(sun)) < 0) {
+	memcpy(sun.sun_path, _PATH_RPCBINDSOCK_ABSTRACT,
+	       sizeof(_PATH_RPCBINDSOCK_ABSTRACT));
+	if (connect(sock, (struct sockaddr *)&sun, SUN_LEN_A(&sun)) == 0) {
 		close(sock);
-		return (FALSE);
+		return (TRUE);
+	}
+
+	strncpy(sun.sun_path, _PATH_RPCBINDSOCK, sizeof(sun.sun_path));
+	if (connect(sock, (struct sockaddr *)&sun, sizeof(sun)) == 0) {
+		close(sock);
+		return (TRUE);
 	}
 
 	close(sock);
-	return (TRUE);
+	return (FALSE);
 }
 #endif
 
diff --git a/tirpc/rpc/rpcb_prot.h b/tirpc/rpc/rpcb_prot.h
index 7ae48b805370..eb3a0c47f66a 100644
--- a/tirpc/rpc/rpcb_prot.h
+++ b/tirpc/rpc/rpcb_prot.h
@@ -477,6 +477,7 @@ extern bool_t xdr_netbuf(XDR *, struct netbuf *);
 #define RPCBVERS_4 RPCBVERS4
 
 #define _PATH_RPCBINDSOCK "/var/run/rpcbind.sock"
+#define _PATH_RPCBINDSOCK_ABSTRACT "\0/run/rpcbind.sock"
 
 #else /* ndef _KERNEL */
 #ifdef __cplusplus
diff --git a/tirpc/rpc/rpcb_prot.x b/tirpc/rpc/rpcb_prot.x
index b21ac3d535f6..472c11ffedd6 100644
--- a/tirpc/rpc/rpcb_prot.x
+++ b/tirpc/rpc/rpcb_prot.x
@@ -411,6 +411,7 @@ program RPCBPROG {
 %#define	RPCBVERS_4		RPCBVERS4
 %
 %#define	_PATH_RPCBINDSOCK	"/var/run/rpcbind.sock"
+%#define	_PATH_RPCBINDSOCK_ABSTRACT "\0/run/rpcbind.sock"
 %
 %#else		/* ndef _KERNEL */
 %#ifdef __cplusplus
-- 
2.43.0

Re: [PATCH 2/3] Change local_rpcb() to take a targaddr pointer.

[Steve Dickson]

On 2/25/24 6:40 PM, NeilBrown wrote:
> Two callers of local_rpcb() want the target-addr, and local_rcpb() has
> easy access to it.  So accept a pointer and fill it in if not NULL.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>   src/rpcb_clnt.c | 35 +++++++++++------------------------
>   1 file changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/src/rpcb_clnt.c b/src/rpcb_clnt.c
> index 68fe69a320ff..f587580228ab 100644
> --- a/src/rpcb_clnt.c
> +++ b/src/rpcb_clnt.c
> @@ -89,7 +89,7 @@ static struct address_cache *copy_of_cached(const char *, char *);
>   static void delete_cache(struct netbuf *);
>   static void add_cache(const char *, const char *, struct netbuf *, char *);
>   static CLIENT *getclnthandle(const char *, const struct netconfig *, char **);
> -static CLIENT *local_rpcb(void);
> +static CLIENT *local_rpcb(char **targaddr);
>   #ifdef NOTUSED
>   static struct netbuf *got_entry(rpcb_entry_list_ptr, const struct netconfig *);
>   #endif
> @@ -430,19 +430,12 @@ getclnthandle(host, nconf, targaddr)
>   	    nconf->nc_netid, si.si_af, si.si_proto, si.si_socktype));
>   
>   	if (nconf->nc_protofmly != NULL && strcmp(nconf->nc_protofmly, NC_LOOPBACK) == 0) {
> -		client = local_rpcb();
> +		client = local_rpcb(targaddr);
>   		if (! client) {
>   			LIBTIRPC_DEBUG(1, ("getclnthandle: %s",
>   				clnt_spcreateerror("local_rpcb failed")));
>   			goto out_err;
>   		} else {
> -			struct sockaddr_un sun;
> -
> -			if (targaddr) {
> -				*targaddr = malloc(sizeof(sun.sun_path));
> -				strncpy(*targaddr, _PATH_RPCBINDSOCK,
> -				    sizeof(sun.sun_path));
> -			}
>   			return (client);
>   		}
>   	} else {
> @@ -541,7 +534,8 @@ getpmaphandle(nconf, hostname, tgtaddr)
>    * rpcbind. Returns NULL on error and free's everything.
>    */
>   static CLIENT *
> -local_rpcb()
> +local_rpcb(targaddr)
> +	char **targaddr;
>   {
>   	CLIENT *client;
>   	static struct netconfig *loopnconf;
> @@ -574,6 +568,8 @@ local_rpcb()
>   	if (client != NULL) {
>   		/* Mark the socket to be closed in destructor */
>   		(void) CLNT_CONTROL(client, CLSET_FD_CLOSE, NULL);
> +		if (targaddr)
> +			*targaddr = strdup(sun.sun_path);
>   		return client;
>   	}
>   
> @@ -632,7 +628,7 @@ try_nconf:
>   		endnetconfig(nc_handle);
>   	}
>   	mutex_unlock(&loopnconf_lock);
> -	client = getclnthandle(hostname, loopnconf, NULL);
> +	client = getclnthandle(hostname, loopnconf, targaddr);
>   	return (client);
>   }
>   
> @@ -661,20 +657,11 @@ rpcb_set(program, version, nconf, address)
>   		rpc_createerr.cf_stat = RPC_UNKNOWNADDR;
>   		return (FALSE);
>   	}
> -	client = local_rpcb();
> +	client = local_rpcb(&parms.r_addr);
>   	if (! client) {
>   		return (FALSE);
>   	}
>   
> -	/* convert to universal */
> -	/*LINTED const castaway*/
> -	parms.r_addr = taddr2uaddr((struct netconfig *) nconf,
> -				   (struct netbuf *)address);
> -	if (!parms.r_addr) {
> -		CLNT_DESTROY(client);
> -		rpc_createerr.cf_stat = RPC_N2AXLATEFAILURE;
> -		return (FALSE); /* no universal address */
> -	}
>   	parms.r_prog = program;
>   	parms.r_vers = version;
>   	parms.r_netid = nconf->nc_netid;
> @@ -712,7 +699,7 @@ rpcb_unset(program, version, nconf)
>   	RPCB parms;
>   	char uidbuf[32];
>   
> -	client = local_rpcb();
> +	client = local_rpcb(NULL);
>   	if (! client) {
>   		return (FALSE);
>   	}
> @@ -1342,7 +1329,7 @@ rpcb_taddr2uaddr(nconf, taddr)
>   		rpc_createerr.cf_stat = RPC_UNKNOWNADDR;
>   		return (NULL);
>   	}
> -	client = local_rpcb();
> +	client = local_rpcb(NULL);
>   	if (! client) {
>   		return (NULL);
>   	}
> @@ -1376,7 +1363,7 @@ rpcb_uaddr2taddr(nconf, uaddr)
>   		rpc_createerr.cf_stat = RPC_UNKNOWNADDR;
>   		return (NULL);
>   	}
> -	client = local_rpcb();
> +	client = local_rpcb(NULL);
>   	if (! client) {
>   		return (NULL);
>   	}
It is not clear why... but this patch stop mountd from
registering with rpcbind (both the old and changed via
the latest patches), which means v3 mounts break.
Not good :-)

Turning debugging on... rpcbind is receiving the set prog
but not recording it, since port 0 is returned when
the client tries to do a v3 mount.

Are you guys seeing this??

I remove this patch and everything works!

steved.

Re: [PATCH 2/3] Change local_rpcb() to take a targaddr pointer.

[NeilBrown]

On Sun, 10 Mar 2024, Steve Dickson wrote:
> 
> On 2/25/24 6:40 PM, NeilBrown wrote:
> > Two callers of local_rpcb() want the target-addr, and local_rcpb() has
> > easy access to it.  So accept a pointer and fill it in if not NULL.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >   src/rpcb_clnt.c | 35 +++++++++++------------------------
> >   1 file changed, 11 insertions(+), 24 deletions(-)
> > 
> > diff --git a/src/rpcb_clnt.c b/src/rpcb_clnt.c
> > index 68fe69a320ff..f587580228ab 100644
> > --- a/src/rpcb_clnt.c
> > +++ b/src/rpcb_clnt.c
> > @@ -89,7 +89,7 @@ static struct address_cache *copy_of_cached(const char *, char *);
> >   static void delete_cache(struct netbuf *);
> >   static void add_cache(const char *, const char *, struct netbuf *, char *);
> >   static CLIENT *getclnthandle(const char *, const struct netconfig *, char **);
> > -static CLIENT *local_rpcb(void);
> > +static CLIENT *local_rpcb(char **targaddr);
> >   #ifdef NOTUSED
> >   static struct netbuf *got_entry(rpcb_entry_list_ptr, const struct netconfig *);
> >   #endif
> > @@ -430,19 +430,12 @@ getclnthandle(host, nconf, targaddr)
> >   	    nconf->nc_netid, si.si_af, si.si_proto, si.si_socktype));
> >   
> >   	if (nconf->nc_protofmly != NULL && strcmp(nconf->nc_protofmly, NC_LOOPBACK) == 0) {
> > -		client = local_rpcb();
> > +		client = local_rpcb(targaddr);
> >   		if (! client) {
> >   			LIBTIRPC_DEBUG(1, ("getclnthandle: %s",
> >   				clnt_spcreateerror("local_rpcb failed")));
> >   			goto out_err;
> >   		} else {
> > -			struct sockaddr_un sun;
> > -
> > -			if (targaddr) {
> > -				*targaddr = malloc(sizeof(sun.sun_path));
> > -				strncpy(*targaddr, _PATH_RPCBINDSOCK,
> > -				    sizeof(sun.sun_path));
> > -			}
> >   			return (client);
> >   		}
> >   	} else {
> > @@ -541,7 +534,8 @@ getpmaphandle(nconf, hostname, tgtaddr)
> >    * rpcbind. Returns NULL on error and free's everything.
> >    */
> >   static CLIENT *
> > -local_rpcb()
> > +local_rpcb(targaddr)
> > +	char **targaddr;
> >   {
> >   	CLIENT *client;
> >   	static struct netconfig *loopnconf;
> > @@ -574,6 +568,8 @@ local_rpcb()
> >   	if (client != NULL) {
> >   		/* Mark the socket to be closed in destructor */
> >   		(void) CLNT_CONTROL(client, CLSET_FD_CLOSE, NULL);
> > +		if (targaddr)
> > +			*targaddr = strdup(sun.sun_path);
> >   		return client;
> >   	}
> >   
> > @@ -632,7 +628,7 @@ try_nconf:
> >   		endnetconfig(nc_handle);
> >   	}
> >   	mutex_unlock(&loopnconf_lock);
> > -	client = getclnthandle(hostname, loopnconf, NULL);
> > +	client = getclnthandle(hostname, loopnconf, targaddr);
> >   	return (client);
> >   }
> >   
> > @@ -661,20 +657,11 @@ rpcb_set(program, version, nconf, address)
> >   		rpc_createerr.cf_stat = RPC_UNKNOWNADDR;
> >   		return (FALSE);
> >   	}
> > -	client = local_rpcb();
> > +	client = local_rpcb(&parms.r_addr);
> >   	if (! client) {
> >   		return (FALSE);
> >   	}
> >   
> > -	/* convert to universal */
> > -	/*LINTED const castaway*/
> > -	parms.r_addr = taddr2uaddr((struct netconfig *) nconf,
> > -				   (struct netbuf *)address);
> > -	if (!parms.r_addr) {
> > -		CLNT_DESTROY(client);
> > -		rpc_createerr.cf_stat = RPC_N2AXLATEFAILURE;
> > -		return (FALSE); /* no universal address */
> > -	}
> >   	parms.r_prog = program;
> >   	parms.r_vers = version;
> >   	parms.r_netid = nconf->nc_netid;
> > @@ -712,7 +699,7 @@ rpcb_unset(program, version, nconf)
> >   	RPCB parms;
> >   	char uidbuf[32];
> >   
> > -	client = local_rpcb();
> > +	client = local_rpcb(NULL);
> >   	if (! client) {
> >   		return (FALSE);
> >   	}
> > @@ -1342,7 +1329,7 @@ rpcb_taddr2uaddr(nconf, taddr)
> >   		rpc_createerr.cf_stat = RPC_UNKNOWNADDR;
> >   		return (NULL);
> >   	}
> > -	client = local_rpcb();
> > +	client = local_rpcb(NULL);
> >   	if (! client) {
> >   		return (NULL);
> >   	}
> > @@ -1376,7 +1363,7 @@ rpcb_uaddr2taddr(nconf, uaddr)
> >   		rpc_createerr.cf_stat = RPC_UNKNOWNADDR;
> >   		return (NULL);
> >   	}
> > -	client = local_rpcb();
> > +	client = local_rpcb(NULL);
> >   	if (! client) {
> >   		return (NULL);
> >   	}
> It is not clear why... but this patch stop mountd from
> registering with rpcbind (both the old and changed via
> the latest patches), which means v3 mounts break.
> Not good :-)
> 
> Turning debugging on... rpcbind is receiving the set prog
> but not recording it, since port 0 is returned when
> the client tries to do a v3 mount.
> 
> Are you guys seeing this??

I am now.  Wonder why I didn't before..

The problem is the removal of that /* convert to universal */ section.

r_addr is meant to be based on 'address', not whatever local_rpcb()
finds.

I'll fix that up as well as a bug I found in patch 1 (a '\0' should have
been '@') and test and resend.

Thanks,
NeilBrown


> 
> I remove this patch and everything works!
> 
> steved.
> 
> 

Re: [PATCH 2/3] Change local_rpcb() to take a targaddr pointer.

[Petr Vorel]

> On 2/25/24 6:40 PM, NeilBrown wrote:
> > Two callers of local_rpcb() want the target-addr, and local_rcpb() has
> > easy access to it.  So accept a pointer and fill it in if not NULL.

> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >   src/rpcb_clnt.c | 35 +++++++++++------------------------
> >   1 file changed, 11 insertions(+), 24 deletions(-)

> > diff --git a/src/rpcb_clnt.c b/src/rpcb_clnt.c
> > index 68fe69a320ff..f587580228ab 100644
> > --- a/src/rpcb_clnt.c
> > +++ b/src/rpcb_clnt.c
> > @@ -89,7 +89,7 @@ static struct address_cache *copy_of_cached(const char *, char *);
> >   static void delete_cache(struct netbuf *);
> >   static void add_cache(const char *, const char *, struct netbuf *, char *);
> >   static CLIENT *getclnthandle(const char *, const struct netconfig *, char **);
> > -static CLIENT *local_rpcb(void);
> > +static CLIENT *local_rpcb(char **targaddr);
> >   #ifdef NOTUSED
> >   static struct netbuf *got_entry(rpcb_entry_list_ptr, const struct netconfig *);
> >   #endif
> > @@ -430,19 +430,12 @@ getclnthandle(host, nconf, targaddr)
> >   	    nconf->nc_netid, si.si_af, si.si_proto, si.si_socktype));
> >   	if (nconf->nc_protofmly != NULL && strcmp(nconf->nc_protofmly, NC_LOOPBACK) == 0) {
> > -		client = local_rpcb();
> > +		client = local_rpcb(targaddr);
> >   		if (! client) {
> >   			LIBTIRPC_DEBUG(1, ("getclnthandle: %s",
> >   				clnt_spcreateerror("local_rpcb failed")));
> >   			goto out_err;
> >   		} else {
> > -			struct sockaddr_un sun;
> > -
> > -			if (targaddr) {
> > -				*targaddr = malloc(sizeof(sun.sun_path));
> > -				strncpy(*targaddr, _PATH_RPCBINDSOCK,
> > -				    sizeof(sun.sun_path));
> > -			}
> >   			return (client);
> >   		}
> >   	} else {
> > @@ -541,7 +534,8 @@ getpmaphandle(nconf, hostname, tgtaddr)
> >    * rpcbind. Returns NULL on error and free's everything.
> >    */
> >   static CLIENT *
> > -local_rpcb()
> > +local_rpcb(targaddr)
> > +	char **targaddr;
> >   {
> >   	CLIENT *client;
> >   	static struct netconfig *loopnconf;
> > @@ -574,6 +568,8 @@ local_rpcb()
> >   	if (client != NULL) {
> >   		/* Mark the socket to be closed in destructor */
> >   		(void) CLNT_CONTROL(client, CLSET_FD_CLOSE, NULL);
> > +		if (targaddr)
> > +			*targaddr = strdup(sun.sun_path);
> >   		return client;
> >   	}
> > @@ -632,7 +628,7 @@ try_nconf:
> >   		endnetconfig(nc_handle);
> >   	}
> >   	mutex_unlock(&loopnconf_lock);
> > -	client = getclnthandle(hostname, loopnconf, NULL);
> > +	client = getclnthandle(hostname, loopnconf, targaddr);
> >   	return (client);
> >   }
> > @@ -661,20 +657,11 @@ rpcb_set(program, version, nconf, address)
> >   		rpc_createerr.cf_stat = RPC_UNKNOWNADDR;
> >   		return (FALSE);
> >   	}
> > -	client = local_rpcb();
> > +	client = local_rpcb(&parms.r_addr);
> >   	if (! client) {
> >   		return (FALSE);
> >   	}
> > -	/* convert to universal */
> > -	/*LINTED const castaway*/
> > -	parms.r_addr = taddr2uaddr((struct netconfig *) nconf,
> > -				   (struct netbuf *)address);
> > -	if (!parms.r_addr) {
> > -		CLNT_DESTROY(client);
> > -		rpc_createerr.cf_stat = RPC_N2AXLATEFAILURE;
> > -		return (FALSE); /* no universal address */
> > -	}
> >   	parms.r_prog = program;
> >   	parms.r_vers = version;
> >   	parms.r_netid = nconf->nc_netid;
> > @@ -712,7 +699,7 @@ rpcb_unset(program, version, nconf)
> >   	RPCB parms;
> >   	char uidbuf[32];
> > -	client = local_rpcb();
> > +	client = local_rpcb(NULL);
> >   	if (! client) {
> >   		return (FALSE);
> >   	}
> > @@ -1342,7 +1329,7 @@ rpcb_taddr2uaddr(nconf, taddr)
> >   		rpc_createerr.cf_stat = RPC_UNKNOWNADDR;
> >   		return (NULL);
> >   	}
> > -	client = local_rpcb();
> > +	client = local_rpcb(NULL);
> >   	if (! client) {
> >   		return (NULL);
> >   	}
> > @@ -1376,7 +1363,7 @@ rpcb_uaddr2taddr(nconf, uaddr)
> >   		rpc_createerr.cf_stat = RPC_UNKNOWNADDR;
> >   		return (NULL);
> >   	}
> > -	client = local_rpcb();
> > +	client = local_rpcb(NULL);
> >   	if (! client) {
> >   		return (NULL);
> >   	}
> It is not clear why... but this patch stop mountd from
> registering with rpcbind (both the old and changed via
> the latest patches), which means v3 mounts break.
> Not good :-)

> Turning debugging on... rpcbind is receiving the set prog
> but not recording it, since port 0 is returned when
> the client tries to do a v3 mount.

> Are you guys seeing this??

Hi Steve,

I'm sorry I haven't had time to test. Strange, I don't see any bug in that
commit.

Kind regards,
Petr

> I remove this patch and everything works!

> steved.