svcrdma: set XPT_CONG_CTRL flag for bc xprt
diff mbox

Message ID 1490618353.6879.3.camel@poochiereds.net
State New
Headers show

Commit Message

Jeff Layton March 27, 2017, 12:39 p.m. UTC
On Mon, 2017-03-27 at 07:07 -0400, Jeff Layton wrote:
> On Sun, 2017-03-26 at 22:41 -0400, Chuck Lever wrote:
> > > On Mar 26, 2017, at 10:38 PM, Chuck Lever <chucklever@gmail.com> wrote:
> > > 
> > > Hey Jeff-
> > > 
> > > 
> > > > > On Mar 26, 2017, at 9:21 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
> > > > > 
> > > > > On Sun, 2017-03-26 at 19:27 -0400, Chuck Lever wrote:
> > > > > Same change as Kinglong Mee's fix for the TCP backchannel service.
> > > > > 
> > > > 
> > > > Good catch. I guess I didn't do a good job of hunting down all of the
> > > > transports where this needed to be set. I'll give them another pass
> > > > again tomorrow to make sure I didn't miss any others.
> > > > 
> > > > > Fixes: 5283b03ee5cd ("nfs/nfsd/sunrpc: enforce transport...")
> > > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > > > ---
> > > > > Some (perhaps late) review comments on 5283b03ee5cd:
> > > > > 
> > > > > I have reservations about returning RPC_PROG_MISMATCH in this case.
> > > > > RPC_PROG_UNAVAIL is more sensible. But the use of UDP with NFSv4 is
> > > > > not an RPC-level error, thus reporting the problem here seems like a
> > > > > layering violation.
> > > > > 
> > > > > I'm not sure why an explicit check is needed: if the server isn't
> > > > > listening on UDP, wouldn't clients see a transport-level rejection
> > > > > (like ECONNREFUSED)?
> > > > > 
> > > > 
> > > > Sure, if the server isn't listening on UDP...
> > > > 
> > > > The point of that patch is to enforce not allowing v4 over UDP when the
> > > > server is listening on UDP to serve earlier versions.
> > > > 
> > > > As far as the error...From RFC 5531:
> > > > 
> > > >            PROG_UNAVAIL  = 1, /* remote hasn't exported program  */
> > > >            PROG_MISMATCH = 2, /* remote can't support version #  */
> > > > 
> > > > Consider the case where the server is listening on both TCP and UDP,
> > > > and is serving both v3 and v4. Someone tries to send a v4 RPC over UDP.
> > > > 
> > > > The RPC program in that case (nfs) is supported over UDP, but the
> > > > version (v4) is not. So I disagree here. PROG_MISMATCH seems like the
> > > > better fit to me.
> > > 
> > > Then the server should report the correct version range in the
> > > rejection. The RPC response I saw on the wire claimed that 4
> > > was the maximum supported version.
> > 
Of course, versions 2 and 3 do not make sense for
> > the backchannel. So I'm not sure what you would report
> > in that case.
> > 
> 
> Yeah, that's clearly a bug. The problem is that we currently track
> min/max versions on a per-program basis, but really we need to track
> them per-program + per-transport.
> 
> Another way to fix it would be to set that info more dynamically at the
> time of the error. Walk the pg_vers array and if we're on a non-
> congestion control transport, skip any entries that require it.
> 

Something like this patch maybe? Builds but is otherwise untested. It
might not DTRT though in the (nonsensical) case where you have a server
that is listening on UDP but doesn't support v2 or v3. Not sure I
really care about that too much.

[PATCH] sunrpc: dynamically set versions in PROG_MISMATCH error reply

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 include/linux/sunrpc/svc.h |  2 --
 net/sunrpc/svc.c           | 47 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 35 insertions(+), 14 deletions(-)

Comments

J . Bruce Fields March 29, 2017, 1:16 a.m. UTC | #1
On Mon, Mar 27, 2017 at 08:39:13AM -0400, Jeff Layton wrote:
> Something like this patch maybe? Builds but is otherwise untested. It
> might not DTRT though in the (nonsensical) case where you have a server
> that is listening on UDP but doesn't support v2 or v3. Not sure I
> really care about that too much.

