[PATCH 0/4] RFC: raw IPv6 address parsing in NFS client

[PATCH 0/4] RFC: raw IPv6 address parsing in NFS client

[Chuck Lever]

Hi-

I'm interested in some review of the following four patches which add to
the kernel's NFS client the ability to parse IPv6 addresses in presentation
format.

Namely, it adds the following:

1.  If the user passes in an IPv6 address as the server name, the colons
    in the address will confuse the logic that splits the device name
    into a server hostname and an export path.   We'll use square brackets
    around IPv6 server addresses to "escape" the colons, as does Solaris.

2.  If the user passes in a link-local IPv6 address as the server name,
    an interface index is also necessary.  We'll use the "%id" suffix on
    the address to pass in the index, and plant that in the sockaddr's
    sin6_scope_id field.

In addition to the following patches in email, a git repo with these
same patches already applied can be found here:

	linux-nfs.org:exports/cel-2.6.git

The basic questions:

Are these reasonable conventions to follow?  Is the parsing logic adequate?
Is there anything I'm forgetting?

Thanks for any review.

---

Chuck Lever (4):
      NFS: handle interface identifiers in incoming IPv6 addresses
      NFS: Add string length argument to nfs_parse_server_address
      NFS: Support raw IPv6 address hostnames during NFS mount operation
      NFS: Use common device name parsing logic for NFSv4 and NFSv2/v3


 fs/nfs/super.c |  311 ++++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 236 insertions(+), 75 deletions(-)

[PATCH 1/4] NFS: Use common device name parsing logic for NFSv4 and NFSv2/v3

[Chuck Lever]

To support passing a raw IPv6 address as a server hostname, we need to
expand the logic that handles splitting the passed-in device name into
a server hostname and export path

Start by pulling device name parsing out of the mount option validation
functions and into separate helper functions.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfs/super.c |  124 ++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 79 insertions(+), 45 deletions(-)


diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 26d64cc..5f299ae 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1211,6 +1211,67 @@ static int nfs_try_mount(struct nfs_parsed_mount_data *args,
 }
 
 /*
+ * Split "dev_name" into "hostname:export_path".
+ *
+ * Note: caller frees hostname and export path, even on error.
+ */
+static int nfs_parse_devname(const char *dev_name,
+			     char **hostname, size_t maxnamlen,
+			     char **export_path, size_t maxpathlen)
+{
+	size_t len;
+	char *colon, *comma;
+
+	colon = strchr(dev_name, ':');
+	if (colon == NULL)
+		goto out_bad_devname;
+
+	len = colon - dev_name;
+	if (len > maxnamlen)
+		goto out_hostname;
+
+	/* N.B. caller will free nfs_server.hostname in all cases */
+	*hostname = kstrndup(dev_name, len, GFP_KERNEL);
+	if (!*hostname)
+		goto out_nomem;
+
+	/* kill possible hostname list: not supported */
+	comma = strchr(*hostname, ',');
+	if (comma != NULL) {
+		if (comma == *hostname)
+			goto out_bad_devname;
+		*comma = '\0';
+	}
+
+	colon++;
+	len = strlen(colon);
+	if (len > maxpathlen)
+		goto out_path;
+	*export_path = kstrndup(colon, len, GFP_KERNEL);
+	if (!*export_path)
+		goto out_nomem;
+
+	dfprintk(MOUNT, "NFS: MNTPATH: '%s'\n", *export_path);
+	return 0;
+
+out_bad_devname:
+	dfprintk(MOUNT, "NFS: device name not in host:path format\n");
+	return -EINVAL;
+
+out_nomem:
+	dfprintk(MOUNT, "NFS: not enough memory to parse device name\n");
+	return -ENOMEM;
+
+out_hostname:
+	dfprintk(MOUNT, "NFS: server hostname too long\n");
+	return -ENAMETOOLONG;
+
+out_path:
+	dfprintk(MOUNT, "NFS: export pathname too long\n");
+	return -ENAMETOOLONG;
+}
+
+/*
  * Validate the NFS2/NFS3 mount data
  * - fills in the mount root filehandle
  *
@@ -1336,8 +1397,6 @@ static int nfs_validate_mount_data(void *options,
 
 		break;
 	default: {
-		unsigned int len;
-		char *c;
 		int status;
 
 		if (nfs_parse_mount_options((char *)options, args) == 0)
@@ -1347,21 +1406,17 @@ static int nfs_validate_mount_data(void *options,
 						&args->nfs_server.address))
 			goto out_no_address;
 
-		c = strchr(dev_name, ':');
-		if (c == NULL)
-			return -EINVAL;
-		len = c - dev_name;
-		/* N.B. caller will free nfs_server.hostname in all cases */
-		args->nfs_server.hostname = kstrndup(dev_name, len, GFP_KERNEL);
-		if (!args->nfs_server.hostname)
-			goto out_nomem;
+		status = nfs_parse_devname(dev_name,
+					   &args->nfs_server.hostname,
+					   PAGE_SIZE,
+					   &args->nfs_server.export_path,
+					   NFS_MAXPATHLEN);
+		if (!status)
+			status = nfs_try_mount(args, mntfh);
 
-		c++;
-		if (strlen(c) > NFS_MAXPATHLEN)
-			return -ENAMETOOLONG;
-		args->nfs_server.export_path = c;
+		kfree(args->nfs_server.export_path);
+		args->nfs_server.export_path = NULL;
 
-		status = nfs_try_mount(args, mntfh);
 		if (status)
 			return status;
 
@@ -1888,7 +1943,7 @@ static int nfs4_validate_mount_data(void *options,
 
 		break;
 	default: {
-		unsigned int len;
+		int status;
 
 		if (nfs_parse_mount_options((char *)options, args) == 0)
 			return -EINVAL;
@@ -1907,34 +1962,17 @@ static int nfs4_validate_mount_data(void *options,
 			goto out_inval_auth;
 		}
 
-		/*
-		 * Split "dev_name" into "hostname:mntpath".
-		 */
-		c = strchr(dev_name, ':');
-		if (c == NULL)
-			return -EINVAL;
-		/* while calculating len, pretend ':' is '\0' */
-		len = c - dev_name;
-		if (len > NFS4_MAXNAMLEN)
-			return -ENAMETOOLONG;
-		/* N.B. caller will free nfs_server.hostname in all cases */
-		args->nfs_server.hostname = kstrndup(dev_name, len, GFP_KERNEL);
-		if (!args->nfs_server.hostname)
-			goto out_nomem;
-
-		c++;			/* step over the ':' */
-		len = strlen(c);
-		if (len > NFS4_MAXPATHLEN)
-			return -ENAMETOOLONG;
-		args->nfs_server.export_path = kstrndup(c, len, GFP_KERNEL);
-		if (!args->nfs_server.export_path)
-			goto out_nomem;
-
-		dprintk("NFS: MNTPATH: '%s'\n", args->nfs_server.export_path);
-
 		if (args->client_address == NULL)
 			goto out_no_client_address;
 
+		status = nfs_parse_devname(dev_name,
+					   &args->nfs_server.hostname,
+					   NFS4_MAXNAMLEN,
+					   &args->nfs_server.export_path,
+					   NFS4_MAXPATHLEN);
+		if (status < 0)
+			return status;
+
 		break;
 		}
 	}
@@ -1950,10 +1988,6 @@ out_inval_auth:
 		 data->auth_flavourlen);
 	return -EINVAL;
 
-out_nomem:
-	dfprintk(MOUNT, "NFS4: not enough memory to handle mount options\n");
-	return -ENOMEM;
-
 out_no_address:
 	dfprintk(MOUNT, "NFS4: mount program didn't pass remote address\n");
 	return -EINVAL;

[PATCH 2/4] NFS: Support raw IPv6 address hostnames during NFS mount operation

[Chuck Lever]

Traditionally the mount command has looked for a ":" to separate the
server's hostname from the export path in the mounted on device name,
like this:

	mount server:/export /mounted/on/dir

The server's hostname is "server" and the export path is "/export".

You can also substitute a specific IPv4 network address for the server
hostname, like this:

	mount 192.168.0.55:/export /mounted/on/dir

Raw IPv6 addresses present a problem, however, because they look
something like this:

	fe80::200:5aff:fe00:30b

Note the use of colons.

To get around the presence of colons, copy the Solaris convention used for
mounting IPv6 servers by address: wrap a raw IPv6 address with square
brackets.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfs/super.c |   90 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 82 insertions(+), 8 deletions(-)


diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 5f299ae..d1cccb6 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1210,14 +1210,9 @@ static int nfs_try_mount(struct nfs_parsed_mount_data *args,
 	return status;
 }
 
