| Message ID | 1490618353.6879.3.camel@poochiereds.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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: