LinuxLists.cc - referrals

[permalink] [raw]

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

On Sun, May 18, 2008 at 11:22:18AM -0400, Chuck Lever wrote:
> On May 16, 2008, at 3:53 PM, J. Bruce Fields wrote:
>> On Sat, May 10, 2008 at 09:07:23PM -0400, david m. richter wrote:
>>> On Sat, May 10, 2008 at 7:50 PM, Chuck Lever
>>> <[email protected]> wrote:
>>>> On May 10, 2008, at 10:32 AM, Trond Myklebust wrote:
>>>>>
>>>>> On Fri, 2008-05-09 at 19:29 -0700, Chuck Lever wrote:
>>>>>>
>>>>>> Should you use in4_pton() instead?
>>>>>
>>>>> Can we rather convert this to use nfs_parse_server_address? We
>>>>> don't
>>>>> need 10 different ways to parse text addresses...
>>>>
>>>> I'm OK with that, as long as there isn't a technical problem with
>>>> using
>>>> in4_pton().
>>>
>>> nfs_parse_server_address() uses in4_pton(), it just also groks ipv6.
>>
>> This is all a bit orthogonal to the actual bug, as all those functions
>> want null-terminated strings too.
>>
>> We could apply the below (compile-tested only) and then add ipv6
>> support
>> and converting to nfs_parse_server_address() in a subsequent patch.
>
> I'm looking at this code for other reasons, but it would be very easy to
> teach nfs_parse_server_address() to take a string length and not assume
> the passed-in address string is null-terminated. Both in4_pton and
> in6_pton will take a string length.

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

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

--b.

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

nfs: Fix misparsing of nfsv4 fs_locations attribute

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

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

diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index 5f9ba41..018292d 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,19 +155,20 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,

s = 0;
while (s < location->nservers) {
+ const struct nfs4_string *buf = &location->servers[s];
struct sockaddr_in addr = {
.sin_family = AF_INET,
.sin_port = htons(NFS_PORT),
};
+ u8 *ip = (u8 *)addr.sin_addr.s_addr;

- if (location->servers[s].len <= 0 ||
- valid_ipaddr4(location->servers[s].data) < 0) {
- s++;
- continue;
- }
+ if (buf->len <= 0 || buf->len >= PAGE_SIZE)
+ goto next;
+ if (!in4_pton(buf->data, buf->len, ip, '\0', NULL))
+ goto next;

- mountdata.hostname = location->servers[s].data;
- addr.sin_addr.s_addr = in_aton(mountdata.hostname),
+ mountdata.hostname = kmalloc(buf->len + 1, GFP_KERNEL);
+ mountdata.hostname[buf->len] = 0;
mountdata.addr = (struct sockaddr *)&addr;
mountdata.addrlen = sizeof(addr);

@@ -193,9 +177,11 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,
mountdata.mnt_path);

mnt = vfs_kern_mount(&nfs4_referral_fs_type, 0, page, &mountdata);
+ kfree(mountdata.hostname);
if (!IS_ERR(mnt)) {
break;
}
+next:
s++;
}
loc++;

2008-05-20 18:08:46

[permalink] [raw]

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

On May 19, 2008, at 10:47 PM, J. Bruce Fields wrote:
> On Sun, May 18, 2008 at 11:22:18AM -0400, Chuck Lever wrote:
>> On May 16, 2008, at 3:53 PM, J. Bruce Fields wrote:
>>> On Sat, May 10, 2008 at 09:07:23PM -0400, david m. richter wrote:
>>>> On Sat, May 10, 2008 at 7:50 PM, Chuck Lever
>>>> <[email protected]> wrote:
>>>>> On May 10, 2008, at 10:32 AM, Trond Myklebust wrote:
>>>>>>
>>>>>> On Fri, 2008-05-09 at 19:29 -0700, Chuck Lever wrote:
>>>>>>>
>>>>>>> Should you use in4_pton() instead?
>>>>>>
>>>>>> Can we rather convert this to use nfs_parse_server_address? We
>>>>>> don't
>>>>>> need 10 different ways to parse text addresses...
>>>>>
>>>>> I'm OK with that, as long as there isn't a technical problem with
>>>>> using
>>>>> in4_pton().
>>>>
>>>> nfs_parse_server_address() uses in4_pton(), it just also groks
>>>> ipv6.
>>>
>>> This is all a bit orthogonal to the actual bug, as all those
>>> functions
>>> want null-terminated strings too.
>>>
>>> We could apply the below (compile-tested only) and then add ipv6
>>> support
>>> and converting to nfs_parse_server_address() in a subsequent patch.
>>
>> I'm looking at this code for other reasons, but it would be very
>> easy to
>> teach nfs_parse_server_address() to take a string length and not
>> assume
>> the passed-in address string is null-terminated. Both in4_pton and
>> in6_pton will take a string length.
>
> Whoops, I missed the srclen argument to in4_pton and in6_pton.
>
> Though I just noticed it doesn't really matter much, since the
> mountdata.hostname needs a null-terminated string.

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

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

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

