nfs: Fix misparsing of nfsv4 fs_locations attribute
Subsystems (2)
PATCH 1/1 nfs: Fix misparsing of nfsv4 fs_locations attribute
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.
fs/nfs/nfs4namespace.c +27 -30
@@ -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++;
fs/nfs/super.c +1 -1
@@ -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;
include/linux/nfs_fs.h +2 -0
@@ -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 */
Quoted 6 lines · J. Bruce Fields
I was looking back at this bug with the misparsing of (non-mull-terminated)
fs_locationsattributes. Thanks to the work onnfs_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 underCONFIG_IPV6? Is there a simpler way?
The use of the new address parser looks correct, but your string handling needs work. :-)
Comments below...
Quoted 65 lines
--b.
nfs: Fix misparsing of nfsv4
fs_locationsattributeThe 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 <[email protected]>fs/nfs/nfs4namespace.c +0
-17@@ -93,23 +93,6 @@ static int nfs4_validate_fspath(const structvfsmount *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 directoryfs/nfs/nfs4namespace.c +3
-10@@ -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; +
fs/nfs/nfs4namespace.c static struct vfsmount · line 168
Quoted 10 lines
fs/nfs/nfs4namespace.c +5
-3+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 '%' ?
fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 169
Quoted 2 lines
fs/nfs/nfs4namespace.c +2
-0+nfs_parse_ip_address (buf->data, buf->len, + mountdata.addr, &mountdata.addrlen);
Now what's so hard about that? :-)
fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 181
Quoted 13 lines
fs/nfs/nfs4namespace.c +12
-0+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.
fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 184
Quoted 3 lines
fs/nfs/nfs4namespace.c +3
-0+ + 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?
fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 188
Quoted 3 lines
fs/nfs/nfs4namespace.c +0
-0snprintf (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.
fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 191
Quoted 2 lines
fs/nfs/nfs4namespace.c +1
-0mnt =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.
Quoted 27 lines
fs/nfs/nfs4namespace.c +1
-0if (!IS_ERR (mnt)) {break ; } +next: s++; } loc++;fs/nfs/super.c +0
-0@@ -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 theaddress
* family toAF_UNSPEC.
*/fs/nfs/super.c +1
-1-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;include/linux/nfs_fs.h +0
-0
include/linux/nfs_fs.h extern const struct inode_operations · line 450
Quoted 7 lines
include/linux/nfs_fs.h +2
-0@@ -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.
Quoted 4 lines
include/linux/nfs_fs.h +0
-0/* *
linux/fs/nfs/unlink.c
*/
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
Quoted 7 lines · Chuck Lever
Quoted 6 lines · J. Bruce Fields
I was looking back at this bug with the misparsing of (non-mull-terminated)
fs_locationsattributes. Thanks to the work onnfs_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 underCONFIG_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.
Quoted 171 lines
Quoted 1 line
Is there a simpler way?
The use of the new address parser looks correct, but your string handling needs work. :-)
Comments below...
Quoted 64 lines
--b.
nfs: Fix misparsing of nfsv4
fs_locationsattributeThe 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 <[email protected]>fs/nfs/nfs4namespace.c +0
-17@@ -93,23 +93,6 @@ static int nfs4_validate_fspath(const structvfsmount *mnt_parent, return 0; } -/* -* Check if the string represents a "valid" IPv4 address -*/ -static inline int valid_ipaddr4 (const char *buf) -{ -int rc, count, in[4 ]; - - rc =sscanf (buf,"%d.%d.%d.%d" , &in[0 ], &in[1 ], &in[2 ], &in[3 ]); -if (rc !=4 ) -return -EINVAL ; -for (count =0 ; count <4 ; count++) { -if (in[count] >255 ) -return -EINVAL ; - } -return 0 ; -} -/** * nfs_follow_referral - set up mountpoint when hitting a referral on moved error * @mnt_parent - mountpoint of parent directory
fs/nfs/nfs4namespace.c +3
-10@@ -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; +fs/nfs/nfs4namespace.c static struct vfsmount · line 168
Quoted 10 lines
fs/nfs/nfs4namespace.c +5
-3+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 '%' ?
fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 169
Quoted 2 lines
fs/nfs/nfs4namespace.c +2
-0+nfs_parse_ip_address (buf->data, buf->len, + mountdata.addr, &mountdata.addrlen);Now what's so hard about that? :-)
fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 181
Quoted 13 lines
fs/nfs/nfs4namespace.c +12
-0+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 tonfs_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 theAF_UNSPECcheck were separate.In fact, as part of this patch you could add a static inline helper in
fs/nfs/internal.hto do the port setting. That can be shared betweenfs/nfs/super.candfs/nfs/nfs4namespace.c.Arguably the addition of the
AF_UNSPECcheck in the switch statement is an optimization that is slightly confusing. You are checking forAF_UNSPECto 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.
fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 184
Quoted 3 lines
fs/nfs/nfs4namespace.c +3
-0+ + 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?fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 188
Quoted 3 lines
fs/nfs/nfs4namespace.c +0
-0snprintf (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->lenandbuf->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.fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 191
Quoted 2 lines
fs/nfs/nfs4namespace.c +1
-0mnt =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-
Quoted 1 line
servers[s].data.
If location->servers[s].data isn't always NUL-terminated, you might make mountdata.hostname an
nfs4_stringso it carries the string length with it; the other users of mountdata.hostname can then see the length.Quoted 26 lines
if (!IS_ERR(mnt)) { break; } +next: s++; } loc++;fs/nfs/super.c +1
-1@@ -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;include/linux/nfs_fs.h +0
-0include/linux/nfs_fs.h extern const struct inode_operations · line 450
Quoted 7 lines
include/linux/nfs_fs.h +2
-0@@ -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.Quoted 1 line
Quoted 7 lines
Quoted 3 lines
/*
*linux/fs/nfs/unlink.c
*/--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
Quoted 9 lines · Chuck Lever
Quoted 7 lines · J. Bruce Fields
I was looking back at this bug with the misparsing of (non-mull-terminated)
fs_locationsattributes. Thanks to the work onnfs_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 underCONFIG_IPV6? Is there a simpler way?
Quoted 4 lines
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:
Quoted 2 lines
Quoted 2 lines
fs/nfs/nfs4namespace.c +0
-0
...
Quoted 4 lines
fs/nfs/nfs4namespace.c
Quoted 2 lines
fs/nfs/nfs4namespace.c +2
-0+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.
Quoted 5 lines
fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 169
Quoted 3 lines
fs/nfs/nfs4namespace.c +2
-0+nfs_parse_ip_address (buf->data, buf->len, + mountdata.addr, &mountdata.addrlen);Now what's so hard about that? :-)
Yeah, yeah....
Quoted 16 lines
fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 181
Quoted 13 lines
fs/nfs/nfs4namespace.c +12
-0+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.
Quoted 11 lines
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 tonfs_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 theAF_UNSPECcheck were separate.In fact, as part of this patch you could add a static inline helper in
fs/nfs/internal.hto do the port setting. That can be shared betweenfs/nfs/super.candfs/nfs/nfs4namespace.c.
Quoted 8 lines
Arguably the addition of the
AF_UNSPECcheck in the switch statement is an optimization that is slightly confusing. You are checking forAF_UNSPECto 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.
Quoted 8 lines
fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 184
Quoted 4 lines
fs/nfs/nfs4namespace.c +3
-0+ + 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.
Quoted 2 lines
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.
Quoted 2 lines
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.
Quoted 8 lines
Quoted 3 lines
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->lenandbuf->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.
Quoted 7 lines
fs/nfs/nfs4namespace.c static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, · line 191
Quoted 3 lines
mnt = vfs_kern_mount(&nfs4_referral_fs_type, 0, page, &mountdata);fs/nfs/nfs4namespace.c +1
-0+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-
Quoted 1 line
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.
Quoted 3 lines
If location->servers[s].data isn't always NUL-terminated, you might make mountdata.hostname an
nfs4_stringso 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).
Quoted 7 lines
Quoted 7 lines
include/linux/nfs_fs.h +0
-0@@ -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 );
Quoted 7 lines
include/linux/nfs_fs.h extern const struct inode_operations · line 450
Quoted 3 lines
include/linux/nfs_fs.h +2
-0+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
This function is a little longer and more deeply nested than necessary.
--- fs/nfs/nfs4namespace.c | 84 ++++++++++++++++++++++++++--------------------- 1 files changed, 46 insertions(+), 38 deletions(-)
fs/nfs/nfs4namespace.c +46 -38
@@ -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++; }
Signature · 1 line
1.5.5.rc1
PATCH 2/4 nfs: replace while loop by for loops in nfs_follow_referral
Whoever wrote this had a bizarre allergy to for loops.
--- fs/nfs/nfs4namespace.c | 17 +++++------------ 1 files changed, 5 insertions(+), 12 deletions(-)
fs/nfs/nfs4namespace.c +5 -12
@@ -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:
Signature · 1 line
1.5.5.rc1
PATCH 3/4 nfs: prepare to share nfs_set_port
We plan to use this function elsewhere.
--- fs/nfs/internal.h | 20 ++++++++++++++++++++ fs/nfs/super.c | 19 ------------------- 2 files changed, 20 insertions(+), 19 deletions(-)
fs/nfs/internal.h +20 -0
@@ -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 ; + } + } +}
fs/nfs/super.c +0 -19
@@ -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
Signature · 1 line
1.5.5.rc1
PATCH 4/4 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.
--- fs/nfs/internal.h | 2 ++ fs/nfs/nfs4namespace.c | 44 ++++++++++++++++++-------------------------- fs/nfs/super.c | 4 +--- 3 files changed, 21 insertions(+), 29 deletions(-)
fs/nfs/internal.h +2 -0
@@ -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
fs/nfs/nfs4namespace.c +18 -26
68 lines not on this page · raw message
fs/nfs/super.c +1 -3
@@ -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;
Signature · 1 line
1.5.5.rc1
Re: [PATCH 3/4] nfs: prepare to share nfs_set_port
In reply to J. Bruce FieldsQuoted 13 lines · J. Bruce Fields
We plan to use this function elsewhere.
Signed-off-by J. Bruce Fields <[email protected]>--- fs/nfs/internal.h | 20 ++++++++++++++++++++ fs/nfs/super.c | 19 ------------------- 2 files changed, 20 insertions(+), 19 deletions(-)fs/nfs/internal.h +0
-0
fs/nfs/internal.h unsigned int nfs_page_array_len(unsigned int base, size_t len) · line 279
Quoted 4 lines
fs/nfs/internal.h +1
-0@@ -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.
Quoted 9 lines
fs/nfs/internal.h +9
-0+/* +* 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;
fs/nfs/internal.h unsigned int nfs_page_array_len(unsigned int base, size_t len) · line 298
Quoted 10 lines
fs/nfs/internal.h +10
-0+ 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.
Signature · 1 line
Chuck Lever
A message that is not in the archive (sent off-list or not yet received): 76bd70e30808201323h32debdeaj31577cd19b87612e-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Re: [PATCH 3/4] nfs: prepare to share nfs_set_port
Quoted 35 lines · Chuck Lever
Quoted 13 lines · J. Bruce Fields
We plan to use this function elsewhere.
Signed-off-by J. Bruce Fields <[email protected]>--- fs/nfs/internal.h | 20 ++++++++++++++++++++ fs/nfs/super.c | 19 ------------------- 2 files changed, 20 insertions(+), 19 deletions(-)fs/nfs/internal.h +0
-0fs/nfs/internal.h unsigned int nfs_page_array_len(unsigned int base, size_t len) · line 279
Quoted 4 lines
fs/nfs/internal.h +1
-0@@ -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.
Quoted 14 lines
fs/nfs/internal.h +14
-0+/* +* 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;
Quoted 8 lines
fs/nfs/internal.h unsigned int nfs_page_array_len(unsigned int base, size_t len) · line 298
Quoted 5 lines
fs/nfs/internal.h +5
-0+ 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.
Quoted 24 lines · J. Bruce Fields
Quoted 9 lines · Chuck Lever
Quoted 7 lines · J. Bruce Fields
I was looking back at this bug with the misparsing of (non-mull-terminated)
fs_locationsattributes. Thanks to the work onnfs_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 underCONFIG_IPV6? Is there a simpler way?Quoted 4 lines
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:
Quoted 2 lines
Quoted 2 lines
fs/nfs/nfs4namespace.c +0
-0...
Quoted 3 lines
Quoted 2 lines
fs/nfs/nfs4namespace.c +2
-0+if (memchr (buf->data,'%' , buf->len)) + goto next;
Quoted 6 lines
Quoted 1 line
Why are you looking for a '%' ?
Would it have been clearer if I'd moved the
IPV6_SCOPE_DELIMITERdefine 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?
Signature · 1 line
Chuck Lever
A message that is not in the archive (sent off-list or not yet received): 76bd70e30808201319j7b59de5gc912fcd01594e8-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 27 lines · Chuck Lever
Quoted 26 lines · J. Bruce Fields
Quoted 9 lines · Chuck Lever
Quoted 7 lines · J. Bruce Fields
I was looking back at this bug with the misparsing of (non-mull-terminated)
fs_locationsattributes. Thanks to the work onnfs_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 underCONFIG_IPV6? Is there a simpler way?Quoted 4 lines
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:
Quoted 2 lines
Quoted 2 lines
fs/nfs/nfs4namespace.c +0
-0...
Quoted 4 lines
fs/nfs/nfs4namespace.c
Quoted 2 lines
fs/nfs/nfs4namespace.c +2
-0+if (memchr (buf->data,'%' , buf->len)) + goto next;Why are you looking for a '%' ?
Quoted 7 lines
Quoted 4 lines
Would it have been clearer if I'd moved the
IPV6_SCOPE_DELIMITERdefine 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.
Quoted 3 lines
Are you just looking for "illegal" or confusing characters? The address parser should handle all that and give you an
AF_UNSPECaddress 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.
Quoted 2 lines
Otherwise, if the returned sockaddr is an IPv6 address, can you just check if the
sin6_scope_ipfield 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.
Quoted 33 lines · J. Bruce Fields
Quoted 32 lines · Chuck Lever
Quoted 30 lines · J. Bruce Fields
Quoted 9 lines · Chuck Lever
Quoted 7 lines · J. Bruce Fields
I was looking back at this bug with the misparsing of (non-mull-terminated)
fs_locationsattributes. Thanks to the work onnfs_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 underCONFIG_IPV6? Is there a simpler way?Quoted 4 lines
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:
Quoted 2 lines
Quoted 2 lines
fs/nfs/nfs4namespace.c +0
-0...
Quoted 4 lines
fs/nfs/nfs4namespace.c
Quoted 2 lines
fs/nfs/nfs4namespace.c +2
-0+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_DELIMITERdefine 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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 7 lines
Quoted 3 lines
Are you just looking for "illegal" or confusing characters? The address parser should handle all that and give you an
AF_UNSPECaddress 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.
Quoted 8 lines
Quoted 2 lines
Otherwise, if the returned sockaddr is an IPv6 address, can you just check if the
sin6_scope_ipfield 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 '%' ?
Signature · 1 line
Chuck Lever
A message that is not in the archive (sent off-list or not yet received): 76bd70e30808201419g5171d7eob7e6b57dd735e07d-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 37 lines · Chuck Lever
Quoted 36 lines · J. Bruce Fields
Quoted 34 lines · Chuck Lever
Quoted 24 lines · J. Bruce Fields
Quoted 9 lines · Chuck Lever
Quoted 7 lines · J. Bruce Fields
I was looking back at this bug with the misparsing of (non-mull-terminated)
fs_locationsattributes. Thanks to the work onnfs_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 underCONFIG_IPV6? Is there a simpler way?Quoted 4 lines
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:
Quoted 2 lines
Quoted 2 lines
fs/nfs/nfs4namespace.c +0
-0...
Quoted 3 lines
Quoted 2 lines
fs/nfs/nfs4namespace.c +2
-0+if (memchr (buf->data,'%' , buf->len)) + goto next;Quoted 6 lines
Quoted 1 line
Why are you looking for a '%' ?
Would it have been clearer if I'd moved the
IPV6_SCOPE_DELIMITERdefine 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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 42 lines · J. Bruce Fields
Quoted 41 lines · Chuck Lever
Quoted 39 lines · J. Bruce Fields
Quoted 27 lines · Chuck Lever
Quoted 26 lines · J. Bruce Fields
Quoted 9 lines · Chuck Lever
Quoted 7 lines · J. Bruce Fields
I was looking back at this bug with the misparsing of (non-mull-terminated)
fs_locationsattributes. Thanks to the work onnfs_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 underCONFIG_IPV6? Is there a simpler way?Quoted 4 lines
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:
Quoted 2 lines
Quoted 2 lines
fs/nfs/nfs4namespace.c +0
-0...
Quoted 4 lines
fs/nfs/nfs4namespace.c
Quoted 2 lines
fs/nfs/nfs4namespace.c +2
-0+if (memchr (buf->data,'%' , buf->len)) + goto next;Why are you looking for a '%' ?
Quoted 7 lines
Quoted 4 lines
Would it have been clearer if I'd moved the
IPV6_SCOPE_DELIMITERdefine 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.
Quoted 5 lines
Quoted 2 lines
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.
Signature · 1 line
Chuck Lever
A message that is not in the archive (sent off-list or not yet received): 76bd70e30808201507l44c85d08o3ec4e8eeb7edda5e-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 7 lines · Chuck Lever
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.
Quoted 5 lines · J. Bruce Fields
Quoted 4 lines · Chuck Lever
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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 4 lines
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.
Signature · 1 line
Chuck Lever
A message that is not in the archive (sent off-list or not yet received): 76bd70e30808201900r699ca044o884584ecedc6a799-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 38 lines · Chuck Lever
Quoted 5 lines · J. Bruce Fields
Quoted 4 lines · Chuck Lever
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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 4 lines
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.
Quoted 3 lines
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.
Quoted 8 lines
The
sockaddr_sin6structure 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 thesin6_scope_idfield of asockaddr_sin6.
Quoted 7 lines
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.
Quoted 3 lines
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.
Quoted 22 lines
You can let the existing
super.c
address parser handle the '%' and check to see ifsin6_scope_idis 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_idfield. Or it can simply decide not to call a client back that passed it a link-local address as a callback address.Signature · 1 line
Chuck Lever
Quoted 39 lines · J. Bruce Fields
Quoted 38 lines · Chuck Lever
Quoted 5 lines · J. Bruce Fields
Quoted 4 lines · Chuck Lever
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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 4 lines
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.
Quoted 7 lines
Quoted 3 lines
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_locationfield 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!
Quoted 3 lines
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.
Quoted 1 line
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.
Quoted 16 lines
Quoted 8 lines
The
sockaddr_sin6structure 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 thesin6_scope_idfield of asockaddr_sin6.Quoted 7 lines
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.
Quoted 2 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 19 lines
Quoted 19 lines
You can let the existing
super.c
address parser handle the '%' and check to see ifsin6_scope_idis 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_idfield. Or it can simply decide not to call a client back that passed it a link-local address as a callback address.
Signature · 3 lines
"If you simplify your English, you are freed from the worst follies of orthodoxy." -- George Orwell
A message that is not in the archive (sent off-list or not yet received): 76bd70e30808211522k7cb6846fs4e371c8003320fe7-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 9 lines · Chuck Lever
I don't think you want to add a lot of specialized logic in
nfs4namespace.corclient.cor where ever that has to figure out what a valid IP address string looks like. Just hand the referral hostname string to thesuper.caddress 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.
Quoted 6 lines · J. Bruce Fields
Quoted 5 lines · Chuck Lever
I don't think you want to add a lot of specialized logic in
nfs4namespace.corclient.cor where ever that has to figure out what a valid IP address string looks like. Just hand the referral hostname string to thesuper.caddress parser. That's what it's there for.
Quoted 8 lines
Quoted 4 lines
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()innfs_parse_ipv6_scope_idfails 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 '%' ?
Signature · 1 line
Chuck Lever
A message that is not in the archive (sent off-list or not yet received): 76bd70e30808211605j3c32cc44v440c19e5fe81bdc9-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 12 lines · Chuck Lever
Quoted 11 lines · J. Bruce Fields
Quoted 9 lines · Chuck Lever
I don't think you want to add a lot of specialized logic in
nfs4namespace.corclient.cor where ever that has to figure out what a valid IP address string looks like. Just hand the referral hostname string to thesuper.caddress 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.
Quoted 6 lines
Quoted 3 lines
The case where the scope id is bad or the
kstrndup()innfs_parse_ipv6_scope_idfails 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.
Signature · 1 line
Chuck Lever