[PATCH RFC] nfs: make DELEGRETURN try harder to determine if a delegation has been revoked

[PATCH RFC] nfs: make DELEGRETURN try harder to determine if a delegation has been revoked

[Andrew Elble]

It's possible for DELEGRETURN to run into corner cases where a
delegation has been revoked by the server, but that fact is being
hidden by an error on the PUTFH/DELEGRETURN. So, in all error
cases where revoked status isn't immediately obvious, do a
TEST_STATEID to see if it is indeed revoked.

Signed-off-by: Andrew Elble <aweits@rit.edu>
---
 fs/nfs/nfs4proc.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 8220a168282e..3f38d281a814 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -386,6 +386,16 @@ static void nfs4_free_revoked_stateid(struct nfs_server *server,
 	__nfs4_free_revoked_stateid(server, &tmp, cred);
 }
 
+static void nfs4_free_possibly_revoked_stateid(struct nfs_server *server,
+					const nfs4_stateid *stateid,
+					struct rpc_cred *cred)
+{
+	nfs4_stateid tmp;
+
+	nfs4_stateid_copy(&tmp, stateid);
+	nfs4_test_and_free_stateid(server, &tmp, cred);
+}
+
 static long nfs4_update_delay(long *timeout)
 {
 	long ret;
@@ -6035,9 +6045,13 @@ static void nfs4_delegreturn_done(struct rpc_task *task, void *calldata)
 		nfs4_free_revoked_stateid(data->res.server,
 				data->args.stateid,
 				task->tk_msg.rpc_cred);
-		/* Fallthrough */
+		task->tk_status = 0;
+		break;
 	case -NFS4ERR_BAD_STATEID:
 	case -NFS4ERR_STALE_STATEID:
+		nfs4_free_possibly_revoked_stateid(data->res.server,
+						data->args.stateid,
+						task->tk_msg.rpc_cred);
 		task->tk_status = 0;
 		break;
 	case -NFS4ERR_OLD_STATEID:
@@ -6058,6 +6072,10 @@ static void nfs4_delegreturn_done(struct rpc_task *task, void *calldata)
 				&exception);
 		if (exception.retry)
 			goto out_restart;
+
+		nfs4_free_possibly_revoked_stateid(data->res.server,
+						data->args.stateid,
+						task->tk_msg.rpc_cred);
 	}
 	data->rpc_status = task->tk_status;
 	return;
-- 
1.8.3.1

Re: [PATCH RFC] nfs: make DELEGRETURN try harder to determine if a delegation has been revoked

[Trond Myklebust]

T24gRnJpLCAyMDE4LTEwLTA1IGF0IDA5OjM0IC0wNDAwLCBBbmRyZXcgRWxibGUgd3JvdGU6DQo+
IEl0J3MgcG9zc2libGUgZm9yIERFTEVHUkVUVVJOIHRvIHJ1biBpbnRvIGNvcm5lciBjYXNlcyB3
aGVyZSBhDQo+IGRlbGVnYXRpb24gaGFzIGJlZW4gcmV2b2tlZCBieSB0aGUgc2VydmVyLCBidXQg
dGhhdCBmYWN0IGlzIGJlaW5nDQo+IGhpZGRlbiBieSBhbiBlcnJvciBvbiB0aGUgUFVURkgvREVM
RUdSRVRVUk4uIFNvLCBpbiBhbGwgZXJyb3INCj4gY2FzZXMgd2hlcmUgcmV2b2tlZCBzdGF0dXMg
aXNuJ3QgaW1tZWRpYXRlbHkgb2J2aW91cywgZG8gYQ0KPiBURVNUX1NUQVRFSUQgdG8gc2VlIGlm
IGl0IGlzIGluZGVlZCByZXZva2VkLg0KPiANCj4gU2lnbmVkLW9mZi1ieTogQW5kcmV3IEVsYmxl
IDxhd2VpdHNAcml0LmVkdT4NCj4gLS0tDQo+ICBmcy9uZnMvbmZzNHByb2MuYyB8IDIwICsrKysr
KysrKysrKysrKysrKystDQo+ICAxIGZpbGUgY2hhbmdlZCwgMTkgaW5zZXJ0aW9ucygrKSwgMSBk
ZWxldGlvbigtKQ0KPiANCj4gZGlmZiAtLWdpdCBhL2ZzL25mcy9uZnM0cHJvYy5jIGIvZnMvbmZz
L25mczRwcm9jLmMNCj4gaW5kZXggODIyMGExNjgyODJlLi4zZjM4ZDI4MWE4MTQgMTAwNjQ0DQo+
IC0tLSBhL2ZzL25mcy9uZnM0cHJvYy5jDQo+ICsrKyBiL2ZzL25mcy9uZnM0cHJvYy5jDQo+IEBA
IC0zODYsNiArMzg2LDE2IEBAIHN0YXRpYyB2b2lkIG5mczRfZnJlZV9yZXZva2VkX3N0YXRlaWQo
c3RydWN0DQo+IG5mc19zZXJ2ZXIgKnNlcnZlciwNCj4gIAlfX25mczRfZnJlZV9yZXZva2VkX3N0
YXRlaWQoc2VydmVyLCAmdG1wLCBjcmVkKTsNCj4gIH0NCj4gIA0KPiArc3RhdGljIHZvaWQgbmZz
NF9mcmVlX3Bvc3NpYmx5X3Jldm9rZWRfc3RhdGVpZChzdHJ1Y3QgbmZzX3NlcnZlcg0KPiAqc2Vy
dmVyLA0KPiArCQkJCQljb25zdCBuZnM0X3N0YXRlaWQgKnN0YXRlaWQsDQo+ICsJCQkJCXN0cnVj
dCBycGNfY3JlZCAqY3JlZCkNCj4gK3sNCj4gKwluZnM0X3N0YXRlaWQgdG1wOw0KPiArDQo+ICsJ
bmZzNF9zdGF0ZWlkX2NvcHkoJnRtcCwgc3RhdGVpZCk7DQo+ICsJbmZzNF90ZXN0X2FuZF9mcmVl
X3N0YXRlaWQoc2VydmVyLCAmdG1wLCBjcmVkKTsNCj4gK30NCj4gKw0KPiAgc3RhdGljIGxvbmcg
bmZzNF91cGRhdGVfZGVsYXkobG9uZyAqdGltZW91dCkNCj4gIHsNCj4gIAlsb25nIHJldDsNCj4g
QEAgLTYwMzUsOSArNjA0NSwxMyBAQCBzdGF0aWMgdm9pZCBuZnM0X2RlbGVncmV0dXJuX2RvbmUo
c3RydWN0DQo+IHJwY190YXNrICp0YXNrLCB2b2lkICpjYWxsZGF0YSkNCj4gIAkJbmZzNF9mcmVl
X3Jldm9rZWRfc3RhdGVpZChkYXRhLT5yZXMuc2VydmVyLA0KPiAgCQkJCWRhdGEtPmFyZ3Muc3Rh
dGVpZCwNCj4gIAkJCQl0YXNrLT50a19tc2cucnBjX2NyZWQpOw0KPiAtCQkvKiBGYWxsdGhyb3Vn
aCAqLw0KPiArCQl0YXNrLT50a19zdGF0dXMgPSAwOw0KPiArCQlicmVhazsNCj4gIAljYXNlIC1O
RlM0RVJSX0JBRF9TVEFURUlEOg0KPiAgCWNhc2UgLU5GUzRFUlJfU1RBTEVfU1RBVEVJRDoNCj4g
KwkJbmZzNF9mcmVlX3Bvc3NpYmx5X3Jldm9rZWRfc3RhdGVpZChkYXRhLT5yZXMuc2VydmVyLA0K
PiArCQkJCQkJZGF0YS0+YXJncy5zdGF0ZWlkLA0KPiArCQkJCQkJdGFzay0+dGtfbXNnLnJwY19j
cmVkKTsNCj4gIAkJdGFzay0+dGtfc3RhdHVzID0gMDsNCj4gIAkJYnJlYWs7DQo+ICAJY2FzZSAt
TkZTNEVSUl9PTERfU1RBVEVJRDoNCj4gQEAgLTYwNTgsNiArNjA3MiwxMCBAQCBzdGF0aWMgdm9p
ZCBuZnM0X2RlbGVncmV0dXJuX2RvbmUoc3RydWN0DQo+IHJwY190YXNrICp0YXNrLCB2b2lkICpj
YWxsZGF0YSkNCj4gIAkJCQkmZXhjZXB0aW9uKTsNCj4gIAkJaWYgKGV4Y2VwdGlvbi5yZXRyeSkN
Cj4gIAkJCWdvdG8gb3V0X3Jlc3RhcnQ7DQo+ICsNCj4gKwkJbmZzNF9mcmVlX3Bvc3NpYmx5X3Jl
dm9rZWRfc3RhdGVpZChkYXRhLT5yZXMuc2VydmVyLA0KPiArCQkJCQkJZGF0YS0+YXJncy5zdGF0
ZWlkLA0KPiArCQkJCQkJdGFzay0+dGtfbXNnLnJwY19jcmVkKTsNCj4gIAl9DQo+ICAJZGF0YS0+
cnBjX3N0YXR1cyA9IHRhc2stPnRrX3N0YXR1czsNCj4gIAlyZXR1cm47DQoNCk5BQ0suIFdlIGRv
bid0IGV2ZXIgd2FudCB0byBydW4gc3luY2hyb25vdXMgUlBDIGNhbGxzIGZyb20gaW5zaWRlIGFu
DQpycGNpb2QgY29udGV4dC4gVGhlcmUgYmUgZGVhZGxvY2tzLi4uDQoNCi0tIA0KVHJvbmQgTXlr
bGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXIsIEhhbW1lcnNwYWNlDQp0cm9uZC5t
eWtsZWJ1c3RAaGFtbWVyc3BhY2UuY29tDQoNCg0K

Re: [PATCH RFC] nfs: make DELEGRETURN try harder to determine if a delegation has been revoked

[Trond Myklebust]

T24gRnJpLCAyMDE4LTEwLTA1IGF0IDEzOjQ0ICswMDAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6
DQo+IE9uIEZyaSwgMjAxOC0xMC0wNSBhdCAwOTozNCAtMDQwMCwgQW5kcmV3IEVsYmxlIHdyb3Rl
Og0KPiA+IEl0J3MgcG9zc2libGUgZm9yIERFTEVHUkVUVVJOIHRvIHJ1biBpbnRvIGNvcm5lciBj
YXNlcyB3aGVyZSBhDQo+ID4gZGVsZWdhdGlvbiBoYXMgYmVlbiByZXZva2VkIGJ5IHRoZSBzZXJ2
ZXIsIGJ1dCB0aGF0IGZhY3QgaXMgYmVpbmcNCj4gPiBoaWRkZW4gYnkgYW4gZXJyb3Igb24gdGhl
IFBVVEZIL0RFTEVHUkVUVVJOLiBTbywgaW4gYWxsIGVycm9yDQo+ID4gY2FzZXMgd2hlcmUgcmV2
b2tlZCBzdGF0dXMgaXNuJ3QgaW1tZWRpYXRlbHkgb2J2aW91cywgZG8gYQ0KPiA+IFRFU1RfU1RB
VEVJRCB0byBzZWUgaWYgaXQgaXMgaW5kZWVkIHJldm9rZWQuDQo+ID4gDQo+ID4gU2lnbmVkLW9m
Zi1ieTogQW5kcmV3IEVsYmxlIDxhd2VpdHNAcml0LmVkdT4NCj4gPiAtLS0NCj4gPiAgZnMvbmZz
L25mczRwcm9jLmMgfCAyMCArKysrKysrKysrKysrKysrKysrLQ0KPiA+ICAxIGZpbGUgY2hhbmdl
ZCwgMTkgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQ0KPiA+IA0KPiA+IGRpZmYgLS1naXQg
YS9mcy9uZnMvbmZzNHByb2MuYyBiL2ZzL25mcy9uZnM0cHJvYy5jDQo+ID4gaW5kZXggODIyMGEx
NjgyODJlLi4zZjM4ZDI4MWE4MTQgMTAwNjQ0DQo+ID4gLS0tIGEvZnMvbmZzL25mczRwcm9jLmMN
Cj4gPiArKysgYi9mcy9uZnMvbmZzNHByb2MuYw0KPiA+IEBAIC0zODYsNiArMzg2LDE2IEBAIHN0
YXRpYyB2b2lkIG5mczRfZnJlZV9yZXZva2VkX3N0YXRlaWQoc3RydWN0DQo+ID4gbmZzX3NlcnZl
ciAqc2VydmVyLA0KPiA+ICAJX19uZnM0X2ZyZWVfcmV2b2tlZF9zdGF0ZWlkKHNlcnZlciwgJnRt
cCwgY3JlZCk7DQo+ID4gIH0NCj4gPiAgDQo+ID4gK3N0YXRpYyB2b2lkIG5mczRfZnJlZV9wb3Nz
aWJseV9yZXZva2VkX3N0YXRlaWQoc3RydWN0IG5mc19zZXJ2ZXINCj4gPiAqc2VydmVyLA0KPiA+
ICsJCQkJCWNvbnN0IG5mczRfc3RhdGVpZCAqc3RhdGVpZCwNCj4gPiArCQkJCQlzdHJ1Y3QgcnBj
X2NyZWQgKmNyZWQpDQo+ID4gK3sNCj4gPiArCW5mczRfc3RhdGVpZCB0bXA7DQo+ID4gKw0KPiA+
ICsJbmZzNF9zdGF0ZWlkX2NvcHkoJnRtcCwgc3RhdGVpZCk7DQo+ID4gKwluZnM0X3Rlc3RfYW5k
X2ZyZWVfc3RhdGVpZChzZXJ2ZXIsICZ0bXAsIGNyZWQpOw0KPiA+ICt9DQo+ID4gKw0KPiA+ICBz
dGF0aWMgbG9uZyBuZnM0X3VwZGF0ZV9kZWxheShsb25nICp0aW1lb3V0KQ0KPiA+ICB7DQo+ID4g
IAlsb25nIHJldDsNCj4gPiBAQCAtNjAzNSw5ICs2MDQ1LDEzIEBAIHN0YXRpYyB2b2lkIG5mczRf
ZGVsZWdyZXR1cm5fZG9uZShzdHJ1Y3QNCj4gPiBycGNfdGFzayAqdGFzaywgdm9pZCAqY2FsbGRh
dGEpDQo+ID4gIAkJbmZzNF9mcmVlX3Jldm9rZWRfc3RhdGVpZChkYXRhLT5yZXMuc2VydmVyLA0K
PiA+ICAJCQkJZGF0YS0+YXJncy5zdGF0ZWlkLA0KPiA+ICAJCQkJdGFzay0+dGtfbXNnLnJwY19j
cmVkKTsNCj4gPiAtCQkvKiBGYWxsdGhyb3VnaCAqLw0KPiA+ICsJCXRhc2stPnRrX3N0YXR1cyA9
IDA7DQo+ID4gKwkJYnJlYWs7DQo+ID4gIAljYXNlIC1ORlM0RVJSX0JBRF9TVEFURUlEOg0KPiA+
ICAJY2FzZSAtTkZTNEVSUl9TVEFMRV9TVEFURUlEOg0KPiA+ICsJCW5mczRfZnJlZV9wb3NzaWJs
eV9yZXZva2VkX3N0YXRlaWQoZGF0YS0+cmVzLnNlcnZlciwNCj4gPiArCQkJCQkJZGF0YS0+YXJn
cy5zdGF0ZWlkLA0KPiA+ICsJCQkJCQl0YXNrLT50a19tc2cucnBjX2NyZWQpOw0KPiA+ICAJCXRh
c2stPnRrX3N0YXR1cyA9IDA7DQo+ID4gIAkJYnJlYWs7DQo+ID4gIAljYXNlIC1ORlM0RVJSX09M
RF9TVEFURUlEOg0KPiA+IEBAIC02MDU4LDYgKzYwNzIsMTAgQEAgc3RhdGljIHZvaWQgbmZzNF9k
ZWxlZ3JldHVybl9kb25lKHN0cnVjdA0KPiA+IHJwY190YXNrICp0YXNrLCB2b2lkICpjYWxsZGF0
YSkNCj4gPiAgCQkJCSZleGNlcHRpb24pOw0KPiA+ICAJCWlmIChleGNlcHRpb24ucmV0cnkpDQo+
ID4gIAkJCWdvdG8gb3V0X3Jlc3RhcnQ7DQo+ID4gKw0KPiA+ICsJCW5mczRfZnJlZV9wb3NzaWJs
eV9yZXZva2VkX3N0YXRlaWQoZGF0YS0+cmVzLnNlcnZlciwNCj4gPiArCQkJCQkJZGF0YS0+YXJn
cy5zdGF0ZWlkLA0KPiA+ICsJCQkJCQl0YXNrLT50a19tc2cucnBjX2NyZWQpOw0KPiA+ICAJfQ0K
PiA+ICAJZGF0YS0+cnBjX3N0YXR1cyA9IHRhc2stPnRrX3N0YXR1czsNCj4gPiAgCXJldHVybjsN
Cj4gDQo+IE5BQ0suIFdlIGRvbid0IGV2ZXIgd2FudCB0byBydW4gc3luY2hyb25vdXMgUlBDIGNh
bGxzIGZyb20gaW5zaWRlIGFuDQo+IHJwY2lvZCBjb250ZXh0LiBUaGVyZSBiZSBkZWFkbG9ja3Mu
Li4NCg0KU28sIG15IHF1ZXN0aW9uIGlzIHdoeSB3b3VsZCB3ZSBuZWVkIHRvIGNoYW5nZSBuZnM0
X2RlbGVncmV0dXJuX2RvbmUgYXQNCmFsbD8gSXQgc2hvdWxkIGFscmVhZHkgYmUgc2VuZGluZyBh
IEZSRUVfU1RBVEVJRCB3aGVuIHRoZSBzZXJ2ZXINCnJldHVybnMgTkZTNEVSUl9FWFBJUkVEIG9y
IE5GUzRFUlJfREVMRUdfUkVWT0tFRCB0aGFua3MgdG8gdGhlIGNhbGwgdG8NCm5mczRfZnJlZV9y
ZXZva2VkX3N0YXRlaWQoKS4NCg0KSWYgdGhlIHNlcnZlciBpcyByZXR1cm5pbmcgYW55dGhpbmcg
b3RoZXIgdGhhbiB0aG9zZSAyIGVycm9ycyBmb3IgYQ0Kc3RhdGVpZCB0aGF0IGlzIHBlbmRpbmcg
YSBGUkVFX1NUQVRFSUQgZnJvbSB0aGUgY2xpZW50LCB0aGVuIHRoYXQNCnNlcnZlciBpcyBicm9r
ZW4uDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXIs
IEhhbW1lcnNwYWNlDQp0cm9uZC5teWtsZWJ1c3RAaGFtbWVyc3BhY2UuY29tDQoNCg0K

Re: [PATCH RFC] nfs: make DELEGRETURN try harder to determine if a delegation has been revoked

[Andrew W Elble]

>> NACK. We don't ever want to run synchronous RPC calls from inside an
>> rpciod context. There be deadlocks...

ahhh yes, I missed the fact that nfs4_free_revoked_stateid() is
safe in calling nfs4_test_and_free_stateid() because it is bypassing the
TEST_STATEID by setting stateid->type = NFS4_REVOKED_STATEID_TYPE.

> So, my question is why would we need to change nfs4_delegreturn_done at
> all? It should already be sending a FREE_STATEID when the server
> returns NFS4ERR_EXPIRED or NFS4ERR_DELEG_REVOKED thanks to the call to
> nfs4_free_revoked_stateid().
>
> If the server is returning anything other than those 2 errors for a
> stateid that is pending a FREE_STATEID from the client, then that
> server is broken.

My intent (with the BAD/STALE_STATEID part at least)
was to fail safe even in the presence of a broken server.

As to the other bit, the hypothesis is a bad response to PUTFH. (I
cannot prove that I or others have seen this, BTW)

What is the appropriate client/server response(s) if the delegation is
revoked and the PUTFH fails? The server should send the error on the
PUTFH before evaluating the DELEGRETURN, correct?