-/*
- * Split "dev_name" into "hostname:export_path".
- *
- * Note: caller frees hostname and export path, even on error.
- */
-static int nfs_parse_devname(const char *dev_name,
-			     char **hostname, size_t maxnamlen,
-			     char **export_path, size_t maxpathlen)
+static int nfs_parse_simple_hostname(const char *dev_name,
+				     char **hostname, size_t maxnamlen,
+				     char **export_path, size_t maxpathlen)
 {
 	size_t len;
 	char *colon, *comma;
@@ -1272,6 +1267,85 @@ out_path:
 }
 
 /*
+ * Hostname has square brackets around it because it contains one or
+ * more colons.  We look for the first closing square bracket, and a
+ * colon must follow it.
+ */
+static int nfs_parse_protected_hostname(const char *dev_name,
+					char **hostname, size_t maxnamlen,
+					char **export_path, size_t maxpathlen)
+{
+	size_t len;
+	char *start, *end;
+
+	start = (char *)(dev_name + 1);
+
+	end = strchr(start, ']');
+	if (end == NULL)
+		goto out_bad_devname;
+	if (*(end + 1) != ':')
+		goto out_bad_devname;
+
+	len = end - start;
+	if (len > maxnamlen)
+		goto out_hostname;
+
+	/* N.B. caller will free nfs_server.hostname in all cases */
+	*hostname = kstrndup(start, len, GFP_KERNEL);
+	if (*hostname == NULL)
+		goto out_nomem;
+
+	end += 2;
+	len = strlen(end);
+	if (len > maxpathlen)
+		goto out_path;
+	*export_path = kstrndup(end, len, GFP_KERNEL);
+	if (!*export_path)
+		goto out_nomem;
+
+	return 0;
+
+out_bad_devname:
+	dfprintk(MOUNT, "NFS: device name not in host:path format\n");
+	return -EINVAL;
+
+out_nomem:
+	dfprintk(MOUNT, "NFS: not enough memory to parse device name\n");
+	return -ENOMEM;
+
+out_hostname:
+	dfprintk(MOUNT, "NFS: server hostname too long\n");
+	return -ENAMETOOLONG;
+
+out_path:
+	dfprintk(MOUNT, "NFS: export pathname too long\n");
+	return -ENAMETOOLONG;
+}
+
+/*
+ * Split "dev_name" into "hostname:export_path".
+ *
+ * The leftmost colon demarks the split between the server's hostname
+ * and the export path.  If the hostname starts with a left square
+ * bracket, then it may contain colons.
+ *
+ * Note: caller frees hostname and export path, even on error.
+ */
+static int nfs_parse_devname(const char *dev_name,
+			     char **hostname, size_t maxnamlen,
+			     char **export_path, size_t maxpathlen)
+{
+	if (*dev_name == '[')
+		return nfs_parse_protected_hostname(dev_name,
+						    hostname, maxnamlen,
+						    export_path, maxpathlen);
+
+	return nfs_parse_simple_hostname(dev_name,
+					 hostname, maxnamlen,
+					 export_path, maxpathlen);
+}
+
+/*
  * Validate the NFS2/NFS3 mount data
  * - fills in the mount root filehandle
  *

Re: [PATCH 2/4] NFS: Support raw IPv6 address hostnames during NFS mount operation

[Trond Myklebust]

On Sun, 2008-05-18 at 17:20 -0400, Chuck Lever wrote:
> Traditionally the mount command has looked for a ":" to separate the
> server's hostname from the export path in the mounted on device name,
> like this:
> 
> 	mount server:/export /mounted/on/dir
> 
> The server's hostname is "server" and the export path is "/export".
> 
> You can also substitute a specific IPv4 network address for the server
> hostname, like this:
> 
> 	mount 192.168.0.55:/export /mounted/on/dir
> 
> Raw IPv6 addresses present a problem, however, because they look
> something like this:
> 
> 	fe80::200:5aff:fe00:30b
> 
> Note the use of colons.
> 
> To get around the presence of colons, copy the Solaris convention used for
> mounting IPv6 servers by address: wrap a raw IPv6 address with square
> brackets.

Standardising is good, so we should definitely support any pre-existing
Solaris conventions. However is there any reason why we couldn't use the
sequence ':/' as the separator for the IP address and the pathname for
cases where we're in doubt?

In other words, if it turns out that 'fe80' is the actual hostname, and
':200:5aff:fe00:30b' is the root pathname component, then is there any
reason why 'mount.nfs' couldn't reformat that as
'fe80:/:200:5aff:fe00:30b'?

Cheers
  Trond

Re: [PATCH 2/4] NFS: Support raw IPv6 address hostnames during NFS mount operation

[Chuck Lever]

On May 18, 2008, at 6:23 PM, Trond Myklebust wrote:
> On Sun, 2008-05-18 at 17:20 -0400, Chuck Lever wrote:
>> Traditionally the mount command has looked for a ":" to separate the
>> server's hostname from the export path in the mounted on device name,
>> like this:
>>
>> 	mount server:/export /mounted/on/dir
>>
>> The server's hostname is "server" and the export path is "/export".
>>
>> You can also substitute a specific IPv4 network address for the  
>> server
>> hostname, like this:
>>
>> 	mount 192.168.0.55:/export /mounted/on/dir
>>
>> Raw IPv6 addresses present a problem, however, because they look
>> something like this:
>>
>> 	fe80::200:5aff:fe00:30b
>>
>> Note the use of colons.
>>
>> To get around the presence of colons, copy the Solaris convention  
>> used for
>> mounting IPv6 servers by address: wrap a raw IPv6 address with square
>> brackets.
>
> Standardising is good, so we should definitely support any pre- 
> existing
> Solaris conventions. However is there any reason why we couldn't use  
> the
> sequence ':/' as the separator for the IP address and the pathname for
> cases where we're in doubt?

I wondered about that.  Would we ever have to mount a server where the  
pathname does not begin with a slash?  (Mac OS server over HFS+ might  
use colons instead of slashes?)

In the mount command I also add support for recognizing NFS URLs (and  
throwing a reasonable error, since we don't support them), as they  
will contain the string ":/", like this:

   nfs://servername-or-address:port/exportpath

In this case we need the brackets to distinguish a possible IPv6  
address from the colon that separates the address from the port  
number.  However, the ":/" separates the resource type from the rest  
of the string, and we would end up with the server name being "nfs"  
and the pathname being "servername-or-address:port/export".  :-)

> In other words, if it turns out that 'fe80' is the actual hostname,  
> and
> ':200:5aff:fe00:30b' is the root pathname component, then is there any
> reason why 'mount.nfs' couldn't reformat that as
> 'fe80:/:200:5aff:fe00:30b'?

That's the way it works today.  The mount command (and kernel devname  
parser) will both look for the first colon (left to right), and  
everything after that is the pathname.  So you can certainly get a  
mount command like this to work on today's code.

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

[PATCH 3/4] NFS: Add string length argument to nfs_parse_server_address

[Chuck Lever]

To make nfs_parse_server_address() more generally useful, allow it to
accept input strings that are not terminated with '\0'.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfs/super.c |   98 +++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 68 insertions(+), 30 deletions(-)


diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index d1cccb6..970f976 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -62,6 +62,13 @@
 
 #define NFSDBG_FACILITY		NFSDBG_VFS
 
+#ifndef INET_ADDRSTRLEN
+#define INET_ADDRSTRLEN		(16)
+#endif
+#ifndef INET6_ADDRSTRLEN
+#define INET6_ADDRSTRLEN	(48)
+#endif
+
 enum {
 	/* Mount options that take no arguments */
 	Opt_soft, Opt_hard,
