miscellaneous nfs patches for 2.6.27

miscellaneous nfs patches for 2.6.27

[J. Bruce Fields]

This is just some small stuff that I have lying around.

By the way, I've been keeping the stuff I know of that's been submitted
on the client side in the "for-trond" branch here:

	git://linux-nfs.org/~bfields/linux.git for-trond

So, currently that's just these small things plus a bunch ot stuff from
Chuck.  Would it be any use for me to send that stuff out every now and
then, or do you already have all of it queued up someplace?

--b.

[PATCH 1/6] rpc: bring back cl_chatty

[J. Bruce Fields]

From: Olga Kornievskaia <aglo@citi.umich.edu>

The cl_chatty flag alows us to control whether a given rpc client leaves

	"server X not responding, timed out"

messages in the syslog.  Such messages make sense for ordinary nfs
clients (where an unresponsive server means applications on the
mountpoint are probably hanging), but not for the callback client (which
can fail more commonly, with the only result just of disabling some
optimizations).

Previously cl_chatty was removed, do to lack of users; reinstate it, and
use it for the nfsd's callback client.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfs4callback.c      |    2 +-
 include/linux/sunrpc/clnt.h |    4 +++-
 net/sunrpc/clnt.c           |   16 +++++++++++-----
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 4d4760e..702fa57 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -381,7 +381,7 @@ static int do_probe_callback(void *data)
 		.program	= &cb_program,
 		.version	= nfs_cb_version[1]->number,
 		.authflavor	= RPC_AUTH_UNIX, /* XXX: need AUTH_GSS... */
