[PATCH 0/5] For 3.11

[PATCH 0/5] For 3.11

[Chuck Lever]

Trond-

Please consider these for 3.11.  Thanks.

---

Chuck Lever (5):
      NFS: Set NFS_CS_MIGRATION for NFSv4 mounts
      NFS: Use root's credential for lease management when keytab is missing
      NFS: Never use user credentials for lease renewal
      NFS: Export _nfs_display_fhandle()
      NFS: Save FSID's root FH in nfs_server


 fs/nfs/client.c           |   10 ++++++++--
 fs/nfs/inode.c            |    2 ++
 fs/nfs/internal.h         |   12 ++++++++++++
 fs/nfs/nfs4client.c       |    9 ++++++++-
 fs/nfs/nfs4proc.c         |    4 ++--
 fs/nfs/nfs4state.c        |   20 +++++++++++++++++++-
 include/linux/nfs_fs_sb.h |    1 +
 7 files changed, 52 insertions(+), 6 deletions(-)

-- 
Chuck Lever

[PATCH 1/5] NFS: Set NFS_CS_MIGRATION for NFSv4 mounts

[Chuck Lever]

NFS_CS_MIGRATION makes sense only for NFSv4 mounts.  Introduced by
commit 89652617 (NFS: Introduce "migration" mount option) Fri Sep 14
17:24:11 2012.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/client.c     |    2 --
 fs/nfs/nfs4client.c |    2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index c513b0c..dbb65fb 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -753,8 +753,6 @@ static int nfs_init_server(struct nfs_server *server,
 			data->timeo, data->retrans);
 	if (data->flags & NFS_MOUNT_NORESVPORT)
 		set_bit(NFS_CS_NORESVPORT, &cl_init.init_flags);
-	if (server->options & NFS_OPTION_MIGRATION)
-		set_bit(NFS_CS_MIGRATION, &cl_init.init_flags);
 
 	/* Allocate or find a client reference we can use */
 	clp = nfs_get_client(&cl_init, &timeparms, NULL, RPC_AUTH_UNIX);
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 4cbad5d..b7a9f76 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -626,6 +626,8 @@ static int nfs4_set_client(struct nfs_server *server,
 
 	if (server->flags & NFS_MOUNT_NORESVPORT)
 		set_bit(NFS_CS_NORESVPORT, &cl_init.init_flags);
+	if (server->options & NFS_OPTION_MIGRATION)
+		set_bit(NFS_CS_MIGRATION, &cl_init.init_flags);
 
 	/* Allocate or find a client reference we can use */
 	clp = nfs_get_client(&cl_init, timeparms, ip_addr, authflavour);

[PATCH 2/5] NFS: Use root's credential for lease management when keytab is missing

[Chuck Lever]

Commit 05f4c350 "NFS: Discover NFSv4 server trunking when mounting"
Fri Sep 14 17:24:32 2012 introduced Uniform Client String support,
which forces our NFS client to establish a client ID immediately
during a mount operation rather than waiting until a user wants to
open a file.

Normally machine credentials (eg. from a keytab) are used to perform
a mount operation that is protected by Kerberos.  Before 05fc350,
SETCLIENTID used a machine credential, or fell back to a regular
user's credential if no keytab is available.

On clients that don't have a keytab, performing SETCLIENTID early
means there's no user credential to fall back on, since no regular
user has kinit'd yet.  05f4c350 seems to have broken the ability
to mount with sec=krb5 on clients that don't have a keytab in
kernels 3.7 - 3.10.

To address this regression, commit 4edaa308 (NFS: Use "krb5i" to
establish NFSv4 state whenever possible), Sat Mar 16 15:56:20 2013,
was merged in 3.10.  This commit forces the NFS client to fall back
to AUTH_SYS for lease management operations if no keytab is
available.

Neil Brown noticed that, since root is required to kinit to do a
sec=krb5 mount when a client doesn't have a keytab, we can try to
use root's Kerberos credential before AUTH_SYS.

Now, when determining a principal and flavor to use for lease
management, the NFS client tries in this order:

  1.  Flavor: AUTH_GSS, krb5i
      Principal: service principal (via keytab)

  2.  Flavor: AUTH_GSS, krb5i
      Principal: user principal established for UID 0 (via kinit)

  3.  Flavor: AUTH_SYS
      Principal: UID 0 / GID 0

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/nfs4state.c |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 1fab140..6ceece7 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -154,6 +154,19 @@ struct rpc_cred *nfs4_get_machine_cred_locked(struct nfs_client *clp)
 	return cred;
 }
 
