[PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[J. Bruce Fields]

I was looking back at this bug with the misparsing of
(non-mull-terminated) fs_locations attributes.  Thanks to the work on
nfs_parse_server_address, etc., we can now also more easily support ipv6
addresses here.  But I got lost in the usual maze of twisty struct
sockaddr_*'s, all alike.  Is this right?  Does any of it need to be
under CONFIG_IPV6?  Is there a simpler way?

--b.

    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.
    
    Also support ipv6 addresses.
    
    Compile-tested only.

    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>

diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index b112857..c0f5191 100644
--- a/fs/nfs/nfs4namespace.c
+++ b/fs/nfs/nfs4namespace.c
@@ -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
@@ -172,30 +155,44 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,
 
 		s = 0;
 		while (s < location->nservers) {
-			struct sockaddr_in addr = {
-				.sin_family	= AF_INET,
-				.sin_port	= htons(NFS_PORT),
-			};
-
-			if (location->servers[s].len <= 0 ||
-			    valid_ipaddr4(location->servers[s].data) < 0) {
-				s++;
-				continue;
-			}
+			const struct nfs4_string *buf = &location->servers[s];
+			struct sockaddr_storage addr;
+
+			if (buf->len <= 0 || buf->len >= PAGE_SIZE)
+				goto next;
 
-			mountdata.hostname = location->servers[s].data;
-			addr.sin_addr.s_addr = in_aton(mountdata.hostname),
 			mountdata.addr = (struct sockaddr *)&addr;
-			mountdata.addrlen = sizeof(addr);
+
+			if (memchr(buf->data, '%', buf->len))
+				goto next;
+			nfs_parse_ip_address(buf->data, buf->len,
+					mountdata.addr, &mountdata.addrlen);
+			switch (mountdata.addr->sa_family) {
+			case AF_UNSPEC:
+				goto next;
+			case AF_INET:
+				((struct sockaddr_in *)&addr)->sin_port =
+							htons(NFS_PORT);
+				break;
+			case AF_INET6:
+				((struct sockaddr_in6 *)&addr)->sin6_port =
+							htons(NFS_PORT);
+				break;
+			}
+
+			mountdata.hostname = kmalloc(buf->len + 1, GFP_KERNEL);
+			mountdata.hostname[buf->len] = 0;
 
 			snprintf(page, PAGE_SIZE, "%s:%s",
 					mountdata.hostname,
 					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++;
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 9abcd2b..1d10756 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -798,7 +798,7 @@ static void nfs_parse_ipv6_address(char *string, size_t str_len,
  * 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,
+void nfs_parse_ip_address(char *string, size_t str_len,
 				 struct sockaddr *sap, size_t *addr_len)
 {
 	unsigned int i, colons;
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 78a5922..62ca683 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -444,6 +444,8 @@ extern const struct inode_operations nfs_referral_inode_operations;
 extern int nfs_mountpoint_expiry_timeout;
 extern void nfs_release_automount_timer(void);
 
+void nfs_parse_ip_address(char *, size_t, struct sockaddr *, size_t *);
+
 /*
  * linux/fs/nfs/unlink.c
  */

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[Chuck Lever]

On Aug 14, 2008, at 6:30 PM, J. Bruce Fields wrote:
> I was looking back at this bug with the misparsing of
> (non-mull-terminated) fs_locations attributes.  Thanks to the work on
> nfs_parse_server_address, etc., we can now also more easily support  
> ipv6
> addresses here.  But I got lost in the usual maze of twisty struct
> sockaddr_*'s, all alike.  Is this right?  Does any of it need to be
> under CONFIG_IPV6?  Is there a simpler way?

The use of the new address parser looks correct, but your string  
handling needs work.  :-)

Comments below...

>
>
> --b.
>
>    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.
>
>    Also support ipv6 addresses.
>
>    Compile-tested only.
>
>    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
>
> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> index b112857..c0f5191 100644
> --- a/fs/nfs/nfs4namespace.c
> +++ b/fs/nfs/nfs4namespace.c
> @@ -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
> @@ -172,30 +155,44 @@ static struct vfsmount  
> *nfs_follow_referral(const struct vfsmount *mnt_parent,
>
> 		s = 0;
> 		while (s < location->nservers) {
> -			struct sockaddr_in addr = {
> -				.sin_family	= AF_INET,
> -				.sin_port	= htons(NFS_PORT),
> -			};
> -
> -			if (location->servers[s].len <= 0 ||
> -			    valid_ipaddr4(location->servers[s].data) < 0) {
> -				s++;
> -				continue;
> -			}
> +			const struct nfs4_string *buf = &location->servers[s];
> +			struct sockaddr_storage addr;
> +
> +			if (buf->len <= 0 || buf->len >= PAGE_SIZE)
> +				goto next;
>
> -			mountdata.hostname = location->servers[s].data;
> -			addr.sin_addr.s_addr = in_aton(mountdata.hostname),
> 			mountdata.addr = (struct sockaddr *)&addr;
> -			mountdata.addrlen = sizeof(addr);
> +
> +			if (memchr(buf->data, '%', buf->len))
> +				goto next;

Why are you looking for a '%' ?

> +			nfs_parse_ip_address(buf->data, buf->len,
> +					mountdata.addr, &mountdata.addrlen);

Now what's so hard about that?  :-)

>
> +			switch (mountdata.addr->sa_family) {
> +			case AF_UNSPEC:
> +				goto next;
> +			case AF_INET:
> +				((struct sockaddr_in *)&addr)->sin_port =
> +							htons(NFS_PORT);
> +				break;
> +			case AF_INET6:
> +				((struct sockaddr_in6 *)&addr)->sin6_port =
> +							htons(NFS_PORT);
> +				break;
> +			}

It might be cleaner to pull the whole while {} loop into a separate  
function.  I didn't look closely enough to see if this would be easy.

At least here, you could set the port in a small helper function, and  
then put an "if (unlikely(family == AF_UNSPEC)) goto next;" just after  
the call to nfs_parse_ip_address, to save all the crazy indenting.  If  
we ever create a global helper to manage AF-agnostic port setting,  
that would make it easier to use here if the AF_UNSPEC check were  
separate.

In fact, as part of this patch you could add a static inline helper in  
fs/nfs/internal.h to do the port setting.  That can be shared between  
fs/nfs/super.c and fs/nfs/nfs4namespace.c.

Arguably the addition of the AF_UNSPEC check in the switch statement  
is an optimization that is slightly confusing.  You are checking for  
AF_UNSPEC to see if you should continue the loop, but the other cases  
are for setting a port; two entirely different purposes.

It would be more precise structuring to keep these two separate, even  
though it might add an extra conditional branch.  This makes it easier  
to spot the loop condition expressions.

> +
> +			mountdata.hostname = kmalloc(buf->len + 1, GFP_KERNEL);
> +			mountdata.hostname[buf->len] = 0;

The usual practice is to use '\0' here since it is a character array;  
0 is an int, not a character, so this does an implicit cast.   
Harmless, but using a character constant is more precise.

But I'm not sure why you are not using location->servers[s].data (or  
buf->data).  You kmalloc a buffer for hostname, but aren't setting it  
to anything.  Did you mean kstrndup?

> 			snprintf(page, PAGE_SIZE, "%s:%s",
> 					mountdata.hostname,
> 					mountdata.mnt_path);

For snprintf, you can specify the length of the string with a "%.*s"  
format.  Then you pass buf->len and buf->data (in that order) to it.   
That way you can print a non-NUL-terminated string safely.  That  
should make any memory allocation or string copying unnecessary here.

> 			mnt = vfs_kern_mount(&nfs4_referral_fs_type, 0, page, &mountdata);
> +			kfree(mountdata.hostname);

mountdata.hostname is used in other places... I don't think you can  
just free it here.  Another reason to use a pointer to location- 
 >servers[s].data.

If location->servers[s].data isn't always NUL-terminated, you might  
make mountdata.hostname an nfs4_string so it carries the string length  
with it; the other users of mountdata.hostname can then see the length.

> 			if (!IS_ERR(mnt)) {
> 				break;
> 			}
> +next:
> 			s++;
> 		}
> 		loc++;
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 9abcd2b..1d10756 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -798,7 +798,7 @@ static void nfs_parse_ipv6_address(char *string,  
> size_t str_len,
>  * 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,
> +void nfs_parse_ip_address(char *string, size_t str_len,
> 				 struct sockaddr *sap, size_t *addr_len)
> {
> 	unsigned int i, colons;
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index 78a5922..62ca683 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -444,6 +444,8 @@ extern const struct inode_operations  
> nfs_referral_inode_operations;
> extern int nfs_mountpoint_expiry_timeout;
> extern void nfs_release_automount_timer(void);
>
> +void nfs_parse_ip_address(char *, size_t, struct sockaddr *, size_t  
> *);
> +

Perhaps a better place for this would be fs/nfs/internal.h.  This  
function is used only internally in fs/nfs; no need to expose it to  
user space.

>
> /*
>  * linux/fs/nfs/unlink.c
>  */

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

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[Chuck Lever]

On Aug 15, 2008, at 12:59 PM, Chuck Lever wrote:
> On Aug 14, 2008, at 6:30 PM, J. Bruce Fields wrote:
>> I was looking back at this bug with the misparsing of
>> (non-mull-terminated) fs_locations attributes.  Thanks to the work on
>> nfs_parse_server_address, etc., we can now also more easily support  
>> ipv6
>> addresses here.  But I got lost in the usual maze of twisty struct
>> sockaddr_*'s, all alike.  Is this right?  Does any of it need to be
>> under CONFIG_IPV6?

I didn't answer this question before.

I don't think it will be necessary to add conditional compilation.   
The sockaddr_in6 and sockaddr_storage structs are available  
unconditionally as long as you include the correct headers, and you  
don't appear to use any IPv6-related interfaces or constants that are  
only conditionally available.

One way you can verify this is simply by compiling with CONFIG_IPV6  
and CONFIG_IPV6_MODULE disabled.  It's a recommended part of a typical  
build test these days anyway.

If a server refers the client to an IPv6 server address, but the  
client doesn't have support for AF_INET6 built in,  
nfs_parse_ip_address() should return an AF_UNSPEC address,  in which  
case it will be skipped by the while {} loop below.  Maybe you'd like  
it to generate a log message in this case?  Up to you.

>>  Is there a simpler way?
>
> The use of the new address parser looks correct, but your string  
> handling needs work.  :-)
>
> Comments below...
>
>>
>>
>> --b.
>>
>>   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.
>>
>>   Also support ipv6 addresses.
>>
>>   Compile-tested only.
>>
>>   Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
>>
>> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
>> index b112857..c0f5191 100644
>> --- a/fs/nfs/nfs4namespace.c
>> +++ b/fs/nfs/nfs4namespace.c
>> @@ -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
>> @@ -172,30 +155,44 @@ static struct vfsmount  
>> *nfs_follow_referral(const struct vfsmount *mnt_parent,
>>
>> 		s = 0;
>> 		while (s < location->nservers) {
>> -			struct sockaddr_in addr = {
>> -				.sin_family	= AF_INET,
>> -				.sin_port	= htons(NFS_PORT),
>> -			};
>> -
>> -			if (location->servers[s].len <= 0 ||
>> -			    valid_ipaddr4(location->servers[s].data) < 0) {
>> -				s++;
>> -				continue;
>> -			}
>> +			const struct nfs4_string *buf = &location->servers[s];
>> +			struct sockaddr_storage addr;
>> +
>> +			if (buf->len <= 0 || buf->len >= PAGE_SIZE)
>> +				goto next;
>>
>> -			mountdata.hostname = location->servers[s].data;
>> -			addr.sin_addr.s_addr = in_aton(mountdata.hostname),
>> 			mountdata.addr = (struct sockaddr *)&addr;
>> -			mountdata.addrlen = sizeof(addr);
>> +
>> +			if (memchr(buf->data, '%', buf->len))
>> +				goto next;
>
> Why are you looking for a '%' ?
>
>> +			nfs_parse_ip_address(buf->data, buf->len,
>> +					mountdata.addr, &mountdata.addrlen);
>
> Now what's so hard about that?  :-)
>
>>
>> +			switch (mountdata.addr->sa_family) {
>> +			case AF_UNSPEC:
>> +				goto next;
>> +			case AF_INET:
>> +				((struct sockaddr_in *)&addr)->sin_port =
>> +							htons(NFS_PORT);
>> +				break;
>> +			case AF_INET6:
>> +				((struct sockaddr_in6 *)&addr)->sin6_port =
>> +							htons(NFS_PORT);
>> +				break;
>> +			}
>
> It might be cleaner to pull the whole while {} loop into a separate  
> function.  I didn't look closely enough to see if this would be easy.
>
> At least here, you could set the port in a small helper function,  
> and then put an "if (unlikely(family == AF_UNSPEC)) goto next;" just  
> after the call to nfs_parse_ip_address, to save all the crazy  
> indenting.  If we ever create a global helper to manage AF-agnostic  
> port setting, that would make it easier to use here if the AF_UNSPEC  
> check were separate.
>
> In fact, as part of this patch you could add a static inline helper  
> in fs/nfs/internal.h to do the port setting.  That can be shared  
> between fs/nfs/super.c and fs/nfs/nfs4namespace.c.
>
> Arguably the addition of the AF_UNSPEC check in the switch statement  
> is an optimization that is slightly confusing.  You are checking for  
> AF_UNSPEC to see if you should continue the loop, but the other  
> cases are for setting a port; two entirely different purposes.
>
> It would be more precise structuring to keep these two separate,  
> even though it might add an extra conditional branch.  This makes it  
> easier to spot the loop condition expressions.
>
>> +
>> +			mountdata.hostname = kmalloc(buf->len + 1, GFP_KERNEL);
>> +			mountdata.hostname[buf->len] = 0;
>
> The usual practice is to use '\0' here since it is a character  
> array; 0 is an int, not a character, so this does an implicit cast.   
> Harmless, but using a character constant is more precise.
>
> But I'm not sure why you are not using location->servers[s].data (or  
> buf->data).  You kmalloc a buffer for hostname, but aren't setting  
> it to anything.  Did you mean kstrndup?
>
>> 			snprintf(page, PAGE_SIZE, "%s:%s",
>> 					mountdata.hostname,
>> 					mountdata.mnt_path);
>
> For snprintf, you can specify the length of the string with a "%.*s"  
> format.  Then you pass buf->len and buf->data (in that order) to  
> it.  That way you can print a non-NUL-terminated string safely.   
> That should make any memory allocation or string copying unnecessary  
> here.
>
>> 			mnt = vfs_kern_mount(&nfs4_referral_fs_type, 0, page, &mountdata);
>> +			kfree(mountdata.hostname);
>
> mountdata.hostname is used in other places... I don't think you can  
> just free it here.  Another reason to use a pointer to location- 
> >servers[s].data.
>
> If location->servers[s].data isn't always NUL-terminated, you might  
> make mountdata.hostname an nfs4_string so it carries the string  
> length with it; the other users of mountdata.hostname can then see  
> the length.
>
>> 			if (!IS_ERR(mnt)) {
>> 				break;
>> 			}
>> +next:
>> 			s++;
>> 		}
>> 		loc++;
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index 9abcd2b..1d10756 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -798,7 +798,7 @@ static void nfs_parse_ipv6_address(char  
>> *string, size_t str_len,
>> * 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,
>> +void nfs_parse_ip_address(char *string, size_t str_len,
>> 				 struct sockaddr *sap, size_t *addr_len)
>> {
>> 	unsigned int i, colons;
>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>> index 78a5922..62ca683 100644
>> --- a/include/linux/nfs_fs.h
>> +++ b/include/linux/nfs_fs.h
>> @@ -444,6 +444,8 @@ extern const struct inode_operations  
>> nfs_referral_inode_operations;
>> extern int nfs_mountpoint_expiry_timeout;
>> extern void nfs_release_automount_timer(void);
>>
>> +void nfs_parse_ip_address(char *, size_t, struct sockaddr *,  
>> size_t *);
>> +
>
> Perhaps a better place for this would be fs/nfs/internal.h.  This  
> function is used only internally in fs/nfs; no need to expose it to  
> user space.
>
>>
>> /*
>> * linux/fs/nfs/unlink.c
>> */
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com

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

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[J. Bruce Fields]

On Fri, Aug 15, 2008 at 12:59:09PM -0400, Chuck Lever wrote:
> On Aug 14, 2008, at 6:30 PM, J. Bruce Fields wrote:
>> I was looking back at this bug with the misparsing of
>> (non-mull-terminated) fs_locations attributes.  Thanks to the work on
>> nfs_parse_server_address, etc., we can now also more easily support  
>> ipv6
>> addresses here.  But I got lost in the usual maze of twisty struct
>> sockaddr_*'s, all alike.  Is this right?  Does any of it need to be
>> under CONFIG_IPV6?  Is there a simpler way?
>
> The use of the new address parser looks correct, but your string  
> handling needs work.  :-)
>
> Comments below...

Pffft.  My hope that someone else would pick this up for me was
obviously fantasy. OK, thanks for comments:

>> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
>> index b112857..c0f5191 100644
...
>> +			if (memchr(buf->data, '%', buf->len))
>> +				goto next;
>
> Why are you looking for a '%' ?

Would it have been clearer if I'd moved the IPV6_SCOPE_DELIMITER define
to a common header?  I don't think that has any place in the nfs
protocol.  And we've got less trust in the address we're parsing here
(which came across the wire) then we would in a mount commandline.

>
>> +			nfs_parse_ip_address(buf->data, buf->len,
>> +					mountdata.addr, &mountdata.addrlen);
>
> Now what's so hard about that?  :-)

Yeah, yeah....

>>
>> +			switch (mountdata.addr->sa_family) {
>> +			case AF_UNSPEC:
>> +				goto next;
>> +			case AF_INET:
>> +				((struct sockaddr_in *)&addr)->sin_port =
>> +							htons(NFS_PORT);
>> +				break;
>> +			case AF_INET6:
>> +				((struct sockaddr_in6 *)&addr)->sin6_port =
>> +							htons(NFS_PORT);
>> +				break;
>> +			}
>
> It might be cleaner to pull the whole while {} loop into a separate  
> function.  I didn't look closely enough to see if this would be easy.

Yup, done.

>
> At least here, you could set the port in a small helper function, and  
> then put an "if (unlikely(family == AF_UNSPEC)) goto next;" just after  
> the call to nfs_parse_ip_address, to save all the crazy indenting.  If  
> we ever create a global helper to manage AF-agnostic port setting, that 
> would make it easier to use here if the AF_UNSPEC check were separate.
>
> In fact, as part of this patch you could add a static inline helper in  
> fs/nfs/internal.h to do the port setting.  That can be shared between  
> fs/nfs/super.c and fs/nfs/nfs4namespace.c.
>
> Arguably the addition of the AF_UNSPEC check in the switch statement is 
> an optimization that is slightly confusing.  You are checking for  
> AF_UNSPEC to see if you should continue the loop, but the other cases  
> are for setting a port; two entirely different purposes.
>
> It would be more precise structuring to keep these two separate, even  
> though it might add an extra conditional branch.  This makes it easier  
> to spot the loop condition expressions.

Agreed, done.

>
>> +
>> +			mountdata.hostname = kmalloc(buf->len + 1, GFP_KERNEL);
>> +			mountdata.hostname[buf->len] = 0;
>
> The usual practice is to use '\0' here since it is a character array; 0 
> is an int, not a character, so this does an implicit cast.  Harmless, but 
> using a character constant is more precise.

OK, done.

> But I'm not sure why you are not using location->servers[s].data (or  
> buf->data).

The mountdata->hostname field needs a null terminated string.

> You kmalloc a buffer for hostname, but aren't setting it to 
> anything.  Did you mean kstrndup?

Whoops.  Actually, since the code tries to allocate memory for us at the
start, it probably makes more sense just to use a piece of that memory;
done.

>> 			snprintf(page, PAGE_SIZE, "%s:%s",
>> 					mountdata.hostname,
>> 					mountdata.mnt_path);
>
> For snprintf, you can specify the length of the string with a "%.*s"  
> format.  Then you pass buf->len and buf->data (in that order) to it.   
> That way you can print a non-NUL-terminated string safely.  That should 
> make any memory allocation or string copying unnecessary here.

No, mountdata.hostname independently still needs to be null terminated.

>
>> 			mnt = vfs_kern_mount(&nfs4_referral_fs_type, 0, page, &mountdata);
>> +			kfree(mountdata.hostname);
>
> mountdata.hostname is used in other places... I don't think you can just 
> free it here.  Another reason to use a pointer to location- 
> >servers[s].data.

That memory will go away soon, too.  The previous code is actually just
pointing to data on the stack.  So it's the callee's responsibility to
make any copies it needs before returning.

> If location->servers[s].data isn't always NUL-terminated, you might make 
> mountdata.hostname an nfs4_string so it carries the string length with 
> it; the other users of mountdata.hostname can then see the length.

That's passed around a lot and the assumption that it's null terminated
appears to be common (and convenient).

>> --- a/include/linux/nfs_fs.h
>> +++ b/include/linux/nfs_fs.h
>> @@ -444,6 +444,8 @@ extern const struct inode_operations  
>> nfs_referral_inode_operations;
>> extern int nfs_mountpoint_expiry_timeout;
>> extern void nfs_release_automount_timer(void);
>>
>> +void nfs_parse_ip_address(char *, size_t, struct sockaddr *, size_t  
>> *);
>> +
>
> Perhaps a better place for this would be fs/nfs/internal.h.  This  
> function is used only internally in fs/nfs; no need to expose it to user 
> space.

Yep, done.

--b.

[PATCH 1/4] nfs: break up nfs_follow_referral

[J. Bruce Fields]

This function is a little longer and more deeply nested than necessary.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfs/nfs4namespace.c |   84 ++++++++++++++++++++++++++---------------------
 1 files changed, 46 insertions(+), 38 deletions(-)

diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index b112857..956cbbc 100644
--- a/fs/nfs/nfs4namespace.c
+++ b/fs/nfs/nfs4namespace.c
@@ -110,6 +110,48 @@ static inline int valid_ipaddr4(const char *buf)
 	return 0;
 }
 
+static struct vfsmount *try_location(struct nfs_clone_mount *mountdata,
+				     char *page, char *page2,
+				     const struct nfs4_fs_location *location)
+{
+	struct vfsmount *mnt = ERR_PTR(-ENOENT);
+	char *mnt_path;
+	unsigned int s = 0;
+
+	mnt_path = nfs4_pathname_string(&location->rootpath, page2, PAGE_SIZE);
+	if (IS_ERR(mnt_path))
+		return mnt;
+	mountdata->mnt_path = mnt_path;
+
+	while (s < location->nservers) {
+		struct sockaddr_in addr = {
+			.sin_family	= AF_INET,
+			.sin_port	= htons(NFS_PORT),
+		};
+
+		if (location->servers[s].len <= 0 ||
+		    valid_ipaddr4(location->servers[s].data) < 0) {
+			s++;
+			continue;
+		}
+
+		mountdata->hostname = location->servers[s].data;
+		addr.sin_addr.s_addr = in_aton(mountdata->hostname),
+		mountdata->addr = (struct sockaddr *)&addr;
+		mountdata->addrlen = sizeof(addr);
+
+		snprintf(page, PAGE_SIZE, "%s:%s",
+				mountdata->hostname,
+				mountdata->mnt_path);
+
+		mnt = vfs_kern_mount(&nfs4_referral_fs_type, 0, page, mountdata);
+		if (!IS_ERR(mnt))
+			break;
+		s++;
+	}
+	return mnt;
+}
+
 /**
  * nfs_follow_referral - set up mountpoint when hitting a referral on moved error
  * @mnt_parent - mountpoint of parent directory
@@ -128,7 +170,6 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,
 		.authflavor = NFS_SB(mnt_parent->mnt_sb)->client->cl_auth->au_flavor,
 	};
 	char *page = NULL, *page2 = NULL;
-	unsigned int s;
 	int loc, error;
 
 	if (locations == NULL || locations->nlocations <= 0)
@@ -153,9 +194,8 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,
 	}
 
 	loc = 0;
-	while (loc < locations->nlocations && IS_ERR(mnt)) {
+	while (loc < locations->nlocations) {
 		const struct nfs4_fs_location *location = &locations->locations[loc];
-		char *mnt_path;
 
 		if (location == NULL || location->nservers <= 0 ||
 		    location->rootpath.ncomponents == 0) {
@@ -163,41 +203,9 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,
 			continue;
 		}
 
-		mnt_path = nfs4_pathname_string(&location->rootpath, page2, PAGE_SIZE);
-		if (IS_ERR(mnt_path)) {
-			loc++;
-			continue;
-		}
-		mountdata.mnt_path = mnt_path;
-
-		s = 0;
-		while (s < location->nservers) {
-			struct sockaddr_in addr = {
-				.sin_family	= AF_INET,
-				.sin_port	= htons(NFS_PORT),
-			};
-
-			if (location->servers[s].len <= 0 ||
-			    valid_ipaddr4(location->servers[s].data) < 0) {
-				s++;
-				continue;
-			}
-
-			mountdata.hostname = location->servers[s].data;
-			addr.sin_addr.s_addr = in_aton(mountdata.hostname),
-			mountdata.addr = (struct sockaddr *)&addr;
-			mountdata.addrlen = sizeof(addr);
-
-			snprintf(page, PAGE_SIZE, "%s:%s",
-					mountdata.hostname,
-					mountdata.mnt_path);
-
-			mnt = vfs_kern_mount(&nfs4_referral_fs_type, 0, page, &mountdata);
-			if (!IS_ERR(mnt)) {
-				break;
-			}
-			s++;
-		}
+		mnt = try_location(&mountdata, page, page2, location);
+		if (!IS_ERR(mnt))
+			break;
 		loc++;
 	}
 
-- 
1.5.5.rc1

[PATCH 2/4] nfs: replace while loop by for loops in nfs_follow_referral

[J. Bruce Fields]

Whoever wrote this had a bizarre allergy to for loops.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfs/nfs4namespace.c |   17 +++++------------
 1 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index 956cbbc..6bcc569 100644
--- a/fs/nfs/nfs4namespace.c
+++ b/fs/nfs/nfs4namespace.c
@@ -116,24 +116,22 @@ static struct vfsmount *try_location(struct nfs_clone_mount *mountdata,
 {
 	struct vfsmount *mnt = ERR_PTR(-ENOENT);
 	char *mnt_path;
-	unsigned int s = 0;
+	unsigned int s;
 
 	mnt_path = nfs4_pathname_string(&location->rootpath, page2, PAGE_SIZE);
 	if (IS_ERR(mnt_path))
 		return mnt;
 	mountdata->mnt_path = mnt_path;
 
-	while (s < location->nservers) {
+	for (s = 0; s < location->nservers; s++) {
 		struct sockaddr_in addr = {
 			.sin_family	= AF_INET,
 			.sin_port	= htons(NFS_PORT),
 		};
 
 		if (location->servers[s].len <= 0 ||
-		    valid_ipaddr4(location->servers[s].data) < 0) {
-			s++;
+		    valid_ipaddr4(location->servers[s].data) < 0)
 			continue;
-		}
 
 		mountdata->hostname = location->servers[s].data;
 		addr.sin_addr.s_addr = in_aton(mountdata->hostname),
@@ -147,7 +145,6 @@ static struct vfsmount *try_location(struct nfs_clone_mount *mountdata,
 		mnt = vfs_kern_mount(&nfs4_referral_fs_type, 0, page, mountdata);
 		if (!IS_ERR(mnt))
 			break;
-		s++;
 	}
 	return mnt;
 }
@@ -193,20 +190,16 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,
 		goto out;
 	}
 
-	loc = 0;
-	while (loc < locations->nlocations) {
+	for (loc = 0; loc < locations->nlocations; loc++) {
 		const struct nfs4_fs_location *location = &locations->locations[loc];
 
 		if (location == NULL || location->nservers <= 0 ||
-		    location->rootpath.ncomponents == 0) {
-			loc++;
+		    location->rootpath.ncomponents == 0)
 			continue;
-		}
 
 		mnt = try_location(&mountdata, page, page2, location);
 		if (!IS_ERR(mnt))
 			break;
-		loc++;
 	}
 
 out:
-- 
1.5.5.rc1

[PATCH 3/4] nfs: prepare to share nfs_set_port

[J. Bruce Fields]

We plan to use this function elsewhere.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfs/internal.h |   20 ++++++++++++++++++++
 fs/nfs/super.c    |   19 -------------------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 24241fc..0b30f24 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -276,3 +276,23 @@ unsigned int nfs_page_array_len(unsigned int base, size_t len)
 		PAGE_SIZE - 1) >> PAGE_SHIFT;
 }
 
+
+/*
+ * Set the port number in an address.  Be agnostic about the address
+ * family.
+ */
+static inline void nfs_set_port(struct sockaddr *sap, unsigned short port)
+{
+	switch (sap->sa_family) {
+	case AF_INET: {
+	      struct sockaddr_in *ap = (struct sockaddr_in *)sap;
+	      ap->sin_port = htons(port);
+	      break;
+	}
+	case AF_INET6: {
+	       struct sockaddr_in6 *ap = (struct sockaddr_in6 *)sap;
+	       ap->sin6_port = htons(port);
+	       break;
+	}
+	}
+}
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 9abcd2b..497dd31 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -664,25 +664,6 @@ static void nfs_umount_begin(struct super_block *sb)
 }
 
 /*
- * Set the port number in an address.  Be agnostic about the address family.
- */
-static void nfs_set_port(struct sockaddr *sap, unsigned short port)
-{
-	switch (sap->sa_family) {
-	case AF_INET: {
-		struct sockaddr_in *ap = (struct sockaddr_in *)sap;
-		ap->sin_port = htons(port);
-		break;
-	}
-	case AF_INET6: {
-		struct sockaddr_in6 *ap = (struct sockaddr_in6 *)sap;
-		ap->sin6_port = htons(port);
-		break;
-	}
-	}
-}
-
-/*
  * Sanity-check a server address provided by the mount command.
  *
  * Address family must be initialized, and address must not be
-- 
1.5.5.rc1

[PATCH 4/4] nfs: Fix misparsing of nfsv4 fs_locations attribute

[J. Bruce Fields]

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

Also support ipv6 addresses.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfs/internal.h      |    2 ++
 fs/nfs/nfs4namespace.c |   44 ++++++++++++++++++--------------------------
 fs/nfs/super.c         |    4 +---
 3 files changed, 21 insertions(+), 29 deletions(-)

diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 0b30f24..625abae 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -153,6 +153,7 @@ extern void nfs4_clear_inode(struct inode *);
 void nfs_zap_acl_cache(struct inode *inode);
 
 /* super.c */
+void nfs_parse_ip_address(char *, size_t, struct sockaddr *, size_t *);
 extern struct file_system_type nfs_xdev_fs_type;
 #ifdef CONFIG_NFS_V4
 extern struct file_system_type nfs4_xdev_fs_type;
@@ -276,6 +277,7 @@ unsigned int nfs_page_array_len(unsigned int base, size_t len)
 		PAGE_SIZE - 1) >> PAGE_SHIFT;
 }
 
+#define IPV6_SCOPE_DELIMITER	'%'
 
 /*
  * Set the port number in an address.  Be agnostic about the address
diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index 6bcc569..30befc3 100644
--- a/fs/nfs/nfs4namespace.c
+++ b/fs/nfs/nfs4namespace.c
@@ -93,50 +93,42 @@ 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;
-}
-
 static struct vfsmount *try_location(struct nfs_clone_mount *mountdata,
 				     char *page, char *page2,
 				     const struct nfs4_fs_location *location)
 {
 	struct vfsmount *mnt = ERR_PTR(-ENOENT);
 	char *mnt_path;
+	int page2len;
 	unsigned int s;
 
 	mnt_path = nfs4_pathname_string(&location->rootpath, page2, PAGE_SIZE);
 	if (IS_ERR(mnt_path))
 		return mnt;
 	mountdata->mnt_path = mnt_path;
+	page2 += strlen(mnt_path) + 1;
+	page2len = PAGE_SIZE - strlen(mnt_path) - 1;
 
 	for (s = 0; s < location->nservers; s++) {
-		struct sockaddr_in addr = {
-			.sin_family	= AF_INET,
-			.sin_port	= htons(NFS_PORT),
-		};
+		const struct nfs4_string *buf = &location->servers[s];
+		struct sockaddr_storage addr;
 
-		if (location->servers[s].len <= 0 ||
-		    valid_ipaddr4(location->servers[s].data) < 0)
+		if (buf->len <= 0 || buf->len >= PAGE_SIZE)
 			continue;
 
-		mountdata->hostname = location->servers[s].data;
-		addr.sin_addr.s_addr = in_aton(mountdata->hostname),
 		mountdata->addr = (struct sockaddr *)&addr;
-		mountdata->addrlen = sizeof(addr);
+
+		if (memchr(buf->data, IPV6_SCOPE_DELIMITER, buf->len))
+			continue;
+		nfs_parse_ip_address(buf->data, buf->len,
+				mountdata->addr, &mountdata->addrlen);
+		if (mountdata->addr->sa_family == AF_UNSPEC)
+			continue;
+		nfs_set_port(mountdata->addr, NFS_PORT);
+
+		strncpy(page2, buf->data, page2len);
+		page2[page2len] = '\0';
+		mountdata->hostname = page2;
 
 		snprintf(page, PAGE_SIZE, "%s:%s",
 				mountdata->hostname,
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 497dd31..349cee6 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -705,8 +705,6 @@ static void nfs_parse_ipv4_address(char *string, size_t str_len,
 	*addr_len = 0;
 }
 
-#define IPV6_SCOPE_DELIMITER	'%'
-
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 static void nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
 				    const char *delim,
@@ -779,7 +777,7 @@ static void nfs_parse_ipv6_address(char *string, size_t str_len,
  * 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,
+void nfs_parse_ip_address(char *string, size_t str_len,
 				 struct sockaddr *sap, size_t *addr_len)
 {
 	unsigned int i, colons;
-- 
1.5.5.rc1

Re: [PATCH 3/4] nfs: prepare to share nfs_set_port

[Chuck Lever]

On Wed, Aug 20, 2008 at 4:10 PM, J. Bruce Fields <bfields@citi.umich.edu> wrote:
> We plan to use this function elsewhere.
>
> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> ---
>  fs/nfs/internal.h |   20 ++++++++++++++++++++
>  fs/nfs/super.c    |   19 -------------------
>  2 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 24241fc..0b30f24 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -276,3 +276,23 @@ unsigned int nfs_page_array_len(unsigned int base, size_t len)
>                PAGE_SIZE - 1) >> PAGE_SHIFT;
>  }
>
> +

Extra blank line here.

> +/*
> + * Set the port number in an address.  Be agnostic about the address
> + * family.
> + */
> +static inline void nfs_set_port(struct sockaddr *sap, unsigned short port)
> +{
> +       switch (sap->sa_family) {
> +       case AF_INET: {
> +             struct sockaddr_in *ap = (struct sockaddr_in *)sap;
> +             ap->sin_port = htons(port);
> +             break;
> +       }
> +       case AF_INET6: {
> +              struct sockaddr_in6 *ap = (struct sockaddr_in6 *)sap;
> +              ap->sin6_port = htons(port);
> +              break;
> +       }
> +       }
> +}

Before Trond sees this and busts a blood vessel... you should refactor
the switch statement to get rid of the double braces.

-- 
Chuck Lever

Re: [PATCH 3/4] nfs: prepare to share nfs_set_port

[J. Bruce Fields]

On Wed, Aug 20, 2008 at 04:23:23PM -0400, Chuck Lever wrote:
> On Wed, Aug 20, 2008 at 4:10 PM, J. Bruce Fields <bfields@citi.umich.edu> wrote:
> > We plan to use this function elsewhere.
> >
> > Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> > ---
> >  fs/nfs/internal.h |   20 ++++++++++++++++++++
> >  fs/nfs/super.c    |   19 -------------------
> >  2 files changed, 20 insertions(+), 19 deletions(-)
> >
> > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> > index 24241fc..0b30f24 100644
> > --- a/fs/nfs/internal.h
> > +++ b/fs/nfs/internal.h
> > @@ -276,3 +276,23 @@ unsigned int nfs_page_array_len(unsigned int base, size_t len)
> >                PAGE_SIZE - 1) >> PAGE_SHIFT;
> >  }
> >
> > +
> 
> Extra blank line here.
> 
> > +/*
> > + * Set the port number in an address.  Be agnostic about the address
> > + * family.
> > + */
> > +static inline void nfs_set_port(struct sockaddr *sap, unsigned short port)
> > +{
> > +       switch (sap->sa_family) {
> > +       case AF_INET: {
> > +             struct sockaddr_in *ap = (struct sockaddr_in *)sap;
> > +             ap->sin_port = htons(port);
> > +             break;
> > +       }
> > +       case AF_INET6: {
> > +              struct sockaddr_in6 *ap = (struct sockaddr_in6 *)sap;
> > +              ap->sin6_port = htons(port);
> > +              break;
> > +       }
> > +       }
> > +}
> 
> Before Trond sees this and busts a blood vessel... you should refactor
> the switch statement to get rid of the double braces.

