On Fri, Oct 27, 2023 at 12:32:36PM -0400, Chuck Lever wrote: > 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. Since the underlying cause of the corruption/I/O errors is bugs in the DRC, I'm dropping this patch. > 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; > } > > -- Chuck Lever
