SUNRPC: Make sure authorize svc when meeting SVC_CLOSE

Commit 4d712ef1db05 "svcauth_gss: Close connection when
dropping an incoming message" will close connection,
but forget authorizing the svc when meeting SVC_CLOSE.

That, there will be an module reference to sunrpc, and some memory leak.

When mounting an nfs filesystem, the reference leak increase one.

Signed-off-by Kinglong Mee <[email protected]>

---
 net/sunrpc/svc.c | 5 +++++
 1 file changed, 5 insertions(+)

net/sunrpc/svc.c +5 -0

index 75f290b..63ca2ea 100644
@@ -1155,6 +1155,11 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 	case SVC_DENIED:
 		goto err_bad_auth;
 	case SVC_CLOSE:
+		/*
+		 * Makesure authorise svc if progp->pg_authenticate fail,
+		 * it's harmless if svc_authenticate fail.
+		 */
+		svc_authorise(rqstp);
 		goto close;
 	case SVC_DROP:
 		goto dropit;

Signature · 1 line

2.9.3

Quoted 8 lines · Kinglong Mee

Commit 4d712ef1db05 "svcauth_gss: Close connection when
dropping an incoming message" will close connection,
but forget authorizing the svc when meeting SVC_CLOSE.

That, there will be an module reference to sunrpc, and some memory leak.

When mounting an nfs filesystem, the reference leak increase one.

Thanks, applying for 4.10.

(I'm not too happy with this function--e.g. it'd be easier to avoid this sort of bug if we had a single unavoidable common exit that always called svc_authenticate.

But I'm not sure of the best cleanup on a quick look. And this is a simple bugfix. Maybe we could add some cleanup on top for 4.11.)

Quoted 10 lines · J. Bruce Fields

Quoted 8 lines · Kinglong Mee

Commit 4d712ef1db05 "svcauth_gss: Close connection when
dropping an incoming message" will close connection,
but forget authorizing the svc when meeting SVC_CLOSE.

That, there will be an module reference to sunrpc, and some memory leak.

When mounting an nfs filesystem, the reference leak increase one.

Quoted 8 lines

Thanks, applying for 4.10.

(I'm not too happy with this function--e.g. it'd be easier to avoid this sort of bug if we had a single unavoidable common exit that always called svc_authenticate.

But I'm not sure of the best cleanup on a quick look. And this is a simple bugfix. Maybe we could add some cleanup on top for 4.11.)

Hi Bruce,

I don't see this patch in the latest linux kernel (4.11.0-rc1+), what's your plan about it?

Ps, for the cleanup, I have a draft for it, but it's a good one.

thanks,
Kinglong Mee

-----------------------------------------------------------------------------

net/sunrpc/svc.c +83 -90

index 44e7d49..a390842 100644
@@ -1086,6 +1086,30 @@ void svc_printk(struct svc_rqst *rqstp, const char *fmt, ...)
 static __printf(2,3) void svc_printk(struct svc_rqst *rqstp, const char *fmt, ...) {}
 #endif
 
+#define FMT_ERR_ENCODE(rpc_stat) do {					\
+	svc_printk(rqstp, "svc: decode error ##rpc_stat (%s:%d)\n",	\
+			__FILE__, __LINE__);				\
+	serv->sv_stats->rpcbadfmt++;					\
+	svc_putnl(resv, (rpc_stat));					\
+} while (0)
+
+#define VERS_ERR_ENCODE(lovers, hivers) do {				\
+	FMT_ERR_ENCODE(RPC_PROG_MISMATCH);				\
+	svc_putnl(resv, (lovers));					\
+	svc_putnl(resv, (hivers));					\
+} while (0)
+
+#define AUTH_ERR_ENCODE(auth_stat) do {					\
+	svc_printk(rqstp, "svc: authentication error %d (%s:%d)\n",	\
+			ntohl(auth_stat), __FILE__, __LINE__);		\
+	serv->sv_stats->rpcbadauth++;					\
+	/* Restore write pointer to location of accept status: */	\
+	xdr_ressize_check(rqstp, reply_statp);				\
+	svc_putnl(resv, 1);	/* REJECT */				\
+	svc_putnl(resv, 1);	/* AUTH_ERROR */			\
+	svc_putnl(resv, ntohl(auth_stat));	/* status */		\
+} while (0)
+
 /*
  * Common routine for processing the RPC request.
  */

