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

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

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.

--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;
> 
> 
> 
>