+static void nfs4_root_machine_cred(struct nfs_client *clp)
+{
+	struct rpc_cred *cred, *new;
+
+	new = rpc_lookup_machine_cred(NULL);
+	spin_lock(&clp->cl_lock);
+	cred = clp->cl_machine_cred;
+	clp->cl_machine_cred = new;
+	spin_unlock(&clp->cl_lock);
+	if (cred != NULL)
+		put_rpccred(cred);
+}
+
 static struct rpc_cred *
 nfs4_get_renew_cred_server_locked(struct nfs_server *server)
 {
@@ -1888,9 +1901,14 @@ again:
 	case -NFS4ERR_STALE_CLIENTID:
 		dprintk("NFS: %s after status %d, retrying\n",
 			__func__, status);
+		i = 0;
 		goto again;
 	case -EACCES:
-		if (i++)
+		if (i++ == 0) {
+			nfs4_root_machine_cred(clp);
+			goto again;
+		}
+		if (i > 2)
 			break;
 	case -NFS4ERR_CLID_INUSE:
 	case -NFS4ERR_WRONGSEC:

[PATCH 3/5] NFS: Never use user credentials for lease renewal

[Chuck Lever]

Don't try to use a non-UID-0 user credential for lease management,
as that credential can change out from under us.  The server will
block NFSv4 lease recovery with NFS4ERR_CLID_INUSE.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/nfs4proc.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d7ba561..5ba38b3 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6919,7 +6919,7 @@ static const struct nfs4_state_recovery_ops nfs40_reboot_recovery_ops = {
 	.recover_open	= nfs4_open_reclaim,
 	.recover_lock	= nfs4_lock_reclaim,
 	.establish_clid = nfs4_init_clientid,
-	.get_clid_cred	= nfs4_get_setclientid_cred,
+	.get_clid_cred	= nfs4_get_exchange_id_cred,
 	.detect_trunking = nfs40_discover_server_trunking,
 };
 
@@ -6942,7 +6942,7 @@ static const struct nfs4_state_recovery_ops nfs40_nograce_recovery_ops = {
 	.recover_open	= nfs4_open_expired,
 	.recover_lock	= nfs4_lock_expired,
 	.establish_clid = nfs4_init_clientid,
-	.get_clid_cred	= nfs4_get_setclientid_cred,
+	.get_clid_cred	= nfs4_get_exchange_id_cred,
 };
 
 #if defined(CONFIG_NFS_V4_1)

[PATCH 4/5] NFS: Export _nfs_display_fhandle()

[Chuck Lever]

Allow code in nfsv4.ko to use _nfs_display_fhandle().  Useful for
debugging NFSv4 migration.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/inode.c      |    2 ++
 fs/nfs/nfs4client.c |    2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index c1c7a9d..319203d 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1102,6 +1102,7 @@ u32 _nfs_display_fhandle_hash(const struct nfs_fh *fh)
 	 * not on the result */
 	return ~crc32(0xFFFFFFFF, &fh->data[0], fh->size);
 }
+EXPORT_SYMBOL_GPL(_nfs_display_fhandle_hash);
 
 /*
  * _nfs_display_fhandle - display an NFS file handle on the console
@@ -1146,6 +1147,7 @@ void _nfs_display_fhandle(const struct nfs_fh *fh, const char *caption)
 		}
 	}
 }
+EXPORT_SYMBOL_GPL(_nfs_display_fhandle);
 #endif
 
 /**
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index b7a9f76..3aec2de 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -757,7 +757,7 @@ static int nfs4_server_common_setup(struct nfs_server *server,
 	dprintk("Server FSID: %llx:%llx\n",
 			(unsigned long long) server->fsid.major,
 			(unsigned long long) server->fsid.minor);
-	dprintk("Mount FH: %d\n", mntfh->size);
+	nfs_display_fhandle(mntfh, "Mount FH");
 
 	nfs4_session_set_rwsize(server);
 

[PATCH 5/5] NFS: Save FSID's root FH in nfs_server

[Chuck Lever]

Save each FSID's root file handle in the FSID's nfs_server structure
on the client.  This is needed for NFSv4 migration recovery.

nfs_create_server() (used for NFSv2/3) is also updated for
completeness.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/client.c           |    8 ++++++++
 fs/nfs/internal.h         |   12 ++++++++++++
 fs/nfs/nfs4client.c       |    5 +++++
 include/linux/nfs_fs_sb.h |    1 +
 4 files changed, 26 insertions(+)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index dbb65fb..29cf2b9 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -1028,6 +1028,7 @@ void nfs_free_server(struct nfs_server *server)
 	ida_destroy(&server->openowner_id);
 	nfs_free_iostats(server->io_stats);
 	bdi_destroy(&server->backing_dev_info);
+	nfs_free_fhandle(server->rootfh);
 	kfree(server);
 	nfs_release_automount_timer();
 	dprintk("<-- nfs_free_server()\n");
@@ -1053,6 +1054,9 @@ struct nfs_server *nfs_create_server(struct nfs_mount_info *mount_info,
 	fattr = nfs_alloc_fattr();
 	if (fattr == NULL)
 		goto error;
+	nfs_save_root_fh(server, mount_info->mntfh);
+	if (server->rootfh == NULL)
+		goto error;
 
 	/* Get a client representation */
 	error = nfs_init_server(server, mount_info->parsed, nfs_mod);
