referrals

referrals

[J. Bruce Fields]

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.

May  8 19:33:02 piglet2 kernel: ls            D 00000046     0  3023   3006
May  8 19:33:02 piglet2 kernel:        ce527870 00000046 cf9181f0 00000046 00000000 00000000 cf9181f0 cf918450
May  8 19:33:02 piglet2 kernel:        cf918450 ce527850 c013976d c1201b88 00000246 ce527860 cf14a4c0 ce5278a8
May  8 19:33:02 piglet2 kernel:        c1201b88 ce527878 ce5278a0 00000000 ce5278a8 ce527878 c053b497 ce527894
May  8 19:33:02 piglet2 kernel: Call Trace:
May  8 19:33:02 piglet2 kernel:  [<c013976d>] ? trace_hardirqs_on+0x9d/0x110
May  8 19:33:02 piglet2 kernel:  [<c053b497>] rpc_wait_bit_killable+0x17/0x30
May  8 19:33:02 piglet2 kernel:  [<c058a045>] __wait_on_bit+0x55/0x80
May  8 19:33:02 piglet2 kernel:  [<c053b480>] ? rpc_wait_bit_killable+0x0/0x30
May  8 19:33:02 piglet2 kernel:  [<c053b480>] ? rpc_wait_bit_killable+0x0/0x30
May  8 19:33:02 piglet2 kernel:  [<c058a0b8>] out_of_line_wait_on_bit+0x48/0x50
May  8 19:33:02 piglet2 kernel:  [<c012f5b0>] ? wake_bit_function+0x0/0x50
May  8 19:33:02 piglet2 kernel:  [<c053be87>] __rpc_execute+0xa7/0x240
May  8 19:33:02 piglet2 kernel:  [<c053c047>] rpc_execute+0x17/0x20
May  8 19:33:02 piglet2 kernel:  [<c0534a25>] rpc_run_task+0x25/0x60
May  8 19:33:02 piglet2 kernel:  [<c0534b01>] rpc_call_sync+0x41/0x60
May  8 19:33:02 piglet2 kernel:  [<c0534b63>] rpc_ping+0x43/0x60
May  8 19:33:02 piglet2 kernel:  [<c053662c>] rpc_create+0x46c/0x510
May  8 19:33:02 piglet2 kernel:  [<c0139da2>] ? __lock_acquire+0x4b2/0xc30
May  8 19:33:02 piglet2 kernel:  [<c053cb61>] ? rpcauth_lookup_credcache+0xe1/0x220
May  8 19:33:02 piglet2 kernel:  [<c021ee18>] nfs_create_rpc_client+0xa8/0xe0
May  8 19:33:02 piglet2 kernel:  [<c058bbd7>] ? _spin_unlock+0x27/0x40
May  8 19:33:02 piglet2 kernel:  [<c021f547>] nfs4_set_client+0x67/0x170
May  8 19:33:02 piglet2 kernel:  [<c021fd6b>] nfs4_create_referral_server+0x7b/0x230
May  8 19:33:02 piglet2 kernel:  [<c0139da2>] ? __lock_acquire+0x4b2/0xc30
May  8 19:33:02 piglet2 last message repeated 2 times
May  8 19:33:02 piglet2 kernel:  [<c0165430>] ? poison_obj+0x20/0x40
May  8 19:33:02 piglet2 kernel:  [<c0228f91>] nfs4_referral_get_sb+0x31/0x190
May  8 19:33:02 piglet2 kernel:  [<c013976d>] ? trace_hardirqs_on+0x9d/0x110
May  8 19:33:02 piglet2 kernel:  [<c0165882>] ? check_poison_obj+0x22/0x1b0
May  8 19:33:02 piglet2 kernel:  [<c0165430>] ? poison_obj+0x20/0x40
May  8 19:33:02 piglet2 kernel:  [<c0165131>] ? dbg_redzone1+0x11/0x20
May  8 19:33:02 piglet2 kernel:  [<c0181375>] ? alloc_vfsmnt+0xd5/0x110
May  8 19:33:02 piglet2 kernel:  [<c0165a81>] ? cache_alloc_debugcheck_after+0x71/0x1a0
May  8 19:33:02 piglet2 kernel:  [<c01671b0>] ? __kmalloc+0x100/0x140
May  8 19:33:02 piglet2 kernel:  [<c016716e>] ? __kmalloc+0xbe/0x140
May  8 19:33:02 piglet2 kernel:  [<c0181375>] ? alloc_vfsmnt+0xd5/0x110
May  8 19:33:02 piglet2 kernel:  [<c0181375>] ? alloc_vfsmnt+0xd5/0x110
May  8 19:33:02 piglet2 kernel:  [<c016bc33>] vfs_kern_mount+0x53/0x120
May  8 19:33:02 piglet2 kernel:  [<c02464fd>] nfs_do_refmount+0x65d/0x680
May  8 19:33:02 piglet2 kernel:  [<c02317b2>] nfs_follow_mountpoint+0x232/0x410
May  8 19:33:02 piglet2 kernel:  [<c058bbd7>] ? _spin_unlock+0x27/0x40
May  8 19:33:02 piglet2 kernel:  [<c018017d>] ? mnt_drop_write+0x5d/0x120
May  8 19:33:02 piglet2 kernel:  [<c0174bb4>] do_follow_link+0x104/0x300
May  8 19:33:02 piglet2 kernel:  [<c017294c>] ? do_lookup+0x5c/0x170
May  8 19:33:02 piglet2 kernel:  [<c0172fd5>] __link_path_walk+0x575/0x7e0
May  8 19:33:02 piglet2 kernel:  [<c0173286>] path_walk+0x46/0xb0
May  8 19:33:02 piglet2 kernel:  [<c01734b8>] do_path_lookup+0x68/0x160
May  8 19:33:02 piglet2 kernel:  [<c017183d>] ? getname+0x9d/0xb0
May  8 19:33:02 piglet2 kernel:  [<c0173ec0>] __user_walk_fd+0x30/0x50
May  8 19:33:02 piglet2 kernel:  [<c016d8e9>] vfs_stat_fd+0x19/0x40
May  8 19:33:02 piglet2 kernel:  [<c0157eac>] ? handle_mm_fault+0xfc/0x5a0
May  8 19:33:02 piglet2 kernel:  [<c016d9e1>] vfs_stat+0x11/0x20
May  8 19:33:02 piglet2 kernel:  [<c016da04>] sys_stat64+0x14/0x30
May  8 19:33:02 piglet2 kernel:  [<c0132886>] ? up_read+0x16/0x30
May  8 19:33:02 piglet2 kernel:  [<c0103123>] ? restore_nocheck+0x12/0x15
May  8 19:33:02 piglet2 kernel:  [<c058d4c0>] ? do_page_fault+0x0/0x6c0
May  8 19:33:02 piglet2 kernel:  [<c013976d>] ? trace_hardirqs_on+0x9d/0x110
May  8 19:33:02 piglet2 kernel:  [<c0103123>] ? restore_nocheck+0x12/0x15
May  8 19:33:02 piglet2 kernel:  [<c01030c2>] syscall_call+0x7/0xb

