From: Chuck Lever <chuck.lever@xxxxxxxxxx> RFC 2203 Section 5.3.3.1 says that the sequence number window check on a server can advance the highest received sequence number only if GSS_VerifyMIC() returns GSS_S_COMPLETE. Thus NFSD's implementation calls GSS_VerifyMIC() first, then checks and updates the sequence window under a spin lock. The problem with this arrangement is that the kernel's implementation of GSS_VerifyMIC() can sleep and schedule. Or a version of checksum verification that hides timing could be in use. Sometimes it can take several milliseconds for GSS_VerifyMIC() to return. By that time, on a fast transport, the client has advanced the GSS sequence number until the current sequence number being checked falls below the current window. RFC 2203 mandates that the server silently discard the request in that case, which translates to a dropped connection and retransmit. In some cases this leads to spurious I/O errors or even data corruption. This issue affects all NFS versions using GSS Kerberos. To avoid the latency of GSS_VerifyMIC() triggering GSS sequence window bounds check failures, perform the lower bound check /before/ calling gss_verify_mic(). Closes: https://bugzilla.linux-nfs.org/show_bug.cgi?id=416 Cc: Russell Cattelan <cattelan@xxxxxxxxxxx> Cc: David Howells <dhowells@xxxxxxxxxx> Cc: Simo Sorce <simo@xxxxxxxxxx> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx> --- net/sunrpc/auth_gss/svcauth_gss.c | 63 ++++++++++++++++++++++++++++--------- 1 file changed, 47 insertions(+), 16 deletions(-) This is untested, but it's a possible way to reduce spurious failures seen when using sec=krb5[ip]. At this point, I'm interested in thoughts and comments. diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c index 18734e70c5dd..ebaa5c68d22f 100644 --- a/net/sunrpc/auth_gss/svcauth_gss.c +++ b/net/sunrpc/auth_gss/svcauth_gss.c @@ -640,7 +640,38 @@ gss_svc_searchbyctx(struct cache_detail *cd, struct xdr_netobj *handle) } /** - * gss_check_seq_num - GSS sequence number window check + * gss_seq_num_lower_bound - GSS sequence number window check + * @rqstp: RPC Call to use when reporting errors + * @rsci: cached GSS context state (updated on return) + * @seq_num: sequence number to check + * + * Implements the lower bound check part of the sequence number + * algorithm as specified in RFC 2203, Section 5.3.3.1. "Context + * Management". This is lockless since we're not updating the + * window here. Also, it happens before GSS_VerifyMIC() since MIC + * verification can take a long time during which the window can + * advance considerably. + * + * Return values: + * %true: @rqstp's GSS sequence number is inside the window + * %false: @rqstp's GSS sequence number is below the window + */ +static bool gss_seq_num_lower_bound(const struct svc_rqst *rqstp, + const struct rsc *rsci, u32 seq_num) +{ + const struct gss_svc_seq_data *sd = &rsci->seqdata; + unsigned int max = READ_ONCE(sd->sd_max); + + if (seq_num + GSS_SEQ_WIN <= max) { + trace_rpcgss_svc_seqno_low(rqstp, seq_num, + max - GSS_SEQ_WIN, max); + return false; + } + return true; +} + +/** + * gss_seq_num_update - GSS sequence number window update * @rqstp: RPC Call to use when reporting errors * @rsci: cached GSS context state (updated on return) * @seq_num: sequence number to check @@ -652,8 +683,8 @@ gss_svc_searchbyctx(struct cache_detail *cd, struct xdr_netobj *handle) * %true: @rqstp's GSS sequence number is inside the window * %false: @rqstp's GSS sequence number is outside the window */ -static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci, - u32 seq_num) +static bool gss_seq_num_update(const struct svc_rqst *rqstp, struct rsc *rsci, + u32 seq_num) { struct gss_svc_seq_data *sd = &rsci->seqdata; bool result = false; @@ -669,8 +700,6 @@ static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci, } __set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win); goto ok; - } else if (seq_num + GSS_SEQ_WIN <= sd->sd_max) { - goto toolow; } if (__test_and_set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win)) goto alreadyseen; @@ -681,11 +710,6 @@ static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci, spin_unlock(&sd->sd_lock); return result; -toolow: - trace_rpcgss_svc_seqno_low(rqstp, seq_num, - sd->sd_max - GSS_SEQ_WIN, - sd->sd_max); - goto out; alreadyseen: trace_rpcgss_svc_seqno_seen(rqstp, seq_num); goto out; @@ -709,6 +733,14 @@ svcauth_gss_verify_header(struct svc_rqst *rqstp, struct rsc *rsci, struct xdr_netobj checksum; struct kvec iov; + if (unlikely(gc->gc_seq > MAXSEQ)) { + trace_rpcgss_svc_seqno_large(rqstp, gc->gc_seq); + rqstp->rq_auth_stat = rpcsec_gsserr_ctxproblem; + return SVC_DENIED; + } + if (!gss_seq_num_lower_bound(rqstp, rsci, gc->gc_seq)) + return SVC_DROP; + /* * Compute the checksum of the incoming Call from the * XID field to credential field: @@ -738,12 +770,11 @@ svcauth_gss_verify_header(struct svc_rqst *rqstp, struct rsc *rsci, return SVC_DENIED; } - if (gc->gc_seq > MAXSEQ) { - trace_rpcgss_svc_seqno_large(rqstp, gc->gc_seq); - rqstp->rq_auth_stat = rpcsec_gsserr_ctxproblem; - return SVC_DENIED; - } - if (!gss_check_seq_num(rqstp, rsci, gc->gc_seq)) + /* + * RFC 2203 states that the sequence number window should + * be updated only if GSS_VerifyMIC returns GSS_S_COMPLETE. + */ + if (!gss_seq_num_update(rqstp, rsci, gc->gc_seq)) return SVC_DROP; return SVC_OK; }
