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 <[email protected]>
---
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;
}
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;
}
--
2.4.6
Hi Andrew,
On Wed, Nov 4, 2015 at 7:43 PM, Andrew Elble <[email protected]> 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 <[email protected]>
> ---
> 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
> 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
--
Andrew W. Elble
[email protected]
Infrastructure Engineer, Communications Technical Lead
Rochester Institute of Technology
PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912
On Thu, Nov 5, 2015 at 10:19 AM, Andrew W Elble <[email protected]> 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?
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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 9, 2015 at 1:03 PM, J. Bruce Fields <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> > ---
>> > 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.