Re: referrals

[Trond Myklebust]

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

Re: referrals

[J. Bruce Fields]

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.

Re: referrals

[J. Bruce Fields]

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.

Re: referrals

[J. Bruce Fields]

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.

[PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[J. Bruce Fields]

From: J. Bruce Fields <bfields@citi.umich.edu>

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 <bfields@citi.umich.edu>
---
 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

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

[Benny Halevy]

On May. 09, 2008, 16:59 -0700, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> From: J. Bruce Fields <bfields@citi.umich.edu>
> 
> 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 <bfields@citi.umich.edu>
> ---
>  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
bhalevy@panasas.com
 
Panasas, Inc.
Leader in Parallel Storage
www.panasas.com

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

[J. Bruce Fields]

On Fri, May 09, 2008 at 05:15:47PM -0700, Benny Halevy wrote:
> On May. 09, 2008, 16:59 -0700, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > From: J. Bruce Fields <bfields@citi.umich.edu>
> > 
> > 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 <bfields@citi.umich.edu>
> > ---
> >  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
> bhalevy@panasas.com
>  
> Panasas, Inc.
> Leader in Parallel Storage
> www.panasas.com

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

[Chuck Lever]

Should you use in4_pton() instead?

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

> From: J. Bruce Fields <bfields@citi.umich.edu>
>
> 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 <bfields@citi.umich.edu>
> ---
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

[Trond Myklebust]

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

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

[Chuck Lever]

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

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

[david m. richter]

On Sat, May 10, 2008 at 7:50 PM, Chuck Lever <chuck.lever@oracle.com> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[J. Bruce Fields]

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 <chuck.lever@oracle.com> 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 <bfields@citi.umich.edu>
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 <bfields@citi.umich.edu>
---
 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

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

[Chuck Lever]

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  
>> <chuck.lever@oracle.com> 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

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

[Chuck Lever]

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  
>> <chuck.lever@oracle.com> 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 <bfields@citi.umich.edu>
> 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 <bfields@citi.umich.edu>
> ---
> 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

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

[J. Bruce Fields]

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  
>>> <chuck.lever@oracle.com> 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 <bfields@citi.umich.edu>
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 <bfields@citi.umich.edu>

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++;

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

[Chuck Lever]

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
>>>> <chuck.lever@oracle.com> 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 <bfields@citi.umich.edu>
> 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 <bfields@citi.umich.edu>
>
> 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

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

[Trond Myklebust]

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

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

[Chuck Lever]

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

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

[Trond Myklebust]

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

[PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute

[J. Bruce Fields]

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

--b.

    nfs: Fix misparsing of nfsv4 fs_locations attribute
    
    The code incorrectly assumes here that the server name (or ip address)
    is null-terminated.  This can cause referrals to fail in some cases.
    
    Also support ipv6 addresses.
    
    Compile-tested only.

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

diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index b112857..c0f5191 100644
--- a/fs/nfs/nfs4namespace.c
+++ b/fs/nfs/nfs4namespace.c
@@ -93,23 +93,6 @@ static int nfs4_validate_fspath(const struct vfsmount *mnt_parent,
 	return 0;
 }
 
-/*
- * Check if the string represents a "valid" IPv4 address
- */
-static inline int valid_ipaddr4(const char *buf)
-{
-	int rc, count, in[4];
-
-	rc = sscanf(buf, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
-	if (rc != 4)
-		return -EINVAL;
-	for (count = 0; count < 4; count++) {
-		if (in[count] > 255)
-			return -EINVAL;
-	}
-	return 0;
-}
-
 /**
  * nfs_follow_referral - set up mountpoint when hitting a referral on moved error
  * @mnt_parent - mountpoint of parent directory
@@ -172,30 +155,44 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent,
 
 		s = 0;
 		while (s < location->nservers) {
-			struct sockaddr_in addr = {
-				.sin_family	= AF_INET,
-				.sin_port	= htons(NFS_PORT),
-			};
-
-			if (location->servers[s].len <= 0 ||
-			    valid_ipaddr4(location->servers[s].data) < 0) {
-				s++;
-				continue;
-			}
+			const struct nfs4_string *buf = &location->servers[s];
+			struct sockaddr_storage addr;
+
+			if (buf->len <= 0 || buf->len >= PAGE_SIZE)
+				goto next;
 
-			mountdata.hostname = location->servers[s].data;
-			addr.sin_addr.s_addr = in_aton(mountdata.hostname),
 			mountdata.addr = (struct sockaddr *)&addr;
-			mountdata.addrlen = sizeof(addr);
+
+			if (memchr(buf->data, '%', buf->len))
+				goto next;
+			nfs_parse_ip_address(buf->data, buf->len,
+					mountdata.addr, &mountdata.addrlen);
+			switch (mountdata.addr->sa_family) {
+			case AF_UNSPEC:
+				goto next;
+			case AF_INET:
+				((struct sockaddr_in *)&addr)->sin_port =
+							htons(NFS_PORT);
+				break;
+			case AF_INET6:
+				((struct sockaddr_in6 *)&addr)->sin6_port =
+							htons(NFS_PORT);
+				break;
+			}
+
+			mountdata.hostname = kmalloc(buf->len + 1, GFP_KERNEL);
+			mountdata.hostname[buf->len] = 0;
 
 			snprintf(page, PAGE_SIZE, "%s:%s",
 					mountdata.hostname,
 					mountdata.mnt_path);
 
 			mnt = vfs_kern_mount(&nfs4_referral_fs_type, 0, page, &mountdata);
+			kfree(mountdata.hostname);
 			if (!IS_ERR(mnt)) {
 				break;
 			}
+next:
 			s++;
 		}
 		loc++;
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 9abcd2b..1d10756 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -798,7 +798,7 @@ static void nfs_parse_ipv6_address(char *string, size_t str_len,
  * If there is a problem constructing the new sockaddr, set the address
  * family to AF_UNSPEC.
  */
-static void nfs_parse_ip_address(char *string, size_t str_len,
+void nfs_parse_ip_address(char *string, size_t str_len,
 				 struct sockaddr *sap, size_t *addr_len)
 {
 	unsigned int i, colons;
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 78a5922..62ca683 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -444,6 +444,8 @@ extern const struct inode_operations nfs_referral_inode_operations;
 extern int nfs_mountpoint_expiry_timeout;
 extern void nfs_release_automount_timer(void);
 
+void nfs_parse_ip_address(char *, size_t, struct sockaddr *, size_t *);
+
 /*
  * linux/fs/nfs/unlink.c
  */

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

[Chuck Lever]

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

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

Comments below...

>
>
> --b.
>
>    nfs: Fix misparsing of nfsv4 fs_locations attribute
>
>    The code incorrectly assumes here that the server name (or ip  
> address)
>    is null-terminated.  This can cause referrals to fail in some  
> cases.
>
>    Also support ipv6 addresses.
>
>    Compile-tested only.
>
>    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
>
> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> index b112857..c0f5191 100644
> --- a/fs/nfs/nfs4namespace.c
> +++ b/fs/nfs/nfs4namespace.c
> @@ -93,23 +93,6 @@ static int nfs4_validate_fspath(const struct  
> vfsmount *mnt_parent,
> 	return 0;
> }
>
> -/*
> - * Check if the string represents a "valid" IPv4 address
> - */
> -static inline int valid_ipaddr4(const char *buf)
> -{
> -	int rc, count, in[4];
> -
> -	rc = sscanf(buf, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]);
> -	if (rc != 4)
> -		return -EINVAL;
> -	for (count = 0; count < 4; count++) {
> -		if (in[count] > 255)
> -			return -EINVAL;
> -	}
> -	return 0;
> -}
> -
> /**
>  * nfs_follow_referral - set up mountpoint when hitting a referral  
> on moved error
>  * @mnt_parent - mountpoint of parent directory
> @@ -172,30 +155,44 @@ static struct vfsmount  
> *nfs_follow_referral(const struct vfsmount *mnt_parent,
>
> 		s = 0;
> 		while (s < location->nservers) {
> -			struct sockaddr_in addr = {
> -				.sin_family	= AF_INET,
> -				.sin_port	= htons(NFS_PORT),
> -			};
> -
> -			if (location->servers[s].len <= 0 ||
> -			    valid_ipaddr4(location->servers[s].data) < 0) {
> -				s++;
> -				continue;
> -			}
> +			const struct nfs4_string *buf = &location->servers[s];
> +			struct sockaddr_storage addr;
> +
> +			if (buf->len <= 0 || buf->len >= PAGE_SIZE)
> +				goto next;
>
> -			mountdata.hostname = location->servers[s].data;
> -			addr.sin_addr.s_addr = in_aton(mountdata.hostname),
> 			mountdata.addr = (struct sockaddr *)&addr;
> -			mountdata.addrlen = sizeof(addr);
> +
> +			if (memchr(buf->data, '%', buf->len))
> +				goto next;

Why are you looking for a '%' ?

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[Chuck Lever]

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

I didn't answer this question before.

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

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

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

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

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

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

[J. Bruce Fields]

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

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

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

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

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

Yeah, yeah....

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

Yup, done.

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

Agreed, done.

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

OK, done.

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

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

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

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

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

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

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

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

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

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

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

Yep, done.

--b.

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

[Chuck Lever]

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

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

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

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

-- 
Chuck Lever

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

[J. Bruce Fields]

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

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

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

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

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

Oh, sure, that'd be OK too.

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

--b.

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

[Chuck Lever]

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

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

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

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

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

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

-- 
Chuck Lever

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

[J. Bruce Fields]

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

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

--b.

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

[Chuck Lever]

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

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

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

-- 
Chuck Lever

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

[J. Bruce Fields]

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

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

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

--b.

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

[Chuck Lever]

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

Link-local is not the same as loopback.

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

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

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

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

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

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

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

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

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

-- 
Chuck Lever

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

[J. Bruce Fields]

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

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

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

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

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

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

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

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

--b.

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

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

[Chuck Lever]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

  ipv6_addr_type(addr) & IPV6_ADDR_LINKLOCAL

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

See include/net/ipv6.h.

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

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

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

[J. Bruce Fields]

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

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

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

--b.

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

[Chuck Lever]

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

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

-- 
Chuck Lever

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

[Chuck Lever]

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

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

-- 
Chuck Lever