| Message ID | 1446684228-37765-1-git-send-email-aweits@rit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andrew, On Wed, Nov 4, 2015 at 7:43 PM, Andrew Elble <aweits@rit.edu> wrote: > In the NFS4.1 case, revoked delegations need to be processed via > FREE_STATEID, not simply forgotten. > > Fixes: 869f9dfa4d6d ("NFSv4: Fix races between nfs_remove_bad_delegation() and delegation return") > Signed-off-by: Andrew Elble <aweits@rit.edu> > --- > fs/nfs/delegation.c | 6 ++++++ > fs/nfs/nfs4_fs.h | 2 ++ > fs/nfs/nfs4proc.c | 18 ++++++++++-------- > 3 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c > index 5166adcfc0fb..dc971adb7af9 100644 > --- a/fs/nfs/delegation.c > +++ b/fs/nfs/delegation.c > @@ -205,6 +205,12 @@ static int nfs_do_return_delegation(struct inode *inode, struct nfs_delegation * > delegation->cred, > &delegation->stateid, > issync); > +#if defined(CONFIG_NFS_V4_1) > + else if (NFS_SERVER(inode)->nfs_client->cl_minorversion) > + res = nfs41_free_stateid(NFS_SERVER(inode), > + &delegation->stateid, > + delegation->cred, issync); > +#endif /* CONFIG_NFS_V4_1 */ > nfs_free_delegation(delegation); > return res; If the server is revoking a delegation, then presumably it will also set one or more of the SEQ4_STATUS_EXPIRED_SOME_STATE_REVOKED, SEQ4_STATUS_ADMIN_STATE_REVOKED, SEQ4_STATUS_RECALLABLE_STATE_REVOKED, which should start up a state manager thread to do the test_stateid/free_stateid dance. So instead of adding the free stateid call above, why can't we just punt the freeing of the delegation to the state manager? Cheers Trond -- 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
> If the server is revoking a delegation, then presumably it will also > set one or more of the SEQ4_STATUS_EXPIRED_SOME_STATE_REVOKED, > SEQ4_STATUS_ADMIN_STATE_REVOKED, SEQ4_STATUS_RECALLABLE_STATE_REVOKED, > which should start up a state manager thread to do the > test_stateid/free_stateid dance. > > So instead of adding the free stateid call above, why can't we just > punt the freeing of the delegation to the state manager? I inferred (perhaps incorrectly) that nfs_do_return_delegation() was a place delegations went to and didn't come back from. I managed to convince myself that since all callers of nfs_do_return_delegation() detach the delegation, the state manager wouldn't be able to perform that function without reattaching the delegation - and that doesn't look quite so safe (and/or possible) in all of those call paths? Thanks, Andy
On Thu, Nov 5, 2015 at 10:19 AM, Andrew W Elble <aweits@rit.edu> wrote: > >> If the server is revoking a delegation, then presumably it will also >> set one or more of the SEQ4_STATUS_EXPIRED_SOME_STATE_REVOKED, >> SEQ4_STATUS_ADMIN_STATE_REVOKED, SEQ4_STATUS_RECALLABLE_STATE_REVOKED, >> which should start up a state manager thread to do the >> test_stateid/free_stateid dance. >> >> So instead of adding the free stateid call above, why can't we just >> punt the freeing of the delegation to the state manager? > > I inferred (perhaps incorrectly) that nfs_do_return_delegation() was a > place delegations went to and didn't come back from. > > I managed to convince myself that since all callers of > nfs_do_return_delegation() detach the delegation, the state manager > wouldn't be able to perform that function without reattaching the > delegation - and that doesn't look quite so safe (and/or possible) > in all of those call paths? Looking again more closely, can we ever have NFS_DELEGATION_REVOKED set without first having called nfs41_check_delegation_stateid(), and therefore presumably having freed the delegation stateid? The only place I can see that might be broken is ff_layout_async_handle_error_v4(), which calls nfs_remove_bad_delegation() directly instead of using the state manager. There is also a question of what to do if the call to DELEGRETURN itself returns NFS4ERR_DELEG_REVOKED, but why would a server do that? -- 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
On Thu, Nov 05, 2015 at 09:27:06AM -0500, Trond Myklebust wrote: > Hi Andrew, > > On Wed, Nov 4, 2015 at 7:43 PM, Andrew Elble <aweits@rit.edu> wrote: > > In the NFS4.1 case, revoked delegations need to be processed via > > FREE_STATEID, not simply forgotten. > > > > Fixes: 869f9dfa4d6d ("NFSv4: Fix races between nfs_remove_bad_delegation() and delegation return") > > Signed-off-by: Andrew Elble <aweits@rit.edu> > > --- > > fs/nfs/delegation.c | 6 ++++++ > > fs/nfs/nfs4_fs.h | 2 ++ > > fs/nfs/nfs4proc.c | 18 ++++++++++-------- > > 3 files changed, 18 insertions(+), 8 deletions(-) > > > > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c > > index 5166adcfc0fb..dc971adb7af9 100644 > > --- a/fs/nfs/delegation.c > > +++ b/fs/nfs/delegation.c > > @@ -205,6 +205,12 @@ static int nfs_do_return_delegation(struct inode *inode, struct nfs_delegation * > > delegation->cred, > > &delegation->stateid, > > issync); > > +#if defined(CONFIG_NFS_V4_1) > > + else if (NFS_SERVER(inode)->nfs_client->cl_minorversion) > > + res = nfs41_free_stateid(NFS_SERVER(inode), > > + &delegation->stateid, > > + delegation->cred, issync); > > +#endif /* CONFIG_NFS_V4_1 */ > > nfs_free_delegation(delegation); > > return res; > > If the server is revoking a delegation, then presumably it will also > set one or more of the SEQ4_STATUS_EXPIRED_SOME_STATE_REVOKED, > SEQ4_STATUS_ADMIN_STATE_REVOKED, SEQ4_STATUS_RECALLABLE_STATE_REVOKED, > which should start up a state manager thread to do the > test_stateid/free_stateid dance. The expiration could occur during the narrow window between the time the server processes the SEQUENCE and the stateid-containing operation. Though in that case there should be another SEQUENCE reply with the flag set soon enough. --b. > > So instead of adding the free stateid call above, why can't we just > punt the freeing of the delegation to the state manager? > > Cheers > Trond > -- > 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
On Mon, Nov 9, 2015 at 1:03 PM, J. Bruce Fields <bfields@fieldses.org> wrote: > On Thu, Nov 05, 2015 at 09:27:06AM -0500, Trond Myklebust wrote: >> Hi Andrew, >> >> On Wed, Nov 4, 2015 at 7:43 PM, Andrew Elble <aweits@rit.edu> wrote: >> > In the NFS4.1 case, revoked delegations need to be processed via >> > FREE_STATEID, not simply forgotten. >> > >> > Fixes: 869f9dfa4d6d ("NFSv4: Fix races between nfs_remove_bad_delegation() and delegation return") >> > Signed-off-by: Andrew Elble <aweits@rit.edu> >> > --- >> > fs/nfs/delegation.c | 6 ++++++ >> > fs/nfs/nfs4_fs.h | 2 ++ >> > fs/nfs/nfs4proc.c | 18 ++++++++++-------- >> > 3 files changed, 18 insertions(+), 8 deletions(-) >> > >> > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c >> > index 5166adcfc0fb..dc971adb7af9 100644 >> > --- a/fs/nfs/delegation.c >> > +++ b/fs/nfs/delegation.c >> > @@ -205,6 +205,12 @@ static int nfs_do_return_delegation(struct inode *inode, struct nfs_delegation * >> > delegation->cred, >> > &delegation->stateid, >> > issync); >> > +#if defined(CONFIG_NFS_V4_1) >> > + else if (NFS_SERVER(inode)->nfs_client->cl_minorversion) >> > + res = nfs41_free_stateid(NFS_SERVER(inode), >> > + &delegation->stateid, >> > + delegation->cred, issync); >> > +#endif /* CONFIG_NFS_V4_1 */ >> > nfs_free_delegation(delegation); >> > return res; >> >> If the server is revoking a delegation, then presumably it will also >> set one or more of the SEQ4_STATUS_EXPIRED_SOME_STATE_REVOKED, >> SEQ4_STATUS_ADMIN_STATE_REVOKED, SEQ4_STATUS_RECALLABLE_STATE_REVOKED, >> which should start up a state manager thread to do the >> test_stateid/free_stateid dance. > > The expiration could occur during the narrow window between the time the > server processes the SEQUENCE and the stateid-containing operation. > Though in that case there should be another SEQUENCE reply with the flag > set soon enough. We shouldn't need to care about the window between SEQUENCE and DELEGRETURN because there should be no more cached locks to recover. -- 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
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c index 5166adcfc0fb..dc971adb7af9 100644 --- a/fs/nfs/delegation.c +++ b/fs/nfs/delegation.c @@ -205,6 +205,12 @@ static int nfs_do_return_delegation(struct inode *inode, struct nfs_delegation * delegation->cred, &delegation->stateid, issync); +#if defined(CONFIG_NFS_V4_1) + else if (NFS_SERVER(inode)->nfs_client->cl_minorversion) + res = nfs41_free_stateid(NFS_SERVER(inode), + &delegation->stateid, + delegation->cred, issync); +#endif /* CONFIG_NFS_V4_1 */ nfs_free_delegation(delegation); return res; } diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h index 4afdee420d25..862fe4e44634 100644 --- a/fs/nfs/nfs4_fs.h +++ b/fs/nfs/nfs4_fs.h @@ -261,6 +261,8 @@ extern int nfs4_set_rw_stateid(nfs4_stateid *stateid, fmode_t fmode); #if defined(CONFIG_NFS_V4_1) +int nfs41_free_stateid(struct nfs_server *, nfs4_stateid *, + struct rpc_cred *, int issync); static inline struct nfs4_session *nfs4_get_session(const struct nfs_server *server) { return server->nfs_client->cl_session; diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 7ed8f2cd97f8..77811c727703 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -88,8 +88,6 @@ static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred, #ifdef CONFIG_NFS_V4_1 static int nfs41_test_stateid(struct nfs_server *, nfs4_stateid *, struct rpc_cred *); -static int nfs41_free_stateid(struct nfs_server *, nfs4_stateid *, - struct rpc_cred *); #endif #ifdef CONFIG_NFS_V4_SECURITY_LABEL @@ -2347,7 +2345,7 @@ static void nfs41_check_delegation_stateid(struct nfs4_state *state) /* Free the stateid unless the server explicitly * informs us the stateid is unrecognized. */ if (status != -NFS4ERR_BAD_STATEID) - nfs41_free_stateid(server, &stateid, cred); + nfs41_free_stateid(server, &stateid, cred, 1); nfs_finish_clear_delegation_stateid(state); } @@ -2381,7 +2379,7 @@ static int nfs41_check_open_stateid(struct nfs4_state *state) /* Free the stateid unless the server explicitly * informs us the stateid is unrecognized. */ if (status != -NFS4ERR_BAD_STATEID) - nfs41_free_stateid(server, stateid, cred); + nfs41_free_stateid(server, stateid, cred, 1); clear_bit(NFS_O_RDONLY_STATE, &state->flags); clear_bit(NFS_O_WRONLY_STATE, &state->flags); @@ -6033,7 +6031,7 @@ static int nfs41_check_expired_locks(struct nfs4_state *state) if (status != -NFS4ERR_BAD_STATEID) nfs41_free_stateid(server, &lsp->ls_stateid, - cred); + cred, 1); clear_bit(NFS_LOCK_INITIALIZED, &lsp->ls_flags); ret = status; } @@ -8553,19 +8551,23 @@ static struct rpc_task *_nfs41_free_stateid(struct nfs_server *server, * Returns NFS_OK if the server freed "stateid". Otherwise a * negative NFS4ERR value is returned. */ -static int nfs41_free_stateid(struct nfs_server *server, +int nfs41_free_stateid(struct nfs_server *server, nfs4_stateid *stateid, - struct rpc_cred *cred) + struct rpc_cred *cred, + int issync) { struct rpc_task *task; - int ret; + int ret = 0; task = _nfs41_free_stateid(server, stateid, cred, true); if (IS_ERR(task)) return PTR_ERR(task); + if (!issync) + goto out; ret = rpc_wait_for_completion_task(task); if (!ret) ret = task->tk_status; +out: rpc_put_task(task); return ret; }
In the NFS4.1 case, revoked delegations need to be processed via FREE_STATEID, not simply forgotten. Fixes: 869f9dfa4d6d ("NFSv4: Fix races between nfs_remove_bad_delegation() and delegation return") Signed-off-by: Andrew Elble <aweits@rit.edu> --- fs/nfs/delegation.c | 6 ++++++ fs/nfs/nfs4_fs.h | 2 ++ fs/nfs/nfs4proc.c | 18 ++++++++++-------- 3 files changed, 18 insertions(+), 8 deletions(-)