2009-04-07 15:25:41

by Jeff Layton

[permalink] [raw]
Subject: [PATCH 0/5] nfs-utils: convert gssd to TI-RPC and add IPv6 support (try #4)

This patchset is the fourth attempt at adding support for IPv6 to
rpc.gssd. The main change from the last set is that this one now uses a
rpcbind query to determine the server's port rather than doing a
getaddrinfo call to query the local services db.

The series should be fully bisectable, but I've only really tested the
end result for anything other than to see if it compiles. With these
patches I've been able to mount an OpenSolaris server using NFSv3/4 +
IPv6 + krb5 auth. I've also done testing with builds with only TIRPC
enabled and with TIRPC and IPv6 both disabled and haven't seen any
regressions.

List of changes since the last set:
- use rpcbind query to determine port for RPC client
- added comment explaining gssd doesn't deal with IPv6 scope_id's
- slight cleanups and clarifications to comments
- properly handle EAI_SYSTEM return code from getnameinfo() call
- changed autoconf check for getnameinfo to check whenever --enable-gss
is set, not just when NFSv4 is also enabled.

Jeff Layton (5):
nfs-utils: make getnameinfo() required for --enable-gss
nfs-utils: store the address given in the upcall for later use
nfs-utils: query for remote port using rpcbind instead of getaddrinfo
nfs-utils: switch gssd to use standard function for getting an RPC
client
nfs-utils: add IPv6 code to gssd

configure.ac | 3 +
utils/gssd/gssd.h | 2 +-
utils/gssd/gssd_proc.c | 286 +++++++++++++++++++++++++++++++++---------------
3 files changed, 204 insertions(+), 87 deletions(-)


2009-04-15 16:02:38

by Steve Dickson

[permalink] [raw]
Subject: Re: [PATCH 0/5] nfs-utils: convert gssd to TI-RPC and add IPv6 support (try #4)



Jeff Layton wrote:
> This patchset is the fourth attempt at adding support for IPv6 to
> rpc.gssd. The main change from the last set is that this one now uses a
> rpcbind query to determine the server's port rather than doing a
> getaddrinfo call to query the local services db.
>
> The series should be fully bisectable, but I've only really tested the
> end result for anything other than to see if it compiles. With these
> patches I've been able to mount an OpenSolaris server using NFSv3/4 +
> IPv6 + krb5 auth. I've also done testing with builds with only TIRPC
> enabled and with TIRPC and IPv6 both disabled and haven't seen any
> regressions.
>
> List of changes since the last set:
> - use rpcbind query to determine port for RPC client
> - added comment explaining gssd doesn't deal with IPv6 scope_id's
> - slight cleanups and clarifications to comments
> - properly handle EAI_SYSTEM return code from getnameinfo() call
> - changed autoconf check for getnameinfo to check whenever --enable-gss
> is set, not just when NFSv4 is also enabled.
>
> Jeff Layton (5):
> nfs-utils: make getnameinfo() required for --enable-gss
> nfs-utils: store the address given in the upcall for later use
> nfs-utils: query for remote port using rpcbind instead of getaddrinfo
> nfs-utils: switch gssd to use standard function for getting an RPC
> client
> nfs-utils: add IPv6 code to gssd
>
> configure.ac | 3 +
> utils/gssd/gssd.h | 2 +-
> utils/gssd/gssd_proc.c | 286 +++++++++++++++++++++++++++++++++---------------
> 3 files changed, 204 insertions(+), 87 deletions(-)
>
Committed....

steved.

2009-04-07 15:25:42

by Jeff Layton

[permalink] [raw]
Subject: [PATCH 1/5] nfs-utils: make getnameinfo() required for --enable-gss

Systems that are so old that they don't have getnameinfo() in glibc are
probably also running kernels that are so old that they don't support
gssapi upcalls anyway.

Make --enable-gss dependent on the presence of the getnameinfo()
function. This allows us to reduce some conditional compilation.

Signed-off-by: Jeff Layton <[email protected]>
---
configure.ac | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac
index e34b7e2..611798d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -236,6 +236,9 @@ AC_SUBST(LIBBSD)
AC_SUBST(LIBBLKID)

if test "$enable_gss" = yes; then
+ dnl 'gss' requires getnameinfo - at least for gssd_proc.c
+ AC_CHECK_FUNC([getnameinfo], , [AC_MSG_ERROR([GSSAPI support requires 'getnameinfo' function])])
+
dnl 'gss' also depends on nfsidmap.h - at least for svcgssd_proc.c
AC_LIBNFSIDMAP

--
1.6.0.6

2009-04-07 15:25:43

by Jeff Layton

[permalink] [raw]
Subject: [PATCH 2/5] nfs-utils: store the address given in the upcall for later use

The current upcall could be more efficient. We first convert the address
to a hostname, and then later when we set up the RPC client, we do a
hostname lookup to convert it back to an address.

Begin to change this by keeping the address in the clnt_info that we get
out of the upcall. Since a sockaddr has a port field, we can also
eliminate the port from the clnt_info.

Finally, switch to getnameinfo() instead of gethostbyaddr(). We'll need
to use that call anyway when we add support for IPv6.

Signed-off-by: Jeff Layton <[email protected]>
---
utils/gssd/gssd.h | 2 +-
utils/gssd/gssd_proc.c | 93 +++++++++++++++++++++++++++++++++++++++--------
2 files changed, 78 insertions(+), 17 deletions(-)

diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index 082039a..3c52f46 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -83,7 +83,7 @@ struct clnt_info {
int krb5_poll_index;
int spkm3_fd;
int spkm3_poll_index;
- int port;
+ struct sockaddr_storage addr;
};

void init_client_list(void);
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 509946e..9d3712b 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -102,10 +102,66 @@ struct pollfd * pollarray;

int pollsize; /* the size of pollaray (in pollfd's) */

+/*
+ * convert a presentation address string to a sockaddr_storage struct. Returns
+ * true on success and false on failure.
+ */
+static int
+addrstr_to_sockaddr(struct sockaddr *sa, const char *addr, const int port)
+{
+ struct sockaddr_in *s4 = (struct sockaddr_in *) sa;
+
+ if (inet_pton(AF_INET, addr, &s4->sin_addr)) {
+ s4->sin_family = AF_INET;
+ s4->sin_port = htons(port);
+ } else {
+ printerr(0, "ERROR: unable to convert %s to address\n", addr);
+ return 0;
+ }
+
+ return 1;
+}
+
+/*
+ * convert a sockaddr to a hostname
+ */
+static char *
+sockaddr_to_hostname(const struct sockaddr *sa, const char *addr)
+{
+ socklen_t addrlen;
+ int err;
+ char *hostname;
+ char hbuf[NI_MAXHOST];
+
+ switch (sa->sa_family) {
+ case AF_INET:
+ addrlen = sizeof(struct sockaddr_in);
+ break;
+ default:
+ printerr(0, "ERROR: unrecognized addr family %d\n",
+ sa->sa_family);
+ return NULL;
+ }
+
+ err = getnameinfo(sa, addrlen, hbuf, sizeof(hbuf), NULL, 0,
+ NI_NAMEREQD);
+ if (err) {
+ printerr(0, "ERROR: unable to resolve %s to hostname: %s\n",
+ addr, err == EAI_SYSTEM ? strerror(err) :
+ gai_strerror(err));
+ return NULL;
+ }
+
+ hostname = strdup(hbuf);
+
+ return hostname;
+}
+
/* XXX buffer problems: */
static int
read_service_info(char *info_file_name, char **servicename, char **servername,
- int *prog, int *vers, char **protocol, int *port) {
+ int *prog, int *vers, char **protocol,
+ struct sockaddr *addr) {
#define INFOBUFLEN 256
char buf[INFOBUFLEN + 1];
static char dummy[128];
@@ -117,10 +173,9 @@ read_service_info(char *info_file_name, char **servicename, char **servername,
char protoname[16];
char cb_port[128];
char *p;
- in_addr_t inaddr;
int fd = -1;
- struct hostent *ent = NULL;
int numfields;
+ int port = 0;

*servicename = *servername = *protocol = NULL;

@@ -160,21 +215,26 @@ read_service_info(char *info_file_name, char **servicename, char **servername,
if((*prog != 100003) || ((*vers != 2) && (*vers != 3) && (*vers != 4)))
goto fail;

- /* create service name */
- inaddr = inet_addr(address);
- if (!(ent = gethostbyaddr(&inaddr, sizeof(inaddr), AF_INET))) {
- printerr(0, "ERROR: can't resolve server %s name\n", address);
- goto fail;
+ if (cb_port[0] != '\0') {
+ port = atoi(cb_port);
+ if (port < 0 || port > 65535)
+ goto fail;
}
- if (!(*servername = calloc(strlen(ent->h_name) + 1, 1)))
+
+ if (!addrstr_to_sockaddr(addr, address, port))
+ goto fail;
+
+ *servername = sockaddr_to_hostname(addr, address);
+ if (*servername == NULL)
goto fail;
- memcpy(*servername, ent->h_name, strlen(ent->h_name));
- snprintf(buf, INFOBUFLEN, "%s@%s", service, ent->h_name);
+
+ nbytes = snprintf(buf, INFOBUFLEN, "%s@%s", service, *servername);
+ if (nbytes > INFOBUFLEN)
+ goto fail;
+
if (!(*servicename = calloc(strlen(buf) + 1, 1)))
goto fail;
memcpy(*servicename, buf, strlen(buf));
- if (cb_port[0] != '\0')
- *port = atoi(cb_port);

if (!(*protocol = strdup(protoname)))
goto fail;
@@ -251,7 +311,7 @@ process_clnt_dir_files(struct clnt_info * clp)
if ((clp->servicename == NULL) &&
read_service_info(info_file_name, &clp->servicename,
&clp->servername, &clp->prog, &clp->vers,
- &clp->protocol, &clp->port))
+ &clp->protocol, (struct sockaddr *) &clp->addr))
return -1;
return 0;
}
@@ -599,8 +659,9 @@ int create_auth_rpc_client(struct clnt_info *clp,
clp->servername, uid);
goto out_fail;
}
- if (clp->port)
- ((struct sockaddr_in *)a->ai_addr)->sin_port = htons(clp->port);
+ if (((struct sockaddr_in) clp->addr).sin_port != 0)
+ ((struct sockaddr_in *) a->ai_addr)->sin_port =
+ ((struct sockaddr_in) clp->addr).sin_port;
if (a->ai_protocol == IPPROTO_TCP) {
if ((rpc_clnt = clnttcp_create(
(struct sockaddr_in *) a->ai_addr,
--
1.6.0.6

2009-04-07 15:25:44

by Jeff Layton

[permalink] [raw]
Subject: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

We already have the server's address from the upcall, so we don't really
need to look it up again, and querying the local services DB for the
port that the remote server is listening on is just plain wrong.

Use rpcbind to set the port for the program and version that we were
given in the upcall. The exception here is NFSv4. Since NFSv4 mounts
are supposed to use a well-defined port then skip the rpcbind query
for that and just set the port to the standard one (2049).

Signed-off-by: Jeff Layton <[email protected]>
---
utils/gssd/gssd_proc.c | 126 +++++++++++++++++++++++++++++-------------------
1 files changed, 76 insertions(+), 50 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 9d3712b..1571329 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -72,6 +72,7 @@
#include "gss_util.h"
#include "krb5_util.h"
#include "context.h"
+#include "nfsrpc.h"

/*
* pollarray:
@@ -541,6 +542,64 @@ out_err:
}

/*
+ * If the port isn't already set, do an rpcbind query to the remote server
+ * using the program and version and get the port. Newer kernels send the
+ * port in the upcall, so this is really only for compatibility with older
+ * ones.
+ */
+static int
+populate_port(struct sockaddr *sa, const socklen_t salen,
+ const rpcprog_t program, const rpcvers_t version,
+ const unsigned short protocol)
+{
+ struct sockaddr_in *s4 = (struct sockaddr_in *) sa;
+ unsigned short port;
+
+ /*
+ * Newer kernels send the port in the upcall. If we already have
+ * the port, there's no need to look it up.
+ */
+ switch (sa->sa_family) {
+ case AF_INET:
+ if (s4->sin_port != 0) {
+ printerr(2, "DEBUG: port already set to %d\n",
+ ntohs(s4->sin_port));
+ return 1;
+ }
+ break;
+ default:
+ printerr(0, "ERROR: unsupported address family %d\n",
+ sa->sa_family);
+ return 0;
+ }
+
+ /* Use standard NFS port for NFSv4 */
+ if (program == 100003 && version == 4) {
+ port = 2049;
+ goto set_port;
+ }
+
+ port = nfs_getport(sa, salen, program, version, protocol);
+ if (!port) {
+ printerr(0, "ERROR: unable to obtain port for prog %ld "
+ "vers %ld\n", program, version);
+ return 0;
+ }
+
+set_port:
+ printerr(2, "DEBUG: setting port to %hu for prog %lu vers %lu\n", port,
+ program, version);
+
+ switch (sa->sa_family) {
+ case AF_INET:
+ s4->sin_port = htons(port);
+ break;
+ }
+
+ return 1;
+}
+
+/*
* Create an RPC connection and establish an authenticated
* gss context with a server.
*/
@@ -555,14 +614,13 @@ int create_auth_rpc_client(struct clnt_info *clp,
AUTH *auth = NULL;
uid_t save_uid = -1;
int retval = -1;
- int errcode;
OM_uint32 min_stat;
char rpc_errmsg[1024];
int sockp = RPC_ANYSOCK;
int sendsz = 32768, recvsz = 32768;
- struct addrinfo ai_hints, *a = NULL;
- char service[64];
- char *at_sign;
+ int protocol;
+ struct sockaddr *addr = (struct sockaddr *) &clp->addr;
+ socklen_t salen;

/* Create the context as the user (not as root) */
save_uid = geteuid();
@@ -616,15 +674,10 @@ int create_auth_rpc_client(struct clnt_info *clp,
printerr(2, "creating %s client for server %s\n", clp->protocol,
clp->servername);

- memset(&ai_hints, '\0', sizeof(ai_hints));
- ai_hints.ai_family = PF_INET;
- ai_hints.ai_flags |= AI_CANONNAME;
if ((strcmp(clp->protocol, "tcp")) == 0) {
- ai_hints.ai_socktype = SOCK_STREAM;
- ai_hints.ai_protocol = IPPROTO_TCP;
+ protocol = IPPROTO_TCP;
} else if ((strcmp(clp->protocol, "udp")) == 0) {
- ai_hints.ai_socktype = SOCK_DGRAM;
- ai_hints.ai_protocol = IPPROTO_UDP;
+ protocol = IPPROTO_UDP;
} else {
printerr(0, "WARNING: unrecognized protocol, '%s', requested "
"for connection to server %s for user with uid %d\n",
@@ -632,39 +685,22 @@ int create_auth_rpc_client(struct clnt_info *clp,
goto out_fail;
}

- /* extract the service name from clp->servicename */
- if ((at_sign = strchr(clp->servicename, '@')) == NULL) {
- printerr(0, "WARNING: servicename (%s) not formatted as "
- "expected with service@host\n", clp->servicename);
- goto out_fail;
- }
- if ((at_sign - clp->servicename) >= sizeof(service)) {
- printerr(0, "WARNING: service portion of servicename (%s) "
- "is too long!\n", clp->servicename);
+ switch (addr->sa_family) {
+ case AF_INET:
+ salen = sizeof(struct sockaddr_in);
+ break;
+ default:
+ printerr(1, "ERROR: Unknown address family %d\n",
+ addr->sa_family);
goto out_fail;
}
- strncpy(service, clp->servicename, at_sign - clp->servicename);
- service[at_sign - clp->servicename] = '\0';

- errcode = getaddrinfo(clp->servername, service, &ai_hints, &a);
- if (errcode) {
- printerr(0, "WARNING: Error from getaddrinfo for server "
- "'%s': %s\n", clp->servername, gai_strerror(errcode));
+ if (!populate_port(addr, salen, clp->prog, clp->vers, protocol))
goto out_fail;
- }

- if (a == NULL) {
- printerr(0, "WARNING: No address information found for "
- "connection to server %s for user with uid %d\n",
- clp->servername, uid);
- goto out_fail;
- }
- if (((struct sockaddr_in) clp->addr).sin_port != 0)
- ((struct sockaddr_in *) a->ai_addr)->sin_port =
- ((struct sockaddr_in) clp->addr).sin_port;
- if (a->ai_protocol == IPPROTO_TCP) {
+ if (protocol == IPPROTO_TCP) {
if ((rpc_clnt = clnttcp_create(
- (struct sockaddr_in *) a->ai_addr,
+ (struct sockaddr_in *) addr,
clp->prog, clp->vers, &sockp,
sendsz, recvsz)) == NULL) {
snprintf(rpc_errmsg, sizeof(rpc_errmsg),
@@ -675,10 +711,10 @@ int create_auth_rpc_client(struct clnt_info *clp,
clnt_spcreateerror(rpc_errmsg));
goto out_fail;
}
- } else if (a->ai_protocol == IPPROTO_UDP) {
+ } else if (protocol == IPPROTO_UDP) {
const struct timeval timeout = {5, 0};
if ((rpc_clnt = clntudp_bufcreate(
- (struct sockaddr_in *) a->ai_addr,
+ (struct sockaddr_in *) addr,
clp->prog, clp->vers, timeout,
&sockp, sendsz, recvsz)) == NULL) {
snprintf(rpc_errmsg, sizeof(rpc_errmsg),
@@ -689,16 +725,7 @@ int create_auth_rpc_client(struct clnt_info *clp,
clnt_spcreateerror(rpc_errmsg));
goto out_fail;
}
- } else {
- /* Shouldn't happen! */
- printerr(0, "ERROR: requested protocol '%s', but "
- "got addrinfo with protocol %d\n",
- clp->protocol, a->ai_protocol);
- goto out_fail;
}
- /* We're done with this */
- freeaddrinfo(a);
- a = NULL;

printerr(2, "creating context with server %s\n", clp->servicename);
auth = authgss_create_default(rpc_clnt, clp->servicename, &sec);
@@ -720,7 +747,6 @@ int create_auth_rpc_client(struct clnt_info *clp,
out:
if (sec.cred != GSS_C_NO_CREDENTIAL)
gss_release_cred(&min_stat, &sec.cred);
- if (a != NULL) freeaddrinfo(a);
/* Restore euid to original value */
if ((save_uid != -1) && (setfsuid(save_uid) != uid)) {
printerr(0, "WARNING: Failed to restore fsuid"
--
1.6.0.6

2009-04-07 15:25:45

by Jeff Layton

[permalink] [raw]
Subject: [PATCH 4/5] nfs-utils: switch gssd to use standard function for getting an RPC client

We already have a common function for setting up an RPC client. That
function uses the tirpc API when tirpc is enabled and is also already
IPv6 enabled. Switch gssd to use it.

Signed-off-by: Jeff Layton <[email protected]>
---
utils/gssd/gssd_proc.c | 41 ++++++++++++-----------------------------
1 files changed, 12 insertions(+), 29 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 1571329..5b97d6c 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -616,9 +616,8 @@ int create_auth_rpc_client(struct clnt_info *clp,
int retval = -1;
OM_uint32 min_stat;
char rpc_errmsg[1024];
- int sockp = RPC_ANYSOCK;
- int sendsz = 32768, recvsz = 32768;
int protocol;
+ struct timeval timeout = {5, 0};
struct sockaddr *addr = (struct sockaddr *) &clp->addr;
socklen_t salen;

@@ -698,33 +697,17 @@ int create_auth_rpc_client(struct clnt_info *clp,
if (!populate_port(addr, salen, clp->prog, clp->vers, protocol))
goto out_fail;

- if (protocol == IPPROTO_TCP) {
- if ((rpc_clnt = clnttcp_create(
- (struct sockaddr_in *) addr,
- clp->prog, clp->vers, &sockp,
- sendsz, recvsz)) == NULL) {
- snprintf(rpc_errmsg, sizeof(rpc_errmsg),
- "WARNING: can't create tcp rpc_clnt "
- "for server %s for user with uid %d",
- clp->servername, uid);
- printerr(0, "%s\n",
- clnt_spcreateerror(rpc_errmsg));
- goto out_fail;
- }
- } else if (protocol == IPPROTO_UDP) {
- const struct timeval timeout = {5, 0};
- if ((rpc_clnt = clntudp_bufcreate(
- (struct sockaddr_in *) addr,
- clp->prog, clp->vers, timeout,
- &sockp, sendsz, recvsz)) == NULL) {
- snprintf(rpc_errmsg, sizeof(rpc_errmsg),
- "WARNING: can't create udp rpc_clnt "
- "for server %s for user with uid %d",
- clp->servername, uid);
- printerr(0, "%s\n",
- clnt_spcreateerror(rpc_errmsg));
- goto out_fail;
- }
+ rpc_clnt = nfs_get_rpcclient(addr, salen, protocol, clp->prog,
+ clp->vers, &timeout);
+ if (!rpc_clnt) {
+ snprintf(rpc_errmsg, sizeof(rpc_errmsg),
+ "WARNING: can't create %s rpc_clnt to server %s for "
+ "user with uid %d",
+ protocol == IPPROTO_TCP ? "tcp" : "udp",
+ clp->servername, uid);
+ printerr(0, "%s\n",
+ clnt_spcreateerror(rpc_errmsg));
+ goto out_fail;
}

printerr(2, "creating context with server %s\n", clp->servicename);
--
1.6.0.6

2009-04-07 15:25:46

by Jeff Layton

[permalink] [raw]
Subject: [PATCH 5/5] nfs-utils: add IPv6 code to gssd

All of the pieces to handle IPv6 are now in place. Add IPv6-specific
code wrapped in the proper #ifdef's so that IPv6 support works when
it's enabled at build-time.

Signed-off-by: Jeff Layton <[email protected]>
---
utils/gssd/gssd_proc.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 5b97d6c..70094cf 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -106,15 +106,32 @@ int pollsize; /* the size of pollaray (in pollfd's) */
/*
* convert a presentation address string to a sockaddr_storage struct. Returns
* true on success and false on failure.
+ *
+ * Note that we do not populate the sin6_scope_id field here for IPv6 addrs.
+ * gssd nececessarily relies on hostname resolution and DNS AAAA records
+ * do not generally contain scope-id's. This means that GSSAPI auth really
+ * can't work with IPv6 link-local addresses.
+ *
+ * We *could* consider changing this if we did something like adopt the
+ * Microsoft "standard" of using the ipv6-literal.net domainname, but it's
+ * not really feasible at present.
*/
static int
addrstr_to_sockaddr(struct sockaddr *sa, const char *addr, const int port)
{
struct sockaddr_in *s4 = (struct sockaddr_in *) sa;
+#ifdef IPV6_SUPPORTED
+ struct sockaddr_in6 *s6 = (struct sockaddr_in6 *) sa;
+#endif /* IPV6_SUPPORTED */

if (inet_pton(AF_INET, addr, &s4->sin_addr)) {
s4->sin_family = AF_INET;
s4->sin_port = htons(port);
+#ifdef IPV6_SUPPORTED
+ } else if (inet_pton(AF_INET6, addr, &s6->sin6_addr)) {
+ s6->sin6_family = AF_INET6;
+ s6->sin6_port = htons(port);
+#endif /* IPV6_SUPPORTED */
} else {
printerr(0, "ERROR: unable to convert %s to address\n", addr);
return 0;
@@ -138,6 +155,11 @@ sockaddr_to_hostname(const struct sockaddr *sa, const char *addr)
case AF_INET:
addrlen = sizeof(struct sockaddr_in);
break;
+#ifdef IPV6_SUPPORTED
+ case AF_INET6:
+ addrlen = sizeof(struct sockaddr_in6);
+ break;
+#endif /* IPV6_SUPPORTED */
default:
printerr(0, "ERROR: unrecognized addr family %d\n",
sa->sa_family);
@@ -553,6 +575,9 @@ populate_port(struct sockaddr *sa, const socklen_t salen,
const unsigned short protocol)
{
struct sockaddr_in *s4 = (struct sockaddr_in *) sa;
+#ifdef IPV6_SUPPORTED
+ struct sockaddr_in6 *s6 = (struct sockaddr_in6 *) sa;
+#endif /* IPV6_SUPPORTED */
unsigned short port;

/*
@@ -567,6 +592,15 @@ populate_port(struct sockaddr *sa, const socklen_t salen,
return 1;
}
break;
+#ifdef IPV6_SUPPORTED
+ case AF_INET6:
+ if (s6->sin6_port != 0) {
+ printerr(2, "DEBUG: port already set to %d\n",
+ ntohs(s6->sin6_port));
+ return 1;
+ }
+ break;
+#endif /* IPV6_SUPPORTED */
default:
printerr(0, "ERROR: unsupported address family %d\n",
sa->sa_family);
@@ -594,6 +628,11 @@ set_port:
case AF_INET:
s4->sin_port = htons(port);
break;
+#ifdef IPV6_SUPPORTED
+ case AF_INET6:
+ s6->sin6_port = htons(port);
+ break;
+#endif /* IPV6_SUPPORTED */
}

return 1;
@@ -688,6 +727,11 @@ int create_auth_rpc_client(struct clnt_info *clp,
case AF_INET:
salen = sizeof(struct sockaddr_in);
break;
+#ifdef IPV6_SUPPORTED
+ case AF_INET6:
+ salen = sizeof(struct sockaddr_in6);
+ break;
+#endif /* IPV6_SUPPORTED */
default:
printerr(1, "ERROR: Unknown address family %d\n",
addr->sa_family);
--
1.6.0.6

2009-04-07 16:02:46

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo


On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:

> We already have the server's address from the upcall, so we don't
> really
> need to look it up again, and querying the local services DB for the
> port that the remote server is listening on is just plain wrong.
>
> Use rpcbind to set the port for the program and version that we were
> given in the upcall. The exception here is NFSv4. Since NFSv4 mounts
> are supposed to use a well-defined port then skip the rpcbind query
> for that and just set the port to the standard one (2049).
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> utils/gssd/gssd_proc.c | 126 ++++++++++++++++++++++++++++
> +-------------------
> 1 files changed, 76 insertions(+), 50 deletions(-)
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 9d3712b..1571329 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -72,6 +72,7 @@
> #include "gss_util.h"
> #include "krb5_util.h"
> #include "context.h"
> +#include "nfsrpc.h"
>
> /*
> * pollarray:
> @@ -541,6 +542,64 @@ out_err:
> }
>
> /*
> + * If the port isn't already set, do an rpcbind query to the remote
> server
> + * using the program and version and get the port. Newer kernels
> send the
> + * port in the upcall, so this is really only for compatibility
> with older
> + * ones.
> + */
> +static int
> +populate_port(struct sockaddr *sa, const socklen_t salen,
> + const rpcprog_t program, const rpcvers_t version,
> + const unsigned short protocol)
> +{
> + struct sockaddr_in *s4 = (struct sockaddr_in *) sa;
> + unsigned short port;
> +
> + /*
> + * Newer kernels send the port in the upcall. If we already have
> + * the port, there's no need to look it up.
> + */
> + switch (sa->sa_family) {
> + case AF_INET:
> + if (s4->sin_port != 0) {
> + printerr(2, "DEBUG: port already set to %d\n",
> + ntohs(s4->sin_port));
> + return 1;
> + }
> + break;
> + default:
> + printerr(0, "ERROR: unsupported address family %d\n",
> + sa->sa_family);
> + return 0;
> + }
> +
> + /* Use standard NFS port for NFSv4 */
> + if (program == 100003 && version == 4) {
> + port = 2049;
> + goto set_port;
> + }

I think this patch set looks pretty reasonable. Here's my one
remaining quibble.

You can specify "port=" for nfs4 mounts, in which case we want to use
that value here, too, I think. It would be simpler overall if the
kernel always passed up the value it is using for port= on this mount
point.

The rules for how the kernel uses the port= setting are:

+ if port= is not specified on NFSv2/v3, port= setting is zero
+ if port= is not specified on NFSv4, port= setting is 2049

Then, when setting up a tranport:

+ if the port= setting is zero, do an rpcbind
+ if the port= setting is not zero, use that value

If the kernel always passes the port= setting to gssd, then it can
follow the "if port value is zero, rpcbind; otherwise use port value
as is" rule, and be sure to get correct NFSv4 behavior, even without
the special case you added for NFSv4.

> +
> + port = nfs_getport(sa, salen, program, version, protocol);
> + if (!port) {
> + printerr(0, "ERROR: unable to obtain port for prog %ld "
> + "vers %ld\n", program, version);
> + return 0;
> + }
> +
> +set_port:
> + printerr(2, "DEBUG: setting port to %hu for prog %lu vers %lu\n",
> port,
> + program, version);
> +
> + switch (sa->sa_family) {
> + case AF_INET:
> + s4->sin_port = htons(port);
> + break;
> + }
> +
> + return 1;
> +}
> +
> +/*
> * Create an RPC connection and establish an authenticated
> * gss context with a server.
> */
> @@ -555,14 +614,13 @@ int create_auth_rpc_client(struct clnt_info
> *clp,
> AUTH *auth = NULL;
> uid_t save_uid = -1;
> int retval = -1;
> - int errcode;
> OM_uint32 min_stat;
> char rpc_errmsg[1024];
> int sockp = RPC_ANYSOCK;
> int sendsz = 32768, recvsz = 32768;
> - struct addrinfo ai_hints, *a = NULL;
> - char service[64];
> - char *at_sign;
> + int protocol;
> + struct sockaddr *addr = (struct sockaddr *) &clp->addr;
> + socklen_t salen;
>
> /* Create the context as the user (not as root) */
> save_uid = geteuid();
> @@ -616,15 +674,10 @@ int create_auth_rpc_client(struct clnt_info
> *clp,
> printerr(2, "creating %s client for server %s\n", clp->protocol,
> clp->servername);
>
> - memset(&ai_hints, '\0', sizeof(ai_hints));
> - ai_hints.ai_family = PF_INET;
> - ai_hints.ai_flags |= AI_CANONNAME;
> if ((strcmp(clp->protocol, "tcp")) == 0) {
> - ai_hints.ai_socktype = SOCK_STREAM;
> - ai_hints.ai_protocol = IPPROTO_TCP;
> + protocol = IPPROTO_TCP;
> } else if ((strcmp(clp->protocol, "udp")) == 0) {
> - ai_hints.ai_socktype = SOCK_DGRAM;
> - ai_hints.ai_protocol = IPPROTO_UDP;
> + protocol = IPPROTO_UDP;
> } else {
> printerr(0, "WARNING: unrecognized protocol, '%s', requested "
> "for connection to server %s for user with uid %d\n",
> @@ -632,39 +685,22 @@ int create_auth_rpc_client(struct clnt_info
> *clp,
> goto out_fail;
> }
>
> - /* extract the service name from clp->servicename */
> - if ((at_sign = strchr(clp->servicename, '@')) == NULL) {
> - printerr(0, "WARNING: servicename (%s) not formatted as "
> - "expected with service@host\n", clp->servicename);
> - goto out_fail;
> - }
> - if ((at_sign - clp->servicename) >= sizeof(service)) {
> - printerr(0, "WARNING: service portion of servicename (%s) "
> - "is too long!\n", clp->servicename);
> + switch (addr->sa_family) {
> + case AF_INET:
> + salen = sizeof(struct sockaddr_in);
> + break;
> + default:
> + printerr(1, "ERROR: Unknown address family %d\n",
> + addr->sa_family);
> goto out_fail;
> }
> - strncpy(service, clp->servicename, at_sign - clp->servicename);
> - service[at_sign - clp->servicename] = '\0';
>
> - errcode = getaddrinfo(clp->servername, service, &ai_hints, &a);
> - if (errcode) {
> - printerr(0, "WARNING: Error from getaddrinfo for server "
> - "'%s': %s\n", clp->servername, gai_strerror(errcode));
> + if (!populate_port(addr, salen, clp->prog, clp->vers, protocol))
> goto out_fail;
> - }
>
> - if (a == NULL) {
> - printerr(0, "WARNING: No address information found for "
> - "connection to server %s for user with uid %d\n",
> - clp->servername, uid);
> - goto out_fail;
> - }
> - if (((struct sockaddr_in) clp->addr).sin_port != 0)
> - ((struct sockaddr_in *) a->ai_addr)->sin_port =
> - ((struct sockaddr_in) clp->addr).sin_port;
> - if (a->ai_protocol == IPPROTO_TCP) {
> + if (protocol == IPPROTO_TCP) {
> if ((rpc_clnt = clnttcp_create(
> - (struct sockaddr_in *) a->ai_addr,
> + (struct sockaddr_in *) addr,
> clp->prog, clp->vers, &sockp,
> sendsz, recvsz)) == NULL) {
> snprintf(rpc_errmsg, sizeof(rpc_errmsg),
> @@ -675,10 +711,10 @@ int create_auth_rpc_client(struct clnt_info
> *clp,
> clnt_spcreateerror(rpc_errmsg));
> goto out_fail;
> }
> - } else if (a->ai_protocol == IPPROTO_UDP) {
> + } else if (protocol == IPPROTO_UDP) {
> const struct timeval timeout = {5, 0};
> if ((rpc_clnt = clntudp_bufcreate(
> - (struct sockaddr_in *) a->ai_addr,
> + (struct sockaddr_in *) addr,
> clp->prog, clp->vers, timeout,
> &sockp, sendsz, recvsz)) == NULL) {
> snprintf(rpc_errmsg, sizeof(rpc_errmsg),
> @@ -689,16 +725,7 @@ int create_auth_rpc_client(struct clnt_info *clp,
> clnt_spcreateerror(rpc_errmsg));
> goto out_fail;
> }
> - } else {
> - /* Shouldn't happen! */
> - printerr(0, "ERROR: requested protocol '%s', but "
> - "got addrinfo with protocol %d\n",
> - clp->protocol, a->ai_protocol);
> - goto out_fail;
> }
> - /* We're done with this */
> - freeaddrinfo(a);
> - a = NULL;
>
> printerr(2, "creating context with server %s\n", clp->servicename);
> auth = authgss_create_default(rpc_clnt, clp->servicename, &sec);
> @@ -720,7 +747,6 @@ int create_auth_rpc_client(struct clnt_info *clp,
> out:
> if (sec.cred != GSS_C_NO_CREDENTIAL)
> gss_release_cred(&min_stat, &sec.cred);
> - if (a != NULL) freeaddrinfo(a);
> /* Restore euid to original value */
> if ((save_uid != -1) && (setfsuid(save_uid) != uid)) {
> printerr(0, "WARNING: Failed to restore fsuid"
> --
> 1.6.0.6
>

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2009-04-07 16:20:16

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

On Tue, Apr 07, 2009 at 12:02:46PM -0400, Chuck Lever wrote:
>
> On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>
> > We already have the server's address from the upcall, so we don't
> > really
> > need to look it up again, and querying the local services DB for the
> > port that the remote server is listening on is just plain wrong.
> >
> > Use rpcbind to set the port for the program and version that we were
> > given in the upcall. The exception here is NFSv4. Since NFSv4 mounts
> > are supposed to use a well-defined port then skip the rpcbind query
> > for that and just set the port to the standard one (2049).
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > utils/gssd/gssd_proc.c | 126 ++++++++++++++++++++++++++++
> > +-------------------
> > 1 files changed, 76 insertions(+), 50 deletions(-)
> >
> > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > index 9d3712b..1571329 100644
> > --- a/utils/gssd/gssd_proc.c
> > +++ b/utils/gssd/gssd_proc.c
> > @@ -72,6 +72,7 @@
> > #include "gss_util.h"
> > #include "krb5_util.h"
> > #include "context.h"
> > +#include "nfsrpc.h"
> >
> > /*
> > * pollarray:
> > @@ -541,6 +542,64 @@ out_err:
> > }
> >
> > /*
> > + * If the port isn't already set, do an rpcbind query to the remote
> > server
> > + * using the program and version and get the port. Newer kernels
> > send the
> > + * port in the upcall, so this is really only for compatibility
> > with older
> > + * ones.
> > + */
> > +static int
> > +populate_port(struct sockaddr *sa, const socklen_t salen,
> > + const rpcprog_t program, const rpcvers_t version,
> > + const unsigned short protocol)
> > +{
> > + struct sockaddr_in *s4 = (struct sockaddr_in *) sa;
> > + unsigned short port;
> > +
> > + /*
> > + * Newer kernels send the port in the upcall. If we already have
> > + * the port, there's no need to look it up.
> > + */
> > + switch (sa->sa_family) {
> > + case AF_INET:
> > + if (s4->sin_port != 0) {
> > + printerr(2, "DEBUG: port already set to %d\n",
> > + ntohs(s4->sin_port));
> > + return 1;
> > + }
> > + break;
> > + default:
> > + printerr(0, "ERROR: unsupported address family %d\n",
> > + sa->sa_family);
> > + return 0;
> > + }
> > +
> > + /* Use standard NFS port for NFSv4 */
> > + if (program == 100003 && version == 4) {
> > + port = 2049;
> > + goto set_port;
> > + }
>
> I think this patch set looks pretty reasonable. Here's my one
> remaining quibble.
>
> You can specify "port=" for nfs4 mounts, in which case we want to use
> that value here, too, I think. It would be simpler overall if the
> kernel always passed up the value it is using for port= on this mount
> point.
>
> The rules for how the kernel uses the port= setting are:
>
> + if port= is not specified on NFSv2/v3, port= setting is zero
> + if port= is not specified on NFSv4, port= setting is 2049
>
> Then, when setting up a tranport:
>
> + if the port= setting is zero, do an rpcbind
> + if the port= setting is not zero, use that value
>
> If the kernel always passes the port= setting to gssd, then it can
> follow the "if port value is zero, rpcbind; otherwise use port value
> as is" rule, and be sure to get correct NFSv4 behavior, even without
> the special case you added for NFSv4.

In recent kernels that information is in the "info" file for the given
client in rpc_pipefs. Kevin and Olga have patches to support that in
gssd, if they aren't already in upstream nfs-utils.

And of course it's important to use that when it's available, to avoid
unnecessary rpcbind calls (in a pure nfsv4 environment the only firewall
hole you should have to poke is for the nfsv4 port), to respect the
port= mount option for people running a v4 server on an alternate port,
and for authenticating the callback channel (which won't normally be to
port 2049, and won't (I assume) normally be registered with the
portmapper).

--b.

2009-04-07 16:27:59

by Tom Talpey

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

At 12:02 PM 4/7/2009, Chuck Lever wrote:
>
>On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>> + /* Use standard NFS port for NFSv4 */
>> + if (program == 100003 && version == 4) {
>> + port = 2049;
>> + goto set_port;
>> + }
>
>I think this patch set looks pretty reasonable. Here's my one
>remaining quibble.
>
>You can specify "port=" for nfs4 mounts, in which case we want to use
>that value here, too, I think. It would be simpler overall if the

*Must* use a port= specification. The 2049 definition is only true for
NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
port 20049. So slamming the port to 2049 would break NFSv4/RDMA.

>kernel always passed up the value it is using for port= on this mount
>point.
>
>The rules for how the kernel uses the port= setting are:
>
> + if port= is not specified on NFSv2/v3, port= setting is zero
> + if port= is not specified on NFSv4, port= setting is 2049

This is a tiny bit questionable, since 2049 is only defined for TCP.
But, if port= can override, then that's a workaround, so OK.

>
>Then, when setting up a tranport:
>
> + if the port= setting is zero, do an rpcbind
> + if the port= setting is not zero, use that value
>
>If the kernel always passes the port= setting to gssd, then it can
>follow the "if port value is zero, rpcbind; otherwise use port value
>as is" rule, and be sure to get correct NFSv4 behavior, even without
>the special case you added for NFSv4.

Agree.

Tom.


2009-04-07 16:29:23

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

On Apr 7, 2009, at 12:20 PM, J. Bruce Fields wrote:
> On Tue, Apr 07, 2009 at 12:02:46PM -0400, Chuck Lever wrote:
>>
>> On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>>
>>> We already have the server's address from the upcall, so we don't
>>> really
>>> need to look it up again, and querying the local services DB for the
>>> port that the remote server is listening on is just plain wrong.
>>>
>>> Use rpcbind to set the port for the program and version that we were
>>> given in the upcall. The exception here is NFSv4. Since NFSv4 mounts
>>> are supposed to use a well-defined port then skip the rpcbind query
>>> for that and just set the port to the standard one (2049).
>>>
>>> Signed-off-by: Jeff Layton <[email protected]>
>>> ---
>>> utils/gssd/gssd_proc.c | 126 ++++++++++++++++++++++++++++
>>> +-------------------
>>> 1 files changed, 76 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
>>> index 9d3712b..1571329 100644
>>> --- a/utils/gssd/gssd_proc.c
>>> +++ b/utils/gssd/gssd_proc.c
>>> @@ -72,6 +72,7 @@
>>> #include "gss_util.h"
>>> #include "krb5_util.h"
>>> #include "context.h"
>>> +#include "nfsrpc.h"
>>>
>>> /*
>>> * pollarray:
>>> @@ -541,6 +542,64 @@ out_err:
>>> }
>>>
>>> /*
>>> + * If the port isn't already set, do an rpcbind query to the remote
>>> server
>>> + * using the program and version and get the port. Newer kernels
>>> send the
>>> + * port in the upcall, so this is really only for compatibility
>>> with older
>>> + * ones.
>>> + */
>>> +static int
>>> +populate_port(struct sockaddr *sa, const socklen_t salen,
>>> + const rpcprog_t program, const rpcvers_t version,
>>> + const unsigned short protocol)
>>> +{
>>> + struct sockaddr_in *s4 = (struct sockaddr_in *) sa;
>>> + unsigned short port;
>>> +
>>> + /*
>>> + * Newer kernels send the port in the upcall. If we already have
>>> + * the port, there's no need to look it up.
>>> + */
>>> + switch (sa->sa_family) {
>>> + case AF_INET:
>>> + if (s4->sin_port != 0) {
>>> + printerr(2, "DEBUG: port already set to %d\n",
>>> + ntohs(s4->sin_port));
>>> + return 1;
>>> + }
>>> + break;
>>> + default:
>>> + printerr(0, "ERROR: unsupported address family %d\n",
>>> + sa->sa_family);
>>> + return 0;
>>> + }
>>> +
>>> + /* Use standard NFS port for NFSv4 */
>>> + if (program == 100003 && version == 4) {
>>> + port = 2049;
>>> + goto set_port;
>>> + }

And actually, since this test is _after_ the check to see if zero was
passed in, I think we probably get correct behavior here if the kernel
passes a non-zero port value for an NFSv4 mount. My mistake.

>> I think this patch set looks pretty reasonable. Here's my one
>> remaining quibble.
>>
>> You can specify "port=" for nfs4 mounts, in which case we want to use
>> that value here, too, I think. It would be simpler overall if the
>> kernel always passed up the value it is using for port= on this mount
>> point.
>>
>> The rules for how the kernel uses the port= setting are:
>>
>> + if port= is not specified on NFSv2/v3, port= setting is zero
>> + if port= is not specified on NFSv4, port= setting is 2049
>>
>> Then, when setting up a tranport:
>>
>> + if the port= setting is zero, do an rpcbind
>> + if the port= setting is not zero, use that value
>>
>> If the kernel always passes the port= setting to gssd, then it can
>> follow the "if port value is zero, rpcbind; otherwise use port value
>> as is" rule, and be sure to get correct NFSv4 behavior, even without
>> the special case you added for NFSv4.
>
> In recent kernels that information is in the "info" file for the given
> client in rpc_pipefs. Kevin and Olga have patches to support that in
> gssd, if they aren't already in upstream nfs-utils.

The issue is we're not exactly sure what value the kernel is passing
up in the info file. It seems to change over time.

> And of course it's important to use that when it's available, to avoid
> unnecessary rpcbind calls (in a pure nfsv4 environment the only
> firewall
> hole you should have to poke is for the nfsv4 port), to respect the
> port= mount option for people running a v4 server on an alternate
> port,
> and for authenticating the callback channel (which won't normally be
> to
> port 2049, and won't (I assume) normally be registered with the
> portmapper).

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2009-04-07 16:32:24

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

On Apr 7, 2009, at 12:27 PM, Tom Talpey wrote:
> At 12:02 PM 4/7/2009, Chuck Lever wrote:
>>
>> On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>>> + /* Use standard NFS port for NFSv4 */
>>> + if (program == 100003 && version == 4) {
>>> + port = 2049;
>>> + goto set_port;
>>> + }
>>
>> I think this patch set looks pretty reasonable. Here's my one
>> remaining quibble.
>>
>> You can specify "port=" for nfs4 mounts, in which case we want to use
>> that value here, too, I think. It would be simpler overall if the
>
> *Must* use a port= specification. The 2049 definition is only true for
> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.

>> kernel always passed up the value it is using for port= on this mount
>> point.
>>
>> The rules for how the kernel uses the port= setting are:
>>
>> + if port= is not specified on NFSv2/v3, port= setting is zero
>> + if port= is not specified on NFSv4, port= setting is 2049
>
> This is a tiny bit questionable, since 2049 is only defined for TCP.
> But, if port= can override, then that's a workaround, so OK.

Right. The above rule is done in the kernel's mount option parsing
logic, so that can/should be changed to address this. If the kernel
always passes the correct port up to gssd, then gssd doesn't need to
worry about it.

>> Then, when setting up a tranport:
>>
>> + if the port= setting is zero, do an rpcbind
>> + if the port= setting is not zero, use that value
>>
>> If the kernel always passes the port= setting to gssd, then it can
>> follow the "if port value is zero, rpcbind; otherwise use port value
>> as is" rule, and be sure to get correct NFSv4 behavior, even without
>> the special case you added for NFSv4.
>
> Agree.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2009-04-07 16:32:55

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

On Tue, Apr 07, 2009 at 12:29:23PM -0400, Chuck Lever wrote:
> On Apr 7, 2009, at 12:20 PM, J. Bruce Fields wrote:
>> In recent kernels that information is in the "info" file for the given
>> client in rpc_pipefs. Kevin and Olga have patches to support that in
>> gssd, if they aren't already in upstream nfs-utils.
>
> The issue is we're not exactly sure what value the kernel is passing up
> in the info file. It seems to change over time.

I don't understand. How does it change over time?

--b.

2009-04-07 16:34:54

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

On Apr 7, 2009, at 12:32 PM, J. Bruce Fields wrote:
> On Tue, Apr 07, 2009 at 12:29:23PM -0400, Chuck Lever wrote:
>> On Apr 7, 2009, at 12:20 PM, J. Bruce Fields wrote:
>>> In recent kernels that information is in the "info" file for the
>>> given
>>> client in rpc_pipefs. Kevin and Olga have patches to support that
>>> in
>>> gssd, if they aren't already in upstream nfs-utils.
>>
>> The issue is we're not exactly sure what value the kernel is
>> passing up
>> in the info file. It seems to change over time.
>
> I don't understand. How does it change over time?

Jeff observed some strange behavior while testing yesterday, so I'll
let him address this question.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2009-04-07 17:00:58

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

On Tue, 7 Apr 2009 12:34:54 -0400
Chuck Lever <[email protected]> wrote:

> On Apr 7, 2009, at 12:32 PM, J. Bruce Fields wrote:
> > On Tue, Apr 07, 2009 at 12:29:23PM -0400, Chuck Lever wrote:
> >> On Apr 7, 2009, at 12:20 PM, J. Bruce Fields wrote:
> >>> In recent kernels that information is in the "info" file for the
> >>> given
> >>> client in rpc_pipefs. Kevin and Olga have patches to support that
> >>> in
> >>> gssd, if they aren't already in upstream nfs-utils.
> >>
> >> The issue is we're not exactly sure what value the kernel is
> >> passing up
> >> in the info file. It seems to change over time.
> >
> > I don't understand. How does it change over time?
>
> Jeff observed some strange behavior while testing yesterday, so I'll
> let him address this question.
>

I sort of had expected to see the "port" value in the info file change
once the kernel had done the rpcbind query and set the port in the
socket. It doesn't do this though. In hindsight I think the current
behavior is actually correct. The port value here more or less
represents the port that was given to the kernel at mount time.

> The rules for how the kernel uses the port= setting are:
>
> + if port= is not specified on NFSv2/v3, port= setting is zero
> + if port= is not specified on NFSv4, port= setting is 2049
>
> Then, when setting up a tranport:
>
> + if the port= setting is zero, do an rpcbind
> + if the port= setting is not zero, use that value

I think this patchset provides the behavior you want. For recent
kernels that provide the port: field in the "info" file:

nfsv4: port: 2049
nfsv2/3: port: 0

...if you set port= in the mount options, then the info file displays
that port.

When we have an older kernel that does not display the port, then
populate_port() will see that sin(6)_port is still 0. For nfsv2/3 it'll
do an rpcbind query to get the port, but we don't want to do that for
NFSv4, so I added this:

+ /* Use standard NFS port for NFSv4 */
+ if (program == 100003 && version == 4) {
+ port = 2049;
+ goto set_port;
+ }

The important bit is that the above chunk will only matter for kernels
that don't have a port: field in the info file.

In short, I think the behavior with this set is what you've outlined
above.

--
Jeff Layton <[email protected]>

2009-04-07 17:11:51

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

On Tue, 07 Apr 2009 12:27:49 -0400
Tom Talpey <[email protected]> wrote:

> At 12:02 PM 4/7/2009, Chuck Lever wrote:
> >
> >On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
> >> + /* Use standard NFS port for NFSv4 */
> >> + if (program == 100003 && version == 4) {
> >> + port = 2049;
> >> + goto set_port;
> >> + }
> >
> >I think this patch set looks pretty reasonable. Here's my one
> >remaining quibble.
> >
> >You can specify "port=" for nfs4 mounts, in which case we want to use
> >that value here, too, I think. It would be simpler overall if the
>
> *Must* use a port= specification. The 2049 definition is only true for
> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
>

rpc.gssd doesn't seem to be rdma-enabled at this point. It only seems
to handle "tcp" and "udp" in the existing code.

Does libtirpc handle RDMA properly? If so, this might not be too hard
to enable, but I'd probably rather see it in a follow on patchset (and
maybe by someone with more of a clue about RDMA than I currently have).

--
Jeff Layton <[email protected]>

2009-04-07 17:12:22

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

On Apr 7, 2009, at 1:00 PM, Jeff Layton wrote:
> On Tue, 7 Apr 2009 12:34:54 -0400
> Chuck Lever <[email protected]> wrote:
>
>> On Apr 7, 2009, at 12:32 PM, J. Bruce Fields wrote:
>>> On Tue, Apr 07, 2009 at 12:29:23PM -0400, Chuck Lever wrote:
>>>> On Apr 7, 2009, at 12:20 PM, J. Bruce Fields wrote:
>>>>> In recent kernels that information is in the "info" file for the
>>>>> given
>>>>> client in rpc_pipefs. Kevin and Olga have patches to support that
>>>>> in
>>>>> gssd, if they aren't already in upstream nfs-utils.
>>>>
>>>> The issue is we're not exactly sure what value the kernel is
>>>> passing up
>>>> in the info file. It seems to change over time.
>>>
>>> I don't understand. How does it change over time?
>>
>> Jeff observed some strange behavior while testing yesterday, so I'll
>> let him address this question.
>>
>
> I sort of had expected to see the "port" value in the info file change
> once the kernel had done the rpcbind query and set the port in the
> socket. It doesn't do this though. In hindsight I think the current
> behavior is actually correct. The port value here more or less
> represents the port that was given to the kernel at mount time.
>
>> The rules for how the kernel uses the port= setting are:
>>
>> + if port= is not specified on NFSv2/v3, port= setting is zero
>> + if port= is not specified on NFSv4, port= setting is 2049
>>
>> Then, when setting up a tranport:
>>
>> + if the port= setting is zero, do an rpcbind
>> + if the port= setting is not zero, use that value
>
> I think this patchset provides the behavior you want. For recent
> kernels that provide the port: field in the "info" file:
>
> nfsv4: port: 2049
> nfsv2/3: port: 0
>
> ...if you set port= in the mount options, then the info file displays
> that port.
>
> When we have an older kernel that does not display the port, then
> populate_port() will see that sin(6)_port is still 0. For nfsv2/3
> it'll
> do an rpcbind query to get the port, but we don't want to do that for
> NFSv4, so I added this:
>
> + /* Use standard NFS port for NFSv4 */
> + if (program == 100003 && version == 4) {
> + port = 2049;
> + goto set_port;
> + }
>
> The important bit is that the above chunk will only matter for kernels
> that don't have a port: field in the info file.

Ah. I think the comment should mention this is for supporting legacy
kernels.

> In short, I think the behavior with this set is what you've outlined
> above.

Good. Ship it ;-)

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2009-04-07 19:43:49

by Tom Talpey

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

At 01:11 PM 4/7/2009, Jeff Layton wrote:
>On Tue, 07 Apr 2009 12:27:49 -0400
>Tom Talpey <[email protected]> wrote:
>
>> At 12:02 PM 4/7/2009, Chuck Lever wrote:
>> >
>> >On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>> >> + /* Use standard NFS port for NFSv4 */
>> >> + if (program == 100003 && version == 4) {
>> >> + port = 2049;
>> >> + goto set_port;
>> >> + }
>> >
>> >I think this patch set looks pretty reasonable. Here's my one
>> >remaining quibble.
>> >
>> >You can specify "port=" for nfs4 mounts, in which case we want to use
>> >that value here, too, I think. It would be simpler overall if the
>>
>> *Must* use a port= specification. The 2049 definition is only true for
>> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
>> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
>>
>
>rpc.gssd doesn't seem to be rdma-enabled at this point. It only seems
>to handle "tcp" and "udp" in the existing code.

Fair enough. But hardwiring 2049 for all transports is going to very
problematic. What's the motivation for bypassing the rpcbind query
altogether (that "goto set_port" skips over it)? Why not at least
try the query first?

>Does libtirpc handle RDMA properly? If so, this might not be too hard
>to enable, but I'd probably rather see it in a follow on patchset (and
>maybe by someone with more of a clue about RDMA than I currently have).

No, libtirpc doesn't have any RDMA support. But, there's no need for
RDMA support in it - only NFS does RDMA, in practice, and currently
that's just in-kernel.

My concern is simply that there be a way to specify, or discover a port
that isn't 2049 here. If mount.nfs supports it, other nfs-utils should too.

Tom.


2009-04-07 19:53:05

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

On Tue, 07 Apr 2009 15:43:39 -0400
Tom Talpey <[email protected]> wrote:

> At 01:11 PM 4/7/2009, Jeff Layton wrote:
> >On Tue, 07 Apr 2009 12:27:49 -0400
> >Tom Talpey <[email protected]> wrote:
> >
> >> At 12:02 PM 4/7/2009, Chuck Lever wrote:
> >> >
> >> >On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
> >> >> + /* Use standard NFS port for NFSv4 */
> >> >> + if (program == 100003 && version == 4) {
> >> >> + port = 2049;
> >> >> + goto set_port;
> >> >> + }
> >> >
> >> >I think this patch set looks pretty reasonable. Here's my one
> >> >remaining quibble.
> >> >
> >> >You can specify "port=" for nfs4 mounts, in which case we want to use
> >> >that value here, too, I think. It would be simpler overall if the
> >>
> >> *Must* use a port= specification. The 2049 definition is only true for
> >> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
> >> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
> >>
> >
> >rpc.gssd doesn't seem to be rdma-enabled at this point. It only seems
> >to handle "tcp" and "udp" in the existing code.
>
> Fair enough. But hardwiring 2049 for all transports is going to very
> problematic. What's the motivation for bypassing the rpcbind query
> altogether (that "goto set_port" skips over it)? Why not at least
> try the query first?
>
> >Does libtirpc handle RDMA properly? If so, this might not be too hard
> >to enable, but I'd probably rather see it in a follow on patchset (and
> >maybe by someone with more of a clue about RDMA than I currently have).
>
> No, libtirpc doesn't have any RDMA support. But, there's no need for
> RDMA support in it - only NFS does RDMA, in practice, and currently
> that's just in-kernel.
>
> My concern is simply that there be a way to specify, or discover a port
> that isn't 2049 here. If mount.nfs supports it, other nfs-utils should too.
>

I forget which version is the cutoff, but newer kernels tell gssd which
port to use. That hardwiring is only for older kernels that don't send
the port in the upcall.

Could we try a short-timeout rpcbind call for those older kernels that
don't send the port? Sure. Is it worth it? I'm not as sure there.

I'd rather see these people just update their kernels so that this
isn't needed...

--
Jeff Layton <[email protected]>

2009-04-07 20:01:31

by Tom Talpey

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

At 03:53 PM 4/7/2009, Jeff Layton wrote:
>On Tue, 07 Apr 2009 15:43:39 -0400
>Tom Talpey <[email protected]> wrote:
>
>> At 01:11 PM 4/7/2009, Jeff Layton wrote:
>> >On Tue, 07 Apr 2009 12:27:49 -0400
>> >Tom Talpey <[email protected]> wrote:
>> >
>> >> At 12:02 PM 4/7/2009, Chuck Lever wrote:
>> >> >
>> >> >On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>> >> >> + /* Use standard NFS port for NFSv4 */
>> >> >> + if (program == 100003 && version == 4) {
>> >> >> + port = 2049;
>> >> >> + goto set_port;
>> >> >> + }
>> >> >
>> >> >I think this patch set looks pretty reasonable. Here's my one
>> >> >remaining quibble.
>> >> >
>> >> >You can specify "port=" for nfs4 mounts, in which case we want to use
>> >> >that value here, too, I think. It would be simpler overall if the
>> >>
>> >> *Must* use a port= specification. The 2049 definition is only true for
>> >> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
>> >> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
>> >>
>> >
>> >rpc.gssd doesn't seem to be rdma-enabled at this point. It only seems
>> >to handle "tcp" and "udp" in the existing code.
>>
>> Fair enough. But hardwiring 2049 for all transports is going to very
>> problematic. What's the motivation for bypassing the rpcbind query
>> altogether (that "goto set_port" skips over it)? Why not at least
>> try the query first?
>>
>> >Does libtirpc handle RDMA properly? If so, this might not be too hard
>> >to enable, but I'd probably rather see it in a follow on patchset (and
>> >maybe by someone with more of a clue about RDMA than I currently have).
>>
>> No, libtirpc doesn't have any RDMA support. But, there's no need for
>> RDMA support in it - only NFS does RDMA, in practice, and currently
>> that's just in-kernel.
>>
>> My concern is simply that there be a way to specify, or discover a port
>> that isn't 2049 here. If mount.nfs supports it, other nfs-utils should too.
>>
>
>I forget which version is the cutoff, but newer kernels tell gssd which
>port to use. That hardwiring is only for older kernels that don't send
>the port in the upcall.

Ah - well, if "older" is < 2.6.24, then no problem since those kernels don't
support NFS/RDMA. There are some backports to earlier kernels, but they
could address this as part of the backport.

>
>Could we try a short-timeout rpcbind call for those older kernels that
>don't send the port? Sure. Is it worth it? I'm not as sure there.

I wouldn't recommend a short timeout, arbitrary numbers rarely work
the way you might want them to. I guess my opinion is that in the
absence of any information, a standard rpcbind call is best, before
concluding a default 2049 is the only option.

>
>I'd rather see these people just update their kernels so that this
>isn't needed...

Agreed. Would it make sense to log a message when the default 2049
is used? At least then, there's a chance an admin will know it's needed.

Tom.

2009-04-07 23:14:06

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

On Tue, Apr 07, 2009 at 03:43:39PM -0400, Tom Talpey wrote:
> At 01:11 PM 4/7/2009, Jeff Layton wrote:
> >On Tue, 07 Apr 2009 12:27:49 -0400
> >Tom Talpey <[email protected]> wrote:
> >
> >> At 12:02 PM 4/7/2009, Chuck Lever wrote:
> >> >
> >> >On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
> >> >> + /* Use standard NFS port for NFSv4 */
> >> >> + if (program == 100003 && version == 4) {
> >> >> + port = 2049;
> >> >> + goto set_port;
> >> >> + }
> >> >
> >> >I think this patch set looks pretty reasonable. Here's my one
> >> >remaining quibble.
> >> >
> >> >You can specify "port=" for nfs4 mounts, in which case we want to use
> >> >that value here, too, I think. It would be simpler overall if the
> >>
> >> *Must* use a port= specification. The 2049 definition is only true for
> >> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
> >> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
> >>
> >
> >rpc.gssd doesn't seem to be rdma-enabled at this point. It only seems
> >to handle "tcp" and "udp" in the existing code.
>
> Fair enough. But hardwiring 2049 for all transports is going to very
> problematic. What's the motivation for bypassing the rpcbind query
> altogether (that "goto set_port" skips over it)? Why not at least
> try the query first?

We're just doing the rpcsec_gss context initiation. Normally that would
be done over an already-established connection--the only reason we don't
is because our implementation is split between the client and the
server, so it's more convenient for us to set up a new connection in
rpc.gssd. But we really shouldn't be doing an entirely new rpcbind
call--somebody else already did that for us and is telling us the
results through the rpc_pipefs info file.

--b.

>
> >Does libtirpc handle RDMA properly? If so, this might not be too hard
> >to enable, but I'd probably rather see it in a follow on patchset (and
> >maybe by someone with more of a clue about RDMA than I currently have).
>
> No, libtirpc doesn't have any RDMA support. But, there's no need for
> RDMA support in it - only NFS does RDMA, in practice, and currently
> that's just in-kernel.
>
> My concern is simply that there be a way to specify, or discover a port
> that isn't 2049 here. If mount.nfs supports it, other nfs-utils should too.
>
> Tom.
>
> _______________________________________________
> NFSv4 mailing list
> [email protected]
> http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

2009-04-07 23:37:30

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

On Tue, 7 Apr 2009 19:14:06 -0400
"J. Bruce Fields" <[email protected]> wrote:

> On Tue, Apr 07, 2009 at 03:43:39PM -0400, Tom Talpey wrote:
> > At 01:11 PM 4/7/2009, Jeff Layton wrote:
> > >On Tue, 07 Apr 2009 12:27:49 -0400
> > >Tom Talpey <[email protected]> wrote:
> > >
> > >> At 12:02 PM 4/7/2009, Chuck Lever wrote:
> > >> >
> > >> >On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
> > >> >> + /* Use standard NFS port for NFSv4 */
> > >> >> + if (program == 100003 && version == 4) {
> > >> >> + port = 2049;
> > >> >> + goto set_port;
> > >> >> + }
> > >> >
> > >> >I think this patch set looks pretty reasonable. Here's my one
> > >> >remaining quibble.
> > >> >
> > >> >You can specify "port=" for nfs4 mounts, in which case we want to use
> > >> >that value here, too, I think. It would be simpler overall if the
> > >>
> > >> *Must* use a port= specification. The 2049 definition is only true for
> > >> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
> > >> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
> > >>
> > >
> > >rpc.gssd doesn't seem to be rdma-enabled at this point. It only seems
> > >to handle "tcp" and "udp" in the existing code.
> >
> > Fair enough. But hardwiring 2049 for all transports is going to very
> > problematic. What's the motivation for bypassing the rpcbind query
> > altogether (that "goto set_port" skips over it)? Why not at least
> > try the query first?
>
> We're just doing the rpcsec_gss context initiation. Normally that would
> be done over an already-established connection--the only reason we don't
> is because our implementation is split between the client and the
> server, so it's more convenient for us to set up a new connection in
> rpc.gssd. But we really shouldn't be doing an entirely new rpcbind
> call--somebody else already did that for us and is telling us the
> results through the rpc_pipefs info file.
>

Technically, the info file doesn't display the port from an rpcbind
call. That field is populated before the kernel's rpcbind call is
done. The contents will be one of:

- the value of the port= mount option if one was specified
- 2049 if it's an nfs4 mount and there is no port= option specified
- 0 if nfsv2/3 and no port= option was specified

...its real value is in allowing gssd to skip the rpcbind call if
someone specified port= when doing the mount. This could probably
be changed, but I'm not sure it's such a big deal. It only means an
extra rpcbind call at mount time for NFSv2/3. Further upcalls for
other creds by the same client don't incur extra rpcbind queries.

--
Jeff Layton <[email protected]>

2009-04-08 19:32:43

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

On Apr 7, 2009, at 7:14 PM, J. Bruce Fields wrote:
> On Tue, Apr 07, 2009 at 03:43:39PM -0400, Tom Talpey wrote:
>> At 01:11 PM 4/7/2009, Jeff Layton wrote:
>>> On Tue, 07 Apr 2009 12:27:49 -0400
>>> Tom Talpey <[email protected]> wrote:
>>>
>>>> At 12:02 PM 4/7/2009, Chuck Lever wrote:
>>>>>
>>>>> On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>>>>>> + /* Use standard NFS port for NFSv4 */
>>>>>> + if (program == 100003 && version == 4) {
>>>>>> + port = 2049;
>>>>>> + goto set_port;
>>>>>> + }
>>>>>
>>>>> I think this patch set looks pretty reasonable. Here's my one
>>>>> remaining quibble.
>>>>>
>>>>> You can specify "port=" for nfs4 mounts, in which case we want
>>>>> to use
>>>>> that value here, too, I think. It would be simpler overall if the
>>>>
>>>> *Must* use a port= specification. The 2049 definition is only
>>>> true for
>>>> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
>>>> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
>>>>
>>>
>>> rpc.gssd doesn't seem to be rdma-enabled at this point. It only
>>> seems
>>> to handle "tcp" and "udp" in the existing code.
>>
>> Fair enough. But hardwiring 2049 for all transports is going to very
>> problematic. What's the motivation for bypassing the rpcbind query
>> altogether (that "goto set_port" skips over it)? Why not at least
>> try the query first?
>
> We're just doing the rpcsec_gss context initiation. Normally that
> would
> be done over an already-established connection--the only reason we
> don't
> is because our implementation is split between the client and the
> server, so it's more convenient for us to set up a new connection in
> rpc.gssd. But we really shouldn't be doing an entirely new rpcbind
> call--somebody else already did that for us and is telling us the
> results through the rpc_pipefs info file.

This is an entirely separate RPC transport being set up for gssd, and
there seem to be a lot of cases where the kernel (rightfully) passes a
zero for the port number. In fact, even if the kernel did the rpcbind
for gssd at some point in the past, gss doesn't have any information
about how old that information is.

So, yes, we do want an rpcbind here.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2009-04-09 16:19:49

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

On Wed, Apr 08, 2009 at 03:32:43PM -0400, Chuck Lever wrote:
> On Apr 7, 2009, at 7:14 PM, J. Bruce Fields wrote:
>> On Tue, Apr 07, 2009 at 03:43:39PM -0400, Tom Talpey wrote:
>>> At 01:11 PM 4/7/2009, Jeff Layton wrote:
>>>> On Tue, 07 Apr 2009 12:27:49 -0400
>>>> Tom Talpey <[email protected]> wrote:
>>>>
>>>>> At 12:02 PM 4/7/2009, Chuck Lever wrote:
>>>>>>
>>>>>> On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>>>>>>> + /* Use standard NFS port for NFSv4 */
>>>>>>> + if (program == 100003 && version == 4) {
>>>>>>> + port = 2049;
>>>>>>> + goto set_port;
>>>>>>> + }
>>>>>>
>>>>>> I think this patch set looks pretty reasonable. Here's my one
>>>>>> remaining quibble.
>>>>>>
>>>>>> You can specify "port=" for nfs4 mounts, in which case we want
>>>>>> to use
>>>>>> that value here, too, I think. It would be simpler overall if the
>>>>>
>>>>> *Must* use a port= specification. The 2049 definition is only
>>>>> true for
>>>>> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
>>>>> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
>>>>>
>>>>
>>>> rpc.gssd doesn't seem to be rdma-enabled at this point. It only
>>>> seems
>>>> to handle "tcp" and "udp" in the existing code.
>>>
>>> Fair enough. But hardwiring 2049 for all transports is going to very
>>> problematic. What's the motivation for bypassing the rpcbind query
>>> altogether (that "goto set_port" skips over it)? Why not at least
>>> try the query first?
>>
>> We're just doing the rpcsec_gss context initiation. Normally that
>> would
>> be done over an already-established connection--the only reason we
>> don't
>> is because our implementation is split between the client and the
>> server, so it's more convenient for us to set up a new connection in
>> rpc.gssd. But we really shouldn't be doing an entirely new rpcbind
>> call--somebody else already did that for us and is telling us the
>> results through the rpc_pipefs info file.
>
> This is an entirely separate RPC transport being set up for gssd, and
> there seem to be a lot of cases where the kernel (rightfully) passes a
> zero for the port number. In fact, even if the kernel did the rpcbind
> for gssd at some point in the past, gss doesn't have any information
> about how old that information is.
>
> So, yes, we do want an rpcbind here.

As long as we don't do it in the NFSv4 case, I'm happy.

My only point is that the above argument is putting the cart before the
horse a bit: the fact that there's an entirely separate RPC transport
being set up is entirely an artifact of our implementation, and if it
became a problem at some point, then we'd need to consider modifying the
kernel/gssd interface to eliminate the need for that.

--b.

2009-04-09 17:34:14

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH 3/5] nfs-utils: query for remote port using rpcbind instead of getaddrinfo

Hi Bruce-

On Apr 9, 2009, at 12:19 PM, J. Bruce Fields wrote:
> On Wed, Apr 08, 2009 at 03:32:43PM -0400, Chuck Lever wrote:
>> On Apr 7, 2009, at 7:14 PM, J. Bruce Fields wrote:
>>> On Tue, Apr 07, 2009 at 03:43:39PM -0400, Tom Talpey wrote:
>>>> At 01:11 PM 4/7/2009, Jeff Layton wrote:
>>>>> On Tue, 07 Apr 2009 12:27:49 -0400
>>>>> Tom Talpey <[email protected]> wrote:
>>>>>
>>>>>> At 12:02 PM 4/7/2009, Chuck Lever wrote:
>>>>>>>
>>>>>>> On Apr 7, 2009, at 11:25 AM, Jeff Layton wrote:
>>>>>>>> + /* Use standard NFS port for NFSv4 */
>>>>>>>> + if (program == 100003 && version == 4) {
>>>>>>>> + port = 2049;
>>>>>>>> + goto set_port;
>>>>>>>> + }
>>>>>>>
>>>>>>> I think this patch set looks pretty reasonable. Here's my one
>>>>>>> remaining quibble.
>>>>>>>
>>>>>>> You can specify "port=" for nfs4 mounts, in which case we want
>>>>>>> to use
>>>>>>> that value here, too, I think. It would be simpler overall if
>>>>>>> the
>>>>>>
>>>>>> *Must* use a port= specification. The 2049 definition is only
>>>>>> true for
>>>>>> NFSv4/TCP, as a counterexample the NFSv4/RDMA IANA binding is
>>>>>> port 20049. So slamming the port to 2049 would break NFSv4/RDMA.
>>>>>>
>>>>>
>>>>> rpc.gssd doesn't seem to be rdma-enabled at this point. It only
>>>>> seems
>>>>> to handle "tcp" and "udp" in the existing code.
>>>>
>>>> Fair enough. But hardwiring 2049 for all transports is going to
>>>> very
>>>> problematic. What's the motivation for bypassing the rpcbind query
>>>> altogether (that "goto set_port" skips over it)? Why not at least
>>>> try the query first?
>>>
>>> We're just doing the rpcsec_gss context initiation. Normally that
>>> would
>>> be done over an already-established connection--the only reason we
>>> don't
>>> is because our implementation is split between the client and the
>>> server, so it's more convenient for us to set up a new connection in
>>> rpc.gssd. But we really shouldn't be doing an entirely new rpcbind
>>> call--somebody else already did that for us and is telling us the
>>> results through the rpc_pipefs info file.
>>
>> This is an entirely separate RPC transport being set up for gssd, and
>> there seem to be a lot of cases where the kernel (rightfully)
>> passes a
>> zero for the port number. In fact, even if the kernel did the
>> rpcbind
>> for gssd at some point in the past, gss doesn't have any information
>> about how old that information is.
>>
>> So, yes, we do want an rpcbind here.
>
> As long as we don't do it in the NFSv4 case, I'm happy.

As far as I understand it, the kernel won't send up a zero for NFSv4
unless the mount command line says "port=0" which forces an rpcbind
for each transport we use to connect to that server.

For NFSv2/v3, however, not specifying port= on the mount command line
means the kernel will send up zero in most common cases, I think.
Using /etc/services on the client to discover the server's NFSD port
means that if the server has registered the NFS service on some other
port, specifying "port=" will be required on the mount command line
for gssd to work, even though rpcbind on the server is advertising the
correct port already.

> My only point is that the above argument is putting the cart before
> the
> horse a bit: the fact that there's an entirely separate RPC transport
> being set up is entirely an artifact of our implementation, and if it
> became a problem at some point, then we'd need to consider modifying
> the
> kernel/gssd interface to eliminate the need for that.


port=0 has a specific well-defined meaning for RPC, which is "do an
rpcbind". If you really think you need to distinguish between "use
this port number," "do an rpcbind," "use the standard port for this
transport," and "consult /etc/services" then I don't think port=0 is
adequate.

Should we even bother to consult /etc/services for NFSv4, since that
port value is required by standard? nfs4mount.c doesn't look at /etc/
services, for example. So today, gssd looks in /etc/services,
nfs4mount.c and the kernel hard-code the port number.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com