-		.flags		= (RPC_CLNT_CREATE_NOPING),
+		.flags		= (RPC_CLNT_CREATE_NOPING | RPC_CLNT_CREATE_QUIET),
 	};
 	struct rpc_message msg = {
 		.rpc_proc       = &nfs4_cb_procedures[NFSPROC4_CLNT_CB_NULL],
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 6fff7f8..764fd4c 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -42,7 +42,8 @@ struct rpc_clnt {
 
 	unsigned int		cl_softrtry : 1,/* soft timeouts */
 				cl_discrtry : 1,/* disconnect before retry */
-				cl_autobind : 1;/* use getport() */
+				cl_autobind : 1,/* use getport() */
+				cl_chatty   : 1;/* be verbose */
 
 	struct rpc_rtt *	cl_rtt;		/* RTO estimator data */
 	const struct rpc_timeout *cl_timeout;	/* Timeout strategy */
@@ -114,6 +115,7 @@ struct rpc_create_args {
 #define RPC_CLNT_CREATE_NONPRIVPORT	(1UL << 3)
 #define RPC_CLNT_CREATE_NOPING		(1UL << 4)
 #define RPC_CLNT_CREATE_DISCRTRY	(1UL << 5)
+#define RPC_CLNT_CREATE_QUIET		(1UL << 6)
 
 struct rpc_clnt *rpc_create(struct rpc_create_args *args);
 struct rpc_clnt	*rpc_bind_new_program(struct rpc_clnt *,
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 8945307..08ec845 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -324,6 +324,8 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args)
 		clnt->cl_autobind = 1;
 	if (args->flags & RPC_CLNT_CREATE_DISCRTRY)
 		clnt->cl_discrtry = 1;
+	if (!(args->flags & RPC_CLNT_CREATE_QUIET))
+		clnt->cl_chatty = 1;
 
 	return clnt;
 }
@@ -1132,7 +1134,8 @@ call_status(struct rpc_task *task)
 		rpc_exit(task, status);
 		break;
 	default:
-		printk("%s: RPC call returned error %d\n",
+		if (clnt->cl_chatty)
+			printk("%s: RPC call returned error %d\n",
 			       clnt->cl_protname, -status);
 		rpc_exit(task, status);
 	}
@@ -1157,7 +1160,8 @@ call_timeout(struct rpc_task *task)
 	task->tk_timeouts++;
 
 	if (RPC_IS_SOFT(task)) {
-		printk(KERN_NOTICE "%s: server %s not responding, timed out\n",
+		if (clnt->cl_chatty)
+			printk(KERN_NOTICE "%s: server %s not responding, timed out\n",
 				clnt->cl_protname, clnt->cl_server);
 		rpc_exit(task, -EIO);
 		return;
@@ -1165,7 +1169,8 @@ call_timeout(struct rpc_task *task)
 
 	if (!(task->tk_flags & RPC_CALL_MAJORSEEN)) {
 		task->tk_flags |= RPC_CALL_MAJORSEEN;
-		printk(KERN_NOTICE "%s: server %s not responding, still trying\n",
+		if (clnt->cl_chatty)
+			printk(KERN_NOTICE "%s: server %s not responding, still trying\n",
 			clnt->cl_protname, clnt->cl_server);
 	}
 	rpc_force_rebind(clnt);
@@ -1196,8 +1201,9 @@ call_decode(struct rpc_task *task)
 			task->tk_pid, task->tk_status);
 
 	if (task->tk_flags & RPC_CALL_MAJORSEEN) {
-		printk(KERN_NOTICE "%s: server %s OK\n",
-			clnt->cl_protname, clnt->cl_server);
+		if (clnt->cl_chatty)
+			printk(KERN_NOTICE "%s: server %s OK\n",
+				clnt->cl_protname, clnt->cl_server);
 		task->tk_flags &= ~RPC_CALL_MAJORSEEN;
 	}
 
-- 
1.5.5.rc1

[PATCH 2/6] rpc: timeout patch

[J. Bruce Fields]

From: Olga Kornievskaia <aglo@citi.umich.edu>

When a client gss upcall times out, we currently give a mesage reminding
the user to check whether gssd is running.

Currently gssd only listens on pipes under nfs/.  We expect to modify it
so it treats all clients the same regardless of protocol (as it probably
should have before), and new functionality may depend on that.  So
people may need to upgrade gssd to get such new functionality.

So it would be helpful if the error message specified which pipe exactly
the upcall was failing on, and suggested that the problem could be due
to an outdated gssd (not just a non-existant gssd).

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 net/sunrpc/auth_gss/auth_gss.c |   42 +++++++++++++++++++++++++--------------
 1 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index cc12d5f..c3ca6f0 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -584,22 +584,34 @@ gss_pipe_destroy_msg(struct rpc_pipe_msg *msg)
 {
 	struct gss_upcall_msg *gss_msg = container_of(msg, struct gss_upcall_msg, msg);
 	static unsigned long ratelimit;
+	unsigned long now = jiffies;
+	struct path path = {
+		.dentry = gss_msg->auth->dentry,
+		.mnt = gss_msg->auth->client->cl_vfsmnt,
+	};
+	char name[128], *p;
 
-	if (msg->errno < 0) {
-		dprintk("RPC:       gss_pipe_destroy_msg releasing msg %p\n",
-				gss_msg);
-		atomic_inc(&gss_msg->count);
-		gss_unhash_msg(gss_msg);
-		if (msg->errno == -ETIMEDOUT) {
-			unsigned long now = jiffies;
-			if (time_after(now, ratelimit)) {
-				printk(KERN_WARNING "RPC: AUTH_GSS upcall timed out.\n"
-						    "Please check user daemon is running!\n");
-				ratelimit = now + 15*HZ;
-			}
-		}
-		gss_release_msg(gss_msg);
-	}
+	if (msg->errno >= 0)
+		return;
+
+	dprintk("RPC:       gss_pipe_destroy_msg releasing msg %p\n", gss_msg);
+	atomic_inc(&gss_msg->count);
+	gss_unhash_msg(gss_msg);
+
+	if (msg->errno != -ETIMEDOUT)
+		goto out;
+
+	if (!time_after(now, ratelimit))
+		goto out;
+
+	p = d_path(&path, name, 128);
+	printk(KERN_WARNING "RPC: AUTH_GSS upcall to <rpc_pipefs_dir>%s "
+			    "timed out.\n Please check user daemon is "
+			    "up-to-date and running!\n", p ? p : "<null>");
+	ratelimit = now + 15*HZ;
+
+out:
+	gss_release_msg(gss_msg);
 }
 
 /*
-- 
1.5.5.rc1

[PATCH 3/6] rpc: eliminate unused variable in auth_gss upcall code

[J. Bruce Fields]

Also, a minor comment grammar fix in the same file.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 net/sunrpc/auth_gss/auth_gss.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index c3ca6f0..92a820e 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -272,7 +272,7 @@ __gss_find_upcall(struct rpc_inode *rpci, uid_t uid)
 	return NULL;
 }
 
-/* Try to add a upcall to the pipefs queue.
+/* Try to add an upcall to the pipefs queue.
  * If an upcall owned by our uid already exists, then we return a reference
  * to that upcall instead of adding the new upcall.
  */
@@ -493,7 +493,6 @@ gss_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
 {
 	const void *p, *end;
 	void *buf;
-	struct rpc_clnt *clnt;
 	struct gss_upcall_msg *gss_msg;
 	struct inode *inode = filp->f_path.dentry->d_inode;
 	struct gss_cl_ctx *ctx;
@@ -507,7 +506,6 @@ gss_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
 	if (!buf)
 		goto out;
 
-	clnt = RPC_I(inode)->private;
 	err = -EFAULT;
 	if (copy_from_user(buf, src, mlen))
 		goto err;
-- 
1.5.5.rc1

[PATCH 4/6] rpc: remove some unused macros

[J. Bruce Fields]

There used to be a print_hexl() function that used isprint(), now gone.
I don't know why NFS_NGROUPS and CA_RUN_AS_MACHINE were here.

I also don't know why another #define that's actually used was marked
"unused".

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 net/sunrpc/auth_gss/auth_gss.c |   13 +------------
 1 files changed, 1 insertions(+), 12 deletions(-)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 92a820e..ff0c2a4 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -63,22 +63,11 @@ static const struct rpc_credops gss_nullops;
 # define RPCDBG_FACILITY	RPCDBG_AUTH
 #endif
 
-#define NFS_NGROUPS	16
-
-#define GSS_CRED_SLACK		1024		/* XXX: unused */
+#define GSS_CRED_SLACK		1024
 /* length of a krb5 verifier (48), plus data added before arguments when
  * using integrity (two 4-byte integers): */
 #define GSS_VERF_SLACK		100
 
-/* XXX this define must match the gssd define
-* as it is passed to gssd to signal the use of
-* machine creds should be part of the shared rpc interface */
-
-#define CA_RUN_AS_MACHINE  0x00000200
-
-/* dump the buffer in `emacs-hexl' style */
-#define isprint(c)      ((c > 0x1f) && (c < 0x7f))
-
 struct gss_auth {
 	struct kref kref;
 	struct rpc_auth rpc_auth;
-- 
1.5.5.rc1

[PATCH 5/6] rpc: minor cleanup of scheduler callback code

[J. Bruce Fields]

Try to make the comment here a little more clear and concise.

Also, this macro definition seems unnecessary.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 include/linux/sunrpc/sched.h |    1 -
 net/sunrpc/sched.c           |   14 +++++---------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index d1a5c8c..64981a2 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -135,7 +135,6 @@ struct rpc_task_setup {
 #define RPC_IS_SWAPPER(t)	((t)->tk_flags & RPC_TASK_SWAPPER)
 #define RPC_DO_ROOTOVERRIDE(t)	((t)->tk_flags & RPC_TASK_ROOTCREDS)
 #define RPC_ASSASSINATED(t)	((t)->tk_flags & RPC_TASK_KILLED)
-#define RPC_DO_CALLBACK(t)	((t)->tk_callback != NULL)
 #define RPC_IS_SOFT(t)		((t)->tk_flags & RPC_TASK_SOFT)
 
 #define RPC_TASK_RUNNING	0
diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index 6eab9bf..6288af0 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -626,19 +626,15 @@ static void __rpc_execute(struct rpc_task *task)
 		/*
 		 * Execute any pending callback.
 		 */
-		if (RPC_DO_CALLBACK(task)) {
-			/* Define a callback save pointer */
+		if (task->tk_callback) {
 			void (*save_callback)(struct rpc_task *);
 
 			/*
-			 * If a callback exists, save it, reset it,
-			 * call it.
-			 * The save is needed to stop from resetting
-			 * another callback set within the callback handler
-			 * - Dave
+			 * We set tk_callback to NULL before calling it,
+			 * in case it sets the tk_callback field itself:
 			 */
-			save_callback=task->tk_callback;
-			task->tk_callback=NULL;
+			save_callback = task->tk_callback;
+			task->tk_callback = NULL;
 			save_callback(task);
 		}
 
-- 
1.5.5.rc1

[PATCH 6/6] nfs: Fix misparsing of nfsv4 fs_locations attribute

[J. Bruce Fields]

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 |   34 ++++++++++------------------------
 1 files changed, 10 insertions(+), 24 deletions(-)

diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index b112857..2f3eabe 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++;
-- 
1.5.5.rc1

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

[Trond Myklebust]

On Mon, 2008-06-09 at 16:51 -0400, J. Bruce Fields wrote:
> 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 |   34 ++++++++++------------------------
>  1 files changed, 10 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> index b112857..2f3eabe 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;

What if it is an IPv6 address? As I've said before, could we please just
adapt nfs_parse_server_address to deal with all these cases?

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

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

[J. Bruce Fields]

On Mon, Jun 09, 2008 at 05:08:16PM -0400, Trond Myklebust wrote:
> 
> What if it is an IPv6 address? As I've said before, could we please just
> adapt nfs_parse_server_address to deal with all these cases?

I haven't had the time to do that, so I thought we'd rather have a fix
for the immediate bug now, then add ipv6 support later.  For all I know
the final patches might end up factoring reasonable cleanly that way
anyway.

--b.

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

[Chuck Lever]

On Jun 9, 2008, at 5:22 PM, J. Bruce Fields wrote:
> On Mon, Jun 09, 2008 at 05:08:16PM -0400, Trond Myklebust wrote:
>>
>> What if it is an IPv6 address? As I've said before, could we please  
>> just
>> adapt nfs_parse_server_address to deal with all these cases?
>
> I haven't had the time to do that, so I thought we'd rather have a fix
> for the immediate bug now, then add ipv6 support later.  For all I  
> know
> the final patches might end up factoring reasonable cleanly that way
> anyway.

I have a patch (upcoming for 2.6.27) that fixes  
nfs_parse_server_address to be careful about '\0'-termination.  It  
changes the function to take a char * and a length.  All you will need  
to do is make it non-static.

(I would have mentioned it yesterday but I thought I had dropped the  
patch from my "for 2.6.27" series).

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

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

[J. Bruce Fields]

On Tue, Jun 10, 2008 at 11:34:14AM -0400, Chuck Lever wrote:
> On Jun 9, 2008, at 5:22 PM, J. Bruce Fields wrote:
>> On Mon, Jun 09, 2008 at 05:08:16PM -0400, Trond Myklebust wrote:
>>>
>>> What if it is an IPv6 address? As I've said before, could we please  
>>> just
>>> adapt nfs_parse_server_address to deal with all these cases?
>>
>> I haven't had the time to do that, so I thought we'd rather have a fix
>> for the immediate bug now, then add ipv6 support later.  For all I  
>> know
>> the final patches might end up factoring reasonable cleanly that way
>> anyway.
>
> I have a patch (upcoming for 2.6.27) that fixes nfs_parse_server_address 
> to be careful about '\0'-termination.  It changes the function to take a 
> char * and a length.  All you will need to do is make it non-static.

OK, good.  That patch and my patch will apply together in either order;
mine ditches the unnecessary valid_ipadd4() and relies on in4_pton()
instead.  Yours makes nfs_parse_server_address more-or-less a drop-in
replacement for in4_pton there.  And it should be easy to switch it over
either in whichever patch comes second or in a small third patch.

So, I was right, this is the natural way to factor out the change
anyway.

The one remaining thing missing for that to work, I think, is to fix the

	struct sockaddr *addr;

field in nfs_clone_mount?

--b.

>
> (I would have mentioned it yesterday but I thought I had dropped the  
> patch from my "for 2.6.27" series).

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

[Chuck Lever]

On Jun 10, 2008, at 3:17 PM, J. Bruce Fields wrote:
> On Tue, Jun 10, 2008 at 11:34:14AM -0400, Chuck Lever wrote:
>> On Jun 9, 2008, at 5:22 PM, J. Bruce Fields wrote:
>>> On Mon, Jun 09, 2008 at 05:08:16PM -0400, Trond Myklebust wrote:
>>>>
>>>> What if it is an IPv6 address? As I've said before, could we please
>>>> just
>>>> adapt nfs_parse_server_address to deal with all these cases?
>>>
>>> I haven't had the time to do that, so I thought we'd rather have a  
>>> fix
>>> for the immediate bug now, then add ipv6 support later.  For all I
>>> know
>>> the final patches might end up factoring reasonable cleanly that way
>>> anyway.
>>
>> I have a patch (upcoming for 2.6.27) that fixes  
>> nfs_parse_server_address
>> to be careful about '\0'-termination.  It changes the function to  
>> take a
>> char * and a length.  All you will need to do is make it non-static.
>
> OK, good.  That patch and my patch will apply together in either  
> order;
> mine ditches the unnecessary valid_ipadd4() and relies on in4_pton()
> instead.  Yours makes nfs_parse_server_address more-or-less a drop-in
> replacement for in4_pton there.  And it should be easy to switch it  
> over
> either in whichever patch comes second or in a small third patch.
>
> So, I was right, this is the natural way to factor out the change
> anyway.
>
> The one remaining thing missing for that to work, I think, is to fix  
> the
>
> 	struct sockaddr *addr;
>
> field in nfs_clone_mount?

"struct sockaddr *" is what we want.  Only the current logic in  
nfs_follow_referral() is AF_INET-specific.

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

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

[J. Bruce Fields]

On Tue, Jun 10, 2008 at 04:51:53PM -0400, Chuck Lever wrote:
> On Jun 10, 2008, at 3:17 PM, J. Bruce Fields wrote:
>> On Tue, Jun 10, 2008 at 11:34:14AM -0400, Chuck Lever wrote:
>>> On Jun 9, 2008, at 5:22 PM, J. Bruce Fields wrote:
>>>> On Mon, Jun 09, 2008 at 05:08:16PM -0400, Trond Myklebust wrote:
>>>>>
>>>>> What if it is an IPv6 address? As I've said before, could we please
>>>>> just
>>>>> adapt nfs_parse_server_address to deal with all these cases?
>>>>
>>>> I haven't had the time to do that, so I thought we'd rather have a  
>>>> fix
>>>> for the immediate bug now, then add ipv6 support later.  For all I
>>>> know
>>>> the final patches might end up factoring reasonable cleanly that way
>>>> anyway.
>>>
>>> I have a patch (upcoming for 2.6.27) that fixes  
>>> nfs_parse_server_address
>>> to be careful about '\0'-termination.  It changes the function to  
>>> take a
>>> char * and a length.  All you will need to do is make it non-static.
>>
>> OK, good.  That patch and my patch will apply together in either  
>> order;
>> mine ditches the unnecessary valid_ipadd4() and relies on in4_pton()
>> instead.  Yours makes nfs_parse_server_address more-or-less a drop-in
>> replacement for in4_pton there.  And it should be easy to switch it  
>> over
>> either in whichever patch comes second or in a small third patch.
>>
>> So, I was right, this is the natural way to factor out the change
>> anyway.
>>
>> The one remaining thing missing for that to work, I think, is to fix  
>> the
>>
>> 	struct sockaddr *addr;
>>
>> field in nfs_clone_mount?
>
> "struct sockaddr *" is what we want.

I'm afraid the various sockaddr types confuse me.  Oh, I see, so it's
just a pointer, so we can store a pointer to a struct sockaddr_storage
there or whatever.

> Only the current logic in  
> nfs_follow_referral() is AF_INET-specific.

OK.

--b.

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

[Chuck Lever]

I thought you had decided this wasn't necessary.

On Mon, Jun 9, 2008 at 4:51 PM, J. Bruce Fields <bfields@citi.umich.edu> wrote:
> 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 |   34 ++++++++++------------------------
>  1 files changed, 10 insertions(+), 24 deletions(-)
>
> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> index b112857..2f3eabe 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++;
> --
> 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
>



-- 
I am certain that these presidents will understand the cry of the
people of Bolivia, of the people of Latin America and the whole world,
which wants to have more food and not more cars. First food, then if
something's left over, more cars, more automobiles. I think that life
has to come first.
-- Evo Morales

Re: [PATCH 2/6] rpc: timeout patch

[Trond Myklebust]

On Mon, 2008-06-09 at 16:51 -0400, J. Bruce Fields wrote:
> From: Olga Kornievskaia <aglo@citi.umich.edu>
> 
> When a client gss upcall times out, we currently give a mesage reminding
> the user to check whether gssd is running.
> 
> Currently gssd only listens on pipes under nfs/.  We expect to modify it
> so it treats all clients the same regardless of protocol (as it probably
> should have before), and new functionality may depend on that.  So
> people may need to upgrade gssd to get such new functionality.
> 
> So it would be helpful if the error message specified which pipe exactly
> the upcall was failing on, and suggested that the problem could be due
> to an outdated gssd (not just a non-existant gssd).

NACK. I belive I've already made clear that I really don't like the idea
of using d_path() in an error message.

Please just correct the existing error message to remind people that
they need an up to date version of gssd, and add a comment in Kconfig
stating exactly _which_ minimal version is needed.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 2/6] rpc: timeout patch

[J. Bruce Fields]

On Mon, Jun 09, 2008 at 05:18:50PM -0400, Trond Myklebust wrote:
> On Mon, 2008-06-09 at 16:51 -0400, J. Bruce Fields wrote:
> > From: Olga Kornievskaia <aglo@citi.umich.edu>
> > 
> > When a client gss upcall times out, we currently give a mesage reminding
> > the user to check whether gssd is running.
> > 
> > Currently gssd only listens on pipes under nfs/.  We expect to modify it
> > so it treats all clients the same regardless of protocol (as it probably
> > should have before), and new functionality may depend on that.  So
> > people may need to upgrade gssd to get such new functionality.
> > 
> > So it would be helpful if the error message specified which pipe exactly
> > the upcall was failing on, and suggested that the problem could be due
> > to an outdated gssd (not just a non-existant gssd).
> 
> NACK. I belive I've already made clear that I really don't like the idea
> of using d_path() in an error message.
> 
> Please just correct the existing error message to remind people that
> they need an up to date version of gssd, and add a comment in Kconfig
> stating exactly _which_ minimal version is needed.

Actually, after another look, maybe we should just do nothing:

The most likely next user of this is the nfsv4 callbacks.  But callbacks
aren't that crucial: if someone doesn't care about delegations, and
wants to delay upgrading gssd, they should be able to do that without
their server's logs getting spammed all the time.

So I'm inclined to just drop this--apologies.

--b.