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 <[email protected]>
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
*/
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 <[email protected]>
>
> 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
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 <[email protected]>
>>
>> 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