Re: [PATCH] svcgss: reply AUTH_BADCRED to RPCSEC_GSS with unkown services

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Aug 27, 2009 at 12:26:23PM -0400, bfields wrote:
> On Thu, Aug 27, 2009 at 10:23:39AM +0800, Wei Yongjun wrote:
> > Hi J. Bruce Fields,
> > 
> > J. Bruce Fields wrote:
> > > On Wed, Aug 26, 2009 at 08:34:39AM +0800, Wei Yongjun wrote:
> > >   
> > >> J. Bruce Fields wrote:
> > >>     
> > >>> On Tue, Aug 04, 2009 at 05:27:52PM +0800, Wei Yongjun wrote:
> > >>>   
> > >>>       
> > >>>> When RPC messages is received with RPCSEC_GSS, and if the RPCSEC_GSS
> > >>>> include unkown services (not RPC_GSS_SVC_NONE, RPC_GSS_SVC_INTEGRITY
> > >>>> and RPC_GSS_SVC_PRIVACY), the response is considered as AUTH_BADCRED
> > >>>> in svcauth_gss_accept(), but the response be drop by
> > >>>> svcauth_gss_release(). I think response with AUTH_BADCRED is correct
> > >>>> one. So this patch fixed it.
> > >>>>     
> > >>>>         
> > >>> Thanks!  How did you find this?  (And how did you test the result?)
> > >>>   
> > >>>       
> > >> I test this used newpynfs, the GSS8 item test for this.
> > >> #./testserver.py nfsserver:/ --security=krb5 GSS8
> > >>     
> > >
> > > Oh, OK--I thought I'd been running the pynfs gss tests, but now I see
> > > that I haven't been; I've fixed my test scripts....  Thanks!--b.
> > >   
> > 
> > Did you test the test case for write? In the old kernel, there was only one
> > test case WRT5 is FAILURE, but in current kernel, the test cases after
> > WRT5 are all fail, the result like the following:
> > WRT1     st_write.testSimpleWrite                                 : PASS
> > WRT1b    st_write.testSimpleWrite2                                : PASS
> > WRT2     st_write.testStateidOne                                  : PASS
> > WRT3     st_write.testWithOpen                                    : PASS
> > WRT4     st_write.testNoData                                      : PASS
> > WRT5     st_write.testLargeData                                   : FAILURE
> >            timed out
> 
> I'm not seeing exactly this, but am seeing timeouts in other tests now
> that I'm running pynfs tests over gss--it may have the same root cause.
> Unfortunately, your patch doesn't seem to fix the failures I'm seeing.
> 
> > WRT6a    st_write.testLink                                        : FAILURE
> >            timed out
> > WRT6c    st_write.testChar                                        : FAILURE
> >            timed out
> > WRT6d    st_write.testDir                                         : FAILURE
> >            timed out
> > WRT6f    st_write.testFifo                                        : FAILURE
> >            timed out
> > WRT6s    st_write.testSocket                                      : FAILURE
> >            timed out
> > WRT7     st_write.testNoFh                                        : FAILURE
> >            timed out
> > WRT8     st_write.testOpenMode                                    : FAILURE
> >            timed out
> > WRT9     st_write.testShareDeny                                   : FAILURE
> >            timed out
> > WRT10    st_write.testBadStateid                                  : FAILURE
> >            timed out
> > WRT11    st_write.testStaleStateid                                : FAILURE
> >            timed out
> > WRT12    st_write.testOldStateid                                  : FAILURE
> >            timed out
> > 
> > Case WRT5 fail because the RPC TCP fragment issue. But the rest test
> > cases are fail seems after this patch:
> >    svc: Move close processing to a single place
> >   
> > http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commitdiff;h=d7979ae4a050a45b78af51832475001b68263d2a
> > 
> > Old kernel will close the xprt after receive error. But new code is
> > check before
> > receive, and can nerver enter the check for CLOSE state.
> > 
> > Can you have a look at this patch?
> 
> OK, thanks, that makes sense.  I won't to investigate a little more
> before applying, though.

Bah, it looks like I was just seeing a disagreement between pynfs and
nfsd about whether the sequence number should be incremented in the case
of an otherwise correct packet with a bad gss_service, which means that
after running GSS8, any subsequent requests with the same context are
dropped (and time out).

Since this sitaution is of no practical interest whatsoever (I can't
see why we'd ever see a request that was broken in this particular way),
I think the correct solution is to just stop running GSS8....

(This is the problem with spending a lot of time on pynfs tests.
They've been useful for catching regressions, but there's a risk of
spending too much time tracking down "problems" that won't actually show
up in real situations.  Time would usually be better spent on bugs
(and/or performance problems) found in actual use.)

--b.

> 
> --b.
> 
> > 
> > [PATCH] sunrpc: move the close processing after do recvfrom method
> > 
> > Commit svc: Move close processing to a single place
> > (d7979ae4a050a45b78af51832475001b68263d2a) moved the
> > close processing before the recvfrom method. This may
> > cause the close processing never be execute. So this
> > patch move it to the right place.
> > 
> > Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> > 
> > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> > index 27d4433..fd118d7 100644
> > --- a/net/sunrpc/svc_xprt.c
> > +++ b/net/sunrpc/svc_xprt.c
> > @@ -710,10 +710,7 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
> >  	spin_unlock_bh(&pool->sp_lock);
> >  
> >  	len = 0;
> > -	if (test_bit(XPT_CLOSE, &xprt->xpt_flags)) {
> > -		dprintk("svc_recv: found XPT_CLOSE\n");
> > -		svc_delete_xprt(xprt);
> > -	} else if (test_bit(XPT_LISTENER, &xprt->xpt_flags)) {
> > +	if (test_bit(XPT_LISTENER, &xprt->xpt_flags)) {
> >  		struct svc_xprt *newxpt;
> >  		newxpt = xprt->xpt_ops->xpo_accept(xprt);
> >  		if (newxpt) {
> > @@ -739,7 +736,7 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
> >  			svc_xprt_received(newxpt);
> >  		}
> >  		svc_xprt_received(xprt);
> > -	} else {
> > +	} else if (!test_bit(XPT_CLOSE, &xprt->xpt_flags)) {
> >  		dprintk("svc: server %p, pool %u, transport %p, inuse=%d\n",
> >  			rqstp, pool->sp_id, xprt,
> >  			atomic_read(&xprt->xpt_ref.refcount));
> > @@ -752,6 +749,11 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
> >  		dprintk("svc: got len=%d\n", len);
> >  	}
> >  
> > +	if (test_bit(XPT_CLOSE, &xprt->xpt_flags)) {
> > +		dprintk("svc_recv: found XPT_CLOSE\n");
> > +		svc_delete_xprt(xprt);
> > +	}
> > +
> >  	/* No data, incomplete (TCP) read, or accept() */
> >  	if (len == 0 || len == -EAGAIN) {
> >  		rqstp->rq_res.len = 0;
> > 
> > 
> > 
> >