@@ -1122,6 +1126,9 @@ struct nfs_server *nfs_clone_server(struct nfs_server *source,
 	fattr_fsinfo = nfs_alloc_fattr();
 	if (fattr_fsinfo == NULL)
 		goto out_free_server;
+	nfs_save_root_fh(server, fh);
+	if (server->rootfh == NULL)
+		goto out_free_server;
 
 	/* Copy data from the source */
 	server->nfs_client = source->nfs_client;
@@ -1161,6 +1168,7 @@ struct nfs_server *nfs_clone_server(struct nfs_server *source,
 	return server;
 
 out_free_server:
+	nfs_free_fhandle(server->rootfh);
 	nfs_free_fattr(fattr_fsinfo);
 	nfs_free_server(server);
 	dprintk("<-- nfs_clone_server() = error %d\n", error);
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 91e59a3..10a3327 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -574,3 +574,15 @@ u64 nfs_timespec_to_change_attr(const struct timespec *ts)
 {
 	return ((u64)ts->tv_sec << 30) + ts->tv_nsec;
 }
+
+/*
+ * Save a copy of the root FH
+ */
+static inline
+void nfs_save_root_fh(struct nfs_server *server, const struct nfs_fh *fh)
+{
+	server->rootfh = nfs_alloc_fhandle();
+	if (server->rootfh == NULL)
+		return;
+	nfs_copy_fh(server->rootfh, fh);
+}
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 3aec2de..18c2154 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -731,6 +731,11 @@ static int nfs4_server_common_setup(struct nfs_server *server,
 	if (fattr == NULL)
 		return -ENOMEM;
 
+	error = -ENOMEM;
+	nfs_save_root_fh(server, mntfh);
+	if (server->rootfh == NULL)
+		goto out;
+
 	/* We must ensure the session is initialised first */
 	error = nfs4_init_session(server);
 	if (error < 0)
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index 3b7fa2a..df4df80 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -170,6 +170,7 @@ struct nfs_server {
 	struct list_head	layouts;
 	struct list_head	delegations;
 	void (*destroy)(struct nfs_server *);
+	struct nfs_fh		*rootfh;
 
 	atomic_t active; /* Keep trace of any activity to this server */
 

Re: [PATCH 5/5] NFS: Save FSID's root FH in nfs_server

[Myklebust, Trond]

On Tue, 2013-06-25 at 12:24 -0400, Chuck Lever wrote:
> Save each FSID's root file handle in the FSID's nfs_server structure
> on the client.  This is needed for NFSv4 migration recovery.

Please just use sb->s_root.


-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 5/5] NFS: Save FSID's root FH in nfs_server

[Chuck Lever]

On Jun 25, 2013, at 12:37 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Tue, 2013-06-25 at 12:24 -0400, Chuck Lever wrote:
>> Save each FSID's root file handle in the FSID's nfs_server structure
>> on the client.  This is needed for NFSv4 migration recovery.
> 
> Please just use sb->s_root.

The migration recovery procedures do not take an inode or dentry argument.  All we have is an nfs_server.  I see

  struct nfs_server *server = NFS_SB(sb);

but what is the preferred method for going the other way?  A naive approach would be to plant a super_block back-pointer in each nfs_server.

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

Re: [PATCH 5/5] NFS: Save FSID's root FH in nfs_server

[Myklebust, Trond]

On Tue, 2013-06-25 at 13:22 -0700, Chuck Lever wrote:
> On Jun 25, 2013, at 12:37 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
> 
> > On Tue, 2013-06-25 at 12:24 -0400, Chuck Lever wrote:
> >> Save each FSID's root file handle in the FSID's nfs_server structure
> >> on the client.  This is needed for NFSv4 migration recovery.
> > 
> > Please just use sb->s_root.
> 
> The migration recovery procedures do not take an inode or dentry argument.  All we have is an nfs_server.  I see
> 
>   struct nfs_server *server = NFS_SB(sb);
> 
> but what is the preferred method for going the other way?  A naive approach would be to plant a super_block back-pointer in each nfs_server.

Why do you not have an inode or dentry? Surely the migration must have
been triggered either by an operation involving a filehandle, in which
case you have an inode, or by a LEASE_MOVED, in which case there is open
state on the target filesystem.

I'm not opposed to putting a backpointer to the super_block in the
nfs_server, but I do want to understand why it is needed.

-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 5/5] NFS: Save FSID's root FH in nfs_server

[Chuck Lever]

On Jun 25, 2013, at 5:15 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Tue, 2013-06-25 at 13:22 -0700, Chuck Lever wrote:
>> On Jun 25, 2013, at 12:37 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
>> 
>>> On Tue, 2013-06-25 at 12:24 -0400, Chuck Lever wrote:
>>>> Save each FSID's root file handle in the FSID's nfs_server structure
>>>> on the client.  This is needed for NFSv4 migration recovery.
>>> 
>>> Please just use sb->s_root.
>> 
>> The migration recovery procedures do not take an inode or dentry argument.  All we have is an nfs_server.  I see
>> 
>>  struct nfs_server *server = NFS_SB(sb);
>> 
>> but what is the preferred method for going the other way?  A naive approach would be to plant a super_block back-pointer in each nfs_server.
> 
> Why do you not have an inode or dentry?

That's the way the APIs happen to be architected.  I wasn't certain that exception->inode in nfs4_handle_exception(), and state->inode in nfs4_async_handle_error(), would always be non-NULL when I needed an inode pointer.

> Surely the migration must have
> been triggered either by an operation involving a filehandle, in which
> case you have an inode, or by a LEASE_MOVED, in which case there is open
> state on the target filesystem.

It's quite convenient to always use the FSID's root FH, but I agree, it's not required by the protocol.

Re: LEASE_MOVED, I'm not confident the client stops sending RENEW operations when it has no more open state.

> I'm not opposed to putting a backpointer to the super_block in the
> nfs_server, but I do want to understand why it is needed.


Alternately, we could save a pointer to the FSID's root dentry or inode.

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

Re: [PATCH 5/5] NFS: Save FSID's root FH in nfs_server

[Myklebust, Trond]

On Tue, 2013-06-25 at 17:51 -0400, Chuck Lever wrote:
> On Jun 25, 2013, at 5:15 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
> 
> > On Tue, 2013-06-25 at 13:22 -0700, Chuck Lever wrote:
> >> On Jun 25, 2013, at 12:37 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
> >> 
> >>> On Tue, 2013-06-25 at 12:24 -0400, Chuck Lever wrote:
> >>>> Save each FSID's root file handle in the FSID's nfs_server structure
> >>>> on the client.  This is needed for NFSv4 migration recovery.
> >>> 
> >>> Please just use sb->s_root.
> >> 
> >> The migration recovery procedures do not take an inode or dentry argument.  All we have is an nfs_server.  I see
> >> 
> >>  struct nfs_server *server = NFS_SB(sb);
> >> 
> >> but what is the preferred method for going the other way?  A naive approach would be to plant a super_block back-pointer in each nfs_server.
> > 
> > Why do you not have an inode or dentry?
> 
> That's the way the APIs happen to be architected.  I wasn't certain that exception->inode in nfs4_handle_exception(), and state->inode in nfs4_async_handle_error(), would always be non-NULL when I needed an inode pointer.

That should be fixable.

> > Surely the migration must have
> > been triggered either by an operation involving a filehandle, in which
> > case you have an inode, or by a LEASE_MOVED, in which case there is open
> > state on the target filesystem.
> 
> It's quite convenient to always use the FSID's root FH, but I agree, it's not required by the protocol.

The problem is that there is no single root FH in NFSv4. The existence
of the pseudofs means that you can have several points of entry to the
same filesystem. Then there are Linux bind mounts...

> Re: LEASE_MOVED, I'm not confident the client stops sending RENEW operations when it has no more open state.

That shouldn't matter. The server isn't allowed to reply with
LEASE_MOVED in that situation.

> > I'm not opposed to putting a backpointer to the super_block in the
> > nfs_server, but I do want to understand why it is needed.
> 
> 
> Alternately, we could save a pointer to the FSID's root dentry or inode.
> 


-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 5/5] NFS: Save FSID's root FH in nfs_server

[Chuck Lever]

On Jun 25, 2013, at 6:10 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Tue, 2013-06-25 at 17:51 -0400, Chuck Lever wrote:
>> On Jun 25, 2013, at 5:15 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
>> 
>>> On Tue, 2013-06-25 at 13:22 -0700, Chuck Lever wrote:
>>>> On Jun 25, 2013, at 12:37 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
>>>> 
>>>>> On Tue, 2013-06-25 at 12:24 -0400, Chuck Lever wrote:
>>>>>> Save each FSID's root file handle in the FSID's nfs_server structure
>>>>>> on the client.  This is needed for NFSv4 migration recovery.
>>>>> 
>>>>> Please just use sb->s_root.
>>>> 
>>>> The migration recovery procedures do not take an inode or dentry argument.  All we have is an nfs_server.  I see
>>>> 
>>>> struct nfs_server *server = NFS_SB(sb);
>>>> 
>>>> but what is the preferred method for going the other way?  A naive approach would be to plant a super_block back-pointer in each nfs_server.
>>> 
>>> Why do you not have an inode or dentry?
>> 
>> That's the way the APIs happen to be architected.  I wasn't certain that exception->inode in nfs4_handle_exception(), and state->inode in nfs4_async_handle_error(), would always be non-NULL when I needed an inode pointer.
> 
> That should be fixable.

I seem to recall some cases where the proc itself didn't have an inode to pass to the exception handler.  But I'll go back and look through them again.  Those cases may be irrelevant.

Passing an inode will probably make for a better API.

> 
>>> Surely the migration must have
>>> been triggered either by an operation involving a filehandle, in which
>>> case you have an inode, or by a LEASE_MOVED, in which case there is open
>>> state on the target filesystem.
>> 
>> It's quite convenient to always use the FSID's root FH, but I agree, it's not required by the protocol.
> 
> The problem is that there is no single root FH in NFSv4. The existence
> of the pseudofs means that you can have several points of entry to the
> same filesystem. Then there are Linux bind mounts...

Fair enough.

> 
>> Re: LEASE_MOVED, I'm not confident the client stops sending RENEW operations when it has no more open state.
> 
> That shouldn't matter. The server isn't allowed to reply with
> LEASE_MOVED in that situation.

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

Re: [PATCH 3/5] NFS: Never use user credentials for lease renewal

[Myklebust, Trond]

On Tue, 2013-06-25 at 12:23 -0400, Chuck Lever wrote:
> Don't try to use a non-UID-0 user credential for lease management,
> as that credential can change out from under us.  The server will
> block NFSv4 lease recovery with NFS4ERR_CLID_INUSE.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfs/nfs4proc.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index d7ba561..5ba38b3 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -6919,7 +6919,7 @@ static const struct nfs4_state_recovery_ops nfs40_reboot_recovery_ops = {
>  	.recover_open	= nfs4_open_reclaim,
>  	.recover_lock	= nfs4_lock_reclaim,
>  	.establish_clid = nfs4_init_clientid,
> -	.get_clid_cred	= nfs4_get_setclientid_cred,
> +	.get_clid_cred	= nfs4_get_exchange_id_cred,
>  	.detect_trunking = nfs40_discover_server_trunking,
>  };
>  
> @@ -6942,7 +6942,7 @@ static const struct nfs4_state_recovery_ops nfs40_nograce_recovery_ops = {
>  	.recover_open	= nfs4_open_expired,
>  	.recover_lock	= nfs4_lock_expired,
>  	.establish_clid = nfs4_init_clientid,
> -	.get_clid_cred	= nfs4_get_setclientid_cred,
> +	.get_clid_cred	= nfs4_get_exchange_id_cred,
>  };
>  
>  #if defined(CONFIG_NFS_V4_1)
> 

Clearly not compile tested...
-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 3/5] NFS: Never use user credentials for lease renewal

