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@xxxxxxxxx> wrote:
> > >
> > > Hey Jeff-
> > >
> > >
> > > > > On Mar 26, 2017, at 9:21 PM, Jeff Layton <jlayton@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxxxxx>
---
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
