Same change as Kinglong Mee's fix for the TCP backchannel service.
Fixes: 5283b03ee5cd ("nfs/nfsd/sunrpc: enforce transport...")
Signed-off-by: Chuck Lever <[email protected]>
---
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)?
Are we certain that all client implementations (including
backchannel clients) will do something useful when presented with
such a rejection? At least in the backchannel case, the Linux server
had no idea what to do with RPC_PROG_MISMATCH on the backchannel.
The workload stopped dead, no error report anywhere.
net/sunrpc/xprtrdma/svc_rdma_transport.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index c13a5c3..fc8f14c 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -127,6 +127,7 @@ static struct svc_xprt *svc_rdma_bc_create(struct svc_serv *serv,
xprt = &cma_xprt->sc_xprt;
svc_xprt_init(net, &svc_rdma_bc_class, xprt, serv);
+ set_bit(XPT_CONG_CTRL, &xprt->xpt_flags);
serv->sv_bc_xprt = xprt;
dprintk("svcrdma: %s(%p)\n", __func__, xprt);
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 <[email protected]>
> ---
> 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.
> Are we certain that all client implementations (including
> backchannel clients) will do something useful when presented with
> such a rejection? At least in the backchannel case, the Linux server
> had no idea what to do with RPC_PROG_MISMATCH on the backchannel.
> The workload stopped dead, no error report anywhere.
>
Ouch. I think this would get translated into EPROTONOSUPPORT in the
client code. That should have ended up with nfsd4_mark_cb_down being
called with that error?...but I think that function may be effectively
neutered:
static void warn_no_callback_path(struct nfs4_client *clp, int reason)
{
dprintk("NFSD: warning: no callback path to client %.*s: error %d\n",
(int)clp->cl_name.len, clp->cl_name.data, reason);
}
Note that it emits a dprintk instead of a printk. Should we promote
that to something more visible?
> net/sunrpc/xprtrdma/svc_rdma_transport.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index c13a5c3..fc8f14c 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -127,6 +127,7 @@ static struct svc_xprt *svc_rdma_bc_create(struct svc_serv *serv,
> xprt = &cma_xprt->sc_xprt;
>
> svc_xprt_init(net, &svc_rdma_bc_class, xprt, serv);
> + set_bit(XPT_CONG_CTRL, &xprt->xpt_flags);
> serv->sv_bc_xprt = xprt;
>
> dprintk("svcrdma: %s(%p)\n", __func__, xprt);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey Jeff-
> On Mar 26, 2017, at 9:21 PM, Jeff Layton <[email protected]> wrote:
>=20
>> On Sun, 2017-03-26 at 19:27 -0400, Chuck Lever wrote:
>> Same change as Kinglong Mee's fix for the TCP backchannel service.
>>=20
>=20
> 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.
>=20
>> Fixes: 5283b03ee5cd ("nfs/nfsd/sunrpc: enforce transport...")
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>> Some (perhaps late) review comments on 5283b03ee5cd:
>>=20
>> 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.
>>=20
>> 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)?
>>=20
>=20
> Sure, if the server isn't listening on UDP...
>=20
> The point of that patch is to enforce not allowing v4 over UDP when the
> server is listening on UDP to serve earlier versions.
>=20
> As far as the error...=46rom RFC 5531:
>=20
> PROG_UNAVAIL =3D 1, /* remote hasn't exported program */
> PROG_MISMATCH =3D 2, /* remote can't support version # */
>=20
> 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.
>=20
> 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.
>> Are we certain that all client implementations (including
>> backchannel clients) will do something useful when presented with
>> such a rejection? At least in the backchannel case, the Linux server
>> had no idea what to do with RPC_PROG_MISMATCH on the backchannel.
>> The workload stopped dead, no error report anywhere.
>>=20
>=20
> Ouch. I think this would get translated into EPROTONOSUPPORT in the
> client code. That should have ended up with nfsd4_mark_cb_down being
> called with that error?...but I think that function may be effectively
> neutered:
>=20
> static void warn_no_callback_path(struct nfs4_client *clp, int reason)
> {
> dprintk("NFSD: warning: no callback path to client %.*s: error %d\n=
",
> (int)clp->cl_name.len, clp->cl_name.data, reason);
> }
>=20
> Note that it emits a dprintk instead of a printk. Should we promote
> that to something more visible?
You don't want a warning if the client never provided a
callback path. But if one was provided, and it disappears,
that might be useful to know.
OTOH some might blanch at the log flood, should something
else go wrong.
An error counter might be the least we can do, if not a
one-shot pr_warn.
>> net/sunrpc/xprtrdma/svc_rdma_transport.c | 1 +
>> 1 file changed, 1 insertion(+)
>>=20
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrd=
ma/svc_rdma_transport.c
>> index c13a5c3..fc8f14c 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -127,6 +127,7 @@ static struct svc_xprt *svc_rdma_bc_create(struct svc=
_serv *serv,
>> xprt =3D &cma_xprt->sc_xprt;
>>=20
>> svc_xprt_init(net, &svc_rdma_bc_class, xprt, serv);
>> + set_bit(XPT_CONG_CTRL, &xprt->xpt_flags);
>> serv->sv_bc_xprt =3D xprt;
>>=20
>> dprintk("svcrdma: %s(%p)\n", __func__, xprt);
>>=20
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>=20
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Mar 26, 2017, at 10:38 PM, Chuck Lever <[email protected]> wrote:
>
> Hey Jeff-
>
>
>>> On Mar 26, 2017, at 9:21 PM, Jeff Layton <[email protected]> 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 <[email protected]>
>>> ---
>>> 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.
>>> Are we certain that all client implementations (including
>>> backchannel clients) will do something useful when presented with
>>> such a rejection? At least in the backchannel case, the Linux server
>>> had no idea what to do with RPC_PROG_MISMATCH on the backchannel.
>>> The workload stopped dead, no error report anywhere.
>>>
>>
>> Ouch. I think this would get translated into EPROTONOSUPPORT in the
>> client code. That should have ended up with nfsd4_mark_cb_down being
>> called with that error?...but I think that function may be effectively
>> neutered:
>>
>> static void warn_no_callback_path(struct nfs4_client *clp, int reason)
>> {
>> dprintk("NFSD: warning: no callback path to client %.*s: error %d\n",
>> (int)clp->cl_name.len, clp->cl_name.data, reason);
>> }
>>
>> Note that it emits a dprintk instead of a printk. Should we promote
>> that to something more visible?
>
> You don't want a warning if the client never provided a
> callback path. But if one was provided, and it disappears,
> that might be useful to know.
>
> OTOH some might blanch at the log flood, should something
> else go wrong.
>
> An error counter might be the least we can do, if not a
> one-shot pr_warn.
>
>
>>> net/sunrpc/xprtrdma/svc_rdma_transport.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> index c13a5c3..fc8f14c 100644
>>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> @@ -127,6 +127,7 @@ static struct svc_xprt *svc_rdma_bc_create(struct svc_serv *serv,
>>> xprt = &cma_xprt->sc_xprt;
>>>
>>> svc_xprt_init(net, &svc_rdma_bc_class, xprt, serv);
>>> + set_bit(XPT_CONG_CTRL, &xprt->xpt_flags);
>>> serv->sv_bc_xprt = xprt;
>>>
>>> dprintk("svcrdma: %s(%p)\n", __func__, xprt);
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2017-03-26 at 22:41 -0400, Chuck Lever wrote:
> > On Mar 26, 2017, at 10:38 PM, Chuck Lever <[email protected]> wrote:
> >
> > Hey Jeff-
> >
> >
> > > > On Mar 26, 2017, at 9:21 PM, Jeff Layton <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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.
That said, I'm not aware of anything that actually uses that version
info, aside from people poking around at it with rpcinfo. Is there
anything that actually does? If not, then I'm not terribly concerned
about getting it right, though it would be nice to have.
--
Jeff Layton <[email protected]>
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 <[email protected]> wrote:
> > >
> > > Hey Jeff-
> > >
> > >
> > > > > On Mar 26, 2017, at 9:21 PM, Jeff Layton <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Sun, Mar 26, 2017 at 09:21:39PM -0400, Jeff Layton wrote:
> On Sun, 2017-03-26 at 19:27 -0400, Chuck Lever wrote:
> > Are we certain that all client implementations (including
> > backchannel clients) will do something useful when presented with
> > such a rejection? At least in the backchannel case, the Linux server
> > had no idea what to do with RPC_PROG_MISMATCH on the backchannel.
> > The workload stopped dead, no error report anywhere.
> >
>
> Ouch. I think this would get translated into EPROTONOSUPPORT in the
> client code. That should have ended up with nfsd4_mark_cb_down being
> called with that error?...but I think that function may be effectively
> neutered:
Are we worrying now about a server that tries to open an NFSv4.0
callback connection using UDP?
That would be a very broken server. And broken in a way that I think is
pretty unlikely to actually happen in practice.
Maybe I'm missing something.
> static void warn_no_callback_path(struct nfs4_client *clp, int reason)
> {
> dprintk("NFSD: warning: no callback path to client %.*s: error %d\n",
> (int)clp->cl_name.len, clp->cl_name.data, reason);
> }
In NFSv4.0 a failing callback connection is absolutely normal (e.g. if
the client is behind a firewall). We might want to provide some better
diagnostics to help people figure out why a given client isn't getting
delegations, but we don't want to log this by default.
Even in the 4.1 case I wonder if some pretty common failures (e.g.
losing contact with the client) might get noticed by the callback code
first.
So, dprintk is right here.
--b.
>
> Note that it emits a dprintk instead of a printk. Should we promote
> that to something more visible?
>
> > net/sunrpc/xprtrdma/svc_rdma_transport.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > index c13a5c3..fc8f14c 100644
> > --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > @@ -127,6 +127,7 @@ static struct svc_xprt *svc_rdma_bc_create(struct svc_serv *serv,
> > xprt = &cma_xprt->sc_xprt;
> >
> > svc_xprt_init(net, &svc_rdma_bc_class, xprt, serv);
> > + set_bit(XPT_CONG_CTRL, &xprt->xpt_flags);
> > serv->sv_bc_xprt = xprt;
> >
> > dprintk("svcrdma: %s(%p)\n", __func__, xprt);
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Mar 26, 2017 at 07:27:35PM -0400, Chuck Lever wrote:
> Same change as Kinglong Mee's fix for the TCP backchannel service.
Thanks, applying!
And, apologies to all, I've let a few patches pile up the last week,
hoping to catch up and pass along a bugfix pull to Linus before next
week (when I'll need to be mostly offline again).
--b.
>
> Fixes: 5283b03ee5cd ("nfs/nfsd/sunrpc: enforce transport...")
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> 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)?
>
> Are we certain that all client implementations (including
> backchannel clients) will do something useful when presented with
> such a rejection? At least in the backchannel case, the Linux server
> had no idea what to do with RPC_PROG_MISMATCH on the backchannel.
> The workload stopped dead, no error report anywhere.
>
> net/sunrpc/xprtrdma/svc_rdma_transport.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index c13a5c3..fc8f14c 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -127,6 +127,7 @@ static struct svc_xprt *svc_rdma_bc_create(struct svc_serv *serv,
> xprt = &cma_xprt->sc_xprt;
>
> svc_xprt_init(net, &svc_rdma_bc_class, xprt, serv);
> + set_bit(XPT_CONG_CTRL, &xprt->xpt_flags);
> serv->sv_bc_xprt = xprt;
>
> dprintk("svcrdma: %s(%p)\n", __func__, xprt);
On Tue, 2017-03-28 at 21:22 -0400, J. Bruce Fields wrote:
> On Sun, Mar 26, 2017 at 09:21:39PM -0400, Jeff Layton wrote:
> > On Sun, 2017-03-26 at 19:27 -0400, Chuck Lever wrote:
> > > Are we certain that all client implementations (including
> > > backchannel clients) will do something useful when presented with
> > > such a rejection? At least in the backchannel case, the Linux server
> > > had no idea what to do with RPC_PROG_MISMATCH on the backchannel.
> > > The workload stopped dead, no error report anywhere.
> > >
> >
> > Ouch. I think this would get translated into EPROTONOSUPPORT in the
> > client code. That should have ended up with nfsd4_mark_cb_down being
> > called with that error?...but I think that function may be effectively
> > neutered:
>
> Are we worrying now about a server that tries to open an NFSv4.0
> callback connection using UDP?
>
> That would be a very broken server. And broken in a way that I think is
> pretty unlikely to actually happen in practice.
>
> Maybe I'm missing something.
>
The client is what was broken here. Chuck's initial patch is correct --
we do need to flag the RDMA backchannel xprt with XPT_CONG_CTRL. When
that wasn't there, the RDMA bc was rejecting callbacks from the sever.
The question at this point is whether we need to do anything further. It
sounds like the answer to that question is "no" and I'm fine with that.
> > static void warn_no_callback_path(struct nfs4_client *clp, int reason)
> > {
> > dprintk("NFSD: warning: no callback path to client %.*s: error %d\n",
> > (int)clp->cl_name.len, clp->cl_name.data, reason);
> > }
>
> In NFSv4.0 a failing callback connection is absolutely normal (e.g. if
> the client is behind a firewall). We might want to provide some better
> diagnostics to help people figure out why a given client isn't getting
> delegations, but we don't want to log this by default.
>
> Even in the 4.1 case I wonder if some pretty common failures (e.g.
> losing contact with the client) might get noticed by the callback code
> first.
>
> So, dprintk is right here.
>
> --b.
> >
> > Note that it emits a dprintk instead of a printk. Should we promote
> > that to something more visible?
> >
> > > net/sunrpc/xprtrdma/svc_rdma_transport.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > > index c13a5c3..fc8f14c 100644
> > > --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > > +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > > @@ -127,6 +127,7 @@ static struct svc_xprt *svc_rdma_bc_create(struct svc_serv *serv,
> > > xprt = &cma_xprt->sc_xprt;
> > >
> > > svc_xprt_init(net, &svc_rdma_bc_class, xprt, serv);
> > > + set_bit(XPT_CONG_CTRL, &xprt->xpt_flags);
> > > serv->sv_bc_xprt = xprt;
> > >
> > > dprintk("svcrdma: %s(%p)\n", __func__, xprt);
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > > the body of a message to [email protected]
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <[email protected]>
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.
--
Jeff Layton <[email protected]>