[Chuck Lever]

On Jun 28, 2013, at 4:02 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Tue, 2013-06-25 at 12:23 -0400, Chuck Lever wrote:
>> Don't try to use a non-UID-0 user credential for lease management,
>> as that credential can change out from under us.  The server will
>> block NFSv4 lease recovery with NFS4ERR_CLID_INUSE.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> fs/nfs/nfs4proc.c |    4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index d7ba561..5ba38b3 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -6919,7 +6919,7 @@ static const struct nfs4_state_recovery_ops nfs40_reboot_recovery_ops = {
>> 	.recover_open	= nfs4_open_reclaim,
>> 	.recover_lock	= nfs4_lock_reclaim,
>> 	.establish_clid = nfs4_init_clientid,
>> -	.get_clid_cred	= nfs4_get_setclientid_cred,
>> +	.get_clid_cred	= nfs4_get_exchange_id_cred,
>> 	.detect_trunking = nfs40_discover_server_trunking,
>> };
>> 
>> @@ -6942,7 +6942,7 @@ static const struct nfs4_state_recovery_ops nfs40_nograce_recovery_ops = {
>> 	.recover_open	= nfs4_open_expired,
>> 	.recover_lock	= nfs4_lock_expired,
>> 	.establish_clid = nfs4_init_clientid,
>> -	.get_clid_cred	= nfs4_get_setclientid_cred,
>> +	.get_clid_cred	= nfs4_get_exchange_id_cred,
>> };
>> 
>> #if defined(CONFIG_NFS_V4_1)
>> 
> 
> Clearly not compile tested...