I don't think this is worth the trouble.

A client that attempts to mount NFSv4 over UDP is operating out of spec,
and we don't owe them much.

I'm not even convinced that transport-specific high/low version returns
are correct.  A client could in theory be configured to prefer UDP and
NFSv3, but to fall back to NFSv4 and TCP if NFSv3 was unavailable, and
this would break that.  That would be legal but admittedly odd client
behavior.

If somebody actually hits a case where this patch would help, then let's
reconsider.

--b.

> 
> [PATCH] sunrpc: dynamically set versions in PROG_MISMATCH error reply
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  include/linux/sunrpc/svc.h |  2 --
>  net/sunrpc/svc.c           | 47 ++++++++++++++++++++++++++++++++++------------
>  2 files changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index e770abeed32d..f19321cfcf21 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -381,8 +381,6 @@ struct svc_deferred_req {
>  struct svc_program {
>  	struct svc_program *	pg_next;	/* other programs (same xprt) */
>  	u32			pg_prog;	/* program number */
> -	unsigned int		pg_lovers;	/* lowest version */
> -	unsigned int		pg_hivers;	/* highest version */
>  	unsigned int		pg_nvers;	/* number of versions */
>  	struct svc_version **	pg_vers;	/* version array */
>  	char *			pg_name;	/* service name */
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index a08aeb56b8e4..c81f68064313 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -441,18 +441,13 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
>  	serv->sv_ops = ops;
>  	xdrsize = 0;
>  	while (prog) {
> -		prog->pg_lovers = prog->pg_nvers-1;
>  		for (vers=0; vers<prog->pg_nvers ; vers++)
> -			if (prog->pg_vers[vers]) {
> -				prog->pg_hivers = vers;
> -				if (prog->pg_lovers > vers)
> -					prog->pg_lovers = vers;
> -				if (prog->pg_vers[vers]->vs_xdrsize > xdrsize)
> -					xdrsize = prog->pg_vers[vers]->vs_xdrsize;
> -			}
> +			if (prog->pg_vers[vers] &&
> +			    prog->pg_vers[vers]->vs_xdrsize > xdrsize)
> +				xdrsize = prog->pg_vers[vers]->vs_xdrsize;
>  		prog = prog->pg_next;
>  	}
> -	serv->sv_xdrsize   = xdrsize;
> +	serv->sv_xdrsize = xdrsize;
>  	INIT_LIST_HEAD(&serv->sv_tempsocks);
>  	INIT_LIST_HEAD(&serv->sv_permsocks);
>  	init_timer(&serv->sv_temptimer);
> @@ -1086,6 +1081,36 @@ 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
>  
> +static void
> +svc_set_prog_mismatch(struct svc_rqst *rqstp, struct svc_program *prog,
> +		      struct kvec *resv)
> +{
> +	unsigned int vers, hi = 0, lo = prog->pg_nvers - 1;
> +	struct svc_version *versp;
> +
> +	for (vers = 0; vers < prog->pg_nvers ; vers++) {
> +		versp = prog->pg_vers[vers];
> +		if (!versp)
> +			continue;
> +
> +		/*
> +		 * Don't report this version if it needs congestion control
> +		 * and the xprt doesn't provide it.
> +		 */
> +		if (versp->vs_need_cong_ctrl &&
> +		    !test_bit(XPT_CONG_CTRL, &rqstp->rq_xprt->xpt_flags))
> +			continue;
> +
> +		hi = vers;
> +		if (lo > vers)
> +			lo = vers;
> +	}
> +
> +	svc_putnl(resv, RPC_PROG_MISMATCH);
> +	svc_putnl(resv, lo);
> +	svc_putnl(resv, hi);
> +}
> +
>  /*
>   * Common routine for processing the RPC request.
>   */
> @@ -1315,9 +1340,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
>  		       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);
> +	svc_set_prog_mismatch(rqstp, progp, resv);
>  	goto sendit;
>  
>  err_bad_proc:
> -- 
> 2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton March 29, 2017, 11:01 a.m. UTC | #2
On Tue, 2017-03-28 at 21:16 -0400, J. Bruce Fields wrote:
> On Mon, Mar 27, 2017 at 08:39:13AM -0400, Jeff Layton wrote:
> > Something like this patch maybe? Builds but is otherwise untested. It
> > might not DTRT though in the (nonsensical) case where you have a server
> > that is listening on UDP but doesn't support v2 or v3. Not sure I
> > really care about that too much.
> 
> I don't think this is worth the trouble.
> 
> A client that attempts to mount NFSv4 over UDP is operating out of spec,
> and we don't owe them much.
> 
> I'm not even convinced that transport-specific high/low version returns
> are correct.  A client could in theory be configured to prefer UDP and
> NFSv3, but to fall back to NFSv4 and TCP if NFSv3 was unavailable, and
> this would break that.  That would be legal but admittedly odd client
> behavior.
> 

That's fine with me. My rationale here was that we have to treat each
listening socket as a different "remote", in RPC parlance, since not all
versions are supported on all socket types.

Again though, the version info reported here is pretty useless.

> If somebody actually hits a case where this patch would help, then let's
> reconsider.
> 

Fine by me.

Patch
diff mbox

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index e770abeed32d..f19321cfcf21 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -381,8 +381,6 @@  struct svc_deferred_req {
 struct svc_program {
 	struct svc_program *	pg_next;	/* other programs (same xprt) */
 	u32			pg_prog;	/* program number */
-	unsigned int		pg_lovers;	/* lowest version */
-	unsigned int		pg_hivers;	/* highest version */
 	unsigned int		pg_nvers;	/* number of versions */
 	struct svc_version **	pg_vers;	/* version array */
 	char *			pg_name;	/* service name */
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index a08aeb56b8e4..c81f68064313 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -441,18 +441,13 @@  __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
 	serv->sv_ops = ops;
 	xdrsize = 0;
 	while (prog) {
-		prog->pg_lovers = prog->pg_nvers-1;
 		for (vers=0; vers<prog->pg_nvers ; vers++)
-			if (prog->pg_vers[vers]) {
-				prog->pg_hivers = vers;
-				if (prog->pg_lovers > vers)
-					prog->pg_lovers = vers;
-				if (prog->pg_vers[vers]->vs_xdrsize > xdrsize)
-					xdrsize = prog->pg_vers[vers]->vs_xdrsize;
-			}
+			if (prog->pg_vers[vers] &&
+			    prog->pg_vers[vers]->vs_xdrsize > xdrsize)
+				xdrsize = prog->pg_vers[vers]->vs_xdrsize;
 		prog = prog->pg_next;
 	}
-	serv->sv_xdrsize   = xdrsize;
+	serv->sv_xdrsize = xdrsize;
 	INIT_LIST_HEAD(&serv->sv_tempsocks);
 	INIT_LIST_HEAD(&serv->sv_permsocks);
 	init_timer(&serv->sv_temptimer);
@@ -1086,6 +1081,36 @@  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
 
+static void
+svc_set_prog_mismatch(struct svc_rqst *rqstp, struct svc_program *prog,
+		      struct kvec *resv)
+{
+	unsigned int vers, hi = 0, lo = prog->pg_nvers - 1;
+	struct svc_version *versp;
+
+	for (vers = 0; vers < prog->pg_nvers ; vers++) {
+		versp = prog->pg_vers[vers];
+		if (!versp)
+			continue;
+
+		/*
+		 * Don't report this version if it needs congestion control
+		 * and the xprt doesn't provide it.
+		 */
+		if (versp->vs_need_cong_ctrl &&
+		    !test_bit(XPT_CONG_CTRL, &rqstp->rq_xprt->xpt_flags))
+			continue;
+
+		hi = vers;
+		if (lo > vers)
+			lo = vers;
+	}
+
+	svc_putnl(resv, RPC_PROG_MISMATCH);
+	svc_putnl(resv, lo);
+	svc_putnl(resv, hi);
+}
+
 /*
  * Common routine for processing the RPC request.
  */
@@ -1315,9 +1340,7 @@  svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 		       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);
+	svc_set_prog_mismatch(rqstp, progp, resv);
 	goto sendit;
 
 err_bad_proc: