referrals

An attempt to follow an nfsv4 referral is leading to a hang. I'm doing an "ls" on the absent directory. A network trace shows the server returning with a sane-looking response to the getattr of fs_locations. I've appended the part of the sysrq-t trace for "ls". Any ideas?

--b.

Quoted 6 lines · J. Bruce Fields

An attempt to follow an nfsv4 referral is leading to a hang. I'm doing an "ls" on the absent directory. A network trace shows the server returning with a sane-looking response to the getattr of fs_locations. I've appended the part of the sysrq-t trace for "ls". Any ideas?

--b.

What kernel?

Quoted 6 lines · Trond Myklebust

Quoted 5 lines · J. Bruce Fields

An attempt to follow an nfsv4 referral is leading to a hang. I'm doing an "ls" on the absent directory. A network trace shows the server returning with a sane-looking response to the getattr of fs_locations. I've appended the part of the sysrq-t trace for "ls". Any ideas?

Quoted 3 lines

Quoted 1 line

--b.

What kernel?

It was a few unrelated nfsd and gss patches on top of e31a94ed371c70855eb30b77c490d6d85dd4da26, which is between 2.6.25 and 2.6.26-rc1 (but I think has all the nfs stuff that went into -rc1). Happy to retest with something different.

Quoted 7 lines · bfields

Quoted 6 lines · Trond Myklebust

Quoted 5 lines · J. Bruce Fields

An attempt to follow an nfsv4 referral is leading to a hang. I'm doing an "ls" on the absent directory. A network trace shows the server returning with a sane-looking response to the getattr of fs_locations. I've appended the part of the sysrq-t trace for "ls". Any ideas?

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

--b.

What kernel?

It was a few unrelated nfsd and gss patches on top of e31a94ed371c70855eb30b77c490d6d85dd4da26, which is between 2.6.25 and 2.6.26-rc1 (but I think has all the nfs stuff that went into -rc1). Happy to retest with something different.

Ah-hah. The server was returning two fslocations records, both for the same server--one with the target server's ip address, one with a hostname for it. Once the server was modified to return only one record (with the ip address), everything worked.

Of course it's a known limitation that the client only handles ip addresses, but a look at the code in fs/nfs/nfs4namespace.c:nfs_follow_referral() shows that it *tries* to skip over any non-ip addresses, so the presence of such shouldn't have changed behavior.

So there must be something wrong with that code in the !valid_ipaddr4() case?

From: J. Bruce Fields <[email protected]>

The code incorrectly assumes here that the server name (or ip address) is null-terminated. This can cause referrals to fail in some cases.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfs/nfs4namespace.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

I think this was the bug causing the referral failures. There must be a less ugly solution, though.

--b.

fs/nfs/nfs4namespace.c +1 -1

