[PATCH 1/2] sunrpc: split the vs_hidden flag into TCP and UDP variants (try #2)

[PATCH 1/2] sunrpc: split the vs_hidden flag into TCP and UDP variants (try #2)

[Jeff Layton]

RFC3530 states:

   Where an NFS version 4 implementation supports operation over the IP
   network protocol, the supported transports between NFS and IP MUST be
   among the IETF-approved congestion control transport protocols, which
   include TCP and SCTP

The NFS server currently registers the NFSv4 UDP port with the
portmapper, which it really shouldn't do. This patch splits the
vs_hidden flag into TCP and UDP variants. A later patch will make
the NFS server skip NFSv4+UDP registration with rpcbind.

Reported-by: Sachin Prabhu <sprabhu@redhat.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/callback_xdr.c      |    3 ++-
 fs/nfsd/nfs2acl.c          |    1 -
 fs/nfsd/nfs3acl.c          |    1 -
 include/linux/sunrpc/svc.h |    5 +++--
 net/sunrpc/svc.c           |   15 +++++++++++----
 5 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index 05af212..30baf97 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -783,7 +783,8 @@ struct svc_version nfs4_callback_version1 = {
 	.vs_proc = nfs4_callback_procedures1,
 	.vs_xdrsize = NFS4_CALLBACK_XDRSIZE,
 	.vs_dispatch = NULL,
-	.vs_hidden = 1,
+	.vs_hidden_tcp = true,
+	.vs_hidden_udp = true,
 };
 
 struct svc_version nfs4_callback_version4 = {
diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
index 6aa5590..ec602b4 100644
--- a/fs/nfsd/nfs2acl.c
+++ b/fs/nfsd/nfs2acl.c
@@ -352,5 +352,4 @@ struct svc_version	nfsd_acl_version2 = {
 		.vs_proc	= nfsd_acl_procedures2,
 		.vs_dispatch	= nfsd_dispatch,
 		.vs_xdrsize	= NFS3_SVC_XDRSIZE,
-		.vs_hidden	= 0,
 };
diff --git a/fs/nfsd/nfs3acl.c b/fs/nfsd/nfs3acl.c
index a596e9d..1949a4c 100644
--- a/fs/nfsd/nfs3acl.c
+++ b/fs/nfsd/nfs3acl.c
@@ -262,6 +262,5 @@ struct svc_version	nfsd_acl_version3 = {
 		.vs_proc	= nfsd_acl_procedures3,
 		.vs_dispatch	= nfsd_dispatch,
 		.vs_xdrsize	= NFS3_SVC_XDRSIZE,
-		.vs_hidden	= 0,
 };
 
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 5a3085b..27ff713 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -370,8 +370,9 @@ struct svc_version {
 	struct svc_procedure *	vs_proc;	/* per-procedure info */
 	u32			vs_xdrsize;	/* xdrsize needed for this version */
 
-	unsigned int		vs_hidden : 1;	/* Don't register with portmapper.
-						 * Only used for nfsacl so far. */
+	/* these flags control whether program is registered with rpcbind */
+	bool			vs_hidden_tcp;
+	bool			vs_hidden_udp;
 
 	/* Override dispatch function (e.g. when caching replies).
 	 * A return value of 0 means drop the request. 
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index d9017d6..487792c 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -868,6 +868,7 @@ int svc_register(const struct svc_serv *serv, const int family,
 	struct svc_program	*progp;
 	unsigned int		i;
 	int			error = 0;
+	bool			vs_hidden;
 
 	BUG_ON(proto == 0 && port == 0);
 
@@ -876,16 +877,20 @@ int svc_register(const struct svc_serv *serv, const int family,
 			if (progp->pg_vers[i] == NULL)
 				continue;
 
+			vs_hidden = (proto == IPPROTO_UDP) ?
+				    progp->pg_vers[i]->vs_hidden_udp :
+				    progp->pg_vers[i]->vs_hidden_tcp;
+
 			dprintk("svc: svc_register(%sv%d, %s, %u, %u)%s\n",
 					progp->pg_name,
 					i,
-					proto == IPPROTO_UDP?  "udp" : "tcp",
+					proto == IPPROTO_UDP ?  "udp" : "tcp",
 					port,
 					family,
-					progp->pg_vers[i]->vs_hidden?
+					vs_hidden ?
 						" (but not telling portmap)" : "");
 
-			if (progp->pg_vers[i]->vs_hidden)
+			if (vs_hidden)
 				continue;
 
 			error = __svc_register(progp->pg_name, progp->pg_prog,
@@ -943,7 +948,9 @@ static void svc_unregister(const struct svc_serv *serv)
 		for (i = 0; i < progp->pg_nvers; i++) {
 			if (progp->pg_vers[i] == NULL)
 				continue;
-			if (progp->pg_vers[i]->vs_hidden)
+
+			if (progp->pg_vers[i]->vs_hidden_tcp &&
+			    progp->pg_vers[i]->vs_hidden_udp)
 				continue;
 
 			__svc_unregister(progp->pg_prog, i, progp->pg_name);
-- 
1.5.5.6

[PATCH 2/2] nfsd: reject NFSv4 requests over UDP

[Jeff Layton]

RFC3530 states:

   Where an NFS version 4 implementation supports operation over the IP
   network protocol, the supported transports between NFS and IP MUST be
   among the IETF-approved congestion control transport protocols, which
   include TCP and SCTP

This patch makes nfsd_dispatch check for NFSv4 requests over UDP and
reject them with a PROG_MISMATCH RPC error.

It also has the NFS server skip rpcbind registration of the NFSv4 UDP
port.

Reported-by: Sachin Prabhu <sprabhu@redhat.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4proc.c |    1 +
 fs/nfsd/nfssvc.c   |    7 +++++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 59ec449..4dfbf08 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1365,6 +1365,7 @@ struct svc_version	nfsd_version4 = {
 		.vs_proc	= nfsd_procedures4,
 		.vs_dispatch	= nfsd_dispatch,
 		.vs_xdrsize	= NFS4_SVC_XDRSIZE,
+		.vs_hidden_udp	= true,
 };
 
 /*
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 6b59d32..9ae06ba 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -535,6 +535,13 @@ nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
 				rqstp->rq_vers, rqstp->rq_proc);
 	proc = rqstp->rq_procinfo;
 
+	/* Reject any NFSv4 request over UDP. Don't bother caching it. */
+	if (rqstp->rq_vers == 4 && rqstp->rq_prot == IPPROTO_UDP) {
+		dprintk("%s: rejecting vers 4 request over UDP\n", __func__);
+		*statp = rpc_prog_mismatch;
+		return 1;
+	}
+
 	/* Check whether we have this call in the cache. */
 	switch (nfsd_cache_lookup(rqstp, proc->pc_cachetype)) {
 	case RC_INTR:
-- 
1.5.5.6

Re: [PATCH 1/2] sunrpc: split the vs_hidden flag into TCP and UDP variants (try #2)

[Chuck Lever]

So this may be a dumb question, but why can't you just change the NFSv4 
server not to create an RPC listener for UDP?


On 06/25/10 11:49 AM, Jeff Layton wrote:
> RFC3530 states:
>
>     Where an NFS version 4 implementation supports operation over the IP
>     network protocol, the supported transports between NFS and IP MUST be
>     among the IETF-approved congestion control transport protocols, which
>     include TCP and SCTP
>
> The NFS server currently registers the NFSv4 UDP port with the
> portmapper, which it really shouldn't do. This patch splits the
> vs_hidden flag into TCP and UDP variants. A later patch will make
> the NFS server skip NFSv4+UDP registration with rpcbind.
>
> Reported-by: Sachin Prabhu<sprabhu@redhat.com>
> Signed-off-by: Jeff Layton<jlayton@redhat.com>
> ---
>   fs/nfs/callback_xdr.c      |    3 ++-
>   fs/nfsd/nfs2acl.c          |    1 -
>   fs/nfsd/nfs3acl.c          |    1 -
>   include/linux/sunrpc/svc.h |    5 +++--
>   net/sunrpc/svc.c           |   15 +++++++++++----
>   5 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
> index 05af212..30baf97 100644
> --- a/fs/nfs/callback_xdr.c
> +++ b/fs/nfs/callback_xdr.c
> @@ -783,7 +783,8 @@ struct svc_version nfs4_callback_version1 = {
>   	.vs_proc = nfs4_callback_procedures1,
>   	.vs_xdrsize = NFS4_CALLBACK_XDRSIZE,
>   	.vs_dispatch = NULL,
> -	.vs_hidden = 1,
> +	.vs_hidden_tcp = true,
> +	.vs_hidden_udp = true,
>   };
>
>   struct svc_version nfs4_callback_version4 = {
> diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
> index 6aa5590..ec602b4 100644
> --- a/fs/nfsd/nfs2acl.c
> +++ b/fs/nfsd/nfs2acl.c
> @@ -352,5 +352,4 @@ struct svc_version	nfsd_acl_version2 = {
>   		.vs_proc	= nfsd_acl_procedures2,
>   		.vs_dispatch	= nfsd_dispatch,
>   		.vs_xdrsize	= NFS3_SVC_XDRSIZE,
> -		.vs_hidden	= 0,
>   };
> diff --git a/fs/nfsd/nfs3acl.c b/fs/nfsd/nfs3acl.c
> index a596e9d..1949a4c 100644
> --- a/fs/nfsd/nfs3acl.c
> +++ b/fs/nfsd/nfs3acl.c
> @@ -262,6 +262,5 @@ struct svc_version	nfsd_acl_version3 = {
>   		.vs_proc	= nfsd_acl_procedures3,
>   		.vs_dispatch	= nfsd_dispatch,
>   		.vs_xdrsize	= NFS3_SVC_XDRSIZE,
> -		.vs_hidden	= 0,
>   };
>
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 5a3085b..27ff713 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -370,8 +370,9 @@ struct svc_version {
>   	struct svc_procedure *	vs_proc;	/* per-procedure info */
>   	u32			vs_xdrsize;	/* xdrsize needed for this version */
>
> -	unsigned int		vs_hidden : 1;	/* Don't register with portmapper.
> -						 * Only used for nfsacl so far. */
> +	/* these flags control whether program is registered with rpcbind */
> +	bool			vs_hidden_tcp;
> +	bool			vs_hidden_udp;
>
>   	/* Override dispatch function (e.g. when caching replies).
>   	 * A return value of 0 means drop the request.
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index d9017d6..487792c 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -868,6 +868,7 @@ int svc_register(const struct svc_serv *serv, const int family,
>   	struct svc_program	*progp;
>   	unsigned int		i;
>   	int			error = 0;
> +	bool			vs_hidden;
>
>   	BUG_ON(proto == 0&&  port == 0);
>
> @@ -876,16 +877,20 @@ int svc_register(const struct svc_serv *serv, const int family,
>   			if (progp->pg_vers[i] == NULL)
>   				continue;
>
> +			vs_hidden = (proto == IPPROTO_UDP) ?
> +				    progp->pg_vers[i]->vs_hidden_udp :
> +				    progp->pg_vers[i]->vs_hidden_tcp;
> +
>   			dprintk("svc: svc_register(%sv%d, %s, %u, %u)%s\n",
>   					progp->pg_name,
>   					i,
> -					proto == IPPROTO_UDP?  "udp" : "tcp",
> +					proto == IPPROTO_UDP ?  "udp" : "tcp",
>   					port,
>   					family,
> -					progp->pg_vers[i]->vs_hidden?
> +					vs_hidden ?
>   						" (but not telling portmap)" : "");
>
> -			if (progp->pg_vers[i]->vs_hidden)
> +			if (vs_hidden)
>   				continue;
>
>   			error = __svc_register(progp->pg_name, progp->pg_prog,
> @@ -943,7 +948,9 @@ static void svc_unregister(const struct svc_serv *serv)
>   		for (i = 0; i<  progp->pg_nvers; i++) {
>   			if (progp->pg_vers[i] == NULL)
>   				continue;
> -			if (progp->pg_vers[i]->vs_hidden)
> +
> +			if (progp->pg_vers[i]->vs_hidden_tcp&&
> +			    progp->pg_vers[i]->vs_hidden_udp)
>   				continue;
>
>   			__svc_unregister(progp->pg_prog, i, progp->pg_name);

Re: [PATCH 1/2] sunrpc: split the vs_hidden flag into TCP and UDP variants (try #2)

[Jeff Layton]

On Fri, 25 Jun 2010 11:50:47 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> So this may be a dumb question, but why can't you just change the NFSv4 
> server not to create an RPC listener for UDP?
> 

Unless I'm misunderstanding the code, it's not really structured to do
what you suggest. We don't really create version-specific listeners. We
create listeners for nfsd. We then hook a svc_program to nfsd that has
multiple svc_version structs attached to it. Requests come into nfsd,
it parses them and dispatches them to the appropriate version handlers.

Now that I think about it though...it may be a little cleaner to create
a nfsd4_dispatch routine that does this check and then calls the generic
nfsd_dispatch. That way we wouldn't penalize NFSv2/3 with this check. I
may respin this patch and do it that way once others have a chance to
comment.

> 
> On 06/25/10 11:49 AM, Jeff Layton wrote:
> > RFC3530 states:
> >
> >     Where an NFS version 4 implementation supports operation over the IP
> >     network protocol, the supported transports between NFS and IP MUST be
> >     among the IETF-approved congestion control transport protocols, which
> >     include TCP and SCTP
> >
> > The NFS server currently registers the NFSv4 UDP port with the
> > portmapper, which it really shouldn't do. This patch splits the
> > vs_hidden flag into TCP and UDP variants. A later patch will make
> > the NFS server skip NFSv4+UDP registration with rpcbind.
> >
> > Reported-by: Sachin Prabhu<sprabhu@redhat.com>
> > Signed-off-by: Jeff Layton<jlayton@redhat.com>
> > ---
> >   fs/nfs/callback_xdr.c      |    3 ++-
> >   fs/nfsd/nfs2acl.c          |    1 -
> >   fs/nfsd/nfs3acl.c          |    1 -
> >   include/linux/sunrpc/svc.h |    5 +++--
> >   net/sunrpc/svc.c           |   15 +++++++++++----
> >   5 files changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
> > index 05af212..30baf97 100644
> > --- a/fs/nfs/callback_xdr.c
> > +++ b/fs/nfs/callback_xdr.c
> > @@ -783,7 +783,8 @@ struct svc_version nfs4_callback_version1 = {
> >   	.vs_proc = nfs4_callback_procedures1,
> >   	.vs_xdrsize = NFS4_CALLBACK_XDRSIZE,
> >   	.vs_dispatch = NULL,
> > -	.vs_hidden = 1,
> > +	.vs_hidden_tcp = true,
> > +	.vs_hidden_udp = true,
> >   };
> >
> >   struct svc_version nfs4_callback_version4 = {
> > diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
> > index 6aa5590..ec602b4 100644
> > --- a/fs/nfsd/nfs2acl.c
> > +++ b/fs/nfsd/nfs2acl.c
> > @@ -352,5 +352,4 @@ struct svc_version	nfsd_acl_version2 = {
> >   		.vs_proc	= nfsd_acl_procedures2,
> >   		.vs_dispatch	= nfsd_dispatch,
> >   		.vs_xdrsize	= NFS3_SVC_XDRSIZE,
> > -		.vs_hidden	= 0,
> >   };
> > diff --git a/fs/nfsd/nfs3acl.c b/fs/nfsd/nfs3acl.c
> > index a596e9d..1949a4c 100644
> > --- a/fs/nfsd/nfs3acl.c
> > +++ b/fs/nfsd/nfs3acl.c
> > @@ -262,6 +262,5 @@ struct svc_version	nfsd_acl_version3 = {
> >   		.vs_proc	= nfsd_acl_procedures3,
> >   		.vs_dispatch	= nfsd_dispatch,
> >   		.vs_xdrsize	= NFS3_SVC_XDRSIZE,
> > -		.vs_hidden	= 0,
> >   };
> >
> > diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> > index 5a3085b..27ff713 100644
> > --- a/include/linux/sunrpc/svc.h
> > +++ b/include/linux/sunrpc/svc.h
> > @@ -370,8 +370,9 @@ struct svc_version {
> >   	struct svc_procedure *	vs_proc;	/* per-procedure info */
> >   	u32			vs_xdrsize;	/* xdrsize needed for this version */
> >
> > -	unsigned int		vs_hidden : 1;	/* Don't register with portmapper.
> > -						 * Only used for nfsacl so far. */
> > +	/* these flags control whether program is registered with rpcbind */
> > +	bool			vs_hidden_tcp;
> > +	bool			vs_hidden_udp;
> >
> >   	/* Override dispatch function (e.g. when caching replies).
> >   	 * A return value of 0 means drop the request.
> > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > index d9017d6..487792c 100644
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -868,6 +868,7 @@ int svc_register(const struct svc_serv *serv, const int family,
> >   	struct svc_program	*progp;
> >   	unsigned int		i;
> >   	int			error = 0;
> > +	bool			vs_hidden;
> >
> >   	BUG_ON(proto == 0&&  port == 0);
> >
> > @@ -876,16 +877,20 @@ int svc_register(const struct svc_serv *serv, const int family,
> >   			if (progp->pg_vers[i] == NULL)
> >   				continue;
> >
> > +			vs_hidden = (proto == IPPROTO_UDP) ?
> > +				    progp->pg_vers[i]->vs_hidden_udp :
> > +				    progp->pg_vers[i]->vs_hidden_tcp;
> > +
> >   			dprintk("svc: svc_register(%sv%d, %s, %u, %u)%s\n",
> >   					progp->pg_name,
> >   					i,
> > -					proto == IPPROTO_UDP?  "udp" : "tcp",
> > +					proto == IPPROTO_UDP ?  "udp" : "tcp",
> >   					port,
> >   					family,
> > -					progp->pg_vers[i]->vs_hidden?
> > +					vs_hidden ?
> >   						" (but not telling portmap)" : "");
> >
> > -			if (progp->pg_vers[i]->vs_hidden)
> > +			if (vs_hidden)
> >   				continue;
> >
> >   			error = __svc_register(progp->pg_name, progp->pg_prog,
> > @@ -943,7 +948,9 @@ static void svc_unregister(const struct svc_serv *serv)
> >   		for (i = 0; i<  progp->pg_nvers; i++) {
> >   			if (progp->pg_vers[i] == NULL)
> >   				continue;
> > -			if (progp->pg_vers[i]->vs_hidden)
> > +
> > +			if (progp->pg_vers[i]->vs_hidden_tcp&&
> > +			    progp->pg_vers[i]->vs_hidden_udp)
> >   				continue;
> >
> >   			__svc_unregister(progp->pg_prog, i, progp->pg_name);
> 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/2] sunrpc: split the vs_hidden flag into TCP and UDP variants (try #2)

[Chuck Lever]

On 06/25/10 12:07 PM, Jeff Layton wrote:
> On Fri, 25 Jun 2010 11:50:47 -0400
> Chuck Lever<chuck.lever@oracle.com>  wrote:
>
>> So this may be a dumb question, but why can't you just change the NFSv4
>> server not to create an RPC listener for UDP?
>>
>
> Unless I'm misunderstanding the code, it's not really structured to do
> what you suggest. We don't really create version-specific listeners. We
> create listeners for nfsd. We then hook a svc_program to nfsd that has
> multiple svc_version structs attached to it. Requests come into nfsd,
> it parses them and dispatches them to the appropriate version handlers.
>
> Now that I think about it though...it may be a little cleaner to create
> a nfsd4_dispatch routine that does this check and then calls the generic
> nfsd_dispatch. That way we wouldn't penalize NFSv2/3 with this check. I
> may respin this patch and do it that way once others have a chance to
> comment.

Alright, I thought that we had the same issue for NFS/RMDA.  Basically 
we want to allow NFSv3 over RDMA, but we don't want to allow NFSv4 over 
RDMA (yet).

Apparently there's nothing in rpc.nfsd or other parts of user space that 
allow fine-grained selection of NFSD transports.  Maybe we should 
consider that.

Allowing UDP for legacy NFSv2 clients, but accepting only TCP for NFSv3 
and NFSv4, is a configuration that might be desirable, for example.

>> On 06/25/10 11:49 AM, Jeff Layton wrote:
>>> RFC3530 states:
>>>
>>>      Where an NFS version 4 implementation supports operation over the IP
>>>      network protocol, the supported transports between NFS and IP MUST be
>>>      among the IETF-approved congestion control transport protocols, which
>>>      include TCP and SCTP
>>>
>>> The NFS server currently registers the NFSv4 UDP port with the
>>> portmapper, which it really shouldn't do. This patch splits the
>>> vs_hidden flag into TCP and UDP variants. A later patch will make
>>> the NFS server skip NFSv4+UDP registration with rpcbind.
>>>
>>> Reported-by: Sachin Prabhu<sprabhu@redhat.com>
>>> Signed-off-by: Jeff Layton<jlayton@redhat.com>
>>> ---
>>>    fs/nfs/callback_xdr.c      |    3 ++-
>>>    fs/nfsd/nfs2acl.c          |    1 -
>>>    fs/nfsd/nfs3acl.c          |    1 -
>>>    include/linux/sunrpc/svc.h |    5 +++--
>>>    net/sunrpc/svc.c           |   15 +++++++++++----
>>>    5 files changed, 16 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
>>> index 05af212..30baf97 100644
>>> --- a/fs/nfs/callback_xdr.c
>>> +++ b/fs/nfs/callback_xdr.c
>>> @@ -783,7 +783,8 @@ struct svc_version nfs4_callback_version1 = {
>>>    	.vs_proc = nfs4_callback_procedures1,
>>>    	.vs_xdrsize = NFS4_CALLBACK_XDRSIZE,
>>>    	.vs_dispatch = NULL,
>>> -	.vs_hidden = 1,
>>> +	.vs_hidden_tcp = true,
>>> +	.vs_hidden_udp = true,
>>>    };
>>>
>>>    struct svc_version nfs4_callback_version4 = {
>>> diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
>>> index 6aa5590..ec602b4 100644
>>> --- a/fs/nfsd/nfs2acl.c
>>> +++ b/fs/nfsd/nfs2acl.c
>>> @@ -352,5 +352,4 @@ struct svc_version	nfsd_acl_version2 = {
>>>    		.vs_proc	= nfsd_acl_procedures2,
>>>    		.vs_dispatch	= nfsd_dispatch,
>>>    		.vs_xdrsize	= NFS3_SVC_XDRSIZE,
>>> -		.vs_hidden	= 0,
>>>    };
>>> diff --git a/fs/nfsd/nfs3acl.c b/fs/nfsd/nfs3acl.c
>>> index a596e9d..1949a4c 100644
>>> --- a/fs/nfsd/nfs3acl.c
>>> +++ b/fs/nfsd/nfs3acl.c
>>> @@ -262,6 +262,5 @@ struct svc_version	nfsd_acl_version3 = {
>>>    		.vs_proc	= nfsd_acl_procedures3,
>>>    		.vs_dispatch	= nfsd_dispatch,
>>>    		.vs_xdrsize	= NFS3_SVC_XDRSIZE,
>>> -		.vs_hidden	= 0,
>>>    };
>>>
>>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>>> index 5a3085b..27ff713 100644
>>> --- a/include/linux/sunrpc/svc.h
>>> +++ b/include/linux/sunrpc/svc.h
>>> @@ -370,8 +370,9 @@ struct svc_version {
>>>    	struct svc_procedure *	vs_proc;	/* per-procedure info */
>>>    	u32			vs_xdrsize;	/* xdrsize needed for this version */
>>>
>>> -	unsigned int		vs_hidden : 1;	/* Don't register with portmapper.
>>> -						 * Only used for nfsacl so far. */
>>> +	/* these flags control whether program is registered with rpcbind */
>>> +	bool			vs_hidden_tcp;
>>> +	bool			vs_hidden_udp;
>>>
>>>    	/* Override dispatch function (e.g. when caching replies).
>>>    	 * A return value of 0 means drop the request.
>>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>>> index d9017d6..487792c 100644
>>> --- a/net/sunrpc/svc.c
>>> +++ b/net/sunrpc/svc.c
>>> @@ -868,6 +868,7 @@ int svc_register(const struct svc_serv *serv, const int family,
>>>    	struct svc_program	*progp;
>>>    	unsigned int		i;
>>>    	int			error = 0;
>>> +	bool			vs_hidden;
>>>
>>>    	BUG_ON(proto == 0&&   port == 0);
>>>
>>> @@ -876,16 +877,20 @@ int svc_register(const struct svc_serv *serv, const int family,
>>>    			if (progp->pg_vers[i] == NULL)
>>>    				continue;
>>>
>>> +			vs_hidden = (proto == IPPROTO_UDP) ?
>>> +				    progp->pg_vers[i]->vs_hidden_udp :
>>> +				    progp->pg_vers[i]->vs_hidden_tcp;
>>> +
>>>    			dprintk("svc: svc_register(%sv%d, %s, %u, %u)%s\n",
>>>    					progp->pg_name,
>>>    					i,
>>> -					proto == IPPROTO_UDP?  "udp" : "tcp",
>>> +					proto == IPPROTO_UDP ?  "udp" : "tcp",
>>>    					port,
>>>    					family,
>>> -					progp->pg_vers[i]->vs_hidden?
>>> +					vs_hidden ?
>>>    						" (but not telling portmap)" : "");
>>>
>>> -			if (progp->pg_vers[i]->vs_hidden)
>>> +			if (vs_hidden)
>>>    				continue;
>>>
>>>    			error = __svc_register(progp->pg_name, progp->pg_prog,
>>> @@ -943,7 +948,9 @@ static void svc_unregister(const struct svc_serv *serv)
>>>    		for (i = 0; i<   progp->pg_nvers; i++) {
>>>    			if (progp->pg_vers[i] == NULL)
>>>    				continue;
>>> -			if (progp->pg_vers[i]->vs_hidden)
>>> +
>>> +			if (progp->pg_vers[i]->vs_hidden_tcp&&
>>> +			    progp->pg_vers[i]->vs_hidden_udp)
>>>    				continue;
>>>
>>>    			__svc_unregister(progp->pg_prog, i, progp->pg_name);
>>
>
>

Re: [PATCH 1/2] sunrpc: split the vs_hidden flag into TCP and UDP variants (try #2)

[Jeff Layton]

On Fri, 25 Jun 2010 12:29:19 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> On 06/25/10 12:07 PM, Jeff Layton wrote:
> > On Fri, 25 Jun 2010 11:50:47 -0400
> > Chuck Lever<chuck.lever@oracle.com>  wrote:
> >
> >> So this may be a dumb question, but why can't you just change the NFSv4
> >> server not to create an RPC listener for UDP?
> >>
> >
> > Unless I'm misunderstanding the code, it's not really structured to do
> > what you suggest. We don't really create version-specific listeners. We
> > create listeners for nfsd. We then hook a svc_program to nfsd that has
> > multiple svc_version structs attached to it. Requests come into nfsd,
> > it parses them and dispatches them to the appropriate version handlers.
> >
> > Now that I think about it though...it may be a little cleaner to create
> > a nfsd4_dispatch routine that does this check and then calls the generic
> > nfsd_dispatch. That way we wouldn't penalize NFSv2/3 with this check. I
> > may respin this patch and do it that way once others have a chance to
> > comment.
> 
> Alright, I thought that we had the same issue for NFS/RMDA.  Basically 
> we want to allow NFSv3 over RDMA, but we don't want to allow NFSv4 over 
> RDMA (yet).
> 
> Apparently there's nothing in rpc.nfsd or other parts of user space that 
> allow fine-grained selection of NFSD transports.  Maybe we should 
> consider that.
> 
> Allowing UDP for legacy NFSv2 clients, but accepting only TCP for NFSv3 
> and NFSv4, is a configuration that might be desirable, for example.
> 

This just seems overblown to me...I really don't think any of this is
worth changing the userspace interfaces for.

We have a rather simple problem that needs to be solved -- we want to
reject NFSv4 over UDP since it's not RFC compliant.

nfsd shares the same sockets between all NFS versions. We can't change
that since you don't know what NFS version you're getting a call for
until you read in the data. There's really no way around this.

Once we read in the RPC, we can deal with it in several different
ways. I have one here, a simple check for IPPROTO_UDP (assuming I
change the patch do add a new vs_dispatch wrapper routine for NFSv4).

We could try to do this in different ways -- maybe add some new per-xprt
abstraction that sits between the svc_program and svc_version.
Abstraction layers have a cost too though and I'm not at all convinced
that cost will be lower than what I'm proposing here.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/2] sunrpc: split the vs_hidden flag into TCP and UDP variants (try #2)

[Chuck Lever]

On 06/25/10 01:47 PM, Jeff Layton wrote:
> nfsd shares the same sockets between all NFS versions. We can't change
> that since you don't know what NFS version you're getting a call for
> until you read in the data. There's really no way around this.

That is worth mentioning in a documenting comment before nfsd4_dispatch().

I feel that we're adding yet another kludge to the NFSD infrastructure 
to preserve an aging user space API and transport design that was built 
for an era when all NFS versions would always use every transport and 
address family.

I agree with Peter that we should make an effort to use the RPC 
infrastructure as it was intended, in order to advertise the correct 
services and transport capabilities; and update our user space API to 
allow user space to have more fine-grained control over what the kernel 
will advertise and run.  But perhaps that's for another day.

Re: [PATCH 1/2] sunrpc: split the vs_hidden flag into TCP and UDP variants (try #2)

[Jeff Layton]

On Fri, 25 Jun 2010 16:25:42 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> On 06/25/10 01:47 PM, Jeff Layton wrote:
> > nfsd shares the same sockets between all NFS versions. We can't change
> > that since you don't know what NFS version you're getting a call for
> > until you read in the data. There's really no way around this.
> 
> That is worth mentioning in a documenting comment before nfsd4_dispatch().
> 
> I feel that we're adding yet another kludge to the NFSD infrastructure 
> to preserve an aging user space API and transport design that was built 
> for an era when all NFS versions would always use every transport and 
> address family.
> 
> I agree with Peter that we should make an effort to use the RPC 
> infrastructure as it was intended, in order to advertise the correct 
> services and transport capabilities; and update our user space API to 
> allow user space to have more fine-grained control over what the kernel 
> will advertise and run.  But perhaps that's for another day.

Now that I've started poking at this, there are some other
irregularities. For instance doing this:

$ rpcinfo -n 2049 -u server 100003 1
rpcinfo: RPC: Program/version mismatch; low version = 2, high version = 4
program 100003 version 1 is not available

That looks reasonable, but those low/high versions are set at service
create time and don't seem to be governed by the write_versions
interfaces. We could certainly fix that with another set of hacks, but
maybe you and Peter have a point here and it would be better to fix
this at a more fundamental level.

I think I need to step back and ponder this a bit. Thanks to all for
the comments so far...
-- 
Jeff Layton <jlayton@redhat.com>

RE: [PATCH 1/2] sunrpc: split the vs_hidden flag into TCP and UDP variants (try #2)

[Staubach_Peter]

RPC already provides the framework to get this sort of thing for free.
It is unfortunate that this can't be taken advantage of.  Perhaps doing
something like creating the version specific listeners would be a better
approach, rather than adding more overhead?

		ps


-----Original Message-----
From: linux-nfs-owner@vger.kernel.org
[mailto:linux-nfs-owner@vger.kernel.org] On Behalf Of Jeff Layton
Sent: Friday, June 25, 2010 12:07 PM
To: Chuck Lever
Cc: linux-nfs@vger.kernel.org; trond.myklebust@fys.uio.no; Staubach,
Peter; bfields@fieldses.org
Subject: Re: [PATCH 1/2] sunrpc: split the vs_hidden flag into TCP and
UDP variants (try #2)

On Fri, 25 Jun 2010 11:50:47 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> So this may be a dumb question, but why can't you just change the
NFSv4 
> server not to create an RPC listener for UDP?
> 

Unless I'm misunderstanding the code, it's not really structured to do
what you suggest. We don't really create version-specific listeners. We
create listeners for nfsd. We then hook a svc_program to nfsd that has
multiple svc_version structs attached to it. Requests come into nfsd,
it parses them and dispatches them to the appropriate version handlers.

Now that I think about it though...it may be a little cleaner to create
a nfsd4_dispatch routine that does this check and then calls the generic
nfsd_dispatch. That way we wouldn't penalize NFSv2/3 with this check. I
may respin this patch and do it that way once others have a chance to
comment.

> 
> On 06/25/10 11:49 AM, Jeff Layton wrote:
> > RFC3530 states:
> >
> >     Where an NFS version 4 implementation supports operation over
the IP
> >     network protocol, the supported transports between NFS and IP
MUST be
> >     among the IETF-approved congestion control transport protocols,
which
> >     include TCP and SCTP
> >
> > The NFS server currently registers the NFSv4 UDP port with the
> > portmapper, which it really shouldn't do. This patch splits the
> > vs_hidden flag into TCP and UDP variants. A later patch will make
> > the NFS server skip NFSv4+UDP registration with rpcbind.
> >
> > Reported-by: Sachin Prabhu<sprabhu@redhat.com>
> > Signed-off-by: Jeff Layton<jlayton@redhat.com>
> > ---
> >   fs/nfs/callback_xdr.c      |    3 ++-
> >   fs/nfsd/nfs2acl.c          |    1 -
> >   fs/nfsd/nfs3acl.c          |    1 -
> >   include/linux/sunrpc/svc.h |    5 +++--
> >   net/sunrpc/svc.c           |   15 +++++++++++----
> >   5 files changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
> > index 05af212..30baf97 100644
> > --- a/fs/nfs/callback_xdr.c
> > +++ b/fs/nfs/callback_xdr.c
> > @@ -783,7 +783,8 @@ struct svc_version nfs4_callback_version1 = {
> >   	.vs_proc = nfs4_callback_procedures1,
> >   	.vs_xdrsize = NFS4_CALLBACK_XDRSIZE,
> >   	.vs_dispatch = NULL,
> > -	.vs_hidden = 1,
> > +	.vs_hidden_tcp = true,
> > +	.vs_hidden_udp = true,
> >   };
> >
> >   struct svc_version nfs4_callback_version4 = {
> > diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
> > index 6aa5590..ec602b4 100644
> > --- a/fs/nfsd/nfs2acl.c
> > +++ b/fs/nfsd/nfs2acl.c
> > @@ -352,5 +352,4 @@ struct svc_version	nfsd_acl_version2 = {
> >   		.vs_proc	= nfsd_acl_procedures2,
> >   		.vs_dispatch	= nfsd_dispatch,
> >   		.vs_xdrsize	= NFS3_SVC_XDRSIZE,
> > -		.vs_hidden	= 0,
> >   };
> > diff --git a/fs/nfsd/nfs3acl.c b/fs/nfsd/nfs3acl.c
> > index a596e9d..1949a4c 100644
> > --- a/fs/nfsd/nfs3acl.c
> > +++ b/fs/nfsd/nfs3acl.c
> > @@ -262,6 +262,5 @@ struct svc_version	nfsd_acl_version3 = {
> >   		.vs_proc	= nfsd_acl_procedures3,
> >   		.vs_dispatch	= nfsd_dispatch,
> >   		.vs_xdrsize	= NFS3_SVC_XDRSIZE,
> > -		.vs_hidden	= 0,
> >   };
> >
> > diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> > index 5a3085b..27ff713 100644
> > --- a/include/linux/sunrpc/svc.h
> > +++ b/include/linux/sunrpc/svc.h
> > @@ -370,8 +370,9 @@ struct svc_version {
> >   	struct svc_procedure *	vs_proc;	/* per-procedure info */
> >   	u32			vs_xdrsize;	/* xdrsize needed for
this version */
> >
> > -	unsigned int		vs_hidden : 1;	/* Don't register with
portmapper.
> > -						 * Only used for nfsacl
so far. */
> > +	/* these flags control whether program is registered with
rpcbind */
> > +	bool			vs_hidden_tcp;
> > +	bool			vs_hidden_udp;
> >
> >   	/* Override dispatch function (e.g. when caching replies).
> >   	 * A return value of 0 means drop the request.
> > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > index d9017d6..487792c 100644
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -868,6 +868,7 @@ int svc_register(const struct svc_serv *serv,
const int family,
> >   	struct svc_program	*progp;
> >   	unsigned int		i;
> >   	int			error = 0;
> > +	bool			vs_hidden;
> >
> >   	BUG_ON(proto == 0&&  port == 0);
> >
> > @@ -876,16 +877,20 @@ int svc_register(const struct svc_serv *serv,
const int family,
> >   			if (progp->pg_vers[i] == NULL)
> >   				continue;
> >
> > +			vs_hidden = (proto == IPPROTO_UDP) ?
> > +				    progp->pg_vers[i]->vs_hidden_udp :
> > +				    progp->pg_vers[i]->vs_hidden_tcp;
> > +
> >   			dprintk("svc: svc_register(%sv%d, %s, %u,
%u)%s\n",
> >   					progp->pg_name,
> >   					i,
> > -					proto == IPPROTO_UDP?  "udp" :
"tcp",
> > +					proto == IPPROTO_UDP ?  "udp" :
"tcp",
> >   					port,
> >   					family,
> > -					progp->pg_vers[i]->vs_hidden?
> > +					vs_hidden ?
> >   						" (but not telling
portmap)" : "");
> >
> > -			if (progp->pg_vers[i]->vs_hidden)
> > +			if (vs_hidden)
> >   				continue;
> >
> >   			error = __svc_register(progp->pg_name,
progp->pg_prog,
> > @@ -943,7 +948,9 @@ static void svc_unregister(const struct svc_serv
*serv)
> >   		for (i = 0; i<  progp->pg_nvers; i++) {
> >   			if (progp->pg_vers[i] == NULL)
> >   				continue;
> > -			if (progp->pg_vers[i]->vs_hidden)
> > +
> > +			if (progp->pg_vers[i]->vs_hidden_tcp&&
> > +			    progp->pg_vers[i]->vs_hidden_udp)
> >   				continue;
> >
> >   			__svc_unregister(progp->pg_prog, i,
progp->pg_name);
> 


-- 
Jeff Layton <jlayton@redhat.com>
--
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