@@ -702,38 +709,67 @@ static int nfs_verify_server_address(struct sockaddr *addr)
 	return 0;
 }
 
-/*
- * Parse string addresses passed in via a mount option,
- * and construct a sockaddr based on the result.
- *
- * If address parsing fails, set the sockaddr's address
- * family to AF_UNSPEC to force nfs_verify_server_address()
- * to punt the mount.
- */
-static void nfs_parse_server_address(char *value,
-				     struct sockaddr *sap,
-				     size_t *len)
+static void nfs_parse_ipv4_address(char *string, size_t str_len,
+				   struct sockaddr *sap, size_t *addr_len)
 {
-	if (strchr(value, ':')) {
-		struct sockaddr_in6 *ap = (struct sockaddr_in6 *)sap;
-		u8 *addr = (u8 *)&ap->sin6_addr.in6_u;
+	struct sockaddr_in *sin = (struct sockaddr_in *)sap;
+	u8 *addr = (u8 *)&sin->sin_addr.s_addr;
 
-		ap->sin6_family = AF_INET6;
-		*len = sizeof(*ap);
-		if (in6_pton(value, -1, addr, '\0', NULL))
+	dfprintk(MOUNT, "NFS: parsing IPv4 address %*s\n",
+			str_len, string);
+
+	if (str_len < INET_ADDRSTRLEN) {
+		sin->sin_family = AF_INET;
+		*addr_len = sizeof(*sin);
+		if (in4_pton(string, str_len, addr, '\0', NULL))
 			return;
-	} else {
-		struct sockaddr_in *ap = (struct sockaddr_in *)sap;
-		u8 *addr = (u8 *)&ap->sin_addr.s_addr;
+	}
+
+	sap->sa_family = AF_UNSPEC;
+	*addr_len = 0;
+}
+
+static void nfs_parse_ipv6_address(char *string, size_t str_len,
+				   struct sockaddr *sap, size_t *addr_len)
+{
+	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
+	u8 *addr = (u8 *)&sin6->sin6_addr.in6_u;
+
+	dfprintk(MOUNT, "NFS: parsing IPv6 address %*s\n",
+			str_len, string);
 
-		ap->sin_family = AF_INET;
-		*len = sizeof(*ap);
-		if (in4_pton(value, -1, addr, '\0', NULL))
+	if (str_len < INET6_ADDRSTRLEN) {
+		sin6->sin6_family = AF_INET6;
+		*addr_len = sizeof(*sin6);
+		if (in6_pton(string, str_len, addr, '\0', NULL))
 			return;
 	}
 
 	sap->sa_family = AF_UNSPEC;
-	*len = 0;
+	*addr_len = 0;
+}
+
+/*
+ * Construct a sockaddr based on the contents of a string that contains
+ * an IP address in presentation format.
+ *
+ * If there is a problem constructing the new sockaddr, set the address
+ * family to AF_UNSPEC.
+ */
+static void nfs_parse_ip_address(char *string, size_t str_len,
+				 struct sockaddr *sap, size_t *addr_len)
+{
+	unsigned int i, colons;
+
+	colons = 0;
+	for (i = 0; i < str_len; i++)
+		if (string[i] == ':')
+			colons++;
+
+	if (colons >= 2)
+		return nfs_parse_ipv6_address(string, str_len, sap, addr_len);
+	else
+		return nfs_parse_ipv4_address(string, str_len, sap, addr_len);
 }
 
 /*
@@ -1055,9 +1091,10 @@ static int nfs_parse_mount_options(char *raw,
 			string = match_strdup(args);
 			if (string == NULL)
 				goto out_nomem;
-			nfs_parse_server_address(string, (struct sockaddr *)
-						 &mnt->nfs_server.address,
-						 &mnt->nfs_server.addrlen);
+			nfs_parse_ip_address(string, strlen(string),
+					     (struct sockaddr *)
+						&mnt->nfs_server.address,
+					     &mnt->nfs_server.addrlen);
 			kfree(string);
 			break;
 		case Opt_clientaddr:
@@ -1078,9 +1115,10 @@ static int nfs_parse_mount_options(char *raw,
 			string = match_strdup(args);
 			if (string == NULL)
 				goto out_nomem;
-			nfs_parse_server_address(string, (struct sockaddr *)
-						 &mnt->mount_server.address,
-						 &mnt->mount_server.addrlen);
+			nfs_parse_ip_address(string, strlen(string),
+					     (struct sockaddr *)
+						&mnt->mount_server.address,
+					     &mnt->mount_server.addrlen);
 			kfree(string);
 			break;
 

[PATCH 4/4] NFS: handle interface identifiers in incoming IPv6 addresses

[Chuck Lever]

Add support in the kernel NFS client's address parser for interface
identifiers.

IPv6 link-local addresses require an additional "interface identifier",
which is an integer that indexes the array of local network interfaces.
They are suffixed to the address with a '%'.  For example:

	fe80::215:c5ff:fe3b:e1b2%2

indicates an index of 2.  Without the interface ID, link-local addresses
are not usable for NFS.

Note these can also be expressed:

	fe80::215:c5ff:fe3b:e1b2%eth0

The mount.nfs command maps the device name to an interface index before
passing in "addr=".

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfs/super.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)


diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 970f976..eb4a001 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -734,6 +734,7 @@ static void nfs_parse_ipv6_address(char *string, size_t str_len,
 {
 	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
 	u8 *addr = (u8 *)&sin6->sin6_addr.in6_u;
+	const char *delim;
 
 	dfprintk(MOUNT, "NFS: parsing IPv6 address %*s\n",
 			str_len, string);
@@ -741,8 +742,22 @@ static void nfs_parse_ipv6_address(char *string, size_t str_len,
 	if (str_len < INET6_ADDRSTRLEN) {
 		sin6->sin6_family = AF_INET6;
 		*addr_len = sizeof(*sin6);
-		if (in6_pton(string, str_len, addr, '\0', NULL))
+		if (in6_pton(string, str_len, addr, '%', &delim)) {
+			if (*delim == '%') {
+				size_t len = (string + str_len) - delim - 1;
+				char *p = kstrndup(delim + 1, len, GFP_KERNEL);
+				if (p) {
+					unsigned long id = 0;
+					/* id is set to zero on error */
+					strict_strtoul(p, 10, &id);
+					kfree(p);
+					sin6->sin6_scope_id = id;
+					dfprintk(MOUNT, "NFS: IPv6 scope "
+							"ID = %lu\n", id);
+				}
+			}
 			return;