@@ -1099,14 +1123,15 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 	kxdrproc_t		xdr;
 	__be32			*statp;
 	u32			prog, vers, proc;
-	__be32			auth_stat, rpc_stat;
+	__be32			auth_stat;
 	int			auth_res;
 	__be32			*reply_statp;
 
-	rpc_stat = rpc_success;
-
-	if (argv->iov_len < 6*4)
-		goto err_short_len;
+	if (argv->iov_len < 6*4) {
+		svc_printk(rqstp, "short len %zd, dropping request\n",
+				argv->iov_len);
+		goto close;
+	}
 
 	/* Will be turned off only in gss privacy case: */
 	set_bit(RQ_SPLICE_OK, &rqstp->rq_flags);

@@ -1124,8 +1149,14 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 	/* First words of reply: */
 	svc_putnl(resv, 1);		/* REPLY */
 
-	if (vers != 2)		/* RPC version number */
-		goto err_bad_rpc;
+	if (vers != 2) {		/* RPC version number */
+		serv->sv_stats->rpcbadfmt++;
+		svc_putnl(resv, 1);     /* REJECT */
+		svc_putnl(resv, 0);	/* RPC_MISMATCH */
+		svc_putnl(resv, 2);	/* Only RPCv2 supported */
+		svc_putnl(resv, 2);
+		goto sendit;
+	}
 
 	/* Save position in case we later decide to reject: */
 	reply_statp = resv->iov_base + resv->iov_len;

@@ -1155,12 +1186,14 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 	case SVC_OK:
 		break;
 	case SVC_GARBAGE:
-		goto err_garbage;
+		FMT_ERR_ENCODE(RPC_GARBAGE_ARGS);
+		goto sendit;
 	case SVC_SYSERR:
-		rpc_stat = rpc_system_err;
-		goto err_bad;
+		FMT_ERR_ENCODE(RPC_SYSTEM_ERR);
+		goto sendit;
 	case SVC_DENIED:
-		goto err_bad_auth;
+		AUTH_ERR_ENCODE(auth_stat);
+		goto sendit;
 	case SVC_CLOSE:
 		/*
 		 * Makesure authorise svc if progp->pg_authenticate fail,

@@ -1174,12 +1207,19 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 		goto sendit;
 	}
 
-	if (progp == NULL)
-		goto err_bad_prog;
+	if (progp == NULL) {
+		FMT_ERR_ENCODE(RPC_PROG_UNAVAIL);
+		svc_printk(rqstp, "svc: unknown program %d\n", prog);
+		goto sendit;
+	}
 
 	if (vers >= progp->pg_nvers ||
-	  !(versp = progp->pg_vers[vers]))
-		goto err_bad_vers;
+	  !(versp = progp->pg_vers[vers])) {
+		VERS_ERR_ENCODE(progp->pg_lovers, progp->pg_hivers);
+		svc_printk(rqstp, "unknown version (%d for prog %d, %s)\n",
+			       vers, prog, progp->pg_name);
+		goto sendit;
+	}
 
 	/*
 	 * Some protocol versions (namely NFSv4) require some form of

@@ -1193,12 +1233,20 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 	 * fit.
 	 */
 	if (versp->vs_need_cong_ctrl &&
-	    !test_bit(XPT_CONG_CTRL, &rqstp->rq_xprt->xpt_flags))
-		goto err_bad_vers;
+	    !test_bit(XPT_CONG_CTRL, &rqstp->rq_xprt->xpt_flags)) {
+		VERS_ERR_ENCODE(progp->pg_lovers, progp->pg_hivers);
+		svc_printk(rqstp, "congestion control (%d for prog %d, %s)\n",
+			       vers, prog, progp->pg_name);
+		goto sendit;
+	}
 
 	procp = versp->vs_proc + proc;
-	if (proc >= versp->vs_nproc || !procp->pc_func)
-		goto err_bad_proc;
+	if (proc >= versp->vs_nproc || !procp->pc_func) {
+		FMT_ERR_ENCODE(RPC_PROC_UNAVAIL);
+		svc_printk(rqstp, "unknown procedure (%d)\n", proc);
+		goto sendit;
+	}
+
 	rqstp->rq_procinfo = procp;
 
 	/* Syntactic check complete */

