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