+		}
 	}
 
 	sap->sa_family = AF_UNSPEC;

Re: [PATCH 0/4] RFC: raw IPv6 address parsing in NFS client

[Jeff Garzik]

Chuck Lever wrote:
> Hi-
> 
> I'm interested in some review of the following four patches which add to
> the kernel's NFS client the ability to parse IPv6 addresses in presentation
> format.
> 
> Namely, it adds the following:
> 
> 1.  If the user passes in an IPv6 address as the server name, the colons
>     in the address will confuse the logic that splits the device name
>     into a server hostname and an export path.   We'll use square brackets
>     around IPv6 server addresses to "escape" the colons, as does Solaris.
> 
> 2.  If the user passes in a link-local IPv6 address as the server name,
>     an interface index is also necessary.  We'll use the "%id" suffix on
>     the address to pass in the index, and plant that in the sockaddr's
>     sin6_scope_id field.
> 
> In addition to the following patches in email, a git repo with these
> same patches already applied can be found here:
> 
> 	linux-nfs.org:exports/cel-2.6.git
> 
> The basic questions:
> 
> Are these reasonable conventions to follow?  Is the parsing logic adequate?
> Is there anything I'm forgetting?

I would take a look at the underlying components of glibc's 
getnameinfo(), which must parse IPv6 addresses according to POSIX 
specifications, IIRC.

Comments:

1) bracket escaping seems reasonably common.  browsers and other apps 
sometimes use that convention too.

2) an interface name rather than index should be used

	Jeff

Re: [PATCH 0/4] RFC: raw IPv6 address parsing in NFS client

[Chuck Lever]