index 5f9ba41..2684c65 100644
@@ -96,11 +96,17 @@ static int nfs4_validate_fspath(const struct vfsmount *mnt_parent,
 /*
  * Check if the string represents a "valid" IPv4 address
  */
-static inline int valid_ipaddr4(const char *buf)
+static inline int valid_ipaddr4(const struct nfs4_string *buf)

fs/nfs/nfs4namespace.c +2 -1

+	rc = sscanf(buf->data, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
 	if (rc != 4)
 		return -EINVAL;
 	for (count = 0; count < 4; count++) {

@@ -178,7 +184,7 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,
 			};
 
 			if (location->servers[s].len <= 0 ||
-			    valid_ipaddr4(location->servers[s].data) < 0) {
+			    valid_ipaddr4(&location->servers[s]) < 0) {
 				s++;
 				continue;
 			}

Quoted 28 lines · J. Bruce Fields

From: J. Bruce Fields <[email protected]>

The code incorrectly assumes here that the server name (or ip address) is null-terminated. This can cause referrals to fail in some cases.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfs/nfs4namespace.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

I think this was the bug causing the referral failures. There must be a less ugly solution, though.

--b.

fs/nfs/nfs4namespace.c +1 -1

index 5f9ba41..2684c65 100644
@@ -96,11 +96,17 @@ static int nfs4_validate_fspath(const struct vfsmount *mnt_parent,
 /*
  * Check if the string represents a "valid" IPv4 address
  */
-static inline int valid_ipaddr4(const char *buf)
+static inline int valid_ipaddr4(const struct nfs4_string *buf)
 {
 	int rc, count, in[4];
 

fs/nfs/nfs4namespace.c static int nfs4_validate_fspath(const struct vfsmount *mnt_parent, · line 108

Quoted 7 lines

fs/nfs/nfs4namespace.c +6 -1

-	rc = sscanf(buf, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
+	/*
+	 * XXX: Depending on the knowledge that the following byte is
+	 * either xdr padding or an xdr length that has already been
+	 * copied:
+	 */
+	buf->data[buf->len] = '\0';

Hmm, can't that overflow if buf->len is word aligned?

Benny

Quoted 13 lines

fs/nfs/nfs4namespace.c +2 -1

+	rc = sscanf(buf->data, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
 	if (rc != 4)
 		return -EINVAL;
 	for (count = 0; count < 4; count++) {

@@ -178,7 +184,7 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,
 			};
 
 			if (location->servers[s].len <= 0 ||
-			    valid_ipaddr4(location->servers[s].data) < 0) {
+			    valid_ipaddr4(&location->servers[s]) < 0) {
 				s++;
 				continue;
 			}

Signature · 10 lines

Benny Halevy
Software Architect
Tel/Fax: +972-3-647-8340
Mobile: +972-54-802-8340
US:      +1-412-203-3187
[email protected]
 
Panasas, Inc.
Leader in Parallel Storage
www.panasas.com

From: J. Bruce Fields <[email protected]>

The code incorrectly assumes here that the server name (or ip address) is null-terminated. This can cause referrals to fail in some cases.

Signed-off-by J. Bruce Fields <[email protected]>

---
fs/nfs/nfs4namespace.c | 12 +++++++++---

1 files changed, 9 insertions(+), 3 deletions(-)

I think this was the bug causing the referral failures. There must be a less ugly solution, though.

--b.

fs/nfs/nfs4namespace.c +8 -2

index 5f9ba41..2684c65 100644
@@ -96,11 +96,17 @@ static int nfs4_validate_fspath(const struct  
 vfsmount *mnt_parent,
 /*
 * Check if the string represents a "valid" IPv4 address
 */
-static inline int valid_ipaddr4(const char *buf)
+static inline int valid_ipaddr4(const struct nfs4_string *buf)
 {
 	int rc, count, in[4];
 
-	rc = sscanf(buf, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
+	/*
+	 * XXX: Depending on the knowledge that the following byte is
+	 * either xdr padding or an xdr length that has already been
+	 * copied:
+	 */
+	buf->data[buf->len] = '\0';
+	rc = sscanf(buf->data, "%d.%d.%d.%d", &in[0], &in[1], &in[2],  
 &in[3]);
 	if (rc != 4)

		return -EINVAL;
	for (count = 0; count < 4; count++) {

fs/nfs/nfs4namespace.c +1 -1

@@ -178,7 +184,7 @@ static struct vfsmount  
 *nfs_follow_referral(const struct vfsmount *mnt_parent,
 			};
 
 			if (location->servers[s].len <= 0 ||
-			    valid_ipaddr4(location->servers[s].data) < 0) {
+			    valid_ipaddr4(&location->servers[s]) < 0) {
 				s++;
 				continue;

			}

Signature · 6 lines

1.5.5.rc1

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Should you use in4_pton() instead?

Quoted 1 line · Chuck Lever

Should you use in4_pton() instead?

Can we rather convert this to use nfs_parse_server_address? We don't need 10 different ways to parse text addresses...

Quoted 7 lines · Trond Myklebust

Quoted 2 lines · Chuck Lever

Should you use in4_pton() instead?

Can we rather convert this to use nfs_parse_server_address? We don't need 10 different ways to parse text addresses...

I'm OK with that, as long as there isn't a technical problem with using in4_pton().

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 6 lines · Chuck Lever

Quoted 5 lines · Trond Myklebust

Quoted 2 lines · Chuck Lever

Should you use in4_pton() instead?

Quoted 5 lines

Quoted 2 lines

Can we rather convert this to use nfs_parse_server_address? We don't need 10 different ways to parse text addresses...

I'm OK with that, as long as there isn't a technical problem with using in4_pton().

nfs_parse_server_address() uses in4_pton(), it just also groks ipv6.

From 530b441f2239d8bcedf9456c3c570d9c179cb406 Mon Sep 17 00:00:00 2001

Quoted 13 lines · david m. richter

Quoted 11 lines · Chuck Lever

Quoted 7 lines · Trond Myklebust

Quoted 2 lines · Chuck Lever

Should you use in4_pton() instead?

Can we rather convert this to use nfs_parse_server_address? We don't need 10 different ways to parse text addresses...

I'm OK with that, as long as there isn't a technical problem with using in4_pton().

Quoted 1 line

nfs_parse_server_address() uses in4_pton(), it just also groks ipv6.

This is all a bit orthogonal to the actual bug, as all those functions want null-terminated strings too.

We could apply the below (compile-tested only) and then add ipv6 support and converting to nfs_parse_server_address() in a subsequent patch.

Quoted 13 lines · david m. richter

Quoted 11 lines · Chuck Lever

Quoted 7 lines · Trond Myklebust

Quoted 2 lines · Chuck Lever

Should you use in4_pton() instead?

Can we rather convert this to use nfs_parse_server_address? We don't need 10 different ways to parse text addresses...

I'm OK with that, as long as there isn't a technical problem with using in4_pton().

Quoted 1 line

nfs_parse_server_address() uses in4_pton(), it just also groks ipv6.

This is all a bit orthogonal to the actual bug, as all those functions want null-terminated strings too.

We could apply the below (compile-tested only) and then add ipv6 support and converting to nfs_parse_server_address() in a subsequent patch.

From 530b441f2239d8bcedf9456c3c570d9c179cb406 Mon Sep 17 00:00:00 2001
From: J. Bruce Fields <[email protected]>
Date: Fri, 9 May 2008 15:10:56 -0700
Subject: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

The code incorrectly assumes here that the server name (or ip address) is null-terminated. This can cause referrals to fail in some cases.

Signed-off-by J. Bruce Fields <[email protected]>

---
fs/nfs/nfs4namespace.c | 13 ++++++++++---

1 files changed, 10 insertions(+), 3 deletions(-)

fs/nfs/nfs4namespace.c +9 -2

index 5f9ba41..40a0209 100644
@@ -93,14 +93,21 @@ static int nfs4_validate_fspath(const struct  
 vfsmount *mnt_parent,
 	return 0;
 }
 
+#define MAX_IPADDR_STRLEN 40
 /*
 * Check if the string represents a "valid" IPv4 address
 */
-static inline int valid_ipaddr4(const char *buf)
+static inline int valid_ipaddr4(const struct nfs4_string *buf)
 {
 	int rc, count, in[4];
+	char str[MAX_IPADDR_STRLEN];
 
-	rc = sscanf(buf, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
+	if (buf->len >= MAX_IPADDR_STRLEN)
+		return -EINVAL;
+	memcpy(str, buf->data, buf->len);
+	str[buf->len] = '\0';
+
+	rc = sscanf(str, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
 	if (rc != 4)
 		return -EINVAL;

	for (count = 0; count < 4; count++) {

fs/nfs/nfs4namespace.c +1 -1

@@ -178,7 +185,7 @@ static struct vfsmount  
 *nfs_follow_referral(const struct vfsmount *mnt_parent,
 			};
 
 			if (location->servers[s].len <= 0 ||
-			    valid_ipaddr4(location->servers[s].data) < 0) {
+			    valid_ipaddr4(&location->servers[s]) < 0) {
 				s++;
 				continue;

			}

Signature · 1 line

1.5.5.rc1

Quoted 22 lines · J. Bruce Fields

Quoted 12 lines · david m. richter

Quoted 10 lines · Chuck Lever

Quoted 8 lines · Trond Myklebust

Quoted 2 lines · Chuck Lever

Should you use in4_pton() instead?

Can we rather convert this to use nfs_parse_server_address? We don't
need 10 different ways to parse text addresses...

Quoted 5 lines

Quoted 3 lines

I'm OK with that, as long as there isn't a technical problem with using
in4_pton().

nfs_parse_server_address() uses in4_pton(), it just also groks ipv6.

This is all a bit orthogonal to the actual bug, as all those functions want null-terminated strings too.

Quoted 3 lines

We could apply the below (compile-tested only) and then add ipv6 support
and converting to nfs_parse_server_address() in a subsequent patch.

I'm looking at this code for other reasons, but it would be very easy to teach nfs_parse_server_address() to take a string length and not assume the passed-in address string is null-terminated. Both in4_pton and in6_pton will take a string length.

Quoted 23 lines · Chuck Lever

Quoted 22 lines · J. Bruce Fields

Quoted 12 lines · david m. richter

Quoted 10 lines · Chuck Lever

Quoted 8 lines · Trond Myklebust

Quoted 2 lines · Chuck Lever

Should you use in4_pton() instead?

Can we rather convert this to use nfs_parse_server_address? We don't
need 10 different ways to parse text addresses...

Quoted 5 lines

Quoted 3 lines

I'm OK with that, as long as there isn't a technical problem with using
in4_pton().

nfs_parse_server_address() uses in4_pton(), it just also groks ipv6.

This is all a bit orthogonal to the actual bug, as all those functions want null-terminated strings too.

Quoted 8 lines

Quoted 3 lines

We could apply the below (compile-tested only) and then add ipv6 support
and converting to nfs_parse_server_address() in a subsequent patch.

I'm looking at this code for other reasons, but it would be very easy to teach nfs_parse_server_address() to take a string length and not assume the passed-in address string is null-terminated. Both in4_pton and in6_pton will take a string length.

Whoops, I missed the srclen argument to in4_pton and in6_pton.

Though I just noticed it doesn't really matter much, since the mountdata.hostname needs a null-terminated string.

commit 109f9a666db58e0511ac5a417e767027b148a9e0

Author: J. Bruce Fields <[email protected]>
Date:   Fri May 9 15:10:56 2008 -0700

nfs: Fix misparsing of nfsv4 fs_locations attribute

The code incorrectly assumes here that the server name (or ip
address)
is null-terminated. This can cause referrals to fail in some
cases.

Signed-off-by J. Bruce Fields <[email protected]>

fs/nfs/nfs4namespace.c +0 -17

index 5f9ba41..018292d 100644
@@ -93,23 +93,6 @@ static int nfs4_validate_fspath(const struct  
 vfsmount *mnt_parent,
 	return 0;
 }
 
-/*
- * Check if the string represents a "valid" IPv4 address
- */
-static inline int valid_ipaddr4(const char *buf)
-{
-	int rc, count, in[4];
-
-	rc = sscanf(buf, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
-	if (rc != 4)
-		return -EINVAL;
-	for (count = 0; count < 4; count++) {
-		if (in[count] > 255)
-			return -EINVAL;
-	}
-	return 0;
-}
-
 /**
 * nfs_follow_referral - set up mountpoint when hitting a referral  

on moved error
* @mnt_parent - mountpoint of parent directory

fs/nfs/nfs4namespace.c +10 -7

@@ -172,19 +155,20 @@ static struct vfsmount  
 *nfs_follow_referral(const struct vfsmount *mnt_parent,
 
 		s = 0;
 		while (s < location->nservers) {
+			const struct nfs4_string *buf = &location->servers[s];
 			struct sockaddr_in addr = {
 				.sin_family	= AF_INET,
 				.sin_port	= htons(NFS_PORT),
 			};
+			u8 *ip = (u8 *)addr.sin_addr.s_addr;
 
-			if (location->servers[s].len <= 0 ||
-			    valid_ipaddr4(location->servers[s].data) < 0) {
-				s++;
-				continue;
-			}
+			if (buf->len <= 0 || buf->len >= PAGE_SIZE)
+				goto next;
+			if (!in4_pton(buf->data, buf->len, ip, '\0', NULL))
+				goto next;
 
-			mountdata.hostname = location->servers[s].data;
-			addr.sin_addr.s_addr = in_aton(mountdata.hostname),
+			mountdata.hostname = kmalloc(buf->len + 1, GFP_KERNEL);
+			mountdata.hostname[buf->len] = 0;
 			mountdata.addr = (struct sockaddr *)&addr;
 			mountdata.addrlen = sizeof(addr);

@@ -193,9 +177,11 @@ static struct vfsmount  
 *nfs_follow_referral(const struct vfsmount *mnt_parent,
 					mountdata.mnt_path);
 
 			mnt = vfs_kern_mount(&nfs4_referral_fs_type, 0, page, &mountdata);
+			kfree(mountdata.hostname);
 			if (!IS_ERR(mnt)) {
 				break;
 			}
+next:
 			s++;
 		}

		loc++;

So, I haven't added IPv6 support in this area because it seems to assume an IPv4 address here, and it needs to handle the case where this might be a hostname (ie there should be a DNS resolution in this path).

If you're going to call in4_pton(), it really should call nfs_parse_server_address() instead; then this path would magically support IPv6 addresses as well.

But in the long run, this path will need either an upcall or it will have to use the key-based kernel DNS resolution facility (Jeff Layton mentioned something like this over lunch last week).

Quoted 5 lines · Chuck Lever

So, I haven't added IPv6 support in this area because it seems to assume an IPv4 address here, and it needs to handle the case where this might be a hostname (ie there should be a DNS resolution in this path).

Quoted 7 lines

If you're going to call in4_pton(), it really should call nfs_parse_server_address() instead; then this path would magically support IPv6 addresses as well.

But in the long run, this path will need either an upcall or it will have to use the key-based kernel DNS resolution facility (Jeff Layton mentioned something like this over lunch last week).

No. The correct way to do this is to replace the current code fs_locations handler so that we do the mount from userspace. In addition to voiding the whole issue of in-kernel DNS resolution, that also has the benefit that it allows us to put policy decisions like which server to mount from under administrator control in userspace.

I've already started coding up a solution.

I assume this new mechanism is intended to handle IPv6 addresses and DNS resolution correctly because it will invoke mount.nfs.