svcgss: reply AUTH_BADCRED to RPCSEC_GSS with unkown services

PATCH In review Wei Yongjun 2 participants 10 messages Aug 4, 2009 – Aug 28, 2009

Subsystems (3)

Networking NFS client NFS server

lore

Share & tools

lore

Wei Yongjun

author

2009-08-04 09:29 UTC #0 lore raw

PATCH 1/1 svcgss: reply AUTH_BADCRED to RPCSEC_GSS with unkown services

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.

Signed-off-by Wei Yongjun <[email protected]>

---
static int
svcauth_gss_accept(struct svc_rqst *rqstp, __be32 *authp)

  {
         switch (gc->gc_svc) {
         case RPC_GSS_SVC_NONE:
         ...
         case RPC_GSS_SVC_INTEGRITY:
         ...
         case RPC_GSS_SVC_PRIVACY:
         ...
         default:
                 goto auth_err; <-- reply with AUTH_BADCRED
         }

...
}

static int
svcauth_gss_release(struct svc_rqst *rqstp)

  {
      ...
      switch (gc->gc_svc) {
      case RPC_GSS_SVC_NONE:
              break;
      case RPC_GSS_SVC_INTEGRITY:
      ...
      case RPC_GSS_SVC_PRIVACY:
      ...
      default:
             goto out_err; <--- will drop it
      }
      ...
  }
---
 net/sunrpc/auth_gss/svcauth_gss.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

net/sunrpc/auth_gss/svcauth_gss.c +1 -1

index 2278a50..6dce327 100644
@@ -1370,7 +1370,7 @@ svcauth_gss_release(struct svc_rqst *rqstp)
 			goto out_err;
 		break;
 	default:
-		goto out_err;
+		goto out;
 	}
 
 out:

Signature · 1 line

1.6.2.2

J. Bruce Fields

2009-08-25 21:40 UTC #1 lore raw

Quoted 6 lines · Wei Yongjun

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?)

net/sunrpc/auth_gss/svcauth_gss.c svcauth_gss_release(struct svc_rqst *rqstp) · line 1376

Quoted 13 lines

net/sunrpc/auth_gss/svcauth_gss.c +1 -1

index 2278a50..6dce327 100644
@@ -1370,7 +1370,7 @@ svcauth_gss_release(struct svc_rqst *rqstp)
 			goto out_err;
 		break;
 	default:
-		goto out_err;
+		goto out;
 	}
 
 out:

The goto seems redundant. How about just leaving out the default case and providing a comment? (See below.)

--b.

commit ab3654a05aaf367b23bbb3d9229ff72a11999719

Author: Wei Yongjun <[email protected]>
Date:   Tue Aug 4 17:27:52 2009 +0800

svcgss: reply AUTH_BADCRED to RPCSEC_GSS with unknown service

When an RPC message is received with RPCSEC_GSS with an unknown service (not RPC_GSS_SVC_NONE, RPC_GSS_SVC_INTEGRITY, or RPC_GSS_SVC_PRIVACY), svcauth_gss_accept() returns AUTH_BADCRED, but svcauth_gss_release() subsequently drops the response entirely, discarding the error.

Fix that so the AUTH_BADCRED error is returned to the client.

Signed-off-by Wei Yongjun <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

net/sunrpc/auth_gss/svcauth_gss.c +4 -2

index 2e6a148..f6c51e5 100644
@@ -1374,8 +1374,10 @@ svcauth_gss_release(struct svc_rqst *rqstp)
 		if (stat)
 			goto out_err;
 		break;
-	default:
-		goto out_err;
+	/*
+	 * For any other gc_svc value, svcauth_gss_accept() already set
+	 * the auth_error appropriately; just fall through:
+	 */
 	}
 
 out:

Wei Yongjun

author

2009-08-26 00:37 UTC #2 lore raw

Quoted 10 lines · J. Bruce Fields

Quoted 7 lines · Wei Yongjun
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.

Quoted 2 lines

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

Quoted 58 lines

Quoted 7 lines
net/sunrpc/auth_gss/svcauth_gss.c +0 -0
index 2278a50..6dce327 100644
net/sunrpc/auth_gss/svcauth_gss.c svcauth_gss_release(struct svc_rqst *rqstp) · line 1376
Quoted 8 lines
net/sunrpc/auth_gss/svcauth_gss.c +1 -1
@@ -1370,7 +1370,7 @@ svcauth_gss_release(struct svc_rqst *rqstp)
 			goto out_err;
 		break;
 	default:
-		goto out_err;
+		goto out;
 	}
 
 out:
The goto seems redundant. How about just leaving out the default case and providing a comment? (See below.)
--b.
commit ab3654a05aaf367b23bbb3d9229ff72a11999719
Author: Wei Yongjun <[email protected]>
Date:   Tue Aug 4 17:27:52 2009 +0800
svcgss: reply AUTH_BADCRED to RPCSEC_GSS with unknown service
When an RPC message is received with RPCSEC_GSS with an unknown service (not RPC_GSS_SVC_NONE, RPC_GSS_SVC_INTEGRITY, or RPC_GSS_SVC_PRIVACY), svcauth_gss_accept() returns AUTH_BADCRED, but svcauth_gss_release() subsequently drops the response entirely, discarding the error.
Fix that so the AUTH_BADCRED error is returned to the client.
Signed-off-by Wei Yongjun <[email protected]>
Signed-off-by J. Bruce Fields <[email protected]>
net/sunrpc/auth_gss/svcauth_gss.c +4 -2
index 2e6a148..f6c51e5 100644
@@ -1374,8 +1374,10 @@ svcauth_gss_release(struct svc_rqst *rqstp)
 		if (stat)
 			goto out_err;
 		break;
-	default:
-		goto out_err;
+	/*
+	 * For any other gc_svc value, svcauth_gss_accept() already set
+	 * the auth_error appropriately; just fall through:
+	 */
 	}
 
 out:

J. Bruce Fields

2009-08-26 20:57 UTC #3 lore raw

Quoted 11 lines · Wei Yongjun

Quoted 10 lines · J. Bruce Fields
Quoted 7 lines · Wei Yongjun
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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 61 lines

Quoted 59 lines
Quoted 8 lines
net/sunrpc/auth_gss/svcauth_gss.c +0 -0
index 2278a50..6dce327 100644
net/sunrpc/auth_gss/svcauth_gss.c svcauth_gss_release(struct svc_rqst *rqstp) · line 1376
Quoted 8 lines
net/sunrpc/auth_gss/svcauth_gss.c +1 -1
@@ -1370,7 +1370,7 @@ svcauth_gss_release(struct svc_rqst *rqstp)
 			goto out_err;
 		break;
 	default:
-		goto out_err;
+		goto out;
 	}
 
 out:
The goto seems redundant. How about just leaving out the default case and providing a comment? (See below.)
--b.
commit ab3654a05aaf367b23bbb3d9229ff72a11999719
Author: Wei Yongjun <[email protected]>
Date:   Tue Aug 4 17:27:52 2009 +0800
svcgss: reply AUTH_BADCRED to RPCSEC_GSS with unknown service
When an RPC message is received with RPCSEC_GSS with an unknown service (not RPC_GSS_SVC_NONE, RPC_GSS_SVC_INTEGRITY, or RPC_GSS_SVC_PRIVACY), svcauth_gss_accept() returns AUTH_BADCRED, but svcauth_gss_release() subsequently drops the response entirely, discarding the error.
Fix that so the AUTH_BADCRED error is returned to the client.
Signed-off-by Wei Yongjun <[email protected]>
Signed-off-by J. Bruce Fields <[email protected]>
net/sunrpc/auth_gss/svcauth_gss.c +4 -2
index 2e6a148..f6c51e5 100644
@@ -1374,8 +1374,10 @@ svcauth_gss_release(struct svc_rqst *rqstp)
 		if (stat)
 			goto out_err;
 		break;
-	default:
-		goto out_err;
+	/*
+	 * For any other gc_svc value, svcauth_gss_accept() already set
+	 * the auth_error appropriately; just fall through:
+	 */
 	}
 
 out:

Wei Yongjun

author

2009-08-27 02:23 UTC #4 lore raw

Hi J. Bruce Fields,

Quoted 18 lines · J. Bruce Fields

Quoted 16 lines · Wei Yongjun
Quoted 14 lines · J. Bruce Fields
Quoted 8 lines · Wei Yongjun
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?)

Quoted 7 lines

Quoted 3 lines
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
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?

[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 <[email protected]>

net/sunrpc/svc_xprt.c +7 -5

index 27d4433..fd118d7 100644
@@ -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;

J. Bruce Fields

2009-08-27 16:26 UTC #5 lore raw

Quoted 32 lines · Wei Yongjun

Hi J. Bruce Fields,
Quoted 18 lines · J. Bruce Fields
Quoted 16 lines · Wei Yongjun
Quoted 14 lines · J. Bruce Fields
Quoted 8 lines · Wei Yongjun
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?)
Quoted 7 lines
Quoted 3 lines
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:

Quoted 7 lines

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.

Quoted 27 lines

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

Quoted 7 lines

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.

Quoted 52 lines

[PATCH] sunrpc: move the close processing after do recvfrom method

Signed-off-by Wei Yongjun <[email protected]>

net/sunrpc/svc_xprt.c +7 -5

index 27d4433..fd118d7 100644
@@ -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;

J. Bruce Fields

2009-08-27 21:05 UTC #6 lore raw

Quoted 74 lines · bfields

Quoted 32 lines · Wei Yongjun
Hi J. Bruce Fields,
Quoted 18 lines · J. Bruce Fields
Quoted 16 lines · Wei Yongjun
Quoted 14 lines · J. Bruce Fields
Quoted 8 lines · Wei Yongjun
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?)
Quoted 7 lines
Quoted 3 lines
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:
Quoted 7 lines
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.
Quoted 29 lines
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

Quoted 8 lines

Quoted 5 lines
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.

Quoted 55 lines

--b.

Quoted 52 lines

[PATCH] sunrpc: move the close processing after do recvfrom method

Signed-off-by Wei Yongjun <[email protected]>

net/sunrpc/svc_xprt.c +7 -5

index 27d4433..fd118d7 100644
@@ -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;

J. Bruce Fields

2009-08-27 21:09 UTC #7 lore raw

Quoted 81 lines · bfields

Quoted 80 lines · bfields
Quoted 32 lines · Wei Yongjun
Hi J. Bruce Fields,
Quoted 18 lines · J. Bruce Fields
Quoted 16 lines · Wei Yongjun
Quoted 14 lines · J. Bruce Fields
Quoted 8 lines · Wei Yongjun
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?)
Quoted 7 lines
Quoted 3 lines
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:
Quoted 7 lines
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.
Quoted 34 lines
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?

Quoted 8 lines

Quoted 2 lines
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).

(Your patch still looks fine to me, though--applied).

--b.

Wei Yongjun

author

2009-08-28 00:56 UTC #8 lore raw

In reply to J. Bruce Fields

Quoted 100 lines · J. Bruce Fields

Quoted 82 lines · bfields
Quoted 38 lines · Wei Yongjun
Hi J. Bruce Fields,
Quoted 23 lines · J. Bruce Fields
Quoted 20 lines · Wei Yongjun
Quoted 17 lines · J. Bruce Fields
Quoted 9 lines · Wei Yongjun
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?)
Quoted 8 lines
Quoted 4 lines
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:
Quoted 8 lines
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.
Quoted 29 lines
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
Quoted 9 lines
Quoted 6 lines
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).

Quoted 4 lines

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

When I test, I just fixed the GSS8 with this patch:

lib/nfs4/servertests/st_gss.py +1 -0

index 6ad3e3e..dfff598 100644
@@ -330,4 +330,5 @@ def testBadService(t, env):
                        "should return AUTH_BADCRED, instead got %s" %
                        (service, e))
     finally:
+        orig.gss_seq_num = c.security.gss_seq_num
         c.security = orig

I am not have a test of all the case with --security=krb5, just test the gss. This is because the krb server does not always works well.^_^

Quoted 6 lines

(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.)

J. Bruce Fields

2009-08-28 16:11 UTC #9 lore raw

lib/nfs4/servertests/st_gss.py def testBadService(t, env): · line 334

Quoted 12 lines · Wei Yongjun

When I test, I just fixed the GSS8 with this patch:

lib/nfs4/servertests/st_gss.py +1 -0

index 6ad3e3e..dfff598 100644
@@ -330,4 +330,5 @@ def testBadService(t, env):
                        "should return AUTH_BADCRED, instead got %s" %
                        (service, e))
     finally:
+        orig.gss_seq_num = c.security.gss_seq_num
         c.security = orig

It might make sense just to apply something like this to upstream pynfs.

The choice of whether to increment the sequence id here isn't obvious, but I actually think the server's more likely to be right (the rfc says to increment when the checksum is succesfully verified, which it was in this case. At that point we know the contents of the header are what the client intended.)

--b.

Quoted 14 lines

I am not have a test of all the case with --security=krb5, just test the gss. This is because the krb server does not always works well.^_^
Quoted 6 lines
(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.)