On May 18, 2008, at 10:13 PM, Jeff Garzik wrote:
> Chuck Lever wrote:
>> Hi-
>> I'm interested in some review of the following four patches which  
>> add to
>> the kernel's NFS client the ability to parse IPv6 addresses in  
>> presentation
>> format.
>> Namely, it adds the following:
>> 1.  If the user passes in an IPv6 address as the server name, the  
>> colons
>>    in the address will confuse the logic that splits the device name
>>    into a server hostname and an export path.   We'll use square  
>> brackets
>>    around IPv6 server addresses to "escape" the colons, as does  
>> Solaris.
>> 2.  If the user passes in a link-local IPv6 address as the server  
>> name,
>>    an interface index is also necessary.  We'll use the "%id"  
>> suffix on
>>    the address to pass in the index, and plant that in the sockaddr's
>>    sin6_scope_id field.
>> In addition to the following patches in email, a git repo with these
>> same patches already applied can be found here:
>> 	linux-nfs.org:exports/cel-2.6.git
>> The basic questions:
>> Are these reasonable conventions to follow?  Is the parsing logic  
>> adequate?
>> Is there anything I'm forgetting?
>
> I would take a look at the underlying components of glibc's  
> getnameinfo(), which must parse IPv6 addresses according to POSIX  
> specifications, IIRC.
>
> Comments:
>
> 1) bracket escaping seems reasonably common.  browsers and other  
> apps sometimes use that convention too.
>
> 2) an interface name rather than index should be used

If you give a raw IPv6 address with an interface name to mount.nfs, it  
passes the whole thing to getaddrinfo(3) which maps the name to an  
index. The address with index is then passed on to the kernel via  
mount(2) via the normal "addr=" mount option.

Is there a way the kernel can do that mapping for itself?

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

Re: [PATCH 0/4] RFC: raw IPv6 address parsing in NFS client

[Jeff Garzik]

Chuck Lever wrote:
> On May 18, 2008, at 10:13 PM, Jeff Garzik wrote:
>> Chuck Lever wrote:
>>> Hi-
>>> I'm interested in some review of the following four patches which add to
>>> the kernel's NFS client the ability to parse IPv6 addresses in 
>>> presentation
>>> format.
>>> Namely, it adds the following:
>>> 1.  If the user passes in an IPv6 address as the server name, the colons
>>>    in the address will confuse the logic that splits the device name
>>>    into a server hostname and an export path.   We'll use square 
>>> brackets
>>>    around IPv6 server addresses to "escape" the colons, as does Solaris.
>>> 2.  If the user passes in a link-local IPv6 address as the server name,
>>>    an interface index is also necessary.  We'll use the "%id" suffix on
>>>    the address to pass in the index, and plant that in the sockaddr's
>>>    sin6_scope_id field.
>>> In addition to the following patches in email, a git repo with these
>>> same patches already applied can be found here:
>>>     linux-nfs.org:exports/cel-2.6.git
>>> The basic questions:
>>> Are these reasonable conventions to follow?  Is the parsing logic 
>>> adequate?
>>> Is there anything I'm forgetting?
>>
>> I would take a look at the underlying components of glibc's 
>> getnameinfo(), which must parse IPv6 addresses according to POSIX 
>> specifications, IIRC.
>>
>> Comments:
>>
>> 1) bracket escaping seems reasonably common.  browsers and other apps 
>> sometimes use that convention too.
>>
>> 2) an interface name rather than index should be used
> 
> If you give a raw IPv6 address with an interface name to mount.nfs, it 
> passes the whole thing to getaddrinfo(3) which maps the name to an 
> index. The address with index is then passed on to the kernel via 
> mount(2) via the normal "addr=" mount option.
> 
> Is there a way the kernel can do that mapping for itself?

The kernel certainly knows the interface names...  IMO that is a bit 
more natural than interface index.

You certainly do not want to _store_ interface index or encourage its 
use, since it might change during runtime if network interfaces change.

I don't know precisely your intended usage, nor where your 
kernel/userland divide is here, but looking at dev_get_by_name() might 
be a start, for accessing netdev via ifname?

	Jeff

Re: [PATCH 0/4] RFC: raw IPv6 address parsing in NFS client

[Chuck Lever]