OK, both done; results are the top four patches of

	git://linux-nfs.org/~bfields/linux.git for-trond

If you have other nfs/ipv6 patches laying around, would you rather queue
these up with them and send them in to Trond yourself?  Or do you want
me to?  Your choice.

--b.

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[Chuck Lever]

On Wed, Aug 20, 2008 at 4:08 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Fri, Aug 15, 2008 at 12:59:09PM -0400, Chuck Lever wrote:
>> On Aug 14, 2008, at 6:30 PM, J. Bruce Fields wrote:
>>> I was looking back at this bug with the misparsing of
>>> (non-mull-terminated) fs_locations attributes.  Thanks to the work on
>>> nfs_parse_server_address, etc., we can now also more easily support
>>> ipv6
>>> addresses here.  But I got lost in the usual maze of twisty struct
>>> sockaddr_*'s, all alike.  Is this right?  Does any of it need to be
>>> under CONFIG_IPV6?  Is there a simpler way?
>>
>> The use of the new address parser looks correct, but your string
>> handling needs work.  :-)
>>
>> Comments below...
>
> Pffft.  My hope that someone else would pick this up for me was
> obviously fantasy. OK, thanks for comments:
>
>>> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
>>> index b112857..c0f5191 100644
> ...
>>> +                    if (memchr(buf->data, '%', buf->len))
>>> +                            goto next;
>>
>> Why are you looking for a '%' ?
>
> Would it have been clearer if I'd moved the IPV6_SCOPE_DELIMITER define
> to a common header?  I don't think that has any place in the nfs
> protocol.  And we've got less trust in the address we're parsing here
> (which came across the wire) then we would in a mount commandline.