> commit 109f9a666db58e0511ac5a417e767027b148a9e0
> Author: J. Bruce Fields <[email protected]>
> Date: Fri May 9 15:10:56 2008 -0700
>
> nfs: Fix misparsing of nfsv4 fs_locations attribute
>
> The code incorrectly assumes here that the server name (or ip
> address)
> is null-terminated. This can cause referrals to fail in some
> cases.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> index 5f9ba41..018292d 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,19 +155,20 @@ static struct vfsmount
> *nfs_follow_referral(const struct vfsmount *mnt_parent,
>
> s = 0;
> while (s < location->nservers) {
> + const struct nfs4_string *buf = &location->servers[s];
> struct sockaddr_in addr = {
> .sin_family = AF_INET,
> .sin_port = htons(NFS_PORT),
> };
> + u8 *ip = (u8 *)addr.sin_addr.s_addr;
>
> - if (location->servers[s].len <= 0 ||
> - valid_ipaddr4(location->servers[s].data) < 0) {
> - s++;
> - continue;
> - }
> + if (buf->len <= 0 || buf->len >= PAGE_SIZE)
> + goto next;
> + if (!in4_pton(buf->data, buf->len, ip, '\0', NULL))
> + goto next;
>
> - mountdata.hostname = location->servers[s].data;
> - addr.sin_addr.s_addr = in_aton(mountdata.hostname),
> + mountdata.hostname = kmalloc(buf->len + 1, GFP_KERNEL);
> + mountdata.hostname[buf->len] = 0;
> mountdata.addr = (struct sockaddr *)&addr;
> mountdata.addrlen = sizeof(addr);
>
> @@ -193,9 +177,11 @@ static struct vfsmount
> *nfs_follow_referral(const struct vfsmount *mnt_parent,
> mountdata.mnt_path);
>
> mnt = vfs_kern_mount(&nfs4_referral_fs_type, 0, page, &mountdata);
> + kfree(mountdata.hostname);
> if (!IS_ERR(mnt)) {
> break;
> }
> +next:
> s++;
> }
> loc++;

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

2008-05-20 19:32:25

[permalink] [raw]

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

On Tue, 2008-05-20 at 12:54 -0400, Chuck Lever wrote:

> So, I haven't added IPv6 support in this area because it seems to
> assume an IPv4 address here, and it needs to handle the case where
> this might be a hostname (ie there should be a DNS resolution in this
> path).
>
> If you're going to call in4_pton(), it really should call
> nfs_parse_server_address() instead; then this path would magically
> support IPv6 addresses as well.
>
> But in the long run, this path will need either an upcall or it will
> have to use the key-based kernel DNS resolution facility (Jeff Layton
> mentioned something like this over lunch last week).

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

I've already started coding up a solution.

Trond

2008-05-20 19:39:44

[permalink] [raw]

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

On May 20, 2008, at 3:32 PM, Trond Myklebust wrote:
> On Tue, 2008-05-20 at 12:54 -0400, Chuck Lever wrote:
>
>> So, I haven't added IPv6 support in this area because it seems to
>> assume an IPv4 address here, and it needs to handle the case where
>> this might be a hostname (ie there should be a DNS resolution in this
>> path).
>>
>> If you're going to call in4_pton(), it really should call
>> nfs_parse_server_address() instead; then this path would magically
>> support IPv6 addresses as well.
>>
>> But in the long run, this path will need either an upcall or it will
>> have to use the key-based kernel DNS resolution facility (Jeff Layton
>> mentioned something like this over lunch last week).
>
> No. The correct way to do this is to replace the current code
> fs_locations handler so that we do the mount from userspace. In
> addition
> to voiding the whole issue of in-kernel DNS resolution, that also has
> the benefit that it allows us to put policy decisions like which
> server
> to mount from under administrator control in userspace.
>
> I've already started coding up a solution.

Okay, I remembered incorrectly.

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

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

2008-05-20 19:42:47

[permalink] [raw]

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

On Tue, 2008-05-20 at 15:38 -0400, Chuck Lever wrote:

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

I hadn't explicitly considered the problem of IPv6 addresses but yes, we
should get all that for free.

Trond

2008-05-09 05:10:47

[permalink] [raw]

Subject: Re: referrals

On Thu, 2008-05-08 at 21:19 -0400, J. Bruce Fields wrote:
> An attempt to follow an nfsv4 referral is leading to a hang. I'm doing
> an "ls" on the absent directory. A network trace shows the server
> returning with a sane-looking response to the getattr of fs_locations.
> I've appended the part of the sysrq-t trace for "ls". Any ideas?
>
> --b.

What kernel?

Trond

2008-05-09 15:27:56

[permalink] [raw]

Subject: Re: referrals

On Thu, May 08, 2008 at 10:10:39PM -0700, Trond Myklebust wrote:
> On Thu, 2008-05-08 at 21:19 -0400, J. Bruce Fields wrote:
> > An attempt to follow an nfsv4 referral is leading to a hang. I'm doing
> > an "ls" on the absent directory. A network trace shows the server
> > returning with a sane-looking response to the getattr of fs_locations.
> > I've appended the part of the sysrq-t trace for "ls". Any ideas?
> >
> > --b.
>
> What kernel?

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

--b.

2008-05-09 16:52:12

[permalink] [raw]

Subject: Re: referrals

On Fri, May 09, 2008 at 11:27:50AM -0400, bfields wrote:
> On Thu, May 08, 2008 at 10:10:39PM -0700, Trond Myklebust wrote:
> > On Thu, 2008-05-08 at 21:19 -0400, J. Bruce Fields wrote:
> > > An attempt to follow an nfsv4 referral is leading to a hang. I'm doing
> > > an "ls" on the absent directory. A network trace shows the server
> > > returning with a sane-looking response to the getattr of fs_locations.
> > > I've appended the part of the sysrq-t trace for "ls". Any ideas?
> > >
> > > --b.
> >
> > What kernel?
>
> It was a few unrelated nfsd and gss patches on top of
> e31a94ed371c70855eb30b77c490d6d85dd4da26, which is between 2.6.25 and
> 2.6.26-rc1 (but I think has all the nfs stuff that went into -rc1).
> Happy to retest with something different.

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

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

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

--b.

2008-05-09 17:12:13

[permalink] [raw]

Subject: Re: referrals

On Fri, May 09, 2008 at 12:52:04PM -0400, bfields wrote:
> On Fri, May 09, 2008 at 11:27:50AM -0400, bfields wrote:
> > On Thu, May 08, 2008 at 10:10:39PM -0700, Trond Myklebust wrote:
> > > On Thu, 2008-05-08 at 21:19 -0400, J. Bruce Fields wrote:
> > > > An attempt to follow an nfsv4 referral is leading to a hang. I'm doing
> > > > an "ls" on the absent directory. A network trace shows the server
> > > > returning with a sane-looking response to the getattr of fs_locations.
> > > > I've appended the part of the sysrq-t trace for "ls". Any ideas?
> > > >
> > > > --b.
> > >
> > > What kernel?
> >
> > It was a few unrelated nfsd and gss patches on top of
> > e31a94ed371c70855eb30b77c490d6d85dd4da26, which is between 2.6.25 and
> > 2.6.26-rc1 (but I think has all the nfs stuff that went into -rc1).
> > Happy to retest with something different.
>
> Ah-hah. The server was returning two fslocations records, both for the
> same server--one with the target server's ip address, one with a
> hostname for it. Once the server was modified to return only one record
> (with the ip address), everything worked.
>
> Of course it's a known limitation that the client only handles ip
> addresses, but a look at the code in
> fs/nfs/nfs4namespace.c:nfs_follow_referral() shows that it *tries* to
> skip over any non-ip addresses, so the presence of such shouldn't have
> changed behavior.
>
> So there must be something wrong with that code in the !valid_ipaddr4()
> case?

(Well, actually that should have been "the valid_ipaddr() < 0 case").

Anyway, looking at the code, I can't see anything wrong. Time to retry
the bad case and take a harder look, I guess.

--b.

2008-05-09 23:59:38

[permalink] [raw]

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

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

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

Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfs/nfs4namespace.c | 12 +++++++++---
1 files changed, 9 insertions(+), 3 deletions(-)

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

--b.

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

- rc = sscanf(buf, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
+ /*
+ * XXX: Depending on the knowledge that the following byte is
+ * either xdr padding or an xdr length that has already been
+ * copied:
+ */
+ buf->data[buf->len] = '\0';
+ rc = sscanf(buf->data, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
if (rc != 4)
return -EINVAL;
for (count = 0; count < 4; count++) {
@@ -178,7 +184,7 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,
};

if (location->servers[s].len <= 0 ||
- valid_ipaddr4(location->servers[s].data) < 0) {
+ valid_ipaddr4(&location->servers[s]) < 0) {
s++;
continue;
}
--
1.5.5.rc1

2008-05-10 00:19:29

by Benny Halevy

[permalink] [raw]

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

On May. 09, 2008, 16:59 -0700, "J. Bruce Fields" <[email protected]> wrote:
> From: J. Bruce Fields <[email protected]>
>
> The code incorrectly assumes here that the server name (or ip address)
> is null-terminated. This can cause referrals to fail in some cases.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/nfs/nfs4namespace.c | 12 +++++++++---
> 1 files changed, 9 insertions(+), 3 deletions(-)
>
> I think this was the bug causing the referral failures. There must be a
> less ugly solution, though.
>
> --b.
>
> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> index 5f9ba41..2684c65 100644
> --- a/fs/nfs/nfs4namespace.c
> +++ b/fs/nfs/nfs4namespace.c
> @@ -96,11 +96,17 @@ static int nfs4_validate_fspath(const struct vfsmount *mnt_parent,
> /*
> * Check if the string represents a "valid" IPv4 address
> */
> -static inline int valid_ipaddr4(const char *buf)
> +static inline int valid_ipaddr4(const struct nfs4_string *buf)
> {
> int rc, count, in[4];
>
> - rc = sscanf(buf, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
> + /*
> + * XXX: Depending on the knowledge that the following byte is
> + * either xdr padding or an xdr length that has already been
> + * copied:
> + */
> + buf->data[buf->len] = '\0';

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

Benny

> + rc = sscanf(buf->data, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
> if (rc != 4)
> return -EINVAL;
> for (count = 0; count < 4; count++) {
> @@ -178,7 +184,7 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,
> };
>
> if (location->servers[s].len <= 0 ||
> - valid_ipaddr4(location->servers[s].data) < 0) {
> + valid_ipaddr4(&location->servers[s]) < 0) {
> s++;
> continue;
> }

--
Benny Halevy
Software Architect
Tel/Fax: +972-3-647-8340
Mobile: +972-54-802-8340
US: +1-412-203-3187
[email protected]

Panasas, Inc.
Leader in Parallel Storage
http://www.panasas.com

2008-05-10 01:07:18

[permalink] [raw]

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

On Fri, May 09, 2008 at 05:15:47PM -0700, Benny Halevy wrote:
> On May. 09, 2008, 16:59 -0700, "J. Bruce Fields" <[email protected]> wrote:
> > From: J. Bruce Fields <[email protected]>
> >
> > The code incorrectly assumes here that the server name (or ip address)
> > is null-terminated. This can cause referrals to fail in some cases.
> >
> > Signed-off-by: J. Bruce Fields <[email protected]>
> > ---
> > fs/nfs/nfs4namespace.c | 12 +++++++++---
> > 1 files changed, 9 insertions(+), 3 deletions(-)
> >
> > I think this was the bug causing the referral failures. There must be a
> > less ugly solution, though.
> >
> > --b.
> >
> > diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> > index 5f9ba41..2684c65 100644
> > --- a/fs/nfs/nfs4namespace.c
> > +++ b/fs/nfs/nfs4namespace.c
> > @@ -96,11 +96,17 @@ static int nfs4_validate_fspath(const struct vfsmount *mnt_parent,
> > /*
> > * Check if the string represents a "valid" IPv4 address
> > */
> > -static inline int valid_ipaddr4(const char *buf)
> > +static inline int valid_ipaddr4(const struct nfs4_string *buf)
> > {
> > int rc, count, in[4];
> >
> > - rc = sscanf(buf, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
> > + /*
> > + * XXX: Depending on the knowledge that the following byte is
> > + * either xdr padding or an xdr length that has already been
> > + * copied:
> > + */
> > + buf->data[buf->len] = '\0';
>
> Hmm, can't that overflow if buf->len is word aligned?

The xdr here is:

struct fs_location4 {
utf8str_cis server<>;
pathname4 rootpath;
};

and we're adding the padding to the end of one of the elements of the
server<> array. So it's guaranteed to be followed by a 4-byte length.

Oh, and we're also keeping the result in a single page, so there's no
crossing of a page boundary.

So it's probably correct. It's too ugly to actually apply, though.

--b.

>
> Benny
>
> > + rc = sscanf(buf->data, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
> > if (rc != 4)
> > return -EINVAL;
> > for (count = 0; count < 4; count++) {
> > @@ -178,7 +184,7 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,
> > };
> >
> > if (location->servers[s].len <= 0 ||
> > - valid_ipaddr4(location->servers[s].data) < 0) {
> > + valid_ipaddr4(&location->servers[s]) < 0) {
> > s++;
> > continue;
> > }
>
>
> --
> Benny Halevy
> Software Architect
> Tel/Fax: +972-3-647-8340
> Mobile: +972-54-802-8340
> US: +1-412-203-3187
> [email protected]
>
> Panasas, Inc.
> Leader in Parallel Storage
> http://www.panasas.com

2008-05-10 02:31:08

[permalink] [raw]

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

Should you use in4_pton() instead?

On May 9, 2008, at 4:59 PM, J. Bruce Fields wrote:

> From: J. Bruce Fields <[email protected]>
>
> The code incorrectly assumes here that the server name (or ip address)
> is null-terminated. This can cause referrals to fail in some cases.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/nfs/nfs4namespace.c | 12 +++++++++---
> 1 files changed, 9 insertions(+), 3 deletions(-)
>
> I think this was the bug causing the referral failures. There must
> be a
> less ugly solution, though.
>
> --b.
>
> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> index 5f9ba41..2684c65 100644
> --- a/fs/nfs/nfs4namespace.c
> +++ b/fs/nfs/nfs4namespace.c
> @@ -96,11 +96,17 @@ static int nfs4_validate_fspath(const struct
> vfsmount *mnt_parent,
> /*
> * Check if the string represents a "valid" IPv4 address
> */
> -static inline int valid_ipaddr4(const char *buf)
> +static inline int valid_ipaddr4(const struct nfs4_string *buf)
> {
> int rc, count, in[4];
>
> - rc = sscanf(buf, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
> + /*
> + * XXX: Depending on the knowledge that the following byte is
> + * either xdr padding or an xdr length that has already been
> + * copied:
> + */
> + buf->data[buf->len] = '\0';
> + rc = sscanf(buf->data, "%d.%d.%d.%d", &in[0], &in[1], &in[2],
> &in[3]);
> if (rc != 4)
> return -EINVAL;
> for (count = 0; count < 4; count++) {
> @@ -178,7 +184,7 @@ static struct vfsmount
> *nfs_follow_referral(const struct vfsmount *mnt_parent,
> };
>
> if (location->servers[s].len <= 0 ||
> - valid_ipaddr4(location->servers[s].data) < 0) {
> + valid_ipaddr4(&location->servers[s]) < 0) {
> s++;
> continue;
> }
> --
> 1.5.5.rc1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"
> in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

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

2008-05-10 17:32:21

[permalink] [raw]

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

On Fri, 2008-05-09 at 19:29 -0700, Chuck Lever wrote:
> Should you use in4_pton() instead?

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

Trond

2008-05-10 23:50:54

[permalink] [raw]

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

On May 10, 2008, at 10:32 AM, Trond Myklebust wrote:
> On Fri, 2008-05-09 at 19:29 -0700, Chuck Lever wrote:
>> Should you use in4_pton() instead?
>
> Can we rather convert this to use nfs_parse_server_address? We don't
> need 10 different ways to parse text addresses...

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

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

2008-05-11 01:07:24

by david m. richter

[permalink] [raw]

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

On Sat, May 10, 2008 at 7:50 PM, Chuck Lever <[email protected]> wrote:
> On May 10, 2008, at 10:32 AM, Trond Myklebust wrote:
>>
>> On Fri, 2008-05-09 at 19:29 -0700, Chuck Lever wrote:
>>>
>>> Should you use in4_pton() instead?
>>
>> Can we rather convert this to use nfs_parse_server_address? We don't
>> need 10 different ways to parse text addresses...
>
> I'm OK with that, as long as there isn't a technical problem with using
> in4_pton().

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

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

2008-05-16 19:53:37

[permalink] [raw]

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

On Sat, May 10, 2008 at 09:07:23PM -0400, david m. richter wrote:
> On Sat, May 10, 2008 at 7:50 PM, Chuck Lever <[email protected]> wrote:
> > On May 10, 2008, at 10:32 AM, Trond Myklebust wrote:
> >>
> >> On Fri, 2008-05-09 at 19:29 -0700, Chuck Lever wrote:
> >>>
> >>> Should you use in4_pton() instead?
> >>
> >> Can we rather convert this to use nfs_parse_server_address? We don't
> >> need 10 different ways to parse text addresses...
> >
> > I'm OK with that, as long as there isn't a technical problem with using
> > in4_pton().
>
> nfs_parse_server_address() uses in4_pton(), it just also groks ipv6.

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

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

--b.

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

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

Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfs/nfs4namespace.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index 5f9ba41..40a0209 100644
--- a/fs/nfs/nfs4namespace.c
+++ b/fs/nfs/nfs4namespace.c
@@ -93,14 +93,21 @@ static int nfs4_validate_fspath(const struct vfsmount *mnt_parent,
return 0;
}

+#define MAX_IPADDR_STRLEN 40
/*
* Check if the string represents a "valid" IPv4 address
*/
-static inline int valid_ipaddr4(const char *buf)
+static inline int valid_ipaddr4(const struct nfs4_string *buf)
{
int rc, count, in[4];
+ char str[MAX_IPADDR_STRLEN];

- rc = sscanf(buf, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
+ if (buf->len >= MAX_IPADDR_STRLEN)
+ return -EINVAL;
+ memcpy(str, buf->data, buf->len);
+ str[buf->len] = '\0';
+
+ rc = sscanf(str, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
if (rc != 4)
return -EINVAL;
for (count = 0; count < 4; count++) {
@@ -178,7 +185,7 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,
};

if (location->servers[s].len <= 0 ||
- valid_ipaddr4(location->servers[s].data) < 0) {
+ valid_ipaddr4(&location->servers[s]) < 0) {
s++;
continue;
}
--
1.5.5.rc1

2008-05-17 02:29:23

[permalink] [raw]

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

On May 16, 2008, at 12:53 PM, J. Bruce Fields wrote:
> On Sat, May 10, 2008 at 09:07:23PM -0400, david m. richter wrote:
>> On Sat, May 10, 2008 at 7:50 PM, Chuck Lever
>> <[email protected]> wrote:
>>> On May 10, 2008, at 10:32 AM, Trond Myklebust wrote:
>>>>
>>>> On Fri, 2008-05-09 at 19:29 -0700, Chuck Lever wrote:
>>>>>
>>>>> Should you use in4_pton() instead?
>>>>
>>>> Can we rather convert this to use nfs_parse_server_address? We
>>>> don't
>>>> need 10 different ways to parse text addresses...
>>>
>>> I'm OK with that, as long as there isn't a technical problem with
>>> using
>>> in4_pton().
>>
>> nfs_parse_server_address() uses in4_pton(), it just also groks ipv6.
>
> This is all a bit orthogonal to the actual bug, as all those functions
> want null-terminated strings too.
>
> We could apply the below (compile-tested only) and then add ipv6
> support
> and converting to nfs_parse_server_address() in a subsequent patch.

It might make more sense to use the utility conversion functions that
are already provided, but just guarantee that we always pass them a
null-terminated string.

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

2008-05-18 15:30:31

[permalink] [raw]

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

On May 16, 2008, at 3:53 PM, J. Bruce Fields wrote:
> On Sat, May 10, 2008 at 09:07:23PM -0400, david m. richter wrote:
>> On Sat, May 10, 2008 at 7:50 PM, Chuck Lever
>> <[email protected]> wrote:
>>> On May 10, 2008, at 10:32 AM, Trond Myklebust wrote:
>>>>
>>>> On Fri, 2008-05-09 at 19:29 -0700, Chuck Lever wrote:
>>>>>
>>>>> Should you use in4_pton() instead?
>>>>
>>>> Can we rather convert this to use nfs_parse_server_address? We
>>>> don't
>>>> need 10 different ways to parse text addresses...
>>>
>>> I'm OK with that, as long as there isn't a technical problem with
>>> using
>>> in4_pton().
>>
>> nfs_parse_server_address() uses in4_pton(), it just also groks ipv6.
>
> This is all a bit orthogonal to the actual bug, as all those functions
> want null-terminated strings too.
>
> We could apply the below (compile-tested only) and then add ipv6
> support
> and converting to nfs_parse_server_address() in a subsequent patch.

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

> From 530b441f2239d8bcedf9456c3c570d9c179cb406 Mon Sep 17 00:00:00 2001
> From: J. Bruce Fields <[email protected]>
> Date: Fri, 9 May 2008 15:10:56 -0700
> Subject: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute
>
> The code incorrectly assumes here that the server name (or ip address)
> is null-terminated. This can cause referrals to fail in some cases.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/nfs/nfs4namespace.c | 13 ++++++++++---
> 1 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> index 5f9ba41..40a0209 100644
> --- a/fs/nfs/nfs4namespace.c
> +++ b/fs/nfs/nfs4namespace.c
> @@ -93,14 +93,21 @@ static int nfs4_validate_fspath(const struct
> vfsmount *mnt_parent,
> return 0;
> }
>
> +#define MAX_IPADDR_STRLEN 40
> /*
> * Check if the string represents a "valid" IPv4 address
> */
> -static inline int valid_ipaddr4(const char *buf)
> +static inline int valid_ipaddr4(const struct nfs4_string *buf)
> {
> int rc, count, in[4];
> + char str[MAX_IPADDR_STRLEN];
>
> - rc = sscanf(buf, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
> + if (buf->len >= MAX_IPADDR_STRLEN)
> + return -EINVAL;
> + memcpy(str, buf->data, buf->len);
> + str[buf->len] = '\0';
> +
> + rc = sscanf(str, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
> if (rc != 4)
> return -EINVAL;
> for (count = 0; count < 4; count++) {
> @@ -178,7 +185,7 @@ static struct vfsmount
> *nfs_follow_referral(const struct vfsmount *mnt_parent,
> };
>
> if (location->servers[s].len <= 0 ||
> - valid_ipaddr4(location->servers[s].data) < 0) {
> + valid_ipaddr4(&location->servers[s]) < 0) {
> s++;
> continue;
> }
> --
> 1.5.5.rc1
>

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

2008-08-20 20:08:29

[permalink] [raw]

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

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.

2008-08-20 20:10:25

[permalink] [raw]

Subject: [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.

Signed-off-by: J. Bruce Fields <[email protected]>
---
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

2008-08-20 20:10:25

[permalink] [raw]

Subject: [PATCH 1/4] nfs: break up nfs_follow_referral

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

Signed-off-by: J. Bruce Fields <[email protected]>
---
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

2008-08-20 20:10:25

[permalink] [raw]

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

Whoever wrote this had a bizarre allergy to for loops.

Signed-off-by: J. Bruce Fields <[email protected]>
---
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

2008-08-20 20:10:25

[permalink] [raw]

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

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(-)

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

2008-08-20 20:19:50

[permalink] [raw]

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

On Wed, Aug 20, 2008 at 4:08 PM, J. Bruce Fields <[email protected]> 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

2008-08-20 20:23:25

[permalink] [raw]

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

On Wed, Aug 20, 2008 at 4:10 PM, J. Bruce Fields <[email protected]> wrote:
> 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(-)
>
> 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

2008-08-20 20:47:53

[permalink] [raw]

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

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

2008-08-20 21:04:05

[permalink] [raw]

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

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 <[email protected]> wrote:
> > 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(-)
> >
> > 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.

2008-08-20 21:19:53

[permalink] [raw]

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

On Wed, Aug 20, 2008 at 4:47 PM, J. Bruce Fields <[email protected]> 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 <[email protected]> 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

2008-08-20 21:29:08

[permalink] [raw]

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

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

2008-08-20 22:07:52

[permalink] [raw]

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

On Wed, Aug 20, 2008 at 5:29 PM, J. Bruce Fields <[email protected]> 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 <[email protected]> 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 <[email protected]> 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

2008-08-20 23:30:25

[permalink] [raw]

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

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.

2008-08-21 02:00:27

[permalink] [raw]

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

On Wed, Aug 20, 2008 at 7:30 PM, J. Bruce Fields <[email protected]> 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

2008-08-21 20:46:22

[permalink] [raw]

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

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

2008-08-21 22:22:05

[permalink] [raw]

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

On Thu, Aug 21, 2008 at 4:46 PM, J. Bruce Fields <[email protected]> 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 <[email protected]> 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

2008-08-21 22:54:15

[permalink] [raw]

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

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.

2008-08-21 23:05:20

[permalink] [raw]

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

On Thu, Aug 21, 2008 at 6:54 PM, J. Bruce Fields <[email protected]> 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

2008-08-22 18:26:00