Yes, it was, with and without NFSV4 enabled.

Do you have a problem to report?

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

Re: [PATCH 1/5] NFS: Set NFS_CS_MIGRATION for NFSv4 mounts

[Myklebust, Trond]

On Tue, 2013-06-25 at 12:23 -0400, Chuck Lever wrote:
> NFS_CS_MIGRATION makes sense only for NFSv4 mounts.  Introduced by
> commit 89652617 (NFS: Introduce "migration" mount option) Fri Sep 14
> 17:24:11 2012.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfs/client.c     |    2 --
>  fs/nfs/nfs4client.c |    2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index c513b0c..dbb65fb 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -753,8 +753,6 @@ static int nfs_init_server(struct nfs_server *server,
>  			data->timeo, data->retrans);
>  	if (data->flags & NFS_MOUNT_NORESVPORT)
>  		set_bit(NFS_CS_NORESVPORT, &cl_init.init_flags);
> -	if (server->options & NFS_OPTION_MIGRATION)
> -		set_bit(NFS_CS_MIGRATION, &cl_init.init_flags);
>  
>  	/* Allocate or find a client reference we can use */
>  	clp = nfs_get_client(&cl_init, &timeparms, NULL, RPC_AUTH_UNIX);
> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> index 4cbad5d..b7a9f76 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -626,6 +626,8 @@ static int nfs4_set_client(struct nfs_server *server,
>  
>  	if (server->flags & NFS_MOUNT_NORESVPORT)
>  		set_bit(NFS_CS_NORESVPORT, &cl_init.init_flags);
> +	if (server->options & NFS_OPTION_MIGRATION)
> +		set_bit(NFS_CS_MIGRATION, &cl_init.init_flags);
>  
>  	/* Allocate or find a client reference we can use */
>  	clp = nfs_get_client(&cl_init, timeparms, ip_addr, authflavour);
> 

Applied
-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 3/5] NFS: Never use user credentials for lease renewal