On May 19, 2008, at 2:56 PM, Jeff Garzik wrote:
> Chuck Lever wrote:
>> On May 18, 2008, at 10:13 PM, Jeff Garzik wrote:
>>> Chuck Lever wrote:
>>>> Hi-
>>>> I'm interested in some review of the following four patches which  
>>>> add to
>>>> the kernel's NFS client the ability to parse IPv6 addresses in  
>>>> presentation
>>>> format.
>>>> Namely, it adds the following:
>>>> 1.  If the user passes in an IPv6 address as the server name, the  
>>>> colons
>>>>   in the address will confuse the logic that splits the device name
>>>>   into a server hostname and an export path.   We'll use square  
>>>> brackets
>>>>   around IPv6 server addresses to "escape" the colons, as does  
>>>> Solaris.
>>>> 2.  If the user passes in a link-local IPv6 address as the server  
>>>> name,
>>>>   an interface index is also necessary.  We'll use the "%id"  
>>>> suffix on
>>>>   the address to pass in the index, and plant that in the  
>>>> sockaddr's
>>>>   sin6_scope_id field.
>>>> In addition to the following patches in email, a git repo with  
>>>> these
>>>> same patches already applied can be found here:
>>>>    linux-nfs.org:exports/cel-2.6.git
>>>> The basic questions:
>>>> Are these reasonable conventions to follow?  Is the parsing logic  
>>>> adequate?
>>>> Is there anything I'm forgetting?
>>>
>>> I would take a look at the underlying components of glibc's  
>>> getnameinfo(), which must parse IPv6 addresses according to POSIX  
>>> specifications, IIRC.
>>>
>>> Comments:
>>>
>>> 1) bracket escaping seems reasonably common.  browsers and other  
>>> apps sometimes use that convention too.
>>>
>>> 2) an interface name rather than index should be used
>> If you give a raw IPv6 address with an interface name to mount.nfs,  
>> it passes the whole thing to getaddrinfo(3) which maps the name to  
>> an index. The address with index is then passed on to the kernel  
>> via mount(2) via the normal "addr=" mount option.
>> Is there a way the kernel can do that mapping for itself?
>
> The kernel certainly knows the interface names...  IMO that is a bit  
> more natural than interface index.
>
> You certainly do not want to _store_ interface index or encourage  
> its use, since it might change during runtime if network interfaces  
> change.

We are storing the address of the server in a struct sockaddr_storage,  
and that would include a scope ID if it were an AF_INET6 address.  We  
are also storing the server hostname in /etc/mtab for umount.nfs to  
use later, and that hostname may be a raw IPv6 address.

So it sounds like we need to store the device name separately in the  
kernel's NFS and RPC data structures and do the devname to scope_id  
conversion during the transport's socket connect.  Oy.

> I don't know precisely your intended usage, nor where your kernel/ 
> userland divide is here, but looking at dev_get_by_name() might be a  
> start, for accessing netdev via ifname?

Yes, the NFS client (or RPC client) could invoke that as long as it is  
always built-in and not a module.

The mount.nfs command always passes the incoming server hostname  
through getaddrinfo(3).  If the hostname is an IPv6 address with  
either a device name or an interface index, getaddrinfo() does set the  
sin6_scope_id field in the resulting sockaddr.

That sockaddr is used to generate a presentation format address string  
to pass to the kernel.  mount.nfs then should always convert the scope  
ID back to a device name at that point, and the kernel should assume  
if a SCOPE_DELIMITER is present, the thing that follows it is always a  
device name.

Since the interface index isn't directly exposed in Linux user space  
anyway, as it is on Solaris (via ifconfig) perhaps it's worth it to  
limit the NFS mount interface to network device names only.

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

Re: [PATCH 0/4] RFC: raw IPv6 address parsing in NFS client

[Brian Haley]

Chuck Lever wrote:
>>>> 2) an interface name rather than index should be used
>>> If you give a raw IPv6 address with an interface name to mount.nfs, 
>>> it passes the whole thing to getaddrinfo(3) which maps the name to an 
>>> index. The address with index is then passed on to the kernel via 
>>> mount(2) via the normal "addr=" mount option.
>>> Is there a way the kernel can do that mapping for itself?
>>
>> The kernel certainly knows the interface names...  IMO that is a bit 
>> more natural than interface index.
>>
>> You certainly do not want to _store_ interface index or encourage its 
>> use, since it might change during runtime if network interfaces change.
> 
> We are storing the address of the server in a struct sockaddr_storage, 
> and that would include a scope ID if it were an AF_INET6 address.  We 
> are also storing the server hostname in /etc/mtab for umount.nfs to use 
> later, and that hostname may be a raw IPv6 address.
> 
> So it sounds like we need to store the device name separately in the 
> kernel's NFS and RPC data structures and do the devname to scope_id 
> conversion during the transport's socket connect.  Oy.

Hmm, the kernel already stores interface indexes in structures today, 
for example if you use SO_BINDTODEVICE, or join a Multicast group. 
Seems to me that's perfectly valid to do, with the caveat that the 
device might change it's index.

-Brian

Re: [PATCH 0/4] RFC: raw IPv6 address parsing in NFS client

[Chuck Lever]