OK, so you wanted a scope delimiter.  Why do you want to punt IPv6
addresses that have a scope delimiter?  Sorry to be dense.

Are you just looking for "illegal" or confusing characters?  The
address parser should handle all that and give you an AF_UNSPEC
address if the string had any weirdness in it.

Otherwise, if the returned sockaddr is an IPv6 address, can you just
check if the sin6_scope_ip field is not zero?

-- 
Chuck Lever

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[J. Bruce Fields]

On Wed, Aug 20, 2008 at 04:19:48PM -0400, Chuck Lever wrote:
> On Wed, Aug 20, 2008 at 4:08 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > On Fri, Aug 15, 2008 at 12:59:09PM -0400, Chuck Lever wrote:
> >> On Aug 14, 2008, at 6:30 PM, J. Bruce Fields wrote:
> >>> I was looking back at this bug with the misparsing of
> >>> (non-mull-terminated) fs_locations attributes.  Thanks to the work on
> >>> nfs_parse_server_address, etc., we can now also more easily support
> >>> ipv6
> >>> addresses here.  But I got lost in the usual maze of twisty struct
> >>> sockaddr_*'s, all alike.  Is this right?  Does any of it need to be
> >>> under CONFIG_IPV6?  Is there a simpler way?
> >>
> >> The use of the new address parser looks correct, but your string
> >> handling needs work.  :-)
> >>
> >> Comments below...
> >
> > Pffft.  My hope that someone else would pick this up for me was
> > obviously fantasy. OK, thanks for comments:
> >
> >>> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> >>> index b112857..c0f5191 100644
> > ...
> >>> +                    if (memchr(buf->data, '%', buf->len))
> >>> +                            goto next;
> >>
> >> Why are you looking for a '%' ?
> >
> > Would it have been clearer if I'd moved the IPV6_SCOPE_DELIMITER define
> > to a common header?  I don't think that has any place in the nfs
> > protocol.  And we've got less trust in the address we're parsing here
> > (which came across the wire) then we would in a mount commandline.
> 
> OK, so you wanted a scope delimiter.  Why do you want to punt IPv6
> addresses that have a scope delimiter?  Sorry to be dense.

