[PATCH 0/4] NFSv4.1: sp4_mach_cred cleanup (v2)

[PATCH 0/4] NFSv4.1: sp4_mach_cred cleanup (v2)

[Weston Andros Adamson]

Version 2 takes a new approach that Trond requested: we rely on the
cl_machine_cred to have a reference held for the life of the nfs_client
structure, so the sp4 stuff doesn't need to do any get/put_rpccred.

There's also two new patches:
 - "fix SECINFO* use of put_rpccred" - recent changes (by me!) called
   put_rpccred of rpc_message.rpc_cred, but this value can change when
   nfs4_state_protect() is called. I searched through the rest of the client
   source and couldn't find another place where this happens.

 - WARN_ON -> WARN_ON_ONCE - oops.

Weston Andros Adamson (4):
  NFSv4.1: sp4_mach_cred: ask for WRITE and COMMIT
  NFSv4.1: fix SECINFO* use of put_rpccred
  NFSv4.1: sp4_mach_cred: no need to ref count creds
  NFSv4.1: sp4_mach_cred: WARN_ON -> WARN_ON_ONCE

 fs/nfs/nfs4_fs.h  | 10 +++++-----
 fs/nfs/nfs4proc.c | 22 ++++++++++++++--------
 2 files changed, 19 insertions(+), 13 deletions(-)

-- 
1.7.12.4 (Apple Git-37)

[PATCH 1/4] NFSv4.1: sp4_mach_cred: ask for WRITE and COMMIT

[Weston Andros Adamson]

Request SP4_MACH_CRED WRITE and COMMIT support in spo_must_allow list --
they're already supported by the client.

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
 fs/nfs/nfs4proc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 39b6cf2..dc2d27b 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6151,11 +6151,13 @@ static const struct nfs41_state_protection nfs4_sp4_mach_cred_request = {
 	},
 	.allow.u.words = {
 		[0] = 1 << (OP_CLOSE) |
-		      1 << (OP_LOCKU),
+		      1 << (OP_LOCKU) |
+		      1 << (OP_COMMIT),
 		[1] = 1 << (OP_SECINFO - 32) |
 		      1 << (OP_SECINFO_NO_NAME - 32) |
 		      1 << (OP_TEST_STATEID - 32) |
-		      1 << (OP_FREE_STATEID - 32)
+		      1 << (OP_FREE_STATEID - 32) |
+		      1 << (OP_WRITE - 32)
 	}
 };
 
-- 
1.7.12.4 (Apple Git-37)

[PATCH 2/4] NFSv4.1: fix SECINFO* use of put_rpccred

[Weston Andros Adamson]

Recent SP4_MACH_CRED changes allows rpc_message.rpc_cred to change,
so keep a separate pointer to the machine cred for put_rpccred.

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
 fs/nfs/nfs4proc.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index dc2d27b..989bb9d 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6001,10 +6001,12 @@ static int _nfs4_proc_secinfo(struct inode *dir, const struct qstr *name, struct
 		.rpc_resp = &res,
 	};
 	struct rpc_clnt *clnt = NFS_SERVER(dir)->client;
+	struct rpc_cred *cred = NULL;
 
 	if (use_integrity) {
 		clnt = NFS_SERVER(dir)->nfs_client->cl_rpcclient;
-		msg.rpc_cred = nfs4_get_clid_cred(NFS_SERVER(dir)->nfs_client);
+		cred = nfs4_get_clid_cred(NFS_SERVER(dir)->nfs_client);
+		msg.rpc_cred = cred;
 	}
 
 	dprintk("NFS call  secinfo %s\n", name->name);
@@ -6016,8 +6018,8 @@ static int _nfs4_proc_secinfo(struct inode *dir, const struct qstr *name, struct
 				&res.seq_res, 0);
 	dprintk("NFS reply  secinfo: %d\n", status);
 
-	if (msg.rpc_cred)
-		put_rpccred(msg.rpc_cred);
+	if (cred)
+		put_rpccred(cred);
 
 	return status;
 }