On May 18, 2008, at 10:13 PM, Jeff Garzik wrote:
> Chuck Lever wrote:
>> Hi-
>> I'm interested in some review of the following four patches which  
>> add to
>> the kernel's NFS client the ability to parse IPv6 addresses in  
>> presentation
>> format.
>> Namely, it adds the following:
>> 1.  If the user passes in an IPv6 address as the server name, the  
>> colons
>>    in the address will confuse the logic that splits the device name
>>    into a server hostname and an export path.   We'll use square  
>> brackets
>>    around IPv6 server addresses to "escape" the colons, as does  
>> Solaris.
>> 2.  If the user passes in a link-local IPv6 address as the server  
>> name,
>>    an interface index is also necessary.  We'll use the "%id"  
>> suffix on
>>    the address to pass in the index, and plant that in the sockaddr's
>>    sin6_scope_id field.
>> In addition to the following patches in email, a git repo with these
>> same patches already applied can be found here:
>> 	linux-nfs.org:exports/cel-2.6.git
>> The basic questions:
>> Are these reasonable conventions to follow?  Is the parsing logic  
>> adequate?
>> Is there anything I'm forgetting?
>
> I would take a look at the underlying components of glibc's  
> getnameinfo(), which must parse IPv6 addresses according to POSIX  
> specifications, IIRC.
>
> Comments:
>
> 1) bracket escaping seems reasonably common.  browsers and other  
> apps sometimes use that convention too.
>
> 2) an interface name rather than index should be used

A few follow-up questions (for anyone, not just Jeff):

Should the kernel ignore any incoming scope/interface identifier  
unless it verifies the address is link-local?

Do we really have to worry about the character set of incoming C  
strings?

        NI_IDN If  this  flag is used, then the name found in the
               lookup process is converted from IDN format to the
               locale’s encoding  if  necessary.  ASCII-only names
               are not affected by the conversion, which makes this
               flag usable in existing programs and environments.

        NI_IDN_ALLOW_UNASSIGNED, NI_IDN_USE_STD3_ASCII_RULES
               Setting these flags will enable the IDNA_ALLOW_UNASSIGNED
               (allow unassigned  Unicode  code  points)  and
               IDNA_USE_STD3_ASCII_RULES (check output to make sure
               it is a  STD3  conforming  host  name) flags
               respectively to be used in the IDNA handling.

I would hope that this level of detail would be handled by utility  
functions provided in net/core -- in6_pton, for example.

(Actually net/core should provide appropriate helpers for mapping  
device names to scope_ids, but I didn't find anything like this).

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

Re: [PATCH 0/4] RFC: raw IPv6 address parsing in NFS client

[Jeff Garzik]

Chuck Lever wrote:
> On May 18, 2008, at 10:13 PM, Jeff Garzik wrote:
>> Chuck Lever wrote:
>>> Hi-
>>> I'm interested in some review of the following four patches which add to
>>> the kernel's NFS client the ability to parse IPv6 addresses in 
>>> presentation
>>> format.
>>> Namely, it adds the following:
>>> 1.  If the user passes in an IPv6 address as the server name, the colons
>>>    in the address will confuse the logic that splits the device name
>>>    into a server hostname and an export path.   We'll use square 
>>> brackets
>>>    around IPv6 server addresses to "escape" the colons, as does Solaris.
>>> 2.  If the user passes in a link-local IPv6 address as the server name,
>>>    an interface index is also necessary.  We'll use the "%id" suffix on
>>>    the address to pass in the index, and plant that in the sockaddr's
>>>    sin6_scope_id field.
>>> In addition to the following patches in email, a git repo with these
>>> same patches already applied can be found here:
>>>     linux-nfs.org:exports/cel-2.6.git
>>> The basic questions:
>>> Are these reasonable conventions to follow?  Is the parsing logic 
>>> adequate?
>>> Is there anything I'm forgetting?
>>
>> I would take a look at the underlying components of glibc's 
>> getnameinfo(), which must parse IPv6 addresses according to POSIX 
>> specifications, IIRC.
>>
>> Comments:
>>
>> 1) bracket escaping seems reasonably common.  browsers and other apps 
>> sometimes use that convention too.
>>
>> 2) an interface name rather than index should be used
> 
> A few follow-up questions (for anyone, not just Jeff):
> 
> Should the kernel ignore any incoming scope/interface identifier unless 
> it verifies the address is link-local?
> 
> Do we really have to worry about the character set of incoming C strings?
> 
>        NI_IDN If  this  flag is used, then the name found in the
>               lookup process is converted from IDN format to the
>               locale’s encoding  if  necessary.  ASCII-only names
>               are not affected by the conversion, which makes this
>               flag usable in existing programs and environments.
> 
>        NI_IDN_ALLOW_UNASSIGNED, NI_IDN_USE_STD3_ASCII_RULES
>               Setting these flags will enable the IDNA_ALLOW_UNASSIGNED
>               (allow unassigned  Unicode  code  points)  and
>               IDNA_USE_STD3_ASCII_RULES (check output to make sure
>               it is a  STD3  conforming  host  name) flags
>               respectively to be used in the IDNA handling.
> 
> I would hope that this level of detail would be handled by utility 
> functions provided in net/core -- in6_pton, for example.

I don't think you need to worry about that?

getnameinfo() parses an IPv6 numeric address, and returns the name 
associated with it.  You only need the first half, the IPv6 address 
string parsing, correct?

If so, you don't need to care about the character sets of the returning 
lookup data.

I presume you are not adding an in-kernel DNS resolver, so this is all 
userspace details?

	Jeff