Return-Path: MIME-Version: 1.0 In-Reply-To: <62BA7359-9421-41F3-9116-8AA25AD1E77F@redhat.com> References: <1465326888-11185-1-git-send-email-smayhew@redhat.com> <62BA7359-9421-41F3-9116-8AA25AD1E77F@redhat.com> From: Andy Adamson Date: Thu, 30 Jun 2016 16:49:13 -0400 Message-ID: Subject: Re: [PATCH RESEND] sunrpc: move NO_CRKEY_TIMEOUT to the auth->au_flags To: Benjamin Coddington Cc: Scott Mayhew , Trond Myklebust , anna.schumaker@netapp.com, Bruce Fields , NFS list Content-Type: text/plain; charset=UTF-8 List-ID: I reviewed the patch, and tested the upstream 4.7.0-rc3 kernel without then with the patch. I was able to recreate the problem without the patch, and the patch fixed the problem. ACK -->Andy On Thu, Jun 16, 2016 at 12:33 PM, Benjamin Coddington wrote: > Hey Scott, after reviewing this work, I recreated the problem at BAT today. > This patch looks good, and fixes the problem for me. Thanks! > > Reviewed-by: Benjamin Coddington > > Ben > > > On 7 Jun 2016, at 15:14, Scott Mayhew wrote: > >> A generic_cred can be used to look up a unx_cred or a gss_cred, so it's >> not really safe to use the the generic_cred->acred->ac_flags to store >> the NO_CRKEY_TIMEOUT flag. A lookup for a unx_cred triggered while the >> KEY_EXPIRE_SOON flag is already set will cause both NO_CRKEY_TIMEOUT and >> KEY_EXPIRE_SOON to be set in the ac_flags, leaving the user associated >> with the auth_cred to be in a state where they're perpetually doing 4K >> NFS_FILE_SYNC writes. >> >> This can be reproduced as follows: >> >> 1. Mount two NFS filesystems, one with sec=krb5 and one with sec=sys. >> They do not need to be the same export, nor do they even need to be from >> the same NFS server. Also, v3 is fine. >> $ sudo mount -o v3,sec=krb5 server1:/export /mnt/krb5 >> $ sudo mount -o v3,sec=sys server2:/export /mnt/sys >> >> 2. As the normal user, before accessing the kerberized mount, kinit with >> a short lifetime (but not so short that renewing the ticket would leave >> you within the 4-minute window again by the time the original ticket >> expires), e.g. >> $ kinit -l 10m -r 60m >> >> 3. Do some I/O to the kerberized mount and verify that the writes are >> wsize, UNSTABLE: >> $ dd if=/dev/zero of=/mnt/krb5/file bs=1M count=1 >> >> 4. Wait until you're within 4 minutes of key expiry, then do some more >> I/O to the kerberized mount to ensure that RPC_CRED_KEY_EXPIRE_SOON gets >> set. Verify that the writes are 4K, FILE_SYNC: >> $ dd if=/dev/zero of=/mnt/krb5/file bs=1M count=1 >> >> 5. Now do some I/O to the sec=sys mount. This will cause >> RPC_CRED_NO_CRKEY_TIMEOUT to be set: >> $ dd if=/dev/zero of=/mnt/sys/file bs=1M count=1 >> >> 6. Writes for that user will now be permanently 4K, FILE_SYNC for that >> user, regardless of which mount is being written to, until you reboot >> the client. Renewing the kerberos ticket (assuming it hasn't already >> expired) will have no effect. Grabbing a new kerberos ticket at this >> point will have no effect either. >> >> Move the flag to the auth->au_flags field (which is currently unused) >> and rename it slightly to reflect that it's no longer associated with >> the auth_cred->ac_flags. Add the rpc_auth to the arg list of >> rpcauth_cred_key_to_expire and check the au_flags there too. Finally, >> add the inode to the arg list of nfs_ctx_key_to_expire so we can >> determine the rpc_auth to pass to rpcauth_cred_key_to_expire. >> >> Signed-off-by: Scott Mayhew >> --- >> fs/nfs/file.c | 4 ++-- >> fs/nfs/internal.h | 2 +- >> fs/nfs/write.c | 6 ++++-- >> include/linux/sunrpc/auth.h | 6 ++++-- >> net/sunrpc/auth.c | 4 +++- >> net/sunrpc/auth_generic.c | 9 +-------- >> net/sunrpc/auth_gss/auth_gss.c | 1 + >> net/sunrpc/auth_null.c | 1 + >> net/sunrpc/auth_unix.c | 1 + >> 9 files changed, 18 insertions(+), 16 deletions(-) >> >> diff --git a/fs/nfs/file.c b/fs/nfs/file.c >> index 717a8d6..6bcd891 100644 >> --- a/fs/nfs/file.c >> +++ b/fs/nfs/file.c >> @@ -432,7 +432,7 @@ static int nfs_write_end(struct file *file, struct >> address_space *mapping, >> return status; >> NFS_I(mapping->host)->write_io += copied; >> >> - if (nfs_ctx_key_to_expire(ctx)) { >> + if (nfs_ctx_key_to_expire(ctx, mapping->host)) { >> status = nfs_wb_all(mapping->host); >> if (status < 0) >> return status; >> @@ -645,7 +645,7 @@ static int nfs_need_check_write(struct file *filp, >> struct inode *inode) >> >> ctx = nfs_file_open_context(filp); >> if (test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags) || >> - nfs_ctx_key_to_expire(ctx)) >> + nfs_ctx_key_to_expire(ctx, inode)) >> return 1; >> return 0; >> } >> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h >> index 5154fa6..ca87787 100644 >> --- a/fs/nfs/internal.h >> +++ b/fs/nfs/internal.h >> @@ -496,7 +496,7 @@ void nfs_init_cinfo(struct nfs_commit_info *cinfo, >> struct inode *inode, >> struct nfs_direct_req *dreq); >> int nfs_key_timeout_notify(struct file *filp, struct inode *inode); >> -bool nfs_ctx_key_to_expire(struct nfs_open_context *ctx); >> +bool nfs_ctx_key_to_expire(struct nfs_open_context *ctx, struct inode >> *inode); >> void nfs_pageio_stop_mirroring(struct nfs_pageio_descriptor *pgio); >> >> #ifdef CONFIG_MIGRATION >> diff --git a/fs/nfs/write.c b/fs/nfs/write.c >> index e1c74d3..0b949a0 100644 >> --- a/fs/nfs/write.c >> +++ b/fs/nfs/write.c >> @@ -1195,9 +1195,11 @@ nfs_key_timeout_notify(struct file *filp, struct >> inode *inode) >> /* >> * Test if the open context credential key is marked to expire soon. >> */ >> -bool nfs_ctx_key_to_expire(struct nfs_open_context *ctx) >> +bool nfs_ctx_key_to_expire(struct nfs_open_context *ctx, struct inode >> *inode) >> { >> - return rpcauth_cred_key_to_expire(ctx->cred); >> + struct rpc_auth *auth = NFS_SERVER(inode)->client->cl_auth; >> + >> + return rpcauth_cred_key_to_expire(auth, ctx->cred); >> } >> >> /* >> diff --git a/include/linux/sunrpc/auth.h b/include/linux/sunrpc/auth.h >> index 8997915..f890a29 100644 >> --- a/include/linux/sunrpc/auth.h >> +++ b/include/linux/sunrpc/auth.h >> @@ -37,7 +37,6 @@ struct rpcsec_gss_info; >> >> /* auth_cred ac_flags bits */ >> enum { >> - RPC_CRED_NO_CRKEY_TIMEOUT = 0, /* underlying cred has no key >> timeout */ >> RPC_CRED_KEY_EXPIRE_SOON = 1, /* underlying cred key will expire >> soon */ >> RPC_CRED_NOTIFY_TIMEOUT = 2, /* nofity generic cred when >> underlying >> key will expire soon */ >> @@ -82,6 +81,9 @@ struct rpc_cred { >> >> #define RPCAUTH_CRED_MAGIC 0x0f4aa4f0 >> >> +/* rpc_auth au_flags */ >> +#define RPCAUTH_AUTH_NO_CRKEY_TIMEOUT 0x0001 /* underlying cred has no >> key timeout */ >> + >> /* >> * Client authentication handle >> */ >> @@ -196,7 +198,7 @@ void >> rpcauth_destroy_credcache(struct rpc_auth *); >> void rpcauth_clear_credcache(struct rpc_cred_cache *); >> int rpcauth_key_timeout_notify(struct rpc_auth *, >> struct rpc_cred *); >> -bool rpcauth_cred_key_to_expire(struct rpc_cred *); >> +bool rpcauth_cred_key_to_expire(struct rpc_auth *, >> struct rpc_cred *); >> char * rpcauth_stringify_acceptor(struct rpc_cred *); >> >> static inline >> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c >> index 040ff62..696eb39 100644 >> --- a/net/sunrpc/auth.c >> +++ b/net/sunrpc/auth.c >> @@ -359,8 +359,10 @@ rpcauth_key_timeout_notify(struct rpc_auth *auth, >> struct rpc_cred *cred) >> EXPORT_SYMBOL_GPL(rpcauth_key_timeout_notify); >> >> bool >> -rpcauth_cred_key_to_expire(struct rpc_cred *cred) >> +rpcauth_cred_key_to_expire(struct rpc_auth *auth, struct rpc_cred *cred) >> { >> + if (auth->au_flags & RPCAUTH_AUTH_NO_CRKEY_TIMEOUT) >> + return false; >> if (!cred->cr_ops->crkey_to_expire) >> return false; >> return cred->cr_ops->crkey_to_expire(cred); >> diff --git a/net/sunrpc/auth_generic.c b/net/sunrpc/auth_generic.c >> index 54dd3fd..1682195 100644 >> --- a/net/sunrpc/auth_generic.c >> +++ b/net/sunrpc/auth_generic.c >> @@ -224,7 +224,7 @@ generic_key_timeout(struct rpc_auth *auth, struct >> rpc_cred *cred) >> >> >> /* Fast track for non crkey_timeout (no key) underlying >> credentials */ >> - if (test_bit(RPC_CRED_NO_CRKEY_TIMEOUT, &acred->ac_flags)) >> + if (auth->au_flags & RPCAUTH_AUTH_NO_CRKEY_TIMEOUT) >> return 0; >> >> /* Fast track for the normal case */ >> @@ -236,12 +236,6 @@ generic_key_timeout(struct rpc_auth *auth, struct >> rpc_cred *cred) >> if (IS_ERR(tcred)) >> return -EACCES; >> >> - if (!tcred->cr_ops->crkey_timeout) { >> - set_bit(RPC_CRED_NO_CRKEY_TIMEOUT, &acred->ac_flags); >> - ret = 0; >> - goto out_put; >> - } >> - >> /* Test for the almost error case */ >> ret = tcred->cr_ops->crkey_timeout(tcred); >> if (ret != 0) { >> @@ -257,7 +251,6 @@ generic_key_timeout(struct rpc_auth *auth, struct >> rpc_cred *cred) >> set_bit(RPC_CRED_NOTIFY_TIMEOUT, &acred->ac_flags); >> } >> >> -out_put: >> put_rpccred(tcred); >> return ret; >> } >> diff --git a/net/sunrpc/auth_gss/auth_gss.c >> b/net/sunrpc/auth_gss/auth_gss.c >> index e64ae93..813a3cd 100644 >> --- a/net/sunrpc/auth_gss/auth_gss.c >> +++ b/net/sunrpc/auth_gss/auth_gss.c >> @@ -1015,6 +1015,7 @@ gss_create_new(struct rpc_auth_create_args *args, >> struct rpc_clnt *clnt) >> auth = &gss_auth->rpc_auth; >> auth->au_cslack = GSS_CRED_SLACK >> 2; >> auth->au_rslack = GSS_VERF_SLACK >> 2; >> + auth->au_flags = 0; >> auth->au_ops = &authgss_ops; >> auth->au_flavor = flavor; >> atomic_set(&auth->au_count, 1); >> diff --git a/net/sunrpc/auth_null.c b/net/sunrpc/auth_null.c >> index 8d9eb4d..4d17376 100644 >> --- a/net/sunrpc/auth_null.c >> +++ b/net/sunrpc/auth_null.c >> @@ -115,6 +115,7 @@ static >> struct rpc_auth null_auth = { >> .au_cslack = NUL_CALLSLACK, >> .au_rslack = NUL_REPLYSLACK, >> + .au_flags = RPCAUTH_AUTH_NO_CRKEY_TIMEOUT, >> .au_ops = &authnull_ops, >> .au_flavor = RPC_AUTH_NULL, >> .au_count = ATOMIC_INIT(0), >> diff --git a/net/sunrpc/auth_unix.c b/net/sunrpc/auth_unix.c >> index 9f65452..a99278c 100644 >> --- a/net/sunrpc/auth_unix.c >> +++ b/net/sunrpc/auth_unix.c >> @@ -228,6 +228,7 @@ static >> struct rpc_auth unix_auth = { >> .au_cslack = UNX_CALLSLACK, >> .au_rslack = NUL_REPLYSLACK, >> + .au_flags = RPCAUTH_AUTH_NO_CRKEY_TIMEOUT, >> .au_ops = &authunix_ops, >> .au_flavor = RPC_AUTH_UNIX, >> .au_count = ATOMIC_INIT(0), >> -- >> 2.4.11 >> >> -- >> 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