@@ -7498,11 +7500,13 @@ _nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
 		.rpc_resp = &res,
 	};
 	struct rpc_clnt *clnt = server->client;
+	struct rpc_cred *cred = NULL;
 	int status;
 
 	if (use_integrity) {
 		clnt = server->nfs_client->cl_rpcclient;
-		msg.rpc_cred = nfs4_get_clid_cred(server->nfs_client);
+		cred = nfs4_get_clid_cred(server->nfs_client);
+		msg.rpc_cred = cred;
 	}
 
 	dprintk("--> %s\n", __func__);
@@ -7510,8 +7514,8 @@ _nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
 				&res.seq_res, 0);
 	dprintk("<-- %s status=%d\n", __func__, status);
 
-	if (msg.rpc_cred)
-		put_rpccred(msg.rpc_cred);
+	if (cred)
+		put_rpccred(cred);
 
 	return status;
 }
-- 
1.7.12.4 (Apple Git-37)

[PATCH 3/4] NFSv4.1: sp4_mach_cred: no need to ref count creds

[Weston Andros Adamson]

The cl_machine_cred doesn't need to be reference counted here -
a reference is held is for the lifetime of the struct nfs_client.
Also, no need to put_rpccred the rpc_message.rpc_cred.

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
 fs/nfs/nfs4_fs.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index f520a11..07a8aa9 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -279,10 +279,10 @@ _nfs4_state_protect(struct nfs_client *clp, unsigned long sp4_mode,
 	if (test_bit(sp4_mode, &clp->cl_sp4_flags)) {
 		spin_lock(&clp->cl_lock);
 		if (clp->cl_machine_cred != NULL)
-			newcred = get_rpccred(clp->cl_machine_cred);
+			/* don't call get_rpccred on the machine cred -
+			 * a reference will be held for life of clp */
+			newcred = clp->cl_machine_cred;
 		spin_unlock(&clp->cl_lock);
-		if (msg->rpc_cred)
-			put_rpccred(msg->rpc_cred);
 		msg->rpc_cred = newcred;
 
 		flavor = clp->cl_rpcclient->cl_auth->au_flavor;
-- 
1.7.12.4 (Apple Git-37)

[PATCH 4/4] NFSv4.1: sp4_mach_cred: WARN_ON -> WARN_ON_ONCE

[Weston Andros Adamson]

No need to spam the logs

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
 fs/nfs/nfs4_fs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 07a8aa9..28842ab 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -286,8 +286,8 @@ _nfs4_state_protect(struct nfs_client *clp, unsigned long sp4_mode,
 		msg->rpc_cred = newcred;
 
 		flavor = clp->cl_rpcclient->cl_auth->au_flavor;
-		WARN_ON(flavor != RPC_AUTH_GSS_KRB5I &&
-			flavor != RPC_AUTH_GSS_KRB5P);
+		WARN_ON_ONCE(flavor != RPC_AUTH_GSS_KRB5I &&
+			     flavor != RPC_AUTH_GSS_KRB5P);
 		*clntp = clp->cl_rpcclient;
 
 		return true;
-- 
1.7.12.4 (Apple Git-37)

Re: [PATCH 3/4] NFSv4.1: sp4_mach_cred: no need to ref count creds

[Anna Schumaker]

Do we need to do a get_rpccred() after looking up the machine cred in
nfs_alloc_client()?  Where does our guaranteed reference come from?

On Tue, Sep 10, 2013 at 6:44 PM, Weston Andros Adamson <dros@netapp.com> wrote:
> The cl_machine_cred doesn't need to be reference counted here -
> a reference is held is for the lifetime of the struct nfs_client.
> Also, no need to put_rpccred the rpc_message.rpc_cred.
>
> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
> ---
>  fs/nfs/nfs4_fs.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index f520a11..07a8aa9 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -279,10 +279,10 @@ _nfs4_state_protect(struct nfs_client *clp, unsigned long sp4_mode,
>         if (test_bit(sp4_mode, &clp->cl_sp4_flags)) {
>                 spin_lock(&clp->cl_lock);
>                 if (clp->cl_machine_cred != NULL)
> -                       newcred = get_rpccred(clp->cl_machine_cred);
> +                       /* don't call get_rpccred on the machine cred -
> +                        * a reference will be held for life of clp */
> +                       newcred = clp->cl_machine_cred;
>                 spin_unlock(&clp->cl_lock);
> -               if (msg->rpc_cred)
> -                       put_rpccred(msg->rpc_cred);
>                 msg->rpc_cred = newcred;
>
>                 flavor = clp->cl_rpcclient->cl_auth->au_flavor;
> --
> 1.7.12.4 (Apple Git-37)
>
> --
> 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 3/4] NFSv4.1: sp4_mach_cred: no need to ref count creds

[Myklebust, Trond]

T24gV2VkLCAyMDEzLTA5LTExIGF0IDA4OjU5IC0wNDAwLCBBbm5hIFNjaHVtYWtlciB3cm90ZToN
Cj4gRG8gd2UgbmVlZCB0byBkbyBhIGdldF9ycGNjcmVkKCkgYWZ0ZXIgbG9va2luZyB1cCB0aGUg
bWFjaGluZSBjcmVkIGluDQo+IG5mc19hbGxvY19jbGllbnQoKT8gIFdoZXJlIGRvZXMgb3VyIGd1
YXJhbnRlZWQgcmVmZXJlbmNlIGNvbWUgZnJvbT8NCg0KU2VlIG15IGNvbW1lbnQgdG8gRHJvcycg
Zmlyc3QgcGF0Y2hzZXQgeWVzdGVyZGF5LiBJbiB0aGUgY2FzZSBvZg0KU1A0X01BQ0hfQ1JFRCwg
d2UgY2Fubm90IGNoYW5nZSB0aGUgbWFjaGluZSBjcmVkZW50aWFsLCBzaW5jZSB0aGUgc3RhdGUN
CnByb3RlY3Rpb24gaXMgY29tcGxldGVseSB0aWVkIHRvIHRoYXQgY3JlZGVudGlhbC4gSW4gZmFj
dCBpZiB5b3UgbG9vayBhdA0KdGhlIGN1cnJlbnQgY29kZSwgd2Ugb25seSBhY3R1YWxseSBmcmVl
IHRoZSBjbF9tYWNoaW5lX2NyZWQgaW4NCm5mc19mcmVlX2NsaWVudCgpLg0KDQpGb3IgdGhhdCBy
ZWFzb24sIGl0IGlzIHNhZmUgdG8gYXNzdW1lIHRoYXQgaXQgaXMgYXZhaWxhYmxlIGZvciB0aGUN
CmR1cmF0aW9uIG9mIHRoZSBSUEMgY2FsbC4gT2YgY291cnNlLCBycGNfdGFza19zZXRfcnBjX21l
c3NhZ2UoKSB3aWxsDQphbHNvIHRha2UgYSByZWZlcmVuY2UsIGJ1dCB0aGF0IHRvbyBpcyByZWR1
bmRhbnQgaW4gdGhpcyBwYXJ0aWN1bGFyDQpjYXNlLg0KDQpDaGVlcnMNCiAgVHJvbmQNCg0KPiBB
bm5hDQo+IA0KPiANCj4gDQo+IE9uIFR1ZSwgU2VwIDEwLCAyMDEzIGF0IDY6NDQgUE0sIFdlc3Rv
biBBbmRyb3MgQWRhbXNvbg0KPiA8ZHJvc0BuZXRhcHAuY29tPiB3cm90ZToNCj4gICAgICAgICBU
aGUgY2xfbWFjaGluZV9jcmVkIGRvZXNuJ3QgbmVlZCB0byBiZSByZWZlcmVuY2UgY291bnRlZCBo
ZXJlDQo+ICAgICAgICAgLQ0KPiAgICAgICAgIGEgcmVmZXJlbmNlIGlzIGhlbGQgaXMgZm9yIHRo
ZSBsaWZldGltZSBvZiB0aGUgc3RydWN0DQo+ICAgICAgICAgbmZzX2NsaWVudC4NCj4gICAgICAg
ICBBbHNvLCBubyBuZWVkIHRvIHB1dF9ycGNjcmVkIHRoZSBycGNfbWVzc2FnZS5ycGNfY3JlZC4N
Cj4gICAgICAgICANCj4gICAgICAgICBTaWduZWQtb2ZmLWJ5OiBXZXN0b24gQW5kcm9zIEFkYW1z
b24gPGRyb3NAbmV0YXBwLmNvbT4NCj4gICAgICAgICAtLS0NCj4gICAgICAgICAgZnMvbmZzL25m
czRfZnMuaCB8IDYgKysrLS0tDQo+ICAgICAgICAgIDEgZmlsZSBjaGFuZ2VkLCAzIGluc2VydGlv
bnMoKyksIDMgZGVsZXRpb25zKC0pDQo+ICAgICAgICAgDQo+ICAgICAgICAgZGlmZiAtLWdpdCBh
L2ZzL25mcy9uZnM0X2ZzLmggYi9mcy9uZnMvbmZzNF9mcy5oDQo+ICAgICAgICAgaW5kZXggZjUy
MGExMS4uMDdhOGFhOSAxMDA2NDQNCj4gICAgICAgICAtLS0gYS9mcy9uZnMvbmZzNF9mcy5oDQo+
ICAgICAgICAgKysrIGIvZnMvbmZzL25mczRfZnMuaA0KPiAgICAgICAgIEBAIC0yNzksMTAgKzI3
OSwxMCBAQCBfbmZzNF9zdGF0ZV9wcm90ZWN0KHN0cnVjdCBuZnNfY2xpZW50DQo+ICAgICAgICAg
KmNscCwgdW5zaWduZWQgbG9uZyBzcDRfbW9kZSwNCj4gICAgICAgICAgICAgICAgIGlmICh0ZXN0
X2JpdChzcDRfbW9kZSwgJmNscC0+Y2xfc3A0X2ZsYWdzKSkgew0KPiAgICAgICAgICAgICAgICAg
ICAgICAgICBzcGluX2xvY2soJmNscC0+Y2xfbG9jayk7DQo+ICAgICAgICAgICAgICAgICAgICAg
ICAgIGlmIChjbHAtPmNsX21hY2hpbmVfY3JlZCAhPSBOVUxMKQ0KPiAgICAgICAgIC0gICAgICAg
ICAgICAgICAgICAgICAgIG5ld2NyZWQgPQ0KPiAgICAgICAgIGdldF9ycGNjcmVkKGNscC0+Y2xf
bWFjaGluZV9jcmVkKTsNCj4gICAgICAgICArICAgICAgICAgICAgICAgICAgICAgICAvKiBkb24n
dCBjYWxsIGdldF9ycGNjcmVkIG9uIHRoZQ0KPiAgICAgICAgIG1hY2hpbmUgY3JlZCAtDQo+ICAg
ICAgICAgKyAgICAgICAgICAgICAgICAgICAgICAgICogYSByZWZlcmVuY2Ugd2lsbCBiZSBoZWxk
IGZvciBsaWZlDQo+ICAgICAgICAgb2YgY2xwICovDQo+ICAgICAgICAgKyAgICAgICAgICAgICAg
ICAgICAgICAgbmV3Y3JlZCA9IGNscC0+Y2xfbWFjaGluZV9jcmVkOw0KPiAgICAgICAgICAgICAg
ICAgICAgICAgICBzcGluX3VubG9jaygmY2xwLT5jbF9sb2NrKTsNCj4gICAgICAgICAtICAgICAg
ICAgICAgICAgaWYgKG1zZy0+cnBjX2NyZWQpDQo+ICAgICAgICAgLSAgICAgICAgICAgICAgICAg
ICAgICAgcHV0X3JwY2NyZWQobXNnLT5ycGNfY3JlZCk7DQo+ICAgICAgICAgICAgICAgICAgICAg
ICAgIG1zZy0+cnBjX2NyZWQgPSBuZXdjcmVkOw0KPiAgICAgICAgIA0KPiAgICAgICAgICAgICAg
ICAgICAgICAgICBmbGF2b3IgPQ0KPiAgICAgICAgIGNscC0+Y2xfcnBjY2xpZW50LT5jbF9hdXRo
LT5hdV9mbGF2b3I7DQo+ICAgICAgICAgLS0NCj4gICAgICAgICAxLjcuMTIuNCAoQXBwbGUgR2l0
LTM3KQ0KPiAgICAgICAgIA0KPiAgICAgICAgIC0tDQo+ICAgICAgICAgVG8gdW5zdWJzY3JpYmUg
ZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlDQo+ICAgICAgICAgbGlu
dXgtbmZzIiBpbg0KPiAgICAgICAgIHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9A
dmdlci5rZXJuZWwub3JnDQo+ICAgICAgICAgTW9yZSBtYWpvcmRvbW8gaW5mbyBhdA0KPiAgICAg
ICAgICBodHRwOi8vdmdlci5rZXJuZWwub3JnL21ham9yZG9tby1pbmZvLmh0bWwNCj4gDQo+IA0K
DQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5l
dEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQo=

Re: [PATCH 3/4] NFSv4.1: sp4_mach_cred: no need to ref count creds

[Anna Schumaker]

Okay, thanks!  I think I missed that explanation yesterday.

On Wed, Sep 11, 2013 at 9:13 AM, Myklebust, Trond
<Trond.Myklebust@netapp.com> wrote:
> On Wed, 2013-09-11 at 08:59 -0400, Anna Schumaker wrote:
>> Do we need to do a get_rpccred() after looking up the machine cred in
>> nfs_alloc_client()?  Where does our guaranteed reference come from?
>
> See my comment to Dros' first patchset yesterday. In the case of
> SP4_MACH_CRED, we cannot change the machine credential, since the state
> protection is completely tied to that credential. In fact if you look at
> the current code, we only actually free the cl_machine_cred in
> nfs_free_client().
>
> For that reason, it is safe to assume that it is available for the
> duration of the RPC call. Of course, rpc_task_set_rpc_message() will
> also take a reference, but that too is redundant in this particular
> case.
>
> Cheers
>   Trond
>
>> Anna
>>
>>
>>
>> On Tue, Sep 10, 2013 at 6:44 PM, Weston Andros Adamson
>> <dros@netapp.com> wrote:
>>         The cl_machine_cred doesn't need to be reference counted here
>>         -
>>         a reference is held is for the lifetime of the struct
>>         nfs_client.
>>         Also, no need to put_rpccred the rpc_message.rpc_cred.
>>
>>         Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>>         ---
>>          fs/nfs/nfs4_fs.h | 6 +++---
>>          1 file changed, 3 insertions(+), 3 deletions(-)
>>
>>         diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
>>         index f520a11..07a8aa9 100644
>>         --- a/fs/nfs/nfs4_fs.h
>>         +++ b/fs/nfs/nfs4_fs.h
>>         @@ -279,10 +279,10 @@ _nfs4_state_protect(struct nfs_client
>>         *clp, unsigned long sp4_mode,
>>                 if (test_bit(sp4_mode, &clp->cl_sp4_flags)) {
>>                         spin_lock(&clp->cl_lock);
>>                         if (clp->cl_machine_cred != NULL)
>>         -                       newcred =
>>         get_rpccred(clp->cl_machine_cred);
>>         +                       /* don't call get_rpccred on the
>>         machine cred -
>>         +                        * a reference will be held for life
>>         of clp */
>>         +                       newcred = clp->cl_machine_cred;
>>                         spin_unlock(&clp->cl_lock);
>>         -               if (msg->rpc_cred)
>>         -                       put_rpccred(msg->rpc_cred);
>>                         msg->rpc_cred = newcred;
>>
>>                         flavor =
>>         clp->cl_rpcclient->cl_auth->au_flavor;
>>         --
>>         1.7.12.4 (Apple Git-37)
>>
>>         --
>>         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
>>
>>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com