@@ -1225,8 +1273,10 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 	if (!versp->vs_dispatch) {
 		/* Decode arguments */
 		xdr = procp->pc_decode;
-		if (xdr && !xdr(rqstp, argv->iov_base, rqstp->rq_argp))
-			goto err_garbage;
+		if (xdr && !xdr(rqstp, argv->iov_base, rqstp->rq_argp)) {
+			FMT_ERR_ENCODE(RPC_GARBAGE_ARGS);
+			goto sendit;
+		}
 
 		*statp = procp->pc_func(rqstp, rqstp->rq_argp, rqstp->rq_resp);
 

@@ -1240,12 +1290,14 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 		if (*statp == rpc_autherr_badcred) {
 			if (procp->pc_release)
 				procp->pc_release(rqstp, NULL, rqstp->rq_resp);
-			goto err_bad_auth;
+
+			AUTH_ERR_ENCODE(auth_stat);
+			goto sendit;
 		}
 		if (*statp == rpc_success &&
 		    (xdr = procp->pc_encode) &&
 		    !xdr(rqstp, resv->iov_base+resv->iov_len, rqstp->rq_resp)) {
-			dprintk("svc: failed to encode reply\n");
+			svc_printk(rqstp, "svc: failed to encode reply\n");
 			/* serv->sv_stats->rpcsystemerr++; */
 			*statp = rpc_system_err;
 		}

@@ -1269,77 +1321,18 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 
 	if (procp->pc_encode == NULL)
 		goto dropit;
-
- sendit:
-	if (svc_authorise(rqstp))
-		goto close;
-	return 1;		/* Caller can now send it */
-
- dropit:
-	svc_authorise(rqstp);	/* doesn't hurt to call this twice */
-	dprintk("svc: svc_process dropit\n");
-	return 0;
-
- close:
+sendit:
+	if (!svc_authorise(rqstp))
+		return 1;		/* Caller can now send it */
+close:
 	if (test_bit(XPT_TEMP, &rqstp->rq_xprt->xpt_flags))
 		svc_close_xprt(rqstp->rq_xprt);
 	dprintk("svc: svc_process close\n");
 	return 0;
-
-err_short_len:
-	svc_printk(rqstp, "short len %zd, dropping request\n",
-			argv->iov_len);
-	goto close;
-
-err_bad_rpc:
-	serv->sv_stats->rpcbadfmt++;
-	svc_putnl(resv, 1);	/* REJECT */
-	svc_putnl(resv, 0);	/* RPC_MISMATCH */
-	svc_putnl(resv, 2);	/* Only RPCv2 supported */
-	svc_putnl(resv, 2);
-	goto sendit;
-
-err_bad_auth:
-	dprintk("svc: authentication failed (%d)\n", ntohl(auth_stat));
-	serv->sv_stats->rpcbadauth++;
-	/* Restore write pointer to location of accept status: */
-	xdr_ressize_check(rqstp, reply_statp);
-	svc_putnl(resv, 1);	/* REJECT */
-	svc_putnl(resv, 1);	/* AUTH_ERROR */
-	svc_putnl(resv, ntohl(auth_stat));	/* status */
-	goto sendit;
-
-err_bad_prog:
-	dprintk("svc: unknown program %d\n", prog);
-	serv->sv_stats->rpcbadfmt++;
-	svc_putnl(resv, RPC_PROG_UNAVAIL);
-	goto sendit;
-
-err_bad_vers:
-	svc_printk(rqstp, "unknown version (%d for prog %d, %s)\n",
-		       vers, prog, progp->pg_name);
-
-	serv->sv_stats->rpcbadfmt++;
-	svc_putnl(resv, RPC_PROG_MISMATCH);
-	svc_putnl(resv, progp->pg_lovers);
-	svc_putnl(resv, progp->pg_hivers);
-	goto sendit;
-
-err_bad_proc:
-	svc_printk(rqstp, "unknown procedure (%d)\n", proc);
-
-	serv->sv_stats->rpcbadfmt++;
-	svc_putnl(resv, RPC_PROC_UNAVAIL);
-	goto sendit;
-
-err_garbage:
-	svc_printk(rqstp, "failed to decode args\n");
-
-	rpc_stat = rpc_garbage_args;
-err_bad:
-	serv->sv_stats->rpcbadfmt++;
-	svc_putnl(resv, ntohl(rpc_stat));
-	goto sendit;
+dropit:
+	svc_authorise(rqstp);	/* doesn't hurt to call this twice */
+	dprintk("svc: svc_process dropit\n");
+	return 0;
 }
 
 /*