The thing we're parsing here is a hostname that the server returned to
us.  It should be either a dns name (which we don't handle yet) or an ip
address.  The scope-delimiter thing isn't legal.

> Are you just looking for "illegal" or confusing characters?  The
> address parser should handle all that and give you an AF_UNSPEC
> address if the string had any weirdness in it.

At least in the case of the scope delimiter it looks like the address
parser actually tries to do something with it.  We don't want that.

> Otherwise, if the returned sockaddr is an IPv6 address, can you just
> check if the sin6_scope_ip field is not zero?

Oh, sure, that'd be OK too.

Honestly in the perfect world I'd rather be able to call a function that
accepted just ip addresses, not whatever odd appendages we also allow on
the mount commandline, on the off-chance that someone decides to add
something even odder some day and doesn't realize the parser also
handles untrusted data from the network.

--b.

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[Chuck Lever]

On Wed, Aug 20, 2008 at 4:47 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Wed, Aug 20, 2008 at 04:19:48PM -0400, Chuck Lever wrote:
>> On Wed, Aug 20, 2008 at 4:08 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > On Fri, Aug 15, 2008 at 12:59:09PM -0400, Chuck Lever wrote:
>> >> On Aug 14, 2008, at 6:30 PM, J. Bruce Fields wrote:
>> >>> I was looking back at this bug with the misparsing of
>> >>> (non-mull-terminated) fs_locations attributes.  Thanks to the work on
>> >>> nfs_parse_server_address, etc., we can now also more easily support
>> >>> ipv6
>> >>> addresses here.  But I got lost in the usual maze of twisty struct
>> >>> sockaddr_*'s, all alike.  Is this right?  Does any of it need to be
>> >>> under CONFIG_IPV6?  Is there a simpler way?
>> >>
>> >> The use of the new address parser looks correct, but your string
>> >> handling needs work.  :-)
>> >>
>> >> Comments below...
>> >
>> > Pffft.  My hope that someone else would pick this up for me was
>> > obviously fantasy. OK, thanks for comments:
>> >
>> >>> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
>> >>> index b112857..c0f5191 100644
>> > ...
>> >>> +                    if (memchr(buf->data, '%', buf->len))
>> >>> +                            goto next;
>> >>
>> >> Why are you looking for a '%' ?
>> >
>> > Would it have been clearer if I'd moved the IPV6_SCOPE_DELIMITER define
>> > to a common header?  I don't think that has any place in the nfs
>> > protocol.  And we've got less trust in the address we're parsing here
>> > (which came across the wire) then we would in a mount commandline.
>>
>> OK, so you wanted a scope delimiter.  Why do you want to punt IPv6
>> addresses that have a scope delimiter?  Sorry to be dense.
>
> The thing we're parsing here is a hostname that the server returned to
> us.  It should be either a dns name (which we don't handle yet) or an ip
> address.  The scope-delimiter thing isn't legal.

Scope delimiters are legal in IPv6 addresses.  Unfortunately I can't
find the relevant RFC at the moment.

>> Are you just looking for "illegal" or confusing characters?  The
>> address parser should handle all that and give you an AF_UNSPEC
>> address if the string had any weirdness in it.
>
> At least in the case of the scope delimiter it looks like the address
> parser actually tries to do something with it.  We don't want that.
>
>> Otherwise, if the returned sockaddr is an IPv6 address, can you just
>> check if the sin6_scope_ip field is not zero?
>
> Oh, sure, that'd be OK too.
>
> Honestly in the perfect world I'd rather be able to call a function that
> accepted just ip addresses, not whatever odd appendages we also allow on
> the mount commandline,

The '%' interface identifier is not specific to NFS mount.nfs.

The kernel's regular IPv6 address parser doesn't handle them for some
reason; there isn't a lot of explicit scope_id support anywhere in the
kernel.  But they are parsed correctly by getaddrinfo(3) and
getnameinfo(3) in user space, both of which are generic programming
interfaces that have nothing to do with mount.

Is '%' specifically not allowed at all for referrals, or is it just
that DNS hostname strings don't allow '%' ?

-- 
Chuck Lever

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[J. Bruce Fields]

On Wed, Aug 20, 2008 at 05:19:50PM -0400, Chuck Lever wrote:
> On Wed, Aug 20, 2008 at 4:47 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > On Wed, Aug 20, 2008 at 04:19:48PM -0400, Chuck Lever wrote:
> >> On Wed, Aug 20, 2008 at 4:08 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >> > On Fri, Aug 15, 2008 at 12:59:09PM -0400, Chuck Lever wrote:
> >> >> On Aug 14, 2008, at 6:30 PM, J. Bruce Fields wrote:
> >> >>> I was looking back at this bug with the misparsing of
> >> >>> (non-mull-terminated) fs_locations attributes.  Thanks to the work on
> >> >>> nfs_parse_server_address, etc., we can now also more easily support
> >> >>> ipv6
> >> >>> addresses here.  But I got lost in the usual maze of twisty struct
> >> >>> sockaddr_*'s, all alike.  Is this right?  Does any of it need to be
> >> >>> under CONFIG_IPV6?  Is there a simpler way?
> >> >>
> >> >> The use of the new address parser looks correct, but your string
> >> >> handling needs work.  :-)
> >> >>
> >> >> Comments below...
> >> >
> >> > Pffft.  My hope that someone else would pick this up for me was
> >> > obviously fantasy. OK, thanks for comments:
> >> >
> >> >>> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> >> >>> index b112857..c0f5191 100644
> >> > ...
> >> >>> +                    if (memchr(buf->data, '%', buf->len))
> >> >>> +                            goto next;
> >> >>
> >> >> Why are you looking for a '%' ?
> >> >
> >> > Would it have been clearer if I'd moved the IPV6_SCOPE_DELIMITER define
> >> > to a common header?  I don't think that has any place in the nfs
> >> > protocol.  And we've got less trust in the address we're parsing here
> >> > (which came across the wire) then we would in a mount commandline.
> >>
> >> OK, so you wanted a scope delimiter.  Why do you want to punt IPv6
> >> addresses that have a scope delimiter?  Sorry to be dense.
> >
> > The thing we're parsing here is a hostname that the server returned to
> > us.  It should be either a dns name (which we don't handle yet) or an ip
> > address.  The scope-delimiter thing isn't legal.
> 
> Scope delimiters are legal in IPv6 addresses.  Unfortunately I can't
> find the relevant RFC at the moment.

Oh, OK, I think I took a quick look and assumed they were something that
only made sense locally.  I'll look again.

--b.

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[Chuck Lever]

On Wed, Aug 20, 2008 at 5:29 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Wed, Aug 20, 2008 at 05:19:50PM -0400, Chuck Lever wrote:
>> On Wed, Aug 20, 2008 at 4:47 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > On Wed, Aug 20, 2008 at 04:19:48PM -0400, Chuck Lever wrote:
>> >> On Wed, Aug 20, 2008 at 4:08 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> >> > On Fri, Aug 15, 2008 at 12:59:09PM -0400, Chuck Lever wrote:
>> >> >> On Aug 14, 2008, at 6:30 PM, J. Bruce Fields wrote:
>> >> >>> I was looking back at this bug with the misparsing of
>> >> >>> (non-mull-terminated) fs_locations attributes.  Thanks to the work on
>> >> >>> nfs_parse_server_address, etc., we can now also more easily support
>> >> >>> ipv6
>> >> >>> addresses here.  But I got lost in the usual maze of twisty struct
>> >> >>> sockaddr_*'s, all alike.  Is this right?  Does any of it need to be
>> >> >>> under CONFIG_IPV6?  Is there a simpler way?
>> >> >>
>> >> >> The use of the new address parser looks correct, but your string
>> >> >> handling needs work.  :-)
>> >> >>
>> >> >> Comments below...
>> >> >
>> >> > Pffft.  My hope that someone else would pick this up for me was
>> >> > obviously fantasy. OK, thanks for comments:
>> >> >
>> >> >>> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
>> >> >>> index b112857..c0f5191 100644
>> >> > ...
>> >> >>> +                    if (memchr(buf->data, '%', buf->len))
>> >> >>> +                            goto next;
>> >> >>
>> >> >> Why are you looking for a '%' ?
>> >> >
>> >> > Would it have been clearer if I'd moved the IPV6_SCOPE_DELIMITER define
>> >> > to a common header?  I don't think that has any place in the nfs
>> >> > protocol.  And we've got less trust in the address we're parsing here
>> >> > (which came across the wire) then we would in a mount commandline.
>> >>
>> >> OK, so you wanted a scope delimiter.  Why do you want to punt IPv6
>> >> addresses that have a scope delimiter?  Sorry to be dense.
>> >
>> > The thing we're parsing here is a hostname that the server returned to
>> > us.  It should be either a dns name (which we don't handle yet) or an ip
>> > address.  The scope-delimiter thing isn't legal.
>>
>> Scope delimiters are legal in IPv6 addresses.  Unfortunately I can't
>> find the relevant RFC at the moment.
>
> Oh, OK, I think I took a quick look and assumed they were something that
> only made sense locally.  I'll look again.

What may be confusing you is that scope delimiters are used almost
exclusively for link-local addresses, which are valid only on the
local host.

If you don't want to handle a referral that uses a link-local address,
or you don't want to handle a link-local address with a scope ID, then
there are explicit checks you can do.

-- 
Chuck Lever

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[J. Bruce Fields]

On Wed, Aug 20, 2008 at 06:07:50PM -0400, Chuck Lever wrote:
> What may be confusing you is that scope delimiters are used almost
> exclusively for link-local addresses, which are valid only on the
> local host.
> 
> If you don't want to handle a referral that uses a link-local address,
> or you don't want to handle a link-local address with a scope ID, then
> there are explicit checks you can do.

Well, the current code does allow a referral to point to 127.0.0.1.  I
don't know what to think of that; it seems unlikely to be useful for
anything but testing, and possibly succeptible to abuse, but of course
the protocol doesn't forbid it.

I google around a bit, but still don't get the scope stuff.  Are they
really part of an "ipv6 address"?  I thought an ipv6 address was just a
128-bit number?  (Or do they just give another way of writing something
that you could already write without the %?)

--b.

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[Chuck Lever]

On Wed, Aug 20, 2008 at 7:30 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Wed, Aug 20, 2008 at 06:07:50PM -0400, Chuck Lever wrote:
>> What may be confusing you is that scope delimiters are used almost
>> exclusively for link-local addresses, which are valid only on the
>> local host.
>>
>> If you don't want to handle a referral that uses a link-local address,
>> or you don't want to handle a link-local address with a scope ID, then
>> there are explicit checks you can do.
>
> Well, the current code does allow a referral to point to 127.0.0.1.  I
> don't know what to think of that; it seems unlikely to be useful for
> anything but testing, and possibly succeptible to abuse, but of course
> the protocol doesn't forbid it.

Link-local is not the same as loopback.

A link-local address is an address that is constructed by a
standards-defined method for each active NIC on a host.  It is not a
route-able address, so it's good only for the local subnet.  It's a
method by which network service is exposed to higher levels in the
network layer without having to use an external service like DHCP.
It's a bootstrap address, more or less.

> I google around a bit, but still don't get the scope stuff.  Are they
> really part of an "ipv6 address"?  I thought an ipv6 address was just a
> 128-bit number?  (Or do they just give another way of writing something
> that you could already write without the %?)

Unfortunately good IPv6 documentation is pretty hard to find.  There
are a lot of RFCs that specify just a little corner of IPv6 (and even
those usually have corrections or amendments in later RFCs), so it's
difficult to know where to begin.  IPv6 is nearly a complete
reconception of the internet, so many IPv4 concepts simply don't
translate.  I don't think there's anything equivalent to a scope
delimiter in IPv4.  The following is my understanding of how this
works.

This is really a question of how to map a presentation format address
(a string that represents the numeric form of the address, like
dotted-quad notation) to a sockaddr.

The sockaddr_sin6 structure has a field for the scope ID,
sin6_scope_id.  The % delimits the host address from a string that
identifies a local NIC.  You can specify a device name, like "eth0,"
or a number, which means the same thing.  The number is referred to as
an interface index, or a scope ID.  The device name is converted by a
locally-defined mechanism to a scope ID (the numeric equivalent) and
stored in the sin6_scope_id field of a sockaddr_sin6.

Link-local addresses require a specific NIC to be identified to work
properly, even if there is only a single NIC on the host, since the
"lo" virtual interface also has an interface index.  You can also use
a scope ID with a non-link-local address to force which NIC is used.
By and large, these interface indices are not meaningful anywhere but
on the host that has that link-local address assigned to one of its
NICs.

It really depends on how the referral hostname string is generated,
but I would say that whatever follows the % should be ignored, or you
could generate a warning or error.  You can let the existing super.c
address parser handle the '%' and check to see if sin6_scope_id is
non-zero on return (or just zero it unconditionally if you want to
ignore these altogether).  If the NFSv4 standard doesn't say anything
about IPv6 interface IDs, then I suppose we are free to treat this any
way we think is reasonable.

I'm still not quite sure how to handle link-local addresses for NFSv4
callback, for example.  Generally it doesn't make sense to hand the
server an interface ID that is valid only on the client, so mount.nfs
just strips the interface ID when generating the callback IP address
string: it won't pass a scope-delimited presentation format address in
the clientaddr= option.

For a link-local callback address, the server then must determine
which of its own interfaces it must use to contact the client via that
link-local address, and append it via a % or by setting the
sin6_scope_id field.  Or it can simply decide not to call a client
back that passed it a link-local address as a callback address.

-- 
Chuck Lever

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[J. Bruce Fields]

On Wed, Aug 20, 2008 at 10:00:25PM -0400, Chuck Lever wrote:
> On Wed, Aug 20, 2008 at 7:30 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > On Wed, Aug 20, 2008 at 06:07:50PM -0400, Chuck Lever wrote:
> >> What may be confusing you is that scope delimiters are used almost
> >> exclusively for link-local addresses, which are valid only on the
> >> local host.
> >>
> >> If you don't want to handle a referral that uses a link-local address,
> >> or you don't want to handle a link-local address with a scope ID, then
> >> there are explicit checks you can do.
> >
> > Well, the current code does allow a referral to point to 127.0.0.1.  I
> > don't know what to think of that; it seems unlikely to be useful for
> > anything but testing, and possibly succeptible to abuse, but of course
> > the protocol doesn't forbid it.
> 
> Link-local is not the same as loopback.
> 
> A link-local address is an address that is constructed by a
> standards-defined method for each active NIC on a host.  It is not a
> route-able address, so it's good only for the local subnet.  It's a
> method by which network service is exposed to higher levels in the
> network layer without having to use an external service like DHCP.
> It's a bootstrap address, more or less.
> 
> > I google around a bit, but still don't get the scope stuff.  Are they
> > really part of an "ipv6 address"?  I thought an ipv6 address was just a
> > 128-bit number?  (Or do they just give another way of writing something
> > that you could already write without the %?)
> 
> Unfortunately good IPv6 documentation is pretty hard to find.  There
> are a lot of RFCs that specify just a little corner of IPv6 (and even
> those usually have corrections or amendments in later RFCs), so it's
> difficult to know where to begin.  IPv6 is nearly a complete
> reconception of the internet, so many IPv4 concepts simply don't
> translate.  I don't think there's anything equivalent to a scope
> delimiter in IPv4.  The following is my understanding of how this
> works.
> 
> This is really a question of how to map a presentation format address
> (a string that represents the numeric form of the address, like
> dotted-quad notation) to a sockaddr.

OK, but what the v4 rfc says isn't that the fs_location field can
contain any representation of a sockaddr; it says that it can contain "a
traditional DNS host name, IPv4 address, or IPv6 address."

Every definition of "IPv6 address" I can find says that it's just the
128 bits you put in sin6_addr.  The scope id field, like a port number,
is something in addition.

So if we see such a string we should just assume it's garbage.

> The sockaddr_sin6 structure has a field for the scope ID,
> sin6_scope_id.  The % delimits the host address from a string that
> identifies a local NIC.  You can specify a device name, like "eth0,"
> or a number, which means the same thing.  The number is referred to as
> an interface index, or a scope ID.  The device name is converted by a
> locally-defined mechanism to a scope ID (the numeric equivalent) and
> stored in the sin6_scope_id field of a sockaddr_sin6.
>
> Link-local addresses require a specific NIC to be identified to work
> properly, even if there is only a single NIC on the host, since the
> "lo" virtual interface also has an interface index.  You can also use
> a scope ID with a non-link-local address to force which NIC is used.
> By and large, these interface indices are not meaningful anywhere but
> on the host that has that link-local address assigned to one of its
> NICs.

OK, so that definitely doesn't sound like something that makes sense
outside the client.

> It really depends on how the referral hostname string is generated,
> but I would say that whatever follows the % should be ignored, or you
> could generate a warning or error.

So I think we should be harsher and just not attempt to parse a string
that looks like this at all.

--b.

> You can let the existing super.c
> address parser handle the '%' and check to see if sin6_scope_id is
> non-zero on return (or just zero it unconditionally if you want to
> ignore these altogether).  If the NFSv4 standard doesn't say anything
> about IPv6 interface IDs, then I suppose we are free to treat this any
> way we think is reasonable.
> 
> I'm still not quite sure how to handle link-local addresses for NFSv4
> callback, for example.  Generally it doesn't make sense to hand the
> server an interface ID that is valid only on the client, so mount.nfs
> just strips the interface ID when generating the callback IP address
> string: it won't pass a scope-delimited presentation format address in
> the clientaddr= option.
> 
> For a link-local callback address, the server then must determine
> which of its own interfaces it must use to contact the client via that
> link-local address, and append it via a % or by setting the
> sin6_scope_id field.  Or it can simply decide not to call a client
> back that passed it a link-local address as a callback address.
> 
> -- 
> Chuck Lever

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[Chuck Lever]

On Thu, Aug 21, 2008 at 4:46 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Wed, Aug 20, 2008 at 10:00:25PM -0400, Chuck Lever wrote:
>> On Wed, Aug 20, 2008 at 7:30 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > On Wed, Aug 20, 2008 at 06:07:50PM -0400, Chuck Lever wrote:
>> >> What may be confusing you is that scope delimiters are used almost
>> >> exclusively for link-local addresses, which are valid only on the
>> >> local host.
>> >>
>> >> If you don't want to handle a referral that uses a link-local address,
>> >> or you don't want to handle a link-local address with a scope ID, then
>> >> there are explicit checks you can do.
>> >
>> > Well, the current code does allow a referral to point to 127.0.0.1.  I
>> > don't know what to think of that; it seems unlikely to be useful for
>> > anything but testing, and possibly succeptible to abuse, but of course
>> > the protocol doesn't forbid it.
>>
>> Link-local is not the same as loopback.
>>
>> A link-local address is an address that is constructed by a
>> standards-defined method for each active NIC on a host.  It is not a
>> route-able address, so it's good only for the local subnet.  It's a
>> method by which network service is exposed to higher levels in the
>> network layer without having to use an external service like DHCP.
>> It's a bootstrap address, more or less.
>>
>> > I google around a bit, but still don't get the scope stuff.  Are they
>> > really part of an "ipv6 address"?  I thought an ipv6 address was just a
>> > 128-bit number?  (Or do they just give another way of writing something
>> > that you could already write without the %?)
>>
>> Unfortunately good IPv6 documentation is pretty hard to find.  There
>> are a lot of RFCs that specify just a little corner of IPv6 (and even
>> those usually have corrections or amendments in later RFCs), so it's
>> difficult to know where to begin.  IPv6 is nearly a complete
>> reconception of the internet, so many IPv4 concepts simply don't
>> translate.  I don't think there's anything equivalent to a scope
>> delimiter in IPv4.  The following is my understanding of how this
>> works.
>>
>> This is really a question of how to map a presentation format address
>> (a string that represents the numeric form of the address, like
>> dotted-quad notation) to a sockaddr.
>
> OK, but what the v4 rfc says isn't that the fs_location field can
> contain any representation of a sockaddr; it says that it can contain "a
> traditional DNS host name, IPv4 address, or IPv6 address."

Since these are strings, clearly what is intended is "presentation
format IP address."  A reference to an RFC that defines what this
means might be nice here.

In fact, this is a bit inconsistent of the NFSv4 spec.  A universal
address string is passed via SETCLIENTID, and via rpcbind.  Why
wouldn't you use a universal address here as well?

Of course, scope delimiting isn't supported by universal addresses at all!

> Every definition of "IPv6 address" I can find says that it's just the
> 128 bits you put in sin6_addr.  The scope id field, like a port number,
> is something in addition.

Let me point out that you can't even create a socket to a server
unless you know both an IP address and a destination port number.  For
NFSv2/v3, this is determined via an rpcbind lookup.  For NFSv4, you
usually use the default NFS port number, but I now see that you can't
perform a referral to an NFSv4 server that is serving requests only
via an alternate port number.  This would be one reason to use a
universal address when referring.

> So if we see such a string we should just assume it's garbage.

Well, maybe.  Just because we don't support it doesn't mean it's
garbage.  We don't support NFS URLs, but they aren't garbage; they are
unsupported.

>> The sockaddr_sin6 structure has a field for the scope ID,
>> sin6_scope_id.  The % delimits the host address from a string that
>> identifies a local NIC.  You can specify a device name, like "eth0,"
>> or a number, which means the same thing.  The number is referred to as
>> an interface index, or a scope ID.  The device name is converted by a
>> locally-defined mechanism to a scope ID (the numeric equivalent) and
>> stored in the sin6_scope_id field of a sockaddr_sin6.
>>
>> Link-local addresses require a specific NIC to be identified to work
>> properly, even if there is only a single NIC on the host, since the
>> "lo" virtual interface also has an interface index.  You can also use
>> a scope ID with a non-link-local address to force which NIC is used.
>> By and large, these interface indices are not meaningful anywhere but
>> on the host that has that link-local address assigned to one of its
>> NICs.
>
> OK, so that definitely doesn't sound like something that makes sense
> outside the client.

I'm not sure what you mean.  Servers can have these too.  Maybe you
mean "doesn't make sense outside the context of the local host."

It seems to me that in some cases an IPv6 link-local address with
scope delimiter could be a valid referral address, however.  For
example, if you have one client and several servers on the same
subnet, they share an administrator, and have only link-local
addresses.  In that case it is perfectly valid and desirable to
support link-local addresses and scope delimiters.

It is also perfectly valid if a DNS lookup on the client (supported at
some point in the future) returns a link-local address with a scope
delimiter for a referred server.  The lookup results could come right
out of /etc/hosts.

>> It really depends on how the referral hostname string is generated,
>> but I would say that whatever follows the % should be ignored, or you
>> could generate a warning or error.
>
> So I think we should be harsher and just not attempt to parse a string
> that looks like this at all.

I don't think you want to add a lot of specialized logic in
nfs4namespace.c or client.c or where ever that has to figure out what
a valid IP address string looks like.  Just hand the referral hostname
string to the super.c address parser.  That's what it's there for.

If something comes out, it's almost sure to be a valid address.  If
you have a scope ID too, then it's unsupported for now, and punt.  You
can also explicitly check for a link-local address (there is a
facility for that) and punt in that case too.

  ipv6_addr_type(addr) & IPV6_ADDR_LINKLOCAL

But that would require #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)

See include/net/ipv6.h.

>> You can let the existing super.c
>> address parser handle the '%' and check to see if sin6_scope_id is
>> non-zero on return (or just zero it unconditionally if you want to
>> ignore these altogether).  If the NFSv4 standard doesn't say anything
>> about IPv6 interface IDs, then I suppose we are free to treat this any
>> way we think is reasonable.
>>
>> I'm still not quite sure how to handle link-local addresses for NFSv4
>> callback, for example.  Generally it doesn't make sense to hand the
>> server an interface ID that is valid only on the client, so mount.nfs
>> just strips the interface ID when generating the callback IP address
>> string: it won't pass a scope-delimited presentation format address in
>> the clientaddr= option.
>>
>> For a link-local callback address, the server then must determine
>> which of its own interfaces it must use to contact the client via that
>> link-local address, and append it via a % or by setting the
>> sin6_scope_id field.  Or it can simply decide not to call a client
>> back that passed it a link-local address as a callback address.

-- 
"If you simplify your English, you are freed from the worst follies of
orthodoxy."
 -- George Orwell

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[J. Bruce Fields]

On Thu, Aug 21, 2008 at 06:22:02PM -0400, Chuck Lever wrote:
> I don't think you want to add a lot of specialized logic in
> nfs4namespace.c or client.c or where ever that has to figure out what
> a valid IP address string looks like.  Just hand the referral hostname
> string to the super.c address parser.  That's what it's there for.
> 
> If something comes out, it's almost sure to be a valid address.  If
> you have a scope ID too, then it's unsupported for now, and punt.  You
> can also explicitly check for a link-local address (there is a
> facility for that) and punt in that case too.

The case where the scope id is bad or the kstrndup() in
nfs_parse_ipv6_scope_id fails seems to be indistinguishable from the
case where there is no scope id specified.

So if I want to rule out scope id's in the referred-to server name
entirely, then I do need to continue checking for that before calling.

--b.

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[Chuck Lever]

On Thu, Aug 21, 2008 at 6:54 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Thu, Aug 21, 2008 at 06:22:02PM -0400, Chuck Lever wrote:
>> I don't think you want to add a lot of specialized logic in
>> nfs4namespace.c or client.c or where ever that has to figure out what
>> a valid IP address string looks like.  Just hand the referral hostname
>> string to the super.c address parser.  That's what it's there for.
>>
>> If something comes out, it's almost sure to be a valid address.  If
>> you have a scope ID too, then it's unsupported for now, and punt.  You
>> can also explicitly check for a link-local address (there is a
>> facility for that) and punt in that case too.
>
> The case where the scope id is bad or the kstrndup() in
> nfs_parse_ipv6_scope_id fails seems to be indistinguishable from the
> case where there is no scope id specified.

This sounds like nfs_parse_ipv6_scope_id() is broken.  You mean if the
hostname just ends with a '%' ?

-- 
Chuck Lever

Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[Chuck Lever]

On Thu, Aug 21, 2008 at 7:05 PM, Chuck Lever <chucklever@gmail.com> wrote:
> On Thu, Aug 21, 2008 at 6:54 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> On Thu, Aug 21, 2008 at 06:22:02PM -0400, Chuck Lever wrote:
>>> I don't think you want to add a lot of specialized logic in
>>> nfs4namespace.c or client.c or where ever that has to figure out what
>>> a valid IP address string looks like.  Just hand the referral hostname
>>> string to the super.c address parser.  That's what it's there for.
>>>
>>> If something comes out, it's almost sure to be a valid address.  If
>>> you have a scope ID too, then it's unsupported for now, and punt.  You
>>> can also explicitly check for a link-local address (there is a
>>> facility for that) and punt in that case too.
>>
>> The case where the scope id is bad or the kstrndup() in
>> nfs_parse_ipv6_scope_id fails seems to be indistinguishable from the
>> case where there is no scope id specified.
>
> This sounds like nfs_parse_ipv6_scope_id() is broken.  You mean if the
> hostname just ends with a '%' ?

I just sent you a patch that should address this issue.  Compile-tested only.

-- 
Chuck Lever