* [PATCH 0/2] Fix CLOSE races @ 2016-11-14 16:19 Trond Myklebust 2016-11-14 16:19 ` [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN Trond Myklebust 0 siblings, 1 reply; 17+ messages in thread From: Trond Myklebust @ 2016-11-14 16:19 UTC (permalink / raw) To: linux-nfs; +Cc: Benjamin Coddington, Jeff Layton Trond Myklebust (2): NFSv4: Fix CLOSE races with OPEN NFSv4: Don't call close if the open stateid has already been cleared fs/nfs/nfs4_fs.h | 7 +++++++ fs/nfs/nfs4proc.c | 15 ++++++++------- 2 files changed, 15 insertions(+), 7 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2016-11-14 16:19 [PATCH 0/2] Fix CLOSE races Trond Myklebust @ 2016-11-14 16:19 ` Trond Myklebust 2016-11-14 16:19 ` [PATCH 2/2] NFSv4: Don't call close if the open stateid has already been cleared Trond Myklebust ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Trond Myklebust @ 2016-11-14 16:19 UTC (permalink / raw) To: linux-nfs; +Cc: Benjamin Coddington, Jeff Layton If the reply to a successful CLOSE call races with an OPEN to the same file, we can end up scribbling over the stateid that represents the new open state. The race looks like: Client Server ====== ====== CLOSE stateid A on file "foo" CLOSE stateid A, return stateid C OPEN file "foo" OPEN "foo", return stateid B Receive reply to OPEN Reset open state for "foo" Associate stateid B to "foo" Receive CLOSE for A Reset open state for "foo" Replace stateid B with C The fix is to examine the argument of the CLOSE, and check for a match with the current stateid "other" field. If the two do not match, then the above race occurred, and we should just ignore the CLOSE. Reported-by: Benjamin Coddington <bcodding@redhat.com> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> --- fs/nfs/nfs4_fs.h | 7 +++++++ fs/nfs/nfs4proc.c | 12 ++++++------ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h index 9b3a82abab07..1452177c822d 100644 --- a/fs/nfs/nfs4_fs.h +++ b/fs/nfs/nfs4_fs.h @@ -542,6 +542,13 @@ static inline bool nfs4_valid_open_stateid(const struct nfs4_state *state) return test_bit(NFS_STATE_RECOVERY_FAILED, &state->flags) == 0; } +static inline bool nfs4_state_match_open_stateid_other(const struct nfs4_state *state, + const nfs4_stateid *stateid) +{ + return test_bit(NFS_OPEN_STATE, &state->flags) && + nfs4_stateid_match_other(&state->open_stateid, stateid); +} + #else #define nfs4_close_state(a, b) do { } while (0) diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index f550ac69ffa0..b7b0080977c0 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -1458,7 +1458,6 @@ static void nfs_resync_open_stateid_locked(struct nfs4_state *state) } static void nfs_clear_open_stateid_locked(struct nfs4_state *state, - nfs4_stateid *arg_stateid, nfs4_stateid *stateid, fmode_t fmode) { clear_bit(NFS_O_RDWR_STATE, &state->flags); @@ -1476,10 +1475,9 @@ static void nfs_clear_open_stateid_locked(struct nfs4_state *state, } if (stateid == NULL) return; - /* Handle races with OPEN */ - if (!nfs4_stateid_match_other(arg_stateid, &state->open_stateid) || - (nfs4_stateid_match_other(stateid, &state->open_stateid) && - !nfs4_stateid_is_newer(stateid, &state->open_stateid))) { + /* Handle OPEN+OPEN_DOWNGRADE races */ + if (nfs4_stateid_match_other(stateid, &state->open_stateid) && + !nfs4_stateid_is_newer(stateid, &state->open_stateid)) { nfs_resync_open_stateid_locked(state); return; } @@ -1493,7 +1491,9 @@ static void nfs_clear_open_stateid(struct nfs4_state *state, nfs4_stateid *stateid, fmode_t fmode) { write_seqlock(&state->seqlock); - nfs_clear_open_stateid_locked(state, arg_stateid, stateid, fmode); + /* Ignore, if the CLOSE argment doesn't match the current stateid */ + if (nfs4_state_match_open_stateid_other(state, arg_stateid)) + nfs_clear_open_stateid_locked(state, stateid, fmode); write_sequnlock(&state->seqlock); if (test_bit(NFS_STATE_RECLAIM_NOGRACE, &state->flags)) nfs4_schedule_state_manager(state->owner->so_server->nfs_client); -- 2.7.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] NFSv4: Don't call close if the open stateid has already been cleared 2016-11-14 16:19 ` [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN Trond Myklebust @ 2016-11-14 16:19 ` Trond Myklebust 2016-11-14 16:40 ` [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN Jeff Layton 2018-02-13 20:08 ` Olga Kornievskaia 2 siblings, 0 replies; 17+ messages in thread From: Trond Myklebust @ 2016-11-14 16:19 UTC (permalink / raw) To: linux-nfs; +Cc: Benjamin Coddington, Jeff Layton Ensure we test to see if the open stateid is actually set, before we send a CLOSE. Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> --- fs/nfs/nfs4proc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index b7b0080977c0..b801040c9585 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -3129,7 +3129,8 @@ static void nfs4_close_prepare(struct rpc_task *task, void *data) } else if (is_rdwr) calldata->arg.fmode |= FMODE_READ|FMODE_WRITE; - if (!nfs4_valid_open_stateid(state)) + if (!nfs4_valid_open_stateid(state) || + test_bit(NFS_OPEN_STATE, &state->flags) == 0) call_close = 0; spin_unlock(&state->owner->so_lock); -- 2.7.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2016-11-14 16:19 ` [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN Trond Myklebust 2016-11-14 16:19 ` [PATCH 2/2] NFSv4: Don't call close if the open stateid has already been cleared Trond Myklebust @ 2016-11-14 16:40 ` Jeff Layton 2016-11-14 16:48 ` Trond Myklebust 2018-02-13 20:08 ` Olga Kornievskaia 2 siblings, 1 reply; 17+ messages in thread From: Jeff Layton @ 2016-11-14 16:40 UTC (permalink / raw) To: Trond Myklebust, linux-nfs; +Cc: Benjamin Coddington On Mon, 2016-11-14 at 11:19 -0500, Trond Myklebust wrote: > If the reply to a successful CLOSE call races with an OPEN to the same > file, we can end up scribbling over the stateid that represents the > new open state. > The race looks like: > > Client Server > ====== ====== > > CLOSE stateid A on file "foo" > CLOSE stateid A, return stateid C > OPEN file "foo" > OPEN "foo", return stateid B > Receive reply to OPEN > Reset open state for "foo" > Associate stateid B to "foo" > > Receive CLOSE for A > Reset open state for "foo" > Replace stateid B with C > > The fix is to examine the argument of the CLOSE, and check for a match > with the current stateid "other" field. If the two do not match, then > the above race occurred, and we should just ignore the CLOSE. > > Reported-by: Benjamin Coddington <bcodding@redhat.com> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> > --- > fs/nfs/nfs4_fs.h | 7 +++++++ > fs/nfs/nfs4proc.c | 12 ++++++------ > 2 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h > index 9b3a82abab07..1452177c822d 100644 > --- a/fs/nfs/nfs4_fs.h > +++ b/fs/nfs/nfs4_fs.h > @@ -542,6 +542,13 @@ static inline bool nfs4_valid_open_stateid(const struct nfs4_state *state) > return test_bit(NFS_STATE_RECOVERY_FAILED, &state->flags) == 0; > } > > +static inline bool nfs4_state_match_open_stateid_other(const struct nfs4_state *state, > + const nfs4_stateid *stateid) > +{ > + return test_bit(NFS_OPEN_STATE, &state->flags) && > + nfs4_stateid_match_other(&state->open_stateid, stateid); > +} > + > #else > > #define nfs4_close_state(a, b) do { } while (0) > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index f550ac69ffa0..b7b0080977c0 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -1458,7 +1458,6 @@ static void nfs_resync_open_stateid_locked(struct nfs4_state *state) > } > > static void nfs_clear_open_stateid_locked(struct nfs4_state *state, > - nfs4_stateid *arg_stateid, > nfs4_stateid *stateid, fmode_t fmode) > { > clear_bit(NFS_O_RDWR_STATE, &state->flags); > @@ -1476,10 +1475,9 @@ static void nfs_clear_open_stateid_locked(struct nfs4_state *state, > } > if (stateid == NULL) > return; > - /* Handle races with OPEN */ > - if (!nfs4_stateid_match_other(arg_stateid, &state->open_stateid) || > - (nfs4_stateid_match_other(stateid, &state->open_stateid) && > - !nfs4_stateid_is_newer(stateid, &state->open_stateid))) { > + /* Handle OPEN+OPEN_DOWNGRADE races */ > + if (nfs4_stateid_match_other(stateid, &state->open_stateid) && > + !nfs4_stateid_is_newer(stateid, &state->open_stateid)) { > nfs_resync_open_stateid_locked(state); > return; > } > @@ -1493,7 +1491,9 @@ static void nfs_clear_open_stateid(struct nfs4_state *state, > nfs4_stateid *stateid, fmode_t fmode) > { > write_seqlock(&state->seqlock); > - nfs_clear_open_stateid_locked(state, arg_stateid, stateid, fmode); > + /* Ignore, if the CLOSE argment doesn't match the current stateid */ > + if (nfs4_state_match_open_stateid_other(state, arg_stateid)) > + nfs_clear_open_stateid_locked(state, stateid, fmode); > write_sequnlock(&state->seqlock); > if (test_bit(NFS_STATE_RECLAIM_NOGRACE, &state->flags)) > nfs4_schedule_state_manager(state->owner->so_server->nfs_client); I still don't quite get it. What's the point of paying any attention at all to the stateid returned by the server in a CLOSE response? It's either: a) completely bogus, if the server is following the SHOULD in RFC5661, section 18.2.4 ...or... b) refers to a now-defunct stateid -- probably a later version of the one sent in the request, but the spec doesn't really spell that out, AFAICT. In either case, I don't think it ought to be trusted. Why not just use the arg_stateid universally here? -- Jeff Layton <jlayton@redhat.com> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2016-11-14 16:40 ` [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN Jeff Layton @ 2016-11-14 16:48 ` Trond Myklebust 0 siblings, 0 replies; 17+ messages in thread From: Trond Myklebust @ 2016-11-14 16:48 UTC (permalink / raw) To: jlayton, linux-nfs; +Cc: bcodding T24gTW9uLCAyMDE2LTExLTE0IGF0IDExOjQwIC0wNTAwLCBKZWZmIExheXRvbiB3cm90ZToNCj4g T24gTW9uLCAyMDE2LTExLTE0IGF0IDExOjE5IC0wNTAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6 DQo+ID4gDQo+ID4gSWYgdGhlIHJlcGx5IHRvIGEgc3VjY2Vzc2Z1bCBDTE9TRSBjYWxsIHJhY2Vz IHdpdGggYW4gT1BFTiB0byB0aGUNCj4gPiBzYW1lDQo+ID4gZmlsZSwgd2UgY2FuIGVuZCB1cCBz Y3JpYmJsaW5nIG92ZXIgdGhlIHN0YXRlaWQgdGhhdCByZXByZXNlbnRzIHRoZQ0KPiA+IG5ldyBv cGVuIHN0YXRlLg0KPiA+IFRoZSByYWNlIGxvb2tzIGxpa2U6DQo+ID4gDQo+ID4gwqAgQ2xpZW50 CQkJCVNlcnZlcg0KPiA+IMKgID09PT09PQkJCQk9PT09PT0NCj4gPiANCj4gPiDCoCBDTE9TRSBz dGF0ZWlkIEEgb24gZmlsZSAiZm9vIg0KPiA+IAkJCQkJQ0xPU0Ugc3RhdGVpZCBBLCByZXR1cm4g c3RhdGVpZA0KPiA+IEMNCj4gPiDCoCBPUEVOIGZpbGUgImZvbyINCj4gPiAJCQkJCU9QRU4gImZv byIsIHJldHVybiBzdGF0ZWlkIEINCj4gPiDCoCBSZWNlaXZlIHJlcGx5IHRvIE9QRU4NCj4gPiDC oCBSZXNldCBvcGVuIHN0YXRlIGZvciAiZm9vIg0KPiA+IMKgIEFzc29jaWF0ZSBzdGF0ZWlkIEIg dG8gImZvbyINCj4gPiANCj4gPiDCoCBSZWNlaXZlIENMT1NFIGZvciBBDQo+ID4gwqAgUmVzZXQg b3BlbiBzdGF0ZSBmb3IgImZvbyINCj4gPiDCoCBSZXBsYWNlIHN0YXRlaWQgQiB3aXRoIEMNCj4g PiANCj4gPiBUaGUgZml4IGlzIHRvIGV4YW1pbmUgdGhlIGFyZ3VtZW50IG9mIHRoZSBDTE9TRSwg YW5kIGNoZWNrIGZvciBhDQo+ID4gbWF0Y2gNCj4gPiB3aXRoIHRoZSBjdXJyZW50IHN0YXRlaWQg Im90aGVyIiBmaWVsZC4gSWYgdGhlIHR3byBkbyBub3QgbWF0Y2gsDQo+ID4gdGhlbg0KPiA+IHRo ZSBhYm92ZSByYWNlIG9jY3VycmVkLCBhbmQgd2Ugc2hvdWxkIGp1c3QgaWdub3JlIHRoZSBDTE9T RS4NCj4gPiANCj4gPiBSZXBvcnRlZC1ieTogQmVuamFtaW4gQ29kZGluZ3RvbiA8YmNvZGRpbmdA cmVkaGF0LmNvbT4NCj4gPiBTaWduZWQtb2ZmLWJ5OiBUcm9uZCBNeWtsZWJ1c3QgPHRyb25kLm15 a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20+DQo+ID4gLS0tDQo+ID4gwqBmcy9uZnMvbmZzNF9mcy5o wqDCoHzCoMKgNyArKysrKysrDQo+ID4gwqBmcy9uZnMvbmZzNHByb2MuYyB8IDEyICsrKysrKy0t LS0tLQ0KPiA+IMKgMiBmaWxlcyBjaGFuZ2VkLCAxMyBpbnNlcnRpb25zKCspLCA2IGRlbGV0aW9u cygtKQ0KPiA+IA0KPiA+IGRpZmYgLS1naXQgYS9mcy9uZnMvbmZzNF9mcy5oIGIvZnMvbmZzL25m czRfZnMuaA0KPiA+IGluZGV4IDliM2E4MmFiYWIwNy4uMTQ1MjE3N2M4MjJkIDEwMDY0NA0KPiA+ IC0tLSBhL2ZzL25mcy9uZnM0X2ZzLmgNCj4gPiArKysgYi9mcy9uZnMvbmZzNF9mcy5oDQo+ID4g QEAgLTU0Miw2ICs1NDIsMTMgQEAgc3RhdGljIGlubGluZSBib29sDQo+ID4gbmZzNF92YWxpZF9v cGVuX3N0YXRlaWQoY29uc3Qgc3RydWN0IG5mczRfc3RhdGUgKnN0YXRlKQ0KPiA+IMKgCXJldHVy biB0ZXN0X2JpdChORlNfU1RBVEVfUkVDT1ZFUllfRkFJTEVELCAmc3RhdGUtPmZsYWdzKQ0KPiA+ ID09IDA7DQo+ID4gwqB9DQo+ID4gwqANCj4gPiArc3RhdGljIGlubGluZSBib29sIG5mczRfc3Rh dGVfbWF0Y2hfb3Blbl9zdGF0ZWlkX290aGVyKGNvbnN0DQo+ID4gc3RydWN0IG5mczRfc3RhdGUg KnN0YXRlLA0KPiA+ICsJCWNvbnN0IG5mczRfc3RhdGVpZCAqc3RhdGVpZCkNCj4gPiArew0KPiA+ ICsJcmV0dXJuIHRlc3RfYml0KE5GU19PUEVOX1NUQVRFLCAmc3RhdGUtPmZsYWdzKSAmJg0KPiA+ ICsJCW5mczRfc3RhdGVpZF9tYXRjaF9vdGhlcigmc3RhdGUtPm9wZW5fc3RhdGVpZCwNCj4gPiBz dGF0ZWlkKTsNCj4gPiArfQ0KPiA+ICsNCj4gPiDCoCNlbHNlDQo+ID4gwqANCj4gPiDCoCNkZWZp bmUgbmZzNF9jbG9zZV9zdGF0ZShhLCBiKSBkbyB7IH0gd2hpbGUgKDApDQo+ID4gZGlmZiAtLWdp dCBhL2ZzL25mcy9uZnM0cHJvYy5jIGIvZnMvbmZzL25mczRwcm9jLmMNCj4gPiBpbmRleCBmNTUw YWM2OWZmYTAuLmI3YjAwODA5NzdjMCAxMDA2NDQNCj4gPiAtLS0gYS9mcy9uZnMvbmZzNHByb2Mu Yw0KPiA+ICsrKyBiL2ZzL25mcy9uZnM0cHJvYy5jDQo+ID4gQEAgLTE0NTgsNyArMTQ1OCw2IEBA IHN0YXRpYyB2b2lkDQo+ID4gbmZzX3Jlc3luY19vcGVuX3N0YXRlaWRfbG9ja2VkKHN0cnVjdCBu ZnM0X3N0YXRlICpzdGF0ZSkNCj4gPiDCoH0NCj4gPiDCoA0KPiA+IMKgc3RhdGljIHZvaWQgbmZz X2NsZWFyX29wZW5fc3RhdGVpZF9sb2NrZWQoc3RydWN0IG5mczRfc3RhdGUNCj4gPiAqc3RhdGUs DQo+ID4gLQkJbmZzNF9zdGF0ZWlkICphcmdfc3RhdGVpZCwNCj4gPiDCoAkJbmZzNF9zdGF0ZWlk ICpzdGF0ZWlkLCBmbW9kZV90IGZtb2RlKQ0KPiA+IMKgew0KPiA+IMKgCWNsZWFyX2JpdChORlNf T19SRFdSX1NUQVRFLCAmc3RhdGUtPmZsYWdzKTsNCj4gPiBAQCAtMTQ3NiwxMCArMTQ3NSw5IEBA IHN0YXRpYyB2b2lkDQo+ID4gbmZzX2NsZWFyX29wZW5fc3RhdGVpZF9sb2NrZWQoc3RydWN0IG5m czRfc3RhdGUgKnN0YXRlLA0KPiA+IMKgCX0NCj4gPiDCoAlpZiAoc3RhdGVpZCA9PSBOVUxMKQ0K PiA+IMKgCQlyZXR1cm47DQo+ID4gLQkvKiBIYW5kbGUgcmFjZXMgd2l0aCBPUEVOICovDQo+ID4g LQlpZiAoIW5mczRfc3RhdGVpZF9tYXRjaF9vdGhlcihhcmdfc3RhdGVpZCwgJnN0YXRlLQ0KPiA+ ID5vcGVuX3N0YXRlaWQpIHx8DQo+ID4gLQnCoMKgwqDCoChuZnM0X3N0YXRlaWRfbWF0Y2hfb3Ro ZXIoc3RhdGVpZCwgJnN0YXRlLQ0KPiA+ID5vcGVuX3N0YXRlaWQpICYmDQo+ID4gLQnCoMKgwqDC oCFuZnM0X3N0YXRlaWRfaXNfbmV3ZXIoc3RhdGVpZCwgJnN0YXRlLQ0KPiA+ID5vcGVuX3N0YXRl aWQpKSkgew0KPiA+ICsJLyogSGFuZGxlIE9QRU4rT1BFTl9ET1dOR1JBREUgcmFjZXMgKi8NCj4g PiArCWlmIChuZnM0X3N0YXRlaWRfbWF0Y2hfb3RoZXIoc3RhdGVpZCwgJnN0YXRlLQ0KPiA+ID5v cGVuX3N0YXRlaWQpICYmDQo+ID4gKwnCoMKgwqDCoCFuZnM0X3N0YXRlaWRfaXNfbmV3ZXIoc3Rh dGVpZCwgJnN0YXRlLT5vcGVuX3N0YXRlaWQpKSANCj4gPiB7DQo+ID4gwqAJCW5mc19yZXN5bmNf b3Blbl9zdGF0ZWlkX2xvY2tlZChzdGF0ZSk7DQo+ID4gwqAJCXJldHVybjsNCj4gPiDCoAl9DQo+ ID4gQEAgLTE0OTMsNyArMTQ5MSw5IEBAIHN0YXRpYyB2b2lkIG5mc19jbGVhcl9vcGVuX3N0YXRl aWQoc3RydWN0DQo+ID4gbmZzNF9zdGF0ZSAqc3RhdGUsDQo+ID4gwqAJbmZzNF9zdGF0ZWlkICpz dGF0ZWlkLCBmbW9kZV90IGZtb2RlKQ0KPiA+IMKgew0KPiA+IMKgCXdyaXRlX3NlcWxvY2soJnN0 YXRlLT5zZXFsb2NrKTsNCj4gPiAtCW5mc19jbGVhcl9vcGVuX3N0YXRlaWRfbG9ja2VkKHN0YXRl LCBhcmdfc3RhdGVpZCwgc3RhdGVpZCwNCj4gPiBmbW9kZSk7DQo+ID4gKwkvKiBJZ25vcmUsIGlm IHRoZSBDTE9TRSBhcmdtZW50IGRvZXNuJ3QgbWF0Y2ggdGhlIGN1cnJlbnQNCj4gPiBzdGF0ZWlk ICovDQo+ID4gKwlpZiAobmZzNF9zdGF0ZV9tYXRjaF9vcGVuX3N0YXRlaWRfb3RoZXIoc3RhdGUs DQo+ID4gYXJnX3N0YXRlaWQpKQ0KPiA+ICsJCW5mc19jbGVhcl9vcGVuX3N0YXRlaWRfbG9ja2Vk KHN0YXRlLCBzdGF0ZWlkLA0KPiA+IGZtb2RlKTsNCj4gPiDCoAl3cml0ZV9zZXF1bmxvY2soJnN0 YXRlLT5zZXFsb2NrKTsNCj4gPiDCoAlpZiAodGVzdF9iaXQoTkZTX1NUQVRFX1JFQ0xBSU1fTk9H UkFDRSwgJnN0YXRlLT5mbGFncykpDQo+ID4gwqAJCW5mczRfc2NoZWR1bGVfc3RhdGVfbWFuYWdl cihzdGF0ZS0+b3duZXItDQo+ID4gPnNvX3NlcnZlci0+bmZzX2NsaWVudCk7DQo+IA0KPiBJIHN0 aWxsIGRvbid0IHF1aXRlIGdldCBpdC4gV2hhdCdzIHRoZSBwb2ludCBvZiBwYXlpbmcgYW55IGF0 dGVudGlvbg0KPiBhdA0KPiBhbGwgdG8gdGhlIHN0YXRlaWQgcmV0dXJuZWQgYnkgdGhlIHNlcnZl ciBpbiBhIENMT1NFIHJlc3BvbnNlPyBJdCdzDQo+IGVpdGhlcjoNCj4gDQo+IGEpIGNvbXBsZXRl bHkgYm9ndXMsIGlmIHRoZSBzZXJ2ZXIgaXMgZm9sbG93aW5nIHRoZSBTSE9VTEQgaW4NCj4gUkZD NTY2MSwNCj4gc2VjdGlvbiAxOC4yLjQNCj4gDQo+IC4uLm9yLi4uDQo+IA0KPiBiKSByZWZlcnMg dG8gYSBub3ctZGVmdW5jdCBzdGF0ZWlkIC0tIHByb2JhYmx5IGEgbGF0ZXIgdmVyc2lvbiBvZiB0 aGUNCj4gb25lIHNlbnQgaW4gdGhlIHJlcXVlc3QsIGJ1dCB0aGUgc3BlYyBkb2Vzbid0IHJlYWxs eSBzcGVsbCB0aGF0IG91dCwNCj4gQUZBSUNULg0KPiANCj4gSW4gZWl0aGVyIGNhc2UsIEkgZG9u J3QgdGhpbmsgaXQgb3VnaHQgdG8gYmUgdHJ1c3RlZC4gV2h5IG5vdCBqdXN0DQo+IHVzZQ0KPiB0 aGUgYXJnX3N0YXRlaWQgdW5pdmVyc2FsbHkgaGVyZT8NCj4gDQoNClRoZSBjb2RlIHBhdGggYWxz byBoYXMgdG8gaGFuZGxlIE9QRU5fRE9XTkdSQURFLg== ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2016-11-14 16:19 ` [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN Trond Myklebust 2016-11-14 16:19 ` [PATCH 2/2] NFSv4: Don't call close if the open stateid has already been cleared Trond Myklebust 2016-11-14 16:40 ` [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN Jeff Layton @ 2018-02-13 20:08 ` Olga Kornievskaia 2018-02-14 15:05 ` Benjamin Coddington 2018-02-15 12:19 ` Mkrtchyan, Tigran 2 siblings, 2 replies; 17+ messages in thread From: Olga Kornievskaia @ 2018-02-13 20:08 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, Benjamin Coddington, Jeff Layton On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust <trond.myklebust@primarydata.com> wrote: > If the reply to a successful CLOSE call races with an OPEN to the same > file, we can end up scribbling over the stateid that represents the > new open state. > The race looks like: > > Client Server > ====== ====== > > CLOSE stateid A on file "foo" > CLOSE stateid A, return stateid C Hi folks, I'd like to understand this particular issue. Specifically I don't understand how can server return stateid C to the close with stateid A. As per RFC 7530 or 5661. It says that state is returned by the close shouldn't be used. Even though CLOSE returns a stateid, this stateid is not useful to the client and should be treated as deprecated. CLOSE "shuts down" the state associated with all OPENs for the file by a single open-owner. As noted above, CLOSE will either release all file locking state or return an error. Therefore, the stateid returned by CLOSE is not useful for the operations that follow. Is this because the spec says "should" and not a "must"? Linux server increments a state's sequenceid on CLOSE. Ontap server does not. I'm not sure what other servers do. Are all these implementations equality correct? > OPEN file "foo" > OPEN "foo", return stateid B > Receive reply to OPEN > Reset open state for "foo" > Associate stateid B to "foo" > > Receive CLOSE for A > Reset open state for "foo" > Replace stateid B with C > > The fix is to examine the argument of the CLOSE, and check for a match > with the current stateid "other" field. If the two do not match, then > the above race occurred, and we should just ignore the CLOSE. > > Reported-by: Benjamin Coddington <bcodding@redhat.com> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> > --- > fs/nfs/nfs4_fs.h | 7 +++++++ > fs/nfs/nfs4proc.c | 12 ++++++------ > 2 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h > index 9b3a82abab07..1452177c822d 100644 > --- a/fs/nfs/nfs4_fs.h > +++ b/fs/nfs/nfs4_fs.h > @@ -542,6 +542,13 @@ static inline bool nfs4_valid_open_stateid(const struct nfs4_state *state) > return test_bit(NFS_STATE_RECOVERY_FAILED, &state->flags) == 0; > } > > +static inline bool nfs4_state_match_open_stateid_other(const struct nfs4_state *state, > + const nfs4_stateid *stateid) > +{ > + return test_bit(NFS_OPEN_STATE, &state->flags) && > + nfs4_stateid_match_other(&state->open_stateid, stateid); > +} > + > #else > > #define nfs4_close_state(a, b) do { } while (0) > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index f550ac69ffa0..b7b0080977c0 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -1458,7 +1458,6 @@ static void nfs_resync_open_stateid_locked(struct nfs4_state *state) > } > > static void nfs_clear_open_stateid_locked(struct nfs4_state *state, > - nfs4_stateid *arg_stateid, > nfs4_stateid *stateid, fmode_t fmode) > { > clear_bit(NFS_O_RDWR_STATE, &state->flags); > @@ -1476,10 +1475,9 @@ static void nfs_clear_open_stateid_locked(struct nfs4_state *state, > } > if (stateid == NULL) > return; > - /* Handle races with OPEN */ > - if (!nfs4_stateid_match_other(arg_stateid, &state->open_stateid) || > - (nfs4_stateid_match_other(stateid, &state->open_stateid) && > - !nfs4_stateid_is_newer(stateid, &state->open_stateid))) { > + /* Handle OPEN+OPEN_DOWNGRADE races */ > + if (nfs4_stateid_match_other(stateid, &state->open_stateid) && > + !nfs4_stateid_is_newer(stateid, &state->open_stateid)) { > nfs_resync_open_stateid_locked(state); > return; > } > @@ -1493,7 +1491,9 @@ static void nfs_clear_open_stateid(struct nfs4_state *state, > nfs4_stateid *stateid, fmode_t fmode) > { > write_seqlock(&state->seqlock); > - nfs_clear_open_stateid_locked(state, arg_stateid, stateid, fmode); > + /* Ignore, if the CLOSE argment doesn't match the current stateid */ > + if (nfs4_state_match_open_stateid_other(state, arg_stateid)) > + nfs_clear_open_stateid_locked(state, stateid, fmode); > write_sequnlock(&state->seqlock); > if (test_bit(NFS_STATE_RECLAIM_NOGRACE, &state->flags)) > nfs4_schedule_state_manager(state->owner->so_server->nfs_client); > -- > 2.7.4 > > -- > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2018-02-13 20:08 ` Olga Kornievskaia @ 2018-02-14 15:05 ` Benjamin Coddington 2018-02-14 15:21 ` Jeff Layton 2018-02-15 12:19 ` Mkrtchyan, Tigran 1 sibling, 1 reply; 17+ messages in thread From: Benjamin Coddington @ 2018-02-14 15:05 UTC (permalink / raw) To: Olga Kornievskaia; +Cc: Trond Myklebust, linux-nfs, Jeff Layton Hi Olga, On 13 Feb 2018, at 15:08, Olga Kornievskaia wrote: > On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust > <trond.myklebust@primarydata.com> wrote: >> If the reply to a successful CLOSE call races with an OPEN to the same >> file, we can end up scribbling over the stateid that represents the >> new open state. >> The race looks like: >> >> Client Server >> ====== ====== >> >> CLOSE stateid A on file "foo" >> CLOSE stateid A, return stateid C > > Hi folks, > > I'd like to understand this particular issue. Specifically I don't > understand how can server return stateid C to the close with stateid > A. I think in this explanation of the race, stateid C is an incremented version of A -- the stateid's "other" fields match -- OR it is the invalid special stateid according to RFC 5661, Section 18.2.4. > As per RFC 7530 or 5661. It says that state is returned by the close > shouldn't be used. > > Even though CLOSE returns a stateid, this stateid is not useful to > the client and should be treated as deprecated. CLOSE "shuts down" > the state associated with all OPENs for the file by a single > open-owner. As noted above, CLOSE will either release all file > locking state or return an error. Therefore, the stateid returned by > CLOSE is not useful for the operations that follow. > > Is this because the spec says "should" and not a "must"? I don't understand - is what because? The state returned from CLOSE is useful to be used by the client for housekeeping, but it won't be re-used in the protocol. > Linux server increments a state's sequenceid on CLOSE. Ontap server > does not. I'm not sure what other servers do. Are all these > implementations equality correct? Ah, good question there.. Even though the stateid is not useful for operations that follow, I think the sequenceid should be incremented because the CLOSE is an operation that modifies the set of locks or state: In RFC 7530, Section 9.1.4.2.: ... When such a set of locks is first created, the server returns a stateid with a seqid value of one. On subsequent operations that modify the set of locks, the server is required to advance the seqid field by one whenever it returns a stateid for the same state-owner/file/type combination and the operation is one that might make some change in the set of locks actually designated. In this case, the server will return a stateid with an "other" field the same as previously used for that state-owner/file/type combination, with an incremented seqid field. But, in RFC 5661, the CLOSE response SHOULD be the invalid special stateid. I don't recall, does the linux server increment the existing stateid or send back the invalid special stateid on v4.1? Ben ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2018-02-14 15:05 ` Benjamin Coddington @ 2018-02-14 15:21 ` Jeff Layton 2018-02-14 15:29 ` Trond Myklebust 0 siblings, 1 reply; 17+ messages in thread From: Jeff Layton @ 2018-02-14 15:21 UTC (permalink / raw) To: Benjamin Coddington, Olga Kornievskaia; +Cc: Trond Myklebust, linux-nfs On Wed, 2018-02-14 at 10:05 -0500, Benjamin Coddington wrote: > Hi Olga, > > On 13 Feb 2018, at 15:08, Olga Kornievskaia wrote: > > > On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust > > <trond.myklebust@primarydata.com> wrote: > > > If the reply to a successful CLOSE call races with an OPEN to the same > > > file, we can end up scribbling over the stateid that represents the > > > new open state. > > > The race looks like: > > > > > > Client Server > > > ====== ====== > > > > > > CLOSE stateid A on file "foo" > > > CLOSE stateid A, return stateid C > > > > Hi folks, > > > > I'd like to understand this particular issue. Specifically I don't > > understand how can server return stateid C to the close with stateid > > A. > > I think in this explanation of the race, stateid C is an incremented version > of A -- the stateid's "other" fields match -- OR it is the invalid special > stateid according to RFC 5661, Section 18.2.4. > > > As per RFC 7530 or 5661. It says that state is returned by the close > > shouldn't be used. > > > > Even though CLOSE returns a stateid, this stateid is not useful to > > the client and should be treated as deprecated. CLOSE "shuts down" > > the state associated with all OPENs for the file by a single > > open-owner. As noted above, CLOSE will either release all file > > locking state or return an error. Therefore, the stateid returned by > > CLOSE is not useful for the operations that follow. > > > > Is this because the spec says "should" and not a "must"? > > I don't understand - is what because? The state returned from CLOSE is > useful to be used by the client for housekeeping, but it won't be re-used in > the protocol. > > > Linux server increments a state's sequenceid on CLOSE. Ontap server > > does not. I'm not sure what other servers do. Are all these > > implementations equality correct? > > Ah, good question there.. Even though the stateid is not useful for > operations that follow, I think the sequenceid should be incremented because > the CLOSE is an operation that modifies the set of locks or state: > It doesn't matter. > In RFC 7530, Section 9.1.4.2.: > ... > When such a set of locks is first created, the server returns a > stateid with a seqid value of one. On subsequent operations that > modify the set of locks, the server is required to advance the > seqid field by one whenever it returns a stateid for the same > state-owner/file/type combination and the operation is one that might > make some change in the set of locks actually designated. In this > case, the server will return a stateid with an "other" field the same > as previously used for that state-owner/file/type combination, with > an incremented seqid field. > > But, in RFC 5661, the CLOSE response SHOULD be the invalid special stateid. > I don't recall, does the linux server increment the existing stateid or send > back the invalid special stateid on v4.1? > A CLOSE, by definition, is destroying the old stateid, so what does it matter what you return on success? It's bogus either way. If knfsd is sending back a "real" stateid there, then it's likely only because that's what the v4.0 spec said to do ~10 years ago. It's probably fine to fix it to just return the invalid special stateid and call it a day. All clients should just ignore it. -- Jeff Layton <jlayton@redhat.com> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2018-02-14 15:21 ` Jeff Layton @ 2018-02-14 15:29 ` Trond Myklebust 2018-02-14 15:42 ` Benjamin Coddington 0 siblings, 1 reply; 17+ messages in thread From: Trond Myklebust @ 2018-02-14 15:29 UTC (permalink / raw) To: bcodding redhat, jlayton, aglo; +Cc: linux-nfs T24gV2VkLCAyMDE4LTAyLTE0IGF0IDEwOjIxIC0wNTAwLCBKZWZmIExheXRvbiB3cm90ZToNCj4g T24gV2VkLCAyMDE4LTAyLTE0IGF0IDEwOjA1IC0wNTAwLCBCZW5qYW1pbiBDb2RkaW5ndG9uIHdy b3RlOg0KPiA+IEhpIE9sZ2EsDQo+ID4gDQo+ID4gT24gMTMgRmViIDIwMTgsIGF0IDE1OjA4LCBP bGdhIEtvcm5pZXZza2FpYSB3cm90ZToNCj4gPiANCj4gPiA+IE9uIE1vbiwgTm92IDE0LCAyMDE2 IGF0IDExOjE5IEFNLCBUcm9uZCBNeWtsZWJ1c3QNCj4gPiA+IDx0cm9uZC5teWtsZWJ1c3RAcHJp bWFyeWRhdGEuY29tPiB3cm90ZToNCj4gPiA+ID4gSWYgdGhlIHJlcGx5IHRvIGEgc3VjY2Vzc2Z1 bCBDTE9TRSBjYWxsIHJhY2VzIHdpdGggYW4gT1BFTiB0bw0KPiA+ID4gPiB0aGUgc2FtZQ0KPiA+ ID4gPiBmaWxlLCB3ZSBjYW4gZW5kIHVwIHNjcmliYmxpbmcgb3ZlciB0aGUgc3RhdGVpZCB0aGF0 IHJlcHJlc2VudHMNCj4gPiA+ID4gdGhlDQo+ID4gPiA+IG5ldyBvcGVuIHN0YXRlLg0KPiA+ID4g PiBUaGUgcmFjZSBsb29rcyBsaWtlOg0KPiA+ID4gPiANCj4gPiA+ID4gICBDbGllbnQgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgIFNlcnZlcg0KPiA+ID4gPiAgID09PT09PSAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgPT09PT09DQo+ID4gPiA+IA0KPiA+ID4gPiAgIENMT1NF IHN0YXRlaWQgQSBvbiBmaWxlICJmb28iDQo+ID4gPiA+ICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICBDTE9TRSBzdGF0ZWlkIEEsIHJldHVybg0KPiA+ID4gPiBzdGF0ZWlk IEMNCj4gPiA+IA0KPiA+ID4gSGkgZm9sa3MsDQo+ID4gPiANCj4gPiA+IEknZCBsaWtlIHRvIHVu ZGVyc3RhbmQgdGhpcyBwYXJ0aWN1bGFyIGlzc3VlLiBTcGVjaWZpY2FsbHkgSQ0KPiA+ID4gZG9u J3QNCj4gPiA+IHVuZGVyc3RhbmQgaG93IGNhbiBzZXJ2ZXIgcmV0dXJuIHN0YXRlaWQgQyB0byB0 aGUgY2xvc2Ugd2l0aA0KPiA+ID4gc3RhdGVpZA0KPiA+ID4gQS4NCj4gPiANCj4gPiBJIHRoaW5r IGluIHRoaXMgZXhwbGFuYXRpb24gb2YgdGhlIHJhY2UsIHN0YXRlaWQgQyBpcyBhbg0KPiA+IGlu Y3JlbWVudGVkIHZlcnNpb24NCj4gPiBvZiBBIC0tIHRoZSBzdGF0ZWlkJ3MgIm90aGVyIiBmaWVs ZHMgbWF0Y2ggLS0gT1IgaXQgaXMgdGhlIGludmFsaWQNCj4gPiBzcGVjaWFsDQo+ID4gc3RhdGVp ZCBhY2NvcmRpbmcgdG8gUkZDIDU2NjEsIFNlY3Rpb24gMTguMi40Lg0KPiA+IA0KPiA+ID4gQXMg cGVyIFJGQyA3NTMwIG9yIDU2NjEuIEl0IHNheXMgdGhhdCBzdGF0ZSBpcyByZXR1cm5lZCBieSB0 aGUNCj4gPiA+IGNsb3NlDQo+ID4gPiBzaG91bGRuJ3QgYmUgdXNlZC4NCj4gPiA+IA0KPiA+ID4g RXZlbiB0aG91Z2ggQ0xPU0UgcmV0dXJucyBhIHN0YXRlaWQsIHRoaXMgc3RhdGVpZCBpcyBub3Qg dXNlZnVsDQo+ID4gPiB0bw0KPiA+ID4gICAgdGhlIGNsaWVudCBhbmQgc2hvdWxkIGJlIHRyZWF0 ZWQgYXMgZGVwcmVjYXRlZC4gIENMT1NFICJzaHV0cw0KPiA+ID4gZG93biINCj4gPiA+ICAgIHRo ZSBzdGF0ZSBhc3NvY2lhdGVkIHdpdGggYWxsIE9QRU5zIGZvciB0aGUgZmlsZSBieSBhIHNpbmds ZQ0KPiA+ID4gICAgb3Blbi1vd25lci4gIEFzIG5vdGVkIGFib3ZlLCBDTE9TRSB3aWxsIGVpdGhl ciByZWxlYXNlIGFsbA0KPiA+ID4gZmlsZQ0KPiA+ID4gICAgbG9ja2luZyBzdGF0ZSBvciByZXR1 cm4gYW4gZXJyb3IuICBUaGVyZWZvcmUsIHRoZSBzdGF0ZWlkDQo+ID4gPiByZXR1cm5lZCBieQ0K PiA+ID4gICAgQ0xPU0UgaXMgbm90IHVzZWZ1bCBmb3IgdGhlIG9wZXJhdGlvbnMgdGhhdCBmb2xs b3cuDQo+ID4gPiANCj4gPiA+IElzIHRoaXMgYmVjYXVzZSB0aGUgc3BlYyBzYXlzICJzaG91bGQi IGFuZCBub3QgYSAibXVzdCI/DQo+ID4gDQo+ID4gSSBkb24ndCB1bmRlcnN0YW5kIC0gaXMgd2hh dCBiZWNhdXNlPyAgVGhlIHN0YXRlIHJldHVybmVkIGZyb20NCj4gPiBDTE9TRSBpcw0KPiA+IHVz ZWZ1bCB0byBiZSB1c2VkIGJ5IHRoZSBjbGllbnQgZm9yIGhvdXNla2VlcGluZywgYnV0IGl0IHdv bid0IGJlDQo+ID4gcmUtdXNlZCBpbg0KPiA+IHRoZSBwcm90b2NvbC4NCj4gPiANCj4gPiA+IExp bnV4IHNlcnZlciBpbmNyZW1lbnRzIGEgc3RhdGUncyBzZXF1ZW5jZWlkIG9uIENMT1NFLiBPbnRh cA0KPiA+ID4gc2VydmVyDQo+ID4gPiBkb2VzIG5vdC4gSSdtIG5vdCBzdXJlIHdoYXQgb3RoZXIg c2VydmVycyBkby4gQXJlIGFsbCB0aGVzZQ0KPiA+ID4gaW1wbGVtZW50YXRpb25zIGVxdWFsaXR5 IGNvcnJlY3Q/DQo+ID4gDQo+ID4gQWgsIGdvb2QgcXVlc3Rpb24gdGhlcmUuLiAgRXZlbiB0aG91 Z2ggdGhlIHN0YXRlaWQgaXMgbm90IHVzZWZ1bA0KPiA+IGZvcg0KPiA+IG9wZXJhdGlvbnMgdGhh dCBmb2xsb3csIEkgdGhpbmsgdGhlIHNlcXVlbmNlaWQgc2hvdWxkIGJlDQo+ID4gaW5jcmVtZW50 ZWQgYmVjYXVzZQ0KPiA+IHRoZSBDTE9TRSBpcyBhbiBvcGVyYXRpb24gdGhhdCBtb2RpZmllcyB0 aGUgc2V0IG9mIGxvY2tzIG9yIHN0YXRlOg0KPiA+IA0KPiANCj4gSXQgZG9lc24ndCBtYXR0ZXIu DQo+IA0KPiA+IEluIFJGQyA3NTMwLCBTZWN0aW9uIDkuMS40LjIuOg0KPiA+IC4uLg0KPiA+ICAg IFdoZW4gc3VjaCBhIHNldCBvZiBsb2NrcyBpcyBmaXJzdCBjcmVhdGVkLCB0aGUgc2VydmVyIHJl dHVybnMgYQ0KPiA+ICAgIHN0YXRlaWQgd2l0aCBhIHNlcWlkIHZhbHVlIG9mIG9uZS4gIE9uIHN1 YnNlcXVlbnQgb3BlcmF0aW9ucw0KPiA+IHRoYXQNCj4gPiAgICBtb2RpZnkgdGhlIHNldCBvZiBs b2NrcywgdGhlIHNlcnZlciBpcyByZXF1aXJlZCB0byBhZHZhbmNlIHRoZQ0KPiA+ICAgIHNlcWlk IGZpZWxkIGJ5IG9uZSB3aGVuZXZlciBpdCByZXR1cm5zIGEgc3RhdGVpZCBmb3IgdGhlIHNhbWUN Cj4gPiAgICBzdGF0ZS1vd25lci9maWxlL3R5cGUgY29tYmluYXRpb24gYW5kIHRoZSBvcGVyYXRp b24gaXMgb25lIHRoYXQNCj4gPiBtaWdodA0KPiA+ICAgIG1ha2Ugc29tZSBjaGFuZ2UgaW4gdGhl IHNldCBvZiBsb2NrcyBhY3R1YWxseSBkZXNpZ25hdGVkLiAgSW4NCj4gPiB0aGlzDQo+ID4gICAg Y2FzZSwgdGhlIHNlcnZlciB3aWxsIHJldHVybiBhIHN0YXRlaWQgd2l0aCBhbiAib3RoZXIiIGZp ZWxkIHRoZQ0KPiA+IHNhbWUNCj4gPiAgICBhcyBwcmV2aW91c2x5IHVzZWQgZm9yIHRoYXQgc3Rh dGUtb3duZXIvZmlsZS90eXBlIGNvbWJpbmF0aW9uLA0KPiA+IHdpdGgNCj4gPiAgICBhbiBpbmNy ZW1lbnRlZCBzZXFpZCBmaWVsZC4NCj4gPiANCj4gPiBCdXQsIGluIFJGQyA1NjYxLCB0aGUgQ0xP U0UgcmVzcG9uc2UgU0hPVUxEIGJlIHRoZSBpbnZhbGlkIHNwZWNpYWwNCj4gPiBzdGF0ZWlkLg0K PiA+IEkgZG9uJ3QgcmVjYWxsLCBkb2VzIHRoZSBsaW51eCBzZXJ2ZXIgaW5jcmVtZW50IHRoZSBl eGlzdGluZw0KPiA+IHN0YXRlaWQgb3Igc2VuZA0KPiA+IGJhY2sgdGhlIGludmFsaWQgc3BlY2lh bCBzdGF0ZWlkIG9uIHY0LjE/DQo+ID4gDQo+IA0KPiBBIENMT1NFLCBieSBkZWZpbml0aW9uLCBp cyBkZXN0cm95aW5nIHRoZSBvbGQgc3RhdGVpZCwgc28gd2hhdCBkb2VzDQo+IGl0DQo+IG1hdHRl ciB3aGF0IHlvdSByZXR1cm4gb24gc3VjY2Vzcz8gSXQncyBib2d1cyBlaXRoZXIgd2F5Lg0KPiAN Cj4gSWYga25mc2QgaXMgc2VuZGluZyBiYWNrIGEgInJlYWwiIHN0YXRlaWQgdGhlcmUsIHRoZW4g aXQncyBsaWtlbHkNCj4gb25seQ0KPiBiZWNhdXNlIHRoYXQncyB3aGF0IHRoZSB2NC4wIHNwZWMg c2FpZCB0byBkbyB+MTAgeWVhcnMgYWdvLiBJdCdzDQo+IHByb2JhYmx5IGZpbmUgdG8gZml4IGl0 IHRvIGp1c3QgcmV0dXJuIHRoZSBpbnZhbGlkIHNwZWNpYWwgc3RhdGVpZA0KPiBhbmQNCj4gY2Fs bCBpdCBhIGRheS4gQWxsIGNsaWVudHMgc2hvdWxkIGp1c3QgaWdub3JlIGl0Lg0KPiANCg0KSXMg dGhlcmUgYSBwcm9wb3NhbCB0byBjaGFuZ2UgdGhlIGN1cnJlbnQgY2xpZW50IGJlaGF2aW91ciBo ZXJlPyBBcyBmYXINCmFzIEkgY2FuIHRlbGwsIHRoZSBhbnN3ZXIgaXMgIm5vIi4gQW0gSSBtaXNz aW5nIHNvbWV0aGluZz8NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQg bWFpbnRhaW5lciwgUHJpbWFyeURhdGENCnRyb25kLm15a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20N Cg== ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2018-02-14 15:29 ` Trond Myklebust @ 2018-02-14 15:42 ` Benjamin Coddington 2018-02-14 16:06 ` Olga Kornievskaia 0 siblings, 1 reply; 17+ messages in thread From: Benjamin Coddington @ 2018-02-14 15:42 UTC (permalink / raw) To: Trond Myklebust; +Cc: jlayton, aglo, linux-nfs On 14 Feb 2018, at 10:29, Trond Myklebust wrote: > On Wed, 2018-02-14 at 10:21 -0500, Jeff Layton wrote: >> On Wed, 2018-02-14 at 10:05 -0500, Benjamin Coddington wrote: >>> Hi Olga, >>> >>> On 13 Feb 2018, at 15:08, Olga Kornievskaia wrote: >>> >>>> On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust >>>> <trond.myklebust@primarydata.com> wrote: >>>> ... >>> Ah, good question there.. Even though the stateid is not useful >>> for >>> operations that follow, I think the sequenceid should be >>> incremented because >>> the CLOSE is an operation that modifies the set of locks or state: >>> >> >> It doesn't matter. Yes, good condensed point. It doesn't matter. >>> ... > Is there a proposal to change the current client behaviour here? As far > as I can tell, the answer is "no". Am I missing something? Not that I can see. I think we're just comparing linux and netapp server behaviors.. Ben ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2018-02-14 15:42 ` Benjamin Coddington @ 2018-02-14 16:06 ` Olga Kornievskaia 2018-02-14 16:59 ` Trond Myklebust 0 siblings, 1 reply; 17+ messages in thread From: Olga Kornievskaia @ 2018-02-14 16:06 UTC (permalink / raw) To: Benjamin Coddington; +Cc: Trond Myklebust, Jeff Layton, linux-nfs On Wed, Feb 14, 2018 at 10:42 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > On 14 Feb 2018, at 10:29, Trond Myklebust wrote: >> On Wed, 2018-02-14 at 10:21 -0500, Jeff Layton wrote: >>> On Wed, 2018-02-14 at 10:05 -0500, Benjamin Coddington wrote: >>>> Hi Olga, >>>> >>>> On 13 Feb 2018, at 15:08, Olga Kornievskaia wrote: >>>> >>>>> On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust >>>>> <trond.myklebust@primarydata.com> wrote: >>>>> ... >>>> Ah, good question there.. Even though the stateid is not useful >>>> for >>>> operations that follow, I think the sequenceid should be >>>> incremented because >>>> the CLOSE is an operation that modifies the set of locks or state: >>>> >>> >>> It doesn't matter. > > Yes, good condensed point. It doesn't matter. > >>>> ... >> Is there a proposal to change the current client behaviour here? As far >> as I can tell, the answer is "no". Am I missing something? > > Not that I can see. I think we're just comparing linux and netapp server > behaviors.. > > Ben I just found very surprising that in nfs4_close_done() which calls eventually calls nfs_clear_open_stateid_locked() we change the state based on the stateid received from the CLOSE. nfs_clear_open_stateid_locked() is only called from nfs4_close_done() and no other function. I guess I'm not wondering if we had this problem described in this patch of the delayed CLOSE, if we didn't update the state after receiving the close... (so yes this is a weak proposal). ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2018-02-14 16:06 ` Olga Kornievskaia @ 2018-02-14 16:59 ` Trond Myklebust 2018-02-14 22:17 ` Olga Kornievskaia 0 siblings, 1 reply; 17+ messages in thread From: Trond Myklebust @ 2018-02-14 16:59 UTC (permalink / raw) To: bcodding redhat, aglo; +Cc: jlayton, linux-nfs T24gV2VkLCAyMDE4LTAyLTE0IGF0IDExOjA2IC0wNTAwLCBPbGdhIEtvcm5pZXZza2FpYSB3cm90 ZToNCj4gT24gV2VkLCBGZWIgMTQsIDIwMTggYXQgMTA6NDIgQU0sIEJlbmphbWluIENvZGRpbmd0 b24NCj4gPGJjb2RkaW5nQHJlZGhhdC5jb20+IHdyb3RlOg0KPiA+IE9uIDE0IEZlYiAyMDE4LCBh dCAxMDoyOSwgVHJvbmQgTXlrbGVidXN0IHdyb3RlOg0KPiA+ID4gT24gV2VkLCAyMDE4LTAyLTE0 IGF0IDEwOjIxIC0wNTAwLCBKZWZmIExheXRvbiB3cm90ZToNCj4gPiA+ID4gT24gV2VkLCAyMDE4 LTAyLTE0IGF0IDEwOjA1IC0wNTAwLCBCZW5qYW1pbiBDb2RkaW5ndG9uIHdyb3RlOg0KPiA+ID4g PiA+IEhpIE9sZ2EsDQo+ID4gPiA+ID4gDQo+ID4gPiA+ID4gT24gMTMgRmViIDIwMTgsIGF0IDE1 OjA4LCBPbGdhIEtvcm5pZXZza2FpYSB3cm90ZToNCj4gPiA+ID4gPiANCj4gPiA+ID4gPiA+IE9u IE1vbiwgTm92IDE0LCAyMDE2IGF0IDExOjE5IEFNLCBUcm9uZCBNeWtsZWJ1c3QNCj4gPiA+ID4g PiA+IDx0cm9uZC5teWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tPiB3cm90ZToNCj4gPiA+ID4gPiA+ IC4uLg0KPiA+ID4gPiA+IA0KPiA+ID4gPiA+IEFoLCBnb29kIHF1ZXN0aW9uIHRoZXJlLi4gIEV2 ZW4gdGhvdWdoIHRoZSBzdGF0ZWlkIGlzIG5vdA0KPiA+ID4gPiA+IHVzZWZ1bA0KPiA+ID4gPiA+ IGZvcg0KPiA+ID4gPiA+IG9wZXJhdGlvbnMgdGhhdCBmb2xsb3csIEkgdGhpbmsgdGhlIHNlcXVl bmNlaWQgc2hvdWxkIGJlDQo+ID4gPiA+ID4gaW5jcmVtZW50ZWQgYmVjYXVzZQ0KPiA+ID4gPiA+ IHRoZSBDTE9TRSBpcyBhbiBvcGVyYXRpb24gdGhhdCBtb2RpZmllcyB0aGUgc2V0IG9mIGxvY2tz IG9yDQo+ID4gPiA+ID4gc3RhdGU6DQo+ID4gPiA+ID4gDQo+ID4gPiA+IA0KPiA+ID4gPiBJdCBk b2Vzbid0IG1hdHRlci4NCj4gPiANCj4gPiBZZXMsIGdvb2QgY29uZGVuc2VkIHBvaW50LiAgSXQg ZG9lc24ndCBtYXR0ZXIuDQo+ID4gDQo+ID4gPiA+ID4gLi4uDQo+ID4gPiANCj4gPiA+IElzIHRo ZXJlIGEgcHJvcG9zYWwgdG8gY2hhbmdlIHRoZSBjdXJyZW50IGNsaWVudCBiZWhhdmlvdXIgaGVy ZT8NCj4gPiA+IEFzIGZhcg0KPiA+ID4gYXMgSSBjYW4gdGVsbCwgdGhlIGFuc3dlciBpcyAibm8i LiBBbSBJIG1pc3Npbmcgc29tZXRoaW5nPw0KPiA+IA0KPiA+IE5vdCB0aGF0IEkgY2FuIHNlZS4g IEkgdGhpbmsgd2UncmUganVzdCBjb21wYXJpbmcgbGludXggYW5kIG5ldGFwcA0KPiA+IHNlcnZl cg0KPiA+IGJlaGF2aW9ycy4uDQo+ID4gDQo+ID4gQmVuDQo+IA0KPiBJIGp1c3QgZm91bmQgdmVy eSBzdXJwcmlzaW5nIHRoYXQgaW4gbmZzNF9jbG9zZV9kb25lKCkgd2hpY2ggY2FsbHMNCj4gZXZl bnR1YWxseSBjYWxscyBuZnNfY2xlYXJfb3Blbl9zdGF0ZWlkX2xvY2tlZCgpIHdlIGNoYW5nZSB0 aGUgc3RhdGUNCj4gYmFzZWQgb24gdGhlIHN0YXRlaWQgcmVjZWl2ZWQgZnJvbSB0aGUgQ0xPU0Uu DQo+IG5mc19jbGVhcl9vcGVuX3N0YXRlaWRfbG9ja2VkKCkgaXMgb25seSBjYWxsZWQgZnJvbSBu ZnM0X2Nsb3NlX2RvbmUoKQ0KPiBhbmQgbm8gb3RoZXIgZnVuY3Rpb24uDQo+IA0KPiBJIGd1ZXNz IEknbSBub3Qgd29uZGVyaW5nIGlmIHdlIGhhZCB0aGlzIHByb2JsZW0gZGVzY3JpYmVkIGluIHRo aXMNCj4gcGF0Y2ggb2YgdGhlIGRlbGF5ZWQgQ0xPU0UsIGlmIHdlIGRpZG4ndCB1cGRhdGUgdGhl IHN0YXRlIGFmdGVyDQo+IHJlY2VpdmluZyB0aGUgY2xvc2UuLi4gKHNvIHllcyB0aGlzIGlzIGEg d2VhayBwcm9wb3NhbCkuDQo+IA0KDQpuZnM0X2Nsb3NlX2RvbmUoKSBkb2Vzbid0IG9ubHkgZGVh bCB3aXRoIENMT1NFLiBJdCBhbHNvIGhhcyB0byBoYW5kbGUNCk9QRU5fRE9XTkdSQURFLg0KDQot LSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQcmltYXJ5 RGF0YQ0KdHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbQ0K ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2018-02-14 16:59 ` Trond Myklebust @ 2018-02-14 22:17 ` Olga Kornievskaia 0 siblings, 0 replies; 17+ messages in thread From: Olga Kornievskaia @ 2018-02-14 22:17 UTC (permalink / raw) To: Trond Myklebust; +Cc: bcodding redhat, jlayton, linux-nfs On Wed, Feb 14, 2018 at 11:59 AM, Trond Myklebust <trondmy@primarydata.com> wrote: > On Wed, 2018-02-14 at 11:06 -0500, Olga Kornievskaia wrote: >> On Wed, Feb 14, 2018 at 10:42 AM, Benjamin Coddington >> <bcodding@redhat.com> wrote: >> > On 14 Feb 2018, at 10:29, Trond Myklebust wrote: >> > > On Wed, 2018-02-14 at 10:21 -0500, Jeff Layton wrote: >> > > > On Wed, 2018-02-14 at 10:05 -0500, Benjamin Coddington wrote: >> > > > > Hi Olga, >> > > > > >> > > > > On 13 Feb 2018, at 15:08, Olga Kornievskaia wrote: >> > > > > >> > > > > > On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust >> > > > > > <trond.myklebust@primarydata.com> wrote: >> > > > > > ... >> > > > > >> > > > > Ah, good question there.. Even though the stateid is not >> > > > > useful >> > > > > for >> > > > > operations that follow, I think the sequenceid should be >> > > > > incremented because >> > > > > the CLOSE is an operation that modifies the set of locks or >> > > > > state: >> > > > > >> > > > >> > > > It doesn't matter. >> > >> > Yes, good condensed point. It doesn't matter. >> > >> > > > > ... >> > > >> > > Is there a proposal to change the current client behaviour here? >> > > As far >> > > as I can tell, the answer is "no". Am I missing something? >> > >> > Not that I can see. I think we're just comparing linux and netapp >> > server >> > behaviors.. >> > >> > Ben >> >> I just found very surprising that in nfs4_close_done() which calls >> eventually calls nfs_clear_open_stateid_locked() we change the state >> based on the stateid received from the CLOSE. >> nfs_clear_open_stateid_locked() is only called from nfs4_close_done() >> and no other function. >> >> I guess I'm not wondering if we had this problem described in this >> patch of the delayed CLOSE, if we didn't update the state after >> receiving the close... (so yes this is a weak proposal). >> > > nfs4_close_done() doesn't only deal with CLOSE. It also has to handle > OPEN_DOWNGRADE. What about doing an update for OPEN_DOWNGRADE but not for the CLOSE (untested): diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index f8a2b226..5868a6a 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -1472,7 +1472,7 @@ static void nfs_clear_open_stateid_locked(struct nfs4_state *state, if (stateid == NULL) return; /* Handle OPEN+OPEN_DOWNGRADE races */ - if (nfs4_stateid_match_other(stateid, &state->open_stateid) && + if (fmode && nfs4_stateid_match_other(stateid, &state->open_stateid) && !nfs4_stateid_is_newer(stateid, &state->open_stateid)) { nfs_resync_open_stateid_locked(state); goto out; > > -- > Trond Myklebust > Linux NFS client maintainer, PrimaryData > trond.myklebust@primarydata.com ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2018-02-13 20:08 ` Olga Kornievskaia 2018-02-14 15:05 ` Benjamin Coddington @ 2018-02-15 12:19 ` Mkrtchyan, Tigran 2018-02-15 12:23 ` Jeff Layton 1 sibling, 1 reply; 17+ messages in thread From: Mkrtchyan, Tigran @ 2018-02-15 12:19 UTC (permalink / raw) To: Olga Kornievskaia Cc: Trond Myklebust, linux-nfs, Benjamin Coddington, Jeff Layton ----- Original Message ----- > From: "Olga Kornievskaia" <aglo@umich.edu> > To: "Trond Myklebust" <trond.myklebust@primarydata.com> > Cc: "linux-nfs" <linux-nfs@vger.kernel.org>, "Benjamin Coddington" <bcodding@redhat.com>, "Jeff Layton" > <jlayton@redhat.com> > Sent: Tuesday, February 13, 2018 9:08:01 PM > Subject: Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN > On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust > <trond.myklebust@primarydata.com> wrote: >> If the reply to a successful CLOSE call races with an OPEN to the same >> file, we can end up scribbling over the stateid that represents the >> new open state. >> The race looks like: >> >> Client Server >> ====== ====== >> >> CLOSE stateid A on file "foo" >> CLOSE stateid A, return stateid C > > Hi folks, > > I'd like to understand this particular issue. Specifically I don't > understand how can server return stateid C to the close with stateid > A. > > As per RFC 7530 or 5661. It says that state is returned by the close > shouldn't be used. > > Even though CLOSE returns a stateid, this stateid is not useful to > the client and should be treated as deprecated. CLOSE "shuts down" > the state associated with all OPENs for the file by a single > open-owner. As noted above, CLOSE will either release all file > locking state or return an error. Therefore, the stateid returned by > CLOSE is not useful for the operations that follow. > > Is this because the spec says "should" and not a "must"? > > Linux server increments a state's sequenceid on CLOSE. Ontap server > does not. I'm not sure what other servers do. Are all these Our server sends back invalid state id for v4.1 and v4.0. Tigran. > implementations equality correct? > >> OPEN file "foo" >> OPEN "foo", return stateid B >> Receive reply to OPEN >> Reset open state for "foo" >> Associate stateid B to "foo" >> >> Receive CLOSE for A >> Reset open state for "foo" >> Replace stateid B with C >> >> The fix is to examine the argument of the CLOSE, and check for a match >> with the current stateid "other" field. If the two do not match, then >> the above race occurred, and we should just ignore the CLOSE. >> >> Reported-by: Benjamin Coddington <bcodding@redhat.com> >> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> >> --- >> fs/nfs/nfs4_fs.h | 7 +++++++ >> fs/nfs/nfs4proc.c | 12 ++++++------ >> 2 files changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h >> index 9b3a82abab07..1452177c822d 100644 >> --- a/fs/nfs/nfs4_fs.h >> +++ b/fs/nfs/nfs4_fs.h >> @@ -542,6 +542,13 @@ static inline bool nfs4_valid_open_stateid(const struct >> nfs4_state *state) >> return test_bit(NFS_STATE_RECOVERY_FAILED, &state->flags) == 0; >> } >> >> +static inline bool nfs4_state_match_open_stateid_other(const struct nfs4_state >> *state, >> + const nfs4_stateid *stateid) >> +{ >> + return test_bit(NFS_OPEN_STATE, &state->flags) && >> + nfs4_stateid_match_other(&state->open_stateid, stateid); >> +} >> + >> #else >> >> #define nfs4_close_state(a, b) do { } while (0) >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index f550ac69ffa0..b7b0080977c0 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -1458,7 +1458,6 @@ static void nfs_resync_open_stateid_locked(struct >> nfs4_state *state) >> } >> >> static void nfs_clear_open_stateid_locked(struct nfs4_state *state, >> - nfs4_stateid *arg_stateid, >> nfs4_stateid *stateid, fmode_t fmode) >> { >> clear_bit(NFS_O_RDWR_STATE, &state->flags); >> @@ -1476,10 +1475,9 @@ static void nfs_clear_open_stateid_locked(struct >> nfs4_state *state, >> } >> if (stateid == NULL) >> return; >> - /* Handle races with OPEN */ >> - if (!nfs4_stateid_match_other(arg_stateid, &state->open_stateid) || >> - (nfs4_stateid_match_other(stateid, &state->open_stateid) && >> - !nfs4_stateid_is_newer(stateid, &state->open_stateid))) { >> + /* Handle OPEN+OPEN_DOWNGRADE races */ >> + if (nfs4_stateid_match_other(stateid, &state->open_stateid) && >> + !nfs4_stateid_is_newer(stateid, &state->open_stateid)) { >> nfs_resync_open_stateid_locked(state); >> return; >> } >> @@ -1493,7 +1491,9 @@ static void nfs_clear_open_stateid(struct nfs4_state >> *state, >> nfs4_stateid *stateid, fmode_t fmode) >> { >> write_seqlock(&state->seqlock); >> - nfs_clear_open_stateid_locked(state, arg_stateid, stateid, fmode); >> + /* Ignore, if the CLOSE argment doesn't match the current stateid */ >> + if (nfs4_state_match_open_stateid_other(state, arg_stateid)) >> + nfs_clear_open_stateid_locked(state, stateid, fmode); >> write_sequnlock(&state->seqlock); >> if (test_bit(NFS_STATE_RECLAIM_NOGRACE, &state->flags)) >> nfs4_schedule_state_manager(state->owner->so_server->nfs_client); >> -- >> 2.7.4 >> >> -- >> 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 > -- > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2018-02-15 12:19 ` Mkrtchyan, Tigran @ 2018-02-15 12:23 ` Jeff Layton 2018-02-15 15:29 ` Bruce Fields 0 siblings, 1 reply; 17+ messages in thread From: Jeff Layton @ 2018-02-15 12:23 UTC (permalink / raw) To: Mkrtchyan, Tigran, Olga Kornievskaia Cc: Trond Myklebust, linux-nfs, Benjamin Coddington, Bruce Fields On Thu, 2018-02-15 at 13:19 +0100, Mkrtchyan, Tigran wrote: > > ----- Original Message ----- > > From: "Olga Kornievskaia" <aglo@umich.edu> > > To: "Trond Myklebust" <trond.myklebust@primarydata.com> > > Cc: "linux-nfs" <linux-nfs@vger.kernel.org>, "Benjamin Coddington" <bcodding@redhat.com>, "Jeff Layton" > > <jlayton@redhat.com> > > Sent: Tuesday, February 13, 2018 9:08:01 PM > > Subject: Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN > > On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust > > <trond.myklebust@primarydata.com> wrote: > > > If the reply to a successful CLOSE call races with an OPEN to the same > > > file, we can end up scribbling over the stateid that represents the > > > new open state. > > > The race looks like: > > > > > > Client Server > > > ====== ====== > > > > > > CLOSE stateid A on file "foo" > > > CLOSE stateid A, return stateid C > > > > Hi folks, > > > > I'd like to understand this particular issue. Specifically I don't > > understand how can server return stateid C to the close with stateid > > A. > > > > As per RFC 7530 or 5661. It says that state is returned by the close > > shouldn't be used. > > > > Even though CLOSE returns a stateid, this stateid is not useful to > > the client and should be treated as deprecated. CLOSE "shuts down" > > the state associated with all OPENs for the file by a single > > open-owner. As noted above, CLOSE will either release all file > > locking state or return an error. Therefore, the stateid returned by > > CLOSE is not useful for the operations that follow. > > > > Is this because the spec says "should" and not a "must"? > > > > Linux server increments a state's sequenceid on CLOSE. Ontap server > > does not. I'm not sure what other servers do. Are all these > > > Our server sends back invalid state id for v4.1 and v4.0. > > Tigran. > That's probably the best thing to do, and we should probably do the same for v4.0 in knfsd. Doing that might cause problems for clients that are not ignoring that field like they should, but they're buggy already. > > implementations equality correct? > > > > > OPEN file "foo" > > > OPEN "foo", return stateid B > > > Receive reply to OPEN > > > Reset open state for "foo" > > > Associate stateid B to "foo" > > > > > > Receive CLOSE for A > > > Reset open state for "foo" > > > Replace stateid B with C > > > > > > The fix is to examine the argument of the CLOSE, and check for a match > > > with the current stateid "other" field. If the two do not match, then > > > the above race occurred, and we should just ignore the CLOSE. > > > > > > Reported-by: Benjamin Coddington <bcodding@redhat.com> > > > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> > > > --- > > > fs/nfs/nfs4_fs.h | 7 +++++++ > > > fs/nfs/nfs4proc.c | 12 ++++++------ > > > 2 files changed, 13 insertions(+), 6 deletions(-) > > > > > > diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h > > > index 9b3a82abab07..1452177c822d 100644 > > > --- a/fs/nfs/nfs4_fs.h > > > +++ b/fs/nfs/nfs4_fs.h > > > @@ -542,6 +542,13 @@ static inline bool nfs4_valid_open_stateid(const struct > > > nfs4_state *state) > > > return test_bit(NFS_STATE_RECOVERY_FAILED, &state->flags) == 0; > > > } > > > > > > +static inline bool nfs4_state_match_open_stateid_other(const struct nfs4_state > > > *state, > > > + const nfs4_stateid *stateid) > > > +{ > > > + return test_bit(NFS_OPEN_STATE, &state->flags) && > > > + nfs4_stateid_match_other(&state->open_stateid, stateid); > > > +} > > > + > > > #else > > > > > > #define nfs4_close_state(a, b) do { } while (0) > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > index f550ac69ffa0..b7b0080977c0 100644 > > > --- a/fs/nfs/nfs4proc.c > > > +++ b/fs/nfs/nfs4proc.c > > > @@ -1458,7 +1458,6 @@ static void nfs_resync_open_stateid_locked(struct > > > nfs4_state *state) > > > } > > > > > > static void nfs_clear_open_stateid_locked(struct nfs4_state *state, > > > - nfs4_stateid *arg_stateid, > > > nfs4_stateid *stateid, fmode_t fmode) > > > { > > > clear_bit(NFS_O_RDWR_STATE, &state->flags); > > > @@ -1476,10 +1475,9 @@ static void nfs_clear_open_stateid_locked(struct > > > nfs4_state *state, > > > } > > > if (stateid == NULL) > > > return; > > > - /* Handle races with OPEN */ > > > - if (!nfs4_stateid_match_other(arg_stateid, &state->open_stateid) || > > > - (nfs4_stateid_match_other(stateid, &state->open_stateid) && > > > - !nfs4_stateid_is_newer(stateid, &state->open_stateid))) { > > > + /* Handle OPEN+OPEN_DOWNGRADE races */ > > > + if (nfs4_stateid_match_other(stateid, &state->open_stateid) && > > > + !nfs4_stateid_is_newer(stateid, &state->open_stateid)) { > > > nfs_resync_open_stateid_locked(state); > > > return; > > > } > > > @@ -1493,7 +1491,9 @@ static void nfs_clear_open_stateid(struct nfs4_state > > > *state, > > > nfs4_stateid *stateid, fmode_t fmode) > > > { > > > write_seqlock(&state->seqlock); > > > - nfs_clear_open_stateid_locked(state, arg_stateid, stateid, fmode); > > > + /* Ignore, if the CLOSE argment doesn't match the current stateid */ > > > + if (nfs4_state_match_open_stateid_other(state, arg_stateid)) > > > + nfs_clear_open_stateid_locked(state, stateid, fmode); > > > write_sequnlock(&state->seqlock); > > > if (test_bit(NFS_STATE_RECLAIM_NOGRACE, &state->flags)) > > > nfs4_schedule_state_manager(state->owner->so_server->nfs_client); > > > -- > > > 2.7.4 > > > > > > -- > > > 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 > > > > -- > > 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 -- Jeff Layton <jlayton@redhat.com> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2018-02-15 12:23 ` Jeff Layton @ 2018-02-15 15:29 ` Bruce Fields 2018-02-15 15:37 ` Trond Myklebust 0 siblings, 1 reply; 17+ messages in thread From: Bruce Fields @ 2018-02-15 15:29 UTC (permalink / raw) To: Jeff Layton Cc: Mkrtchyan, Tigran, Olga Kornievskaia, Trond Myklebust, linux-nfs, Benjamin Coddington On Thu, Feb 15, 2018 at 07:23:00AM -0500, Jeff Layton wrote: > On Thu, 2018-02-15 at 13:19 +0100, Mkrtchyan, Tigran wrote: > > > > ----- Original Message ----- > > > From: "Olga Kornievskaia" <aglo@umich.edu> > > > To: "Trond Myklebust" <trond.myklebust@primarydata.com> > > > Cc: "linux-nfs" <linux-nfs@vger.kernel.org>, "Benjamin Coddington" <bcodding@redhat.com>, "Jeff Layton" > > > <jlayton@redhat.com> > > > Sent: Tuesday, February 13, 2018 9:08:01 PM > > > Subject: Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN > > > On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust > > > <trond.myklebust@primarydata.com> wrote: > > > > If the reply to a successful CLOSE call races with an OPEN to the same > > > > file, we can end up scribbling over the stateid that represents the > > > > new open state. > > > > The race looks like: > > > > > > > > Client Server > > > > ====== ====== > > > > > > > > CLOSE stateid A on file "foo" > > > > CLOSE stateid A, return stateid C > > > > > > Hi folks, > > > > > > I'd like to understand this particular issue. Specifically I don't > > > understand how can server return stateid C to the close with stateid > > > A. > > > > > > As per RFC 7530 or 5661. It says that state is returned by the close > > > shouldn't be used. > > > > > > Even though CLOSE returns a stateid, this stateid is not useful to > > > the client and should be treated as deprecated. CLOSE "shuts down" > > > the state associated with all OPENs for the file by a single > > > open-owner. As noted above, CLOSE will either release all file > > > locking state or return an error. Therefore, the stateid returned by > > > CLOSE is not useful for the operations that follow. > > > > > > Is this because the spec says "should" and not a "must"? > > > > > > Linux server increments a state's sequenceid on CLOSE. Ontap server > > > does not. I'm not sure what other servers do. Are all these > > > > > > Our server sends back invalid state id for v4.1 and v4.0. > > > > Tigran. > > > > That's probably the best thing to do, and we should probably do the same > for v4.0 in knfsd. Doing that might cause problems for clients that are > not ignoring that field like they should, but they're buggy already. Not only buggy in theory, but actually failing in practice, it sounds like. So, a pretty safe change? Returning an all-zeroes stateid would be simple and make the situation really obvious. --b. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN 2018-02-15 15:29 ` Bruce Fields @ 2018-02-15 15:37 ` Trond Myklebust 0 siblings, 0 replies; 17+ messages in thread From: Trond Myklebust @ 2018-02-15 15:37 UTC (permalink / raw) To: bfields, jlayton; +Cc: bcodding redhat, tigran.mkrtchyan, linux-nfs, aglo T24gVGh1LCAyMDE4LTAyLTE1IGF0IDEwOjI5IC0wNTAwLCBCcnVjZSBGaWVsZHMgd3JvdGU6DQo+ IE9uIFRodSwgRmViIDE1LCAyMDE4IGF0IDA3OjIzOjAwQU0gLTA1MDAsIEplZmYgTGF5dG9uIHdy b3RlOg0KPiA+IE9uIFRodSwgMjAxOC0wMi0xNSBhdCAxMzoxOSArMDEwMCwgTWtydGNoeWFuLCBU aWdyYW4gd3JvdGU6DQo+ID4gPiANCj4gPiA+IC0tLS0tIE9yaWdpbmFsIE1lc3NhZ2UgLS0tLS0N Cj4gPiA+ID4gRnJvbTogIk9sZ2EgS29ybmlldnNrYWlhIiA8YWdsb0B1bWljaC5lZHU+DQo+ID4g PiA+IFRvOiAiVHJvbmQgTXlrbGVidXN0IiA8dHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNv bT4NCj4gPiA+ID4gQ2M6ICJsaW51eC1uZnMiIDxsaW51eC1uZnNAdmdlci5rZXJuZWwub3JnPiwg IkJlbmphbWluDQo+ID4gPiA+IENvZGRpbmd0b24iIDxiY29kZGluZ0ByZWRoYXQuY29tPiwgIkpl ZmYgTGF5dG9uIg0KPiA+ID4gPiA8amxheXRvbkByZWRoYXQuY29tPg0KPiA+ID4gPiBTZW50OiBU dWVzZGF5LCBGZWJydWFyeSAxMywgMjAxOCA5OjA4OjAxIFBNDQo+ID4gPiA+IFN1YmplY3Q6IFJl OiBbUEFUQ0ggMS8yXSBORlN2NDogRml4IENMT1NFIHJhY2VzIHdpdGggT1BFTg0KPiA+ID4gPiBP biBNb24sIE5vdiAxNCwgMjAxNiBhdCAxMToxOSBBTSwgVHJvbmQgTXlrbGVidXN0DQo+ID4gPiA+ IDx0cm9uZC5teWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tPiB3cm90ZToNCj4gPiA+ID4gPiBJZiB0 aGUgcmVwbHkgdG8gYSBzdWNjZXNzZnVsIENMT1NFIGNhbGwgcmFjZXMgd2l0aCBhbiBPUEVOIHRv DQo+ID4gPiA+ID4gdGhlIHNhbWUNCj4gPiA+ID4gPiBmaWxlLCB3ZSBjYW4gZW5kIHVwIHNjcmli Ymxpbmcgb3ZlciB0aGUgc3RhdGVpZCB0aGF0DQo+ID4gPiA+ID4gcmVwcmVzZW50cyB0aGUNCj4g PiA+ID4gPiBuZXcgb3BlbiBzdGF0ZS4NCj4gPiA+ID4gPiBUaGUgcmFjZSBsb29rcyBsaWtlOg0K PiA+ID4gPiA+IA0KPiA+ID4gPiA+ICAgQ2xpZW50ICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICBTZXJ2ZXINCj4gPiA+ID4gPiAgID09PT09PSAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgPT09PT09DQo+ID4gPiA+ID4gDQo+ID4gPiA+ID4gICBDTE9TRSBzdGF0ZWlkIEEgb24g ZmlsZSAiZm9vIg0KPiA+ID4gPiA+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICBDTE9TRSBzdGF0ZWlkIEEsDQo+ID4gPiA+ID4gcmV0dXJuIHN0YXRlaWQgQw0KPiA+ID4g PiANCj4gPiA+ID4gSGkgZm9sa3MsDQo+ID4gPiA+IA0KPiA+ID4gPiBJJ2QgbGlrZSB0byB1bmRl cnN0YW5kIHRoaXMgcGFydGljdWxhciBpc3N1ZS4gU3BlY2lmaWNhbGx5IEkNCj4gPiA+ID4gZG9u J3QNCj4gPiA+ID4gdW5kZXJzdGFuZCBob3cgY2FuIHNlcnZlciByZXR1cm4gc3RhdGVpZCBDIHRv IHRoZSBjbG9zZSB3aXRoDQo+ID4gPiA+IHN0YXRlaWQNCj4gPiA+ID4gQS4NCj4gPiA+ID4gDQo+ ID4gPiA+IEFzIHBlciBSRkMgNzUzMCBvciA1NjYxLiBJdCBzYXlzIHRoYXQgc3RhdGUgaXMgcmV0 dXJuZWQgYnkgdGhlDQo+ID4gPiA+IGNsb3NlDQo+ID4gPiA+IHNob3VsZG4ndCBiZSB1c2VkLg0K PiA+ID4gPiANCj4gPiA+ID4gRXZlbiB0aG91Z2ggQ0xPU0UgcmV0dXJucyBhIHN0YXRlaWQsIHRo aXMgc3RhdGVpZCBpcyBub3QgdXNlZnVsDQo+ID4gPiA+IHRvDQo+ID4gPiA+ICAgdGhlIGNsaWVu dCBhbmQgc2hvdWxkIGJlIHRyZWF0ZWQgYXMgZGVwcmVjYXRlZC4gIENMT1NFICJzaHV0cw0KPiA+ ID4gPiBkb3duIg0KPiA+ID4gPiAgIHRoZSBzdGF0ZSBhc3NvY2lhdGVkIHdpdGggYWxsIE9QRU5z IGZvciB0aGUgZmlsZSBieSBhIHNpbmdsZQ0KPiA+ID4gPiAgIG9wZW4tb3duZXIuICBBcyBub3Rl ZCBhYm92ZSwgQ0xPU0Ugd2lsbCBlaXRoZXIgcmVsZWFzZSBhbGwNCj4gPiA+ID4gZmlsZQ0KPiA+ ID4gPiAgIGxvY2tpbmcgc3RhdGUgb3IgcmV0dXJuIGFuIGVycm9yLiAgVGhlcmVmb3JlLCB0aGUg c3RhdGVpZA0KPiA+ID4gPiByZXR1cm5lZCBieQ0KPiA+ID4gPiAgIENMT1NFIGlzIG5vdCB1c2Vm dWwgZm9yIHRoZSBvcGVyYXRpb25zIHRoYXQgZm9sbG93Lg0KPiA+ID4gPiANCj4gPiA+ID4gSXMg dGhpcyBiZWNhdXNlIHRoZSBzcGVjIHNheXMgInNob3VsZCIgYW5kIG5vdCBhICJtdXN0Ij8NCj4g PiA+ID4gDQo+ID4gPiA+IExpbnV4IHNlcnZlciBpbmNyZW1lbnRzIGEgc3RhdGUncyBzZXF1ZW5j ZWlkIG9uIENMT1NFLiBPbnRhcA0KPiA+ID4gPiBzZXJ2ZXINCj4gPiA+ID4gZG9lcyBub3QuIEkn bSBub3Qgc3VyZSB3aGF0IG90aGVyIHNlcnZlcnMgZG8uIEFyZSBhbGwgdGhlc2UNCj4gPiA+IA0K PiA+ID4gDQo+ID4gPiBPdXIgc2VydmVyIHNlbmRzIGJhY2sgaW52YWxpZCBzdGF0ZSBpZCBmb3Ig djQuMSBhbmQgdjQuMC4NCj4gPiA+IA0KPiA+ID4gVGlncmFuLg0KPiA+ID4gDQo+ID4gDQo+ID4g VGhhdCdzIHByb2JhYmx5IHRoZSBiZXN0IHRoaW5nIHRvIGRvLCBhbmQgd2Ugc2hvdWxkIHByb2Jh Ymx5IGRvIHRoZQ0KPiA+IHNhbWUNCj4gPiAgZm9yIHY0LjAgaW4ga25mc2QuIERvaW5nIHRoYXQg bWlnaHQgY2F1c2UgcHJvYmxlbXMgZm9yIGNsaWVudHMNCj4gPiB0aGF0IGFyZQ0KPiA+IG5vdCBp Z25vcmluZyB0aGF0IGZpZWxkIGxpa2UgdGhleSBzaG91bGQsIGJ1dCB0aGV5J3JlIGJ1Z2d5DQo+ ID4gYWxyZWFkeS4NCj4gDQo+IE5vdCBvbmx5IGJ1Z2d5IGluIHRoZW9yeSwgYnV0IGFjdHVhbGx5 IGZhaWxpbmcgaW4gcHJhY3RpY2UsIGl0IHNvdW5kcw0KPiBsaWtlLiAgU28sIGEgcHJldHR5IHNh ZmUgY2hhbmdlPw0KPiANCj4gUmV0dXJuaW5nIGFuIGFsbC16ZXJvZXMgc3RhdGVpZCB3b3VsZCBi ZSBzaW1wbGUgYW5kIG1ha2UgdGhlDQo+IHNpdHVhdGlvbg0KPiByZWFsbHkgb2J2aW91cy4NCj4g DQoNCklmIHlvdSdyZSBnb2luZyB0byBkbyB0aGF0LCB0aGVuIHdoeSBub3QganVzdCBleHBhbmQg ZmI1MDBhN2NmZWU3IHRvDQphcHBseSB0byBORlN2NC4wIHRvbz8gSWYgeW91J3JlIGdvaW5nIHRv IHZpb2xhdGUgdGhlIHY0LjAgc3BlYywgdGhlbg0KeW91IG1pZ2h0IGFzIHdlbGwgZG8gaXQgaW4g YSB3YXkgdGhhdCBpcyBzYW5jdGlvbmVkIGJ5IHY0LjEsIHNvIHRoYXQNCnBlb3BsZSBjYW4gdXNl IHRoZSBzYW1lIHdpcmVzaGFyayBmaWx0ZXJzIHdoZW4gZGVidWdnaW5nLg0KDQotLSANClRyb25k IE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQcmltYXJ5RGF0YQ0KdHJv bmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbQ0K ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-02-15 15:37 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-14 16:19 [PATCH 0/2] Fix CLOSE races Trond Myklebust 2016-11-14 16:19 ` [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN Trond Myklebust 2016-11-14 16:19 ` [PATCH 2/2] NFSv4: Don't call close if the open stateid has already been cleared Trond Myklebust 2016-11-14 16:40 ` [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN Jeff Layton 2016-11-14 16:48 ` Trond Myklebust 2018-02-13 20:08 ` Olga Kornievskaia 2018-02-14 15:05 ` Benjamin Coddington 2018-02-14 15:21 ` Jeff Layton 2018-02-14 15:29 ` Trond Myklebust 2018-02-14 15:42 ` Benjamin Coddington 2018-02-14 16:06 ` Olga Kornievskaia 2018-02-14 16:59 ` Trond Myklebust 2018-02-14 22:17 ` Olga Kornievskaia 2018-02-15 12:19 ` Mkrtchyan, Tigran 2018-02-15 12:23 ` Jeff Layton 2018-02-15 15:29 ` Bruce Fields 2018-02-15 15:37 ` Trond Myklebust
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.