[Myklebust, Trond]

On Fri, 2013-06-28 at 16:03 -0400, Chuck Lever wrote:
> On Jun 28, 2013, at 4:02 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
> 
> > On Tue, 2013-06-25 at 12:23 -0400, Chuck Lever wrote:
> >> Don't try to use a non-UID-0 user credential for lease management,
> >> as that credential can change out from under us.  The server will
> >> block NFSv4 lease recovery with NFS4ERR_CLID_INUSE.
> >> 
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >> fs/nfs/nfs4proc.c |    4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> >> index d7ba561..5ba38b3 100644
> >> --- a/fs/nfs/nfs4proc.c
> >> +++ b/fs/nfs/nfs4proc.c
> >> @@ -6919,7 +6919,7 @@ static const struct nfs4_state_recovery_ops nfs40_reboot_recovery_ops = {
> >> 	.recover_open	= nfs4_open_reclaim,
> >> 	.recover_lock	= nfs4_lock_reclaim,
> >> 	.establish_clid = nfs4_init_clientid,
> >> -	.get_clid_cred	= nfs4_get_setclientid_cred,
> >> +	.get_clid_cred	= nfs4_get_exchange_id_cred,
> >> 	.detect_trunking = nfs40_discover_server_trunking,
> >> };
> >> 
> >> @@ -6942,7 +6942,7 @@ static const struct nfs4_state_recovery_ops nfs40_nograce_recovery_ops = {
> >> 	.recover_open	= nfs4_open_expired,
> >> 	.recover_lock	= nfs4_lock_expired,
> >> 	.establish_clid = nfs4_init_clientid,
> >> -	.get_clid_cred	= nfs4_get_setclientid_cred,
> >> +	.get_clid_cred	= nfs4_get_exchange_id_cred,
> >> };
> >> 
> >> #if defined(CONFIG_NFS_V4_1)
> >> 
> > 
> > Clearly not compile tested...
> 
> Yes, it was, with and without NFSV4 enabled.
> 
> Do you have a problem to report?