Thanks,

Andy

-- 
Andrew W. Elble
Infrastructure Engineer
Information and Technology Services
Rochester Institute of Technology
tel: (585)-475-2411 | aweits@rit.edu
PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912

Re: [PATCH RFC] nfs: make DELEGRETURN try harder to determine if a delegation has been revoked

[Trond Myklebust]

On Fri, 2018-10-05 at 11:25 -0400, Andrew W Elble wrote:
> > > NACK. We don't ever want to run synchronous RPC calls from inside
> > > an
> > > rpciod context. There be deadlocks...
> 
> ahhh yes, I missed the fact that nfs4_free_revoked_stateid() is
> safe in calling nfs4_test_and_free_stateid() because it is bypassing
> the
> TEST_STATEID by setting stateid->type = NFS4_REVOKED_STATEID_TYPE.
> 
> > So, my question is why would we need to change
> > nfs4_delegreturn_done at
> > all? It should already be sending a FREE_STATEID when the server
> > returns NFS4ERR_EXPIRED or NFS4ERR_DELEG_REVOKED thanks to the call
> > to
> > nfs4_free_revoked_stateid().
> > 
> > If the server is returning anything other than those 2 errors for a
> > stateid that is pending a FREE_STATEID from the client, then that
> > server is broken.
> 
> My intent (with the BAD/STALE_STATEID part at least)
> was to fail safe even in the presence of a broken server.

I don't see how that would be useful. If the server is so broken that
it is returning NFS4ERR_BAD_STATEID in a situation where it expects the
client to send a FREE_STATEID, then it can't be trusted to do the right
thing for calls to TEST_STATEID either.

IOW: This isn't a fixable situation on the client.

> As to the other bit, the hypothesis is a bad response to PUTFH. (I
> cannot prove that I or others have seen this, BTW)
> 
> What is the appropriate client/server response(s) if the delegation
> is
> revoked and the PUTFH fails? The server should send the error on the
> PUTFH before evaluating the DELEGRETURN, correct?
> 

You're talking about the case where PUTFH returns NFS4ERR_STALE? Why
should the client be required free state for a file that has been
removed? There is nothing in RFC5661 that says that it has to, and it
wouldn't make any sense to impose such a requirement either. Once the
filehandle is stale, any locks protecting the file contents are a moot
point.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com