!defined(CONFIG_NFS_V4_1) would be the obvious place to start...


-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 2/5] NFS: Use root's credential for lease management when keytab is missing

[Myklebust, Trond]

On Tue, 2013-06-25 at 12:23 -0400, Chuck Lever wrote:
> Commit 05f4c350 "NFS: Discover NFSv4 server trunking when mounting"
> Fri Sep 14 17:24:32 2012 introduced Uniform Client String support,
> which forces our NFS client to establish a client ID immediately
> during a mount operation rather than waiting until a user wants to
> open a file.
> 
> Normally machine credentials (eg. from a keytab) are used to perform
> a mount operation that is protected by Kerberos.  Before 05fc350,
> SETCLIENTID used a machine credential, or fell back to a regular
> user's credential if no keytab is available.
> 
> On clients that don't have a keytab, performing SETCLIENTID early
> means there's no user credential to fall back on, since no regular
> user has kinit'd yet.  05f4c350 seems to have broken the ability
> to mount with sec=krb5 on clients that don't have a keytab in
> kernels 3.7 - 3.10.
> 
> To address this regression, commit 4edaa308 (NFS: Use "krb5i" to
> establish NFSv4 state whenever possible), Sat Mar 16 15:56:20 2013,
> was merged in 3.10.  This commit forces the NFS client to fall back
> to AUTH_SYS for lease management operations if no keytab is
> available.
> 
> Neil Brown noticed that, since root is required to kinit to do a
> sec=krb5 mount when a client doesn't have a keytab, we can try to
> use root's Kerberos credential before AUTH_SYS.
> 
> Now, when determining a principal and flavor to use for lease
> management, the NFS client tries in this order:
> 
>   1.  Flavor: AUTH_GSS, krb5i
>       Principal: service principal (via keytab)
> 
>   2.  Flavor: AUTH_GSS, krb5i
>       Principal: user principal established for UID 0 (via kinit)
> 
>   3.  Flavor: AUTH_SYS
>       Principal: UID 0 / GID 0
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfs/nfs4state.c |   20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 1fab140..6ceece7 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -154,6 +154,19 @@ struct rpc_cred *nfs4_get_machine_cred_locked(struct nfs_client *clp)
>  	return cred;
>  }
>  
> +static void nfs4_root_machine_cred(struct nfs_client *clp)
> +{
> +	struct rpc_cred *cred, *new;
> +
> +	new = rpc_lookup_machine_cred(NULL);
> +	spin_lock(&clp->cl_lock);
> +	cred = clp->cl_machine_cred;
> +	clp->cl_machine_cred = new;
> +	spin_unlock(&clp->cl_lock);
> +	if (cred != NULL)
> +		put_rpccred(cred);
> +}
> +
>  static struct rpc_cred *
>  nfs4_get_renew_cred_server_locked(struct nfs_server *server)
>  {
> @@ -1888,9 +1901,14 @@ again:
>  	case -NFS4ERR_STALE_CLIENTID:
>  		dprintk("NFS: %s after status %d, retrying\n",
>  			__func__, status);
> +		i = 0;

This clearly isn't resetting clp->cl_machine_cred, so how is it intended
to work?

>  		goto again;
>  	case -EACCES:
> -		if (i++)
> +		if (i++ == 0) {
> +			nfs4_root_machine_cred(clp);
> +			goto again;
> +		}
> +		if (i > 2)
>  			break;
>  	case -NFS4ERR_CLID_INUSE:
>  	case -NFS4ERR_WRONGSEC:
> 


-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 2/5] NFS: Use root's credential for lease management when keytab is missing

[Chuck Lever]

On Jun 28, 2013, at 4:11 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Tue, 2013-06-25 at 12:23 -0400, Chuck Lever wrote:
>> Commit 05f4c350 "NFS: Discover NFSv4 server trunking when mounting"
>> Fri Sep 14 17:24:32 2012 introduced Uniform Client String support,
>> which forces our NFS client to establish a client ID immediately
>> during a mount operation rather than waiting until a user wants to
>> open a file.
>> 
>> Normally machine credentials (eg. from a keytab) are used to perform
>> a mount operation that is protected by Kerberos.  Before 05fc350,
>> SETCLIENTID used a machine credential, or fell back to a regular
>> user's credential if no keytab is available.
>> 
>> On clients that don't have a keytab, performing SETCLIENTID early
>> means there's no user credential to fall back on, since no regular
>> user has kinit'd yet.  05f4c350 seems to have broken the ability
>> to mount with sec=krb5 on clients that don't have a keytab in
>> kernels 3.7 - 3.10.
>> 
>> To address this regression, commit 4edaa308 (NFS: Use "krb5i" to
>> establish NFSv4 state whenever possible), Sat Mar 16 15:56:20 2013,
>> was merged in 3.10.  This commit forces the NFS client to fall back
>> to AUTH_SYS for lease management operations if no keytab is
>> available.
>> 
>> Neil Brown noticed that, since root is required to kinit to do a
>> sec=krb5 mount when a client doesn't have a keytab, we can try to
>> use root's Kerberos credential before AUTH_SYS.
>> 
>> Now, when determining a principal and flavor to use for lease
>> management, the NFS client tries in this order:
>> 
>>  1.  Flavor: AUTH_GSS, krb5i
>>      Principal: service principal (via keytab)
>> 
>>  2.  Flavor: AUTH_GSS, krb5i
>>      Principal: user principal established for UID 0 (via kinit)
>> 
>>  3.  Flavor: AUTH_SYS
>>      Principal: UID 0 / GID 0
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> fs/nfs/nfs4state.c |   20 +++++++++++++++++++-
>> 1 file changed, 19 insertions(+), 1 deletion(-)
>> 
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index 1fab140..6ceece7 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -154,6 +154,19 @@ struct rpc_cred *nfs4_get_machine_cred_locked(struct nfs_client *clp)
>> 	return cred;
>> }
>> 
>> +static void nfs4_root_machine_cred(struct nfs_client *clp)
>> +{
>> +	struct rpc_cred *cred, *new;
>> +
>> +	new = rpc_lookup_machine_cred(NULL);
>> +	spin_lock(&clp->cl_lock);
>> +	cred = clp->cl_machine_cred;
>> +	clp->cl_machine_cred = new;
>> +	spin_unlock(&clp->cl_lock);
>> +	if (cred != NULL)
>> +		put_rpccred(cred);
>> +}
>> +
>> static struct rpc_cred *
>> nfs4_get_renew_cred_server_locked(struct nfs_server *server)
>> {
>> @@ -1888,9 +1901,14 @@ again:
>> 	case -NFS4ERR_STALE_CLIENTID:
>> 		dprintk("NFS: %s after status %d, retrying\n",
>> 			__func__, status);
>> +		i = 0;
> 
> This clearly isn't resetting clp->cl_machine_cred, so how is it intended
> to work?

This one line is not needed for the fix, and can be removed.  Or, we can add logic here to reset cl_machine_cred.

You probably want to remove that line, and have a separate patch to make sure the machine cred is reset when the client receives STALE_CLIENTID.

> 
>> 		goto again;
>> 	case -EACCES:
>> -		if (i++)
>> +		if (i++ == 0) {
>> +			nfs4_root_machine_cred(clp);
>> +			goto again;
>> +		}
>> +		if (i > 2)
>> 			break;
>> 	case -NFS4ERR_CLID_INUSE:
>> 	case -NFS4ERR_WRONGSEC:
>> 
> 
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com

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