[PATCH 0/3] NFSv4 callback pg_authenticate fix

[PATCH 0/3] NFSv4 callback pg_authenticate fix

[andros]

The back channel server svc_process_common RPC layer pg_authenticate call
[nfs_callback_authenticate] is shared by both the NFSv4.0 and the NFSv4.1
callback threads. It authenticates the incoming request by finding (and
referencing) an nfs_client struct based on the incoming request address
and the NFS version (4). This is akin to the NFS server which authenticates
requests by matching the server address to the exports file client list.

Since there is no minorversion in the search, it may find the wrong
nfs_client struct. For the nfsv4.0 callback service thread, this means it
could find an NFSv4.1 nfs_client. For the NFSv4.1 callback service thread, it
could find an NFSv4.0 instead of v4.1, or find an NFSv4.1 nfs_client with the
wrong session.

The nfs_client is dereferenced at the end of pg_authenticate. Another
nfs_find_client call is done in the per operation  dispatcher routines
for NFSv4.0 and in the cb_sequence operation dispatcher routine for NFSv4.1
after decoding.

This means the callback server could start processing a callback, passing
the pg_authenticate test, and have the nfs_client struct freed between the
pg_authenticate call and the dispatcher operation call. Or, it could have
found the wrong nfs_client in the pg_authenticate call.

The current code has this behaviour: If the nfs_client is not found in
pg_authenticate, the request is simply dropped (SVC_DROP). If an nfs_client
is not found in the dispatcher routines NFS4ERR_BADSESSION is returned for
v4.1 requests and NFS4ERR_BADHANDLE for v4.0 requests.

The first patch fixes some of these issues. It adds a minorversion to the
nfs_find_client routine which means that the NFSv4.0 back channel service always
finds the correct nfs_client struct. The NFSv4.1 back channel service requires
the sessionid from the CB_SEQUENCE operation to guarantee that the correct
nfs_client struct is being used.

The minorversion is assigned by checking for the existance of the
bc_xprt field in the svc_rqst. This method of determining minorversion will
work until we support stand alone back channels for NFSv4.1.

The SVC_DROP return in pg_authenticate is changed to SVC_DENIED which sends
an rpc auth error back to the caller. This matches nfsd which returns
SVC_DENIED if there is no matching client address in the /etc/exports file.
SVC_DROP is used for a memory allocation error.

0001-NFS-add-minorversion-to-nfs_find_client-search.patch
0002-SQUASHME-pnfs-submit-fix-highest-backchannel-slot-us.patch

Both NFSv4.0 CB_RECALL and NFSv4.1 CB_LAYOUT_RECALL have been tested.

This third patch is for comment only. It compiles but has not been tested.
It returns SVC_DENIED if the dispatcher routines fail to find an
nfs_client struct - replacing the NFS4ERR_BADSESSION and NFS4ERR_BADHANDLE
currently returned. I'm not fully convinced that this is the behavior we want,
comments appreciated.

0003-NFS-return-an-rpc-auth-error-on-back-channel.patch

-->Andy

[PATCH 1/3] NFS add minorversion to nfs_find_client search

[andros]

From: Andy Adamson <andros@netapp.com>

The v4.0 and v4.1 callback threads share a pg_authenticate method.
Minorversions do not share an nfs_client.

Respond with an rpc auth error (SVC_DENIED) if the nfs_client matching the
server address, nfs version, and minorversion is not found.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/callback.c      |   19 +++++++++++++------
 fs/nfs/callback.h      |    4 ++--
 fs/nfs/callback_proc.c |    8 ++++----
 fs/nfs/client.c        |   16 +++++++++++-----
 fs/nfs/internal.h      |    2 +-
 5 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index aeec017..962c5de 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -17,9 +17,7 @@
 #include <linux/freezer.h>
 #include <linux/kthread.h>
 #include <linux/sunrpc/svcauth_gss.h>
-#if defined(CONFIG_NFS_V4_1)
 #include <linux/sunrpc/bc_xprt.h>
-#endif
 
 #include <net/inet_sock.h>
 
@@ -346,19 +344,28 @@ static int check_gss_callback_principal(struct nfs_client *clp,
 	return SVC_OK;
 }
 
+/*
+ * Lookup the nfs_client that corresponds to this backchannel request.
+ *
+ * We only support NFSv4.1 callbacks on the fore channel connection, so
+ * the svc_is_backchannel test indicates which minorversion nfs_client we are
+ * searching for.
+ */
 static int nfs_callback_authenticate(struct svc_rqst *rqstp)
 {
 	struct nfs_client *clp;
 	RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]);
+	u32 minorversion = svc_is_backchannel(rqstp) ? 1 : 0;
 	int ret = SVC_OK;
 
 	/* Don't talk to strangers */
-	clp = nfs_find_client(svc_addr(rqstp), 4);
+	clp = nfs_find_client(svc_addr(rqstp), 4, minorversion);
 	if (clp == NULL)
-		return SVC_DROP;
+		return SVC_DENIED;
 
-	dprintk("%s: %s NFSv4 callback!\n", __func__,
-			svc_print_addr(rqstp, buf, sizeof(buf)));
+	dprintk("%s: %s NFSv4 callback! minorversion %d\n", __func__,
+			svc_print_addr(rqstp, buf, sizeof(buf)),
+			clp->cl_minorversion);
 
 	switch (rqstp->rq_authop->flavour) {
 		case RPC_AUTH_NULL:
diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
index b16dd1f..b69bec5 100644
--- a/fs/nfs/callback.h
+++ b/fs/nfs/callback.h
@@ -177,7 +177,7 @@ static inline void put_session_client(struct nfs4_session *session)
 static inline struct nfs_client *
 find_client_from_cps(struct cb_process_state *cps, struct sockaddr *addr)
 {
-	return cps->session ? cps->session->clp : nfs_find_client(addr, 4);
+	return cps->session ? cps->session->clp : nfs_find_client(addr, 4, 0);
 }
 
 #else /* CONFIG_NFS_V4_1 */
@@ -189,7 +189,7 @@ static inline void nfs_client_return_layouts(struct nfs_client *clp)
 static inline struct nfs_client *
 find_client_from_cps(struct cb_process_state *cps, struct sockaddr *addr)
 {
-	return nfs_find_client(addr, 4);
+	return nfs_find_client(addr, 4, 0);
 }
 
 static inline void put_session_client(struct nfs4_session *session)
diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index b4c68e9..69d085a 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -505,13 +505,13 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
  * Returns NULL if there are no connections with sessions, or if no session
  * matches the one of interest.
  */
- static struct nfs_client *find_client_with_session(
-	const struct sockaddr *addr, u32 nfsversion,
-	struct nfs4_sessionid *sessionid)
+static struct nfs_client *
+find_client_with_session(const struct sockaddr *addr, u32 nfsversion,
+			 struct nfs4_sessionid *sessionid)
 {
 	struct nfs_client *clp;
 
-	clp = nfs_find_client(addr, 4);
+	clp = nfs_find_client(addr, 4, 1);
 	if (clp == NULL)
 		return NULL;
 
diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index dbf43e7..ba7712c 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -371,7 +371,8 @@ EXPORT_SYMBOL(nfs_sockaddr_cmp);
  * Find a client by IP address and protocol version
  * - returns NULL if no such client
  */
-struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion)
+struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion,
+				   u32 minorversion)
 {
 	struct nfs_client *clp;
 
@@ -385,7 +386,8 @@ struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion)
 			continue;
 
 		/* Different NFS versions cannot share the same nfs_client */
-		if (clp->rpc_ops->version != nfsversion)
+		if (clp->rpc_ops->version != nfsversion ||
+		    clp->cl_minorversion != minorversion)
 			continue;
 
 		/* Match only the IP address, not the port number */
@@ -401,13 +403,16 @@ struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion)
 }
 
 /*
- * Find a client by IP address and protocol version
- * - returns NULL if no such client
+ * Callback service RPC layer pg_authenticate method.
+ *
+ * Find a client by IP address, protocol version, and minorversion.
+ * Returns NULL if no such client
  */
 struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
 {
 	struct sockaddr *sap = (struct sockaddr *)&clp->cl_addr;
 	u32 nfsvers = clp->rpc_ops->version;
+	u32 minorversion = clp->cl_minorversion;
 
 	spin_lock(&nfs_client_lock);
 	list_for_each_entry_continue(clp, &nfs_client_list, cl_share_link) {
@@ -418,7 +423,8 @@ struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
 			continue;
 
 		/* Different NFS versions cannot share the same nfs_client */
-		if (clp->rpc_ops->version != nfsvers)
+		if (clp->rpc_ops->version != nfsvers ||
+		    clp->cl_minorversion != minorversion)
 			continue;
 
 		/* Match only the IP address, not the port number */
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 0a9ea58..d89aded 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -131,7 +131,7 @@ extern void nfs_umount(const struct nfs_mount_request *info);
 extern struct rpc_program nfs_program;
 
 extern void nfs_put_client(struct nfs_client *);
-extern struct nfs_client *nfs_find_client(const struct sockaddr *, u32);
+extern struct nfs_client *nfs_find_client(const struct sockaddr *, u32, u32);
 extern struct nfs_client *nfs_find_client_next(struct nfs_client *);
 extern struct nfs_server *nfs_create_server(
 					const struct nfs_parsed_mount_data *,
-- 
1.6.6

[PATCH 2/3] SQUASHME: pnfs-submit: fix highest backchannel slot used

[andros]

From: Andy Adamson <andros@netapp.com>

Patch 9225185aa0cd29bf85dabca5978199a6b4a73ca6
"SQUASHME: pnfs-submit: highest backchannel slot used for !CONFIG_NFS_V4_1"
removed a check for the session existance.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/callback_xdr.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index a77877c..92719f1 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -791,7 +791,8 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r
 
 	*hdr_res.status = status;
 	*hdr_res.nops = htonl(nops);
-	nfs4_callback_free_slot(cps.session);
+	if (cps.session)
+		nfs4_callback_free_slot(cps.session);
 	/* matched by cb_sequence find_client_with_session */
 	put_session_client(cps.session);
 	dprintk("%s: done, status = %u\n", __func__, ntohl(status));
-- 
1.6.6

[PATCH 3/3] NFS return an rpc auth error on back channel

[andros]

From: Andy Adamson <andros@netapp.com>

If a matching nfs_client struct is not found in the back channel NFS processing
return an rpc auth error.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/callback.h               |    3 +++
 fs/nfs/callback_proc.c          |    7 ++++---
 fs/nfs/callback_xdr.c           |    4 ++++
 include/linux/sunrpc/msg_prot.h |    1 +
 include/linux/sunrpc/xdr.h      |    1 +
 net/sunrpc/svc.c                |    5 +++++
 6 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
index b69bec5..3a54628 100644
--- a/fs/nfs/callback.h
+++ b/fs/nfs/callback.h
@@ -14,6 +14,9 @@
 #define NFS4_CALLBACK_XDRSIZE 2048
 #define NFS4_CALLBACK_BUFSIZE (1024 + NFS4_CALLBACK_XDRSIZE)
 
+/* Internal error for back channel server */
+#define nfserr_deny_reply          cpu_to_be32(30003)
+
 enum nfs4_callback_procnum {
 	CB_NULL = 0,
 	CB_COMPOUND = 1,
diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index 69d085a..ec3c84b 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -31,7 +31,7 @@ __be32 nfs4_callback_getattr(struct cb_getattrargs *args,
 	struct inode *inode;
 
 	res->bitmap[0] = res->bitmap[1] = 0;
-	res->status = htonl(NFS4ERR_BADHANDLE);
+	res->status = nfserr_deny_reply;
 	clp = find_client_from_cps(cps, args->addr);
 	if (clp == NULL)
 		goto out;
@@ -39,6 +39,7 @@ __be32 nfs4_callback_getattr(struct cb_getattrargs *args,
 	dprintk("NFS: GETATTR callback request from %s\n",
 		rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR));
 
+	res->status = htonl(NFS4ERR_BADHANDLE);
 	inode = nfs_delegation_find_inode(clp, &args->fh);
 	if (inode == NULL)
 		goto out_putclient;
@@ -76,7 +77,7 @@ __be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy,
 	struct inode *inode;
 	__be32 res;
 	
-	res = htonl(NFS4ERR_BADHANDLE);
+	res = nfserr_deny_reply;
 	clp = find_client_from_cps(cps, args->addr);
 	if (clp == NULL)
 		goto out;
@@ -598,7 +599,7 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 
 	cps->session = NULL;
 
-	status = htonl(NFS4ERR_BADSESSION);
+	status = nfserr_deny_reply;
 	clp = find_client_with_session(args->csa_addr, 4, &args->csa_sessionid);
 	if (clp == NULL)
 		goto out;
diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index 92719f1..8e17464 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -789,6 +789,10 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r
 		nops--;
 	}
 
+	/* An nfs_client struct was not found, return an rpc auth error */
+	if (unlikely(status == nfserr_deny_reply))
+		status = rpc_deny_reply;
+
 	*hdr_res.status = status;
 	*hdr_res.nops = htonl(nops);
 	if (cps.session)
diff --git a/include/linux/sunrpc/msg_prot.h b/include/linux/sunrpc/msg_prot.h
index 77e6248..9ba9422 100644
--- a/include/linux/sunrpc/msg_prot.h
+++ b/include/linux/sunrpc/msg_prot.h
@@ -59,6 +59,7 @@ enum rpc_accept_stat {
 	RPC_SYSTEM_ERR = 5,
 	/* internal use only */
 	RPC_DROP_REPLY = 60000,
+	RPC_DENY_REPLY = 60001,
 };
 
 enum rpc_reject_stat {
diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index 498ab93..9b4645c 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -82,6 +82,7 @@ struct xdr_buf {
 #define	rpc_garbage_args	cpu_to_be32(RPC_GARBAGE_ARGS)
 #define	rpc_system_err		cpu_to_be32(RPC_SYSTEM_ERR)
 #define	rpc_drop_reply		cpu_to_be32(RPC_DROP_REPLY)
+#define	rpc_deny_reply		cpu_to_be32(RPC_DENY_REPLY)
 
 #define	rpc_auth_ok		cpu_to_be32(RPC_AUTH_OK)
 #define	rpc_autherr_badcred	cpu_to_be32(RPC_AUTH_BADCRED)
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 6359c42..2c3e428 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1111,6 +1111,11 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 				procp->pc_release(rqstp, NULL, rqstp->rq_resp);
 			goto dropit;
 		}
+		if (*statp == rpc_deny_reply) {
+			if (procp->pc_release)
+				procp->pc_release(rqstp, NULL, rqstp->rq_resp);
+			goto err_bad_auth;
+		}
 		if (*statp == rpc_success &&
 		    (xdr = procp->pc_encode) &&
 		    !xdr(rqstp, resv->iov_base+resv->iov_len, rqstp->rq_resp)) {
-- 
1.6.6

Re: [PATCH 3/3] NFS return an rpc auth error on back channel

[Trond Myklebust]

On Tue, 2010-11-16 at 22:36 -0500, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> If a matching nfs_client struct is not found in the back channel NFS processing
> return an rpc auth error.

If you set the nfs_client once and for all in the struct
cb_process_state, then you won't need this hack.

In NFSv4.0, you basically want to set the nfs_client in
nfs_callback_compound() (using the server's address and the
'callback_ident' argument).

In NFSv4.1, you need to set it in the OP_SEQUENCE decode callback, but
there you need to be returning NFS4ERR_BADSESSION and/or
NFS4ERR_CONN_NOT_BOUND_TO_SESSION anyway...

Trond

> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/callback.h               |    3 +++
>  fs/nfs/callback_proc.c          |    7 ++++---
>  fs/nfs/callback_xdr.c           |    4 ++++
>  include/linux/sunrpc/msg_prot.h |    1 +
>  include/linux/sunrpc/xdr.h      |    1 +
>  net/sunrpc/svc.c                |    5 +++++
>  6 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
> index b69bec5..3a54628 100644
> --- a/fs/nfs/callback.h
> +++ b/fs/nfs/callback.h
> @@ -14,6 +14,9 @@
>  #define NFS4_CALLBACK_XDRSIZE 2048
>  #define NFS4_CALLBACK_BUFSIZE (1024 + NFS4_CALLBACK_XDRSIZE)
>  
> +/* Internal error for back channel server */
> +#define nfserr_deny_reply          cpu_to_be32(30003)
> +
>  enum nfs4_callback_procnum {
>  	CB_NULL = 0,
>  	CB_COMPOUND = 1,
> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> index 69d085a..ec3c84b 100644
> --- a/fs/nfs/callback_proc.c
> +++ b/fs/nfs/callback_proc.c
> @@ -31,7 +31,7 @@ __be32 nfs4_callback_getattr(struct cb_getattrargs *args,
>  	struct inode *inode;
>  
>  	res->bitmap[0] = res->bitmap[1] = 0;
> -	res->status = htonl(NFS4ERR_BADHANDLE);
> +	res->status = nfserr_deny_reply;
>  	clp = find_client_from_cps(cps, args->addr);
>  	if (clp == NULL)
>  		goto out;
> @@ -39,6 +39,7 @@ __be32 nfs4_callback_getattr(struct cb_getattrargs *args,
>  	dprintk("NFS: GETATTR callback request from %s\n",
>  		rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR));
>  
> +	res->status = htonl(NFS4ERR_BADHANDLE);
>  	inode = nfs_delegation_find_inode(clp, &args->fh);
>  	if (inode == NULL)
>  		goto out_putclient;
> @@ -76,7 +77,7 @@ __be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy,
>  	struct inode *inode;
>  	__be32 res;
>  	
> -	res = htonl(NFS4ERR_BADHANDLE);
> +	res = nfserr_deny_reply;
>  	clp = find_client_from_cps(cps, args->addr);
>  	if (clp == NULL)
>  		goto out;
> @@ -598,7 +599,7 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
>  
>  	cps->session = NULL;
>  
> -	status = htonl(NFS4ERR_BADSESSION);
> +	status = nfserr_deny_reply;
>  	clp = find_client_with_session(args->csa_addr, 4, &args->csa_sessionid);
>  	if (clp == NULL)
>  		goto out;
> diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
> index 92719f1..8e17464 100644
> --- a/fs/nfs/callback_xdr.c
> +++ b/fs/nfs/callback_xdr.c
> @@ -789,6 +789,10 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r
>  		nops--;
>  	}
>  
> +	/* An nfs_client struct was not found, return an rpc auth error */
> +	if (unlikely(status == nfserr_deny_reply))
> +		status = rpc_deny_reply;
> +
>  	*hdr_res.status = status;
>  	*hdr_res.nops = htonl(nops);
>  	if (cps.session)
> diff --git a/include/linux/sunrpc/msg_prot.h b/include/linux/sunrpc/msg_prot.h
> index 77e6248..9ba9422 100644
> --- a/include/linux/sunrpc/msg_prot.h
> +++ b/include/linux/sunrpc/msg_prot.h
> @@ -59,6 +59,7 @@ enum rpc_accept_stat {
>  	RPC_SYSTEM_ERR = 5,
>  	/* internal use only */
>  	RPC_DROP_REPLY = 60000,
> +	RPC_DENY_REPLY = 60001,
>  };
>  
>  enum rpc_reject_stat {
> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> index 498ab93..9b4645c 100644
> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h
> @@ -82,6 +82,7 @@ struct xdr_buf {
>  #define	rpc_garbage_args	cpu_to_be32(RPC_GARBAGE_ARGS)
>  #define	rpc_system_err		cpu_to_be32(RPC_SYSTEM_ERR)
>  #define	rpc_drop_reply		cpu_to_be32(RPC_DROP_REPLY)
> +#define	rpc_deny_reply		cpu_to_be32(RPC_DENY_REPLY)
>  
>  #define	rpc_auth_ok		cpu_to_be32(RPC_AUTH_OK)
>  #define	rpc_autherr_badcred	cpu_to_be32(RPC_AUTH_BADCRED)
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 6359c42..2c3e428 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1111,6 +1111,11 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
>  				procp->pc_release(rqstp, NULL, rqstp->rq_resp);
>  			goto dropit;
>  		}
> +		if (*statp == rpc_deny_reply) {
> +			if (procp->pc_release)
> +				procp->pc_release(rqstp, NULL, rqstp->rq_resp);
> +			goto err_bad_auth;
> +		}
>  		if (*statp == rpc_success &&
>  		    (xdr = procp->pc_encode) &&
>  		    !xdr(rqstp, resv->iov_base+resv->iov_len, rqstp->rq_resp)) {

Re: [PATCH 3/3] NFS return an rpc auth error on back channel

[William A. (Andy) Adamson]

On Wed, Nov 17, 2010 at 6:26 PM, Trond Myklebust
<trond.myklebust@fys.uio.no> wrote:
> On Tue, 2010-11-16 at 22:36 -0500, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> If a matching nfs_client struct is not found in the back channel NFS processing
>> return an rpc auth error.
>
> If you set the nfs_client once and for all in the struct
> cb_process_state, then you won't need this hack.

I do set the nfs_client 'once and for all' in the struct
cb_process_state for v4.1, and I do agree that it should be set in
v4.0.

The reason I added this patch is to ask the question - We SVC_DROP a
callback request when we can't find an nfs_client in the RPC layer. If
the nfs_client can not be found in the NFS layer, should we have
different behaviour?

Below you answer yes, we should have different behaviour.

>
> In NFSv4.0, you basically want to set the nfs_client in
> nfs_callback_compound() (using the server's address and the
> 'callback_ident' argument).

And if the nfs_client is not found should we SVC_DROP the request?
NFS4ERR_BADHANDLE?

>
> In NFSv4.1, you need to set it in the OP_SEQUENCE decode callback, but
> there you need to be returning NFS4ERR_BADSESSION and/or
> NFS4ERR_CONN_NOT_BOUND_TO_SESSION anyway...

I don't see the difference between not finding the proper nfs_client
in the pg_authenticate method and not finding it after decode in
CB_SEQUENCE.

> Trond
>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>>  fs/nfs/callback.h               |    3 +++
>>  fs/nfs/callback_proc.c          |    7 ++++---
>>  fs/nfs/callback_xdr.c           |    4 ++++
>>  include/linux/sunrpc/msg_prot.h |    1 +
>>  include/linux/sunrpc/xdr.h      |    1 +
>>  net/sunrpc/svc.c                |    5 +++++
>>  6 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
>> index b69bec5..3a54628 100644
>> --- a/fs/nfs/callback.h
>> +++ b/fs/nfs/callback.h
>> @@ -14,6 +14,9 @@
>>  #define NFS4_CALLBACK_XDRSIZE 2048
>>  #define NFS4_CALLBACK_BUFSIZE (1024 + NFS4_CALLBACK_XDRSIZE)
>>
>> +/* Internal error for back channel server */
>> +#define nfserr_deny_reply          cpu_to_be32(30003)
>> +
>>  enum nfs4_callback_procnum {
>>       CB_NULL = 0,
>>       CB_COMPOUND = 1,
>> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
>> index 69d085a..ec3c84b 100644
>> --- a/fs/nfs/callback_proc.c
>> +++ b/fs/nfs/callback_proc.c
>> @@ -31,7 +31,7 @@ __be32 nfs4_callback_getattr(struct cb_getattrargs *args,
>>       struct inode *inode;
>>
>>       res->bitmap[0] = res->bitmap[1] = 0;
>> -     res->status = htonl(NFS4ERR_BADHANDLE);
>> +     res->status = nfserr_deny_reply;
>>       clp = find_client_from_cps(cps, args->addr);
>>       if (clp == NULL)
>>               goto out;
>> @@ -39,6 +39,7 @@ __be32 nfs4_callback_getattr(struct cb_getattrargs *args,
>>       dprintk("NFS: GETATTR callback request from %s\n",
>>               rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR));
>>
>> +     res->status = htonl(NFS4ERR_BADHANDLE);
>>       inode = nfs_delegation_find_inode(clp, &args->fh);
>>       if (inode == NULL)
>>               goto out_putclient;
>> @@ -76,7 +77,7 @@ __be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy,
>>       struct inode *inode;
>>       __be32 res;
>>
>> -     res = htonl(NFS4ERR_BADHANDLE);
>> +     res = nfserr_deny_reply;
>>       clp = find_client_from_cps(cps, args->addr);
>>       if (clp == NULL)
>>               goto out;
>> @@ -598,7 +599,7 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
>>
>>       cps->session = NULL;
>>
>> -     status = htonl(NFS4ERR_BADSESSION);
>> +     status = nfserr_deny_reply;
>>       clp = find_client_with_session(args->csa_addr, 4, &args->csa_sessionid);
>>       if (clp == NULL)
>>               goto out;
>> diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
>> index 92719f1..8e17464 100644
>> --- a/fs/nfs/callback_xdr.c
>> +++ b/fs/nfs/callback_xdr.c
>> @@ -789,6 +789,10 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r
>>               nops--;
>>       }
>>
>> +     /* An nfs_client struct was not found, return an rpc auth error */
>> +     if (unlikely(status == nfserr_deny_reply))
>> +             status = rpc_deny_reply;
>> +
>>       *hdr_res.status = status;
>>       *hdr_res.nops = htonl(nops);
>>       if (cps.session)
>> diff --git a/include/linux/sunrpc/msg_prot.h b/include/linux/sunrpc/msg_prot.h
>> index 77e6248..9ba9422 100644
>> --- a/include/linux/sunrpc/msg_prot.h
>> +++ b/include/linux/sunrpc/msg_prot.h
>> @@ -59,6 +59,7 @@ enum rpc_accept_stat {
>>       RPC_SYSTEM_ERR = 5,
>>       /* internal use only */
>>       RPC_DROP_REPLY = 60000,
>> +     RPC_DENY_REPLY = 60001,
>>  };
>>
>>  enum rpc_reject_stat {
>> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
>> index 498ab93..9b4645c 100644
>> --- a/include/linux/sunrpc/xdr.h
>> +++ b/include/linux/sunrpc/xdr.h
>> @@ -82,6 +82,7 @@ struct xdr_buf {
>>  #define      rpc_garbage_args        cpu_to_be32(RPC_GARBAGE_ARGS)
>>  #define      rpc_system_err          cpu_to_be32(RPC_SYSTEM_ERR)
>>  #define      rpc_drop_reply          cpu_to_be32(RPC_DROP_REPLY)
>> +#define      rpc_deny_reply          cpu_to_be32(RPC_DENY_REPLY)
>>
>>  #define      rpc_auth_ok             cpu_to_be32(RPC_AUTH_OK)
>>  #define      rpc_autherr_badcred     cpu_to_be32(RPC_AUTH_BADCRED)
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index 6359c42..2c3e428 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -1111,6 +1111,11 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
>>                               procp->pc_release(rqstp, NULL, rqstp->rq_resp);
>>                       goto dropit;
>>               }
>> +             if (*statp == rpc_deny_reply) {
>> +                     if (procp->pc_release)
>> +                             procp->pc_release(rqstp, NULL, rqstp->rq_resp);
>> +                     goto err_bad_auth;
>> +             }
>>               if (*statp == rpc_success &&
>>                   (xdr = procp->pc_encode) &&
>>                   !xdr(rqstp, resv->iov_base+resv->iov_len, rqstp->rq_resp)) {
>
>
> --
> 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
>

Re: [PATCH 3/3] NFS return an rpc auth error on back channel

[Trond Myklebust]

On Thu, 2010-11-18 at 09:42 -0500, William A. (Andy) Adamson wrote:
> On Wed, Nov 17, 2010 at 6:26 PM, Trond Myklebust
> <trond.myklebust@fys.uio.no> wrote:
> > On Tue, 2010-11-16 at 22:36 -0500, andros@netapp.com wrote:
> >
> > In NFSv4.0, you basically want to set the nfs_client in
> > nfs_callback_compound() (using the server's address and the
> > 'callback_ident' argument).
> 
> And if the nfs_client is not found should we SVC_DROP the request?
> NFS4ERR_BADHANDLE?

We can still drop the request in nfs4_callback_compound().

> >
> > In NFSv4.1, you need to set it in the OP_SEQUENCE decode callback, but
> > there you need to be returning NFS4ERR_BADSESSION and/or
> > NFS4ERR_CONN_NOT_BOUND_TO_SESSION anyway...
> 
> I don't see the difference between not finding the proper nfs_client
> in the pg_authenticate method and not finding it after decode in
> CB_SEQUENCE.

In the NFSv4.1 case, the client callback server knows that the
connection is valid, 'cos we're the ones who set it up. All we care
about is to make sure the session is still valid. If it isn't, then
NFS4ERR_BADSESSION is the correct reply.

NFSv4.0 is an altogether different kettle of fish since we need to
authenticate the connection too.

Trond

Re: [PATCH 3/3] NFS return an rpc auth error on back channel

[William A. (Andy) Adamson]

On Thu, Nov 18, 2010 at 10:05 AM, Trond Myklebust
<trond.myklebust@fys.uio.no> wrote:
> On Thu, 2010-11-18 at 09:42 -0500, William A. (Andy) Adamson wrote:
>> On Wed, Nov 17, 2010 at 6:26 PM, Trond Myklebust
>> <trond.myklebust@fys.uio.no> wrote:
>> > On Tue, 2010-11-16 at 22:36 -0500, andros@netapp.com wrote:
>> >
>> > In NFSv4.0, you basically want to set the nfs_client in
>> > nfs_callback_compound() (using the server's address and the
>> > 'callback_ident' argument).
>>
>> And if the nfs_client is not found should we SVC_DROP the request?
>> NFS4ERR_BADHANDLE?
>
> We can still drop the request in nfs4_callback_compound().

OK

>
>> >
>> > In NFSv4.1, you need to set it in the OP_SEQUENCE decode callback, but
>> > there you need to be returning NFS4ERR_BADSESSION and/or
>> > NFS4ERR_CONN_NOT_BOUND_TO_SESSION anyway...
>>
>> I don't see the difference between not finding the proper nfs_client
>> in the pg_authenticate method and not finding it after decode in
>> CB_SEQUENCE.
>
> In the NFSv4.1 case, the client callback server knows that the
> connection is valid, 'cos we're the ones who set it up. All we care
> about is to make sure the session is still valid. If it isn't, then
> NFS4ERR_BADSESSION is the correct reply.
>
> NFSv4.0 is an altogether different kettle of fish since we need to
> authenticate the connection too.

Got it. I'll put out a version 2.

-->Andy
>
> Trond
>
>

Re: [PATCH 1/3] NFS add minorversion to nfs_find_client search

[Trond Myklebust]

On Tue, 2010-11-16 at 22:36 -0500, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> The v4.0 and v4.1 callback threads share a pg_authenticate method.
> Minorversions do not share an nfs_client.
> 
> Respond with an rpc auth error (SVC_DENIED) if the nfs_client matching the
> server address, nfs version, and minorversion is not found.

No. We should simply drop the request.

> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/callback.c      |   19 +++++++++++++------
>  fs/nfs/callback.h      |    4 ++--
>  fs/nfs/callback_proc.c |    8 ++++----
>  fs/nfs/client.c        |   16 +++++++++++-----
>  fs/nfs/internal.h      |    2 +-
>  5 files changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index aeec017..962c5de 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -17,9 +17,7 @@
>  #include <linux/freezer.h>
>  #include <linux/kthread.h>
>  #include <linux/sunrpc/svcauth_gss.h>
> -#if defined(CONFIG_NFS_V4_1)
>  #include <linux/sunrpc/bc_xprt.h>
> -#endif
>  
>  #include <net/inet_sock.h>
>  
> @@ -346,19 +344,28 @@ static int check_gss_callback_principal(struct nfs_client *clp,
>  	return SVC_OK;
>  }
>  
> +/*
> + * Lookup the nfs_client that corresponds to this backchannel request.
> + *
> + * We only support NFSv4.1 callbacks on the fore channel connection, so
> + * the svc_is_backchannel test indicates which minorversion nfs_client we are
> + * searching for.
> + */
>  static int nfs_callback_authenticate(struct svc_rqst *rqstp)
>  {
>  	struct nfs_client *clp;
>  	RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]);
> +	u32 minorversion = svc_is_backchannel(rqstp) ? 1 : 0;

Hmm... Nope. minorversion == CB_COMPOUND4args->minorversion.

>  	int ret = SVC_OK;
>  
>  	/* Don't talk to strangers */
> -	clp = nfs_find_client(svc_addr(rqstp), 4);
> +	clp = nfs_find_client(svc_addr(rqstp), 4, minorversion);

Why do we need this at all in the NFSv4.1 case? Unlike minor version ==
0, we _know_ that it arrived on a socket that is associated to a
specific session. Can't we find a way to pass that information down to
the callback server thread?

>  	if (clp == NULL)
> -		return SVC_DROP;
> +		return SVC_DENIED;

Nope. SVC_DROP is correct. We shouldn't reply to unsolicited callbacks
at all...

>  
> -	dprintk("%s: %s NFSv4 callback!\n", __func__,
> -			svc_print_addr(rqstp, buf, sizeof(buf)));
> +	dprintk("%s: %s NFSv4 callback! minorversion %d\n", __func__,
> +			svc_print_addr(rqstp, buf, sizeof(buf)),
> +			clp->cl_minorversion);
>  
>  	switch (rqstp->rq_authop->flavour) {
>  		case RPC_AUTH_NULL:
> diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
> index b16dd1f..b69bec5 100644
> --- a/fs/nfs/callback.h
> +++ b/fs/nfs/callback.h
> @@ -177,7 +177,7 @@ static inline void put_session_client(struct nfs4_session *session)
>  static inline struct nfs_client *
>  find_client_from_cps(struct cb_process_state *cps, struct sockaddr *addr)
>  {
> -	return cps->session ? cps->session->clp : nfs_find_client(addr, 4);
> +	return cps->session ? cps->session->clp : nfs_find_client(addr, 4, 0);
>  }

This is ugly. If we can save session info in 'struct cb_process_state',
then why can't we save the NFSv4.0 client too?

>  
>  #else /* CONFIG_NFS_V4_1 */
> @@ -189,7 +189,7 @@ static inline void nfs_client_return_layouts(struct nfs_client *clp)
>  static inline struct nfs_client *
>  find_client_from_cps(struct cb_process_state *cps, struct sockaddr *addr)
>  {
> -	return nfs_find_client(addr, 4);
> +	return nfs_find_client(addr, 4, 0);
>  }
>  
>  static inline void put_session_client(struct nfs4_session *session)
> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> index b4c68e9..69d085a 100644
> --- a/fs/nfs/callback_proc.c
> +++ b/fs/nfs/callback_proc.c
> @@ -505,13 +505,13 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
>   * Returns NULL if there are no connections with sessions, or if no session
>   * matches the one of interest.
>   */
> - static struct nfs_client *find_client_with_session(
> -	const struct sockaddr *addr, u32 nfsversion,
> -	struct nfs4_sessionid *sessionid)
> +static struct nfs_client *
> +find_client_with_session(const struct sockaddr *addr, u32 nfsversion,
> +			 struct nfs4_sessionid *sessionid)
>  {
>  	struct nfs_client *clp;
>  
> -	clp = nfs_find_client(addr, 4);
> +	clp = nfs_find_client(addr, 4, 1);

We pass 'u32 nfsversion' as an argument, yet we hand code the
nfs_find_client version and minor version arguments?

>  	if (clp == NULL)
>  		return NULL;
>  
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index dbf43e7..ba7712c 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -371,7 +371,8 @@ EXPORT_SYMBOL(nfs_sockaddr_cmp);
>   * Find a client by IP address and protocol version
>   * - returns NULL if no such client
>   */
> -struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion)
> +struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion,
> +				   u32 minorversion)
>  {
>  	struct nfs_client *clp;
>  
> @@ -385,7 +386,8 @@ struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion)
>  			continue;
>  
>  		/* Different NFS versions cannot share the same nfs_client */
> -		if (clp->rpc_ops->version != nfsversion)
> +		if (clp->rpc_ops->version != nfsversion ||
> +		    clp->cl_minorversion != minorversion)
>  			continue;
>  
>  		/* Match only the IP address, not the port number */
> @@ -401,13 +403,16 @@ struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion)
>  }
>  
>  /*
> - * Find a client by IP address and protocol version
> - * - returns NULL if no such client
> + * Callback service RPC layer pg_authenticate method.
> + *
> + * Find a client by IP address, protocol version, and minorversion.
> + * Returns NULL if no such client
>   */
>  struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
>  {
>  	struct sockaddr *sap = (struct sockaddr *)&clp->cl_addr;
>  	u32 nfsvers = clp->rpc_ops->version;
> +	u32 minorversion = clp->cl_minorversion;
>  
>  	spin_lock(&nfs_client_lock);
>  	list_for_each_entry_continue(clp, &nfs_client_list, cl_share_link) {
> @@ -418,7 +423,8 @@ struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
>  			continue;
>  
>  		/* Different NFS versions cannot share the same nfs_client */
> -		if (clp->rpc_ops->version != nfsvers)
> +		if (clp->rpc_ops->version != nfsvers ||
> +		    clp->cl_minorversion != minorversion)
>  			continue;
>  
>  		/* Match only the IP address, not the port number */
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 0a9ea58..d89aded 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -131,7 +131,7 @@ extern void nfs_umount(const struct nfs_mount_request *info);
>  extern struct rpc_program nfs_program;
>  
>  extern void nfs_put_client(struct nfs_client *);
> -extern struct nfs_client *nfs_find_client(const struct sockaddr *, u32);
> +extern struct nfs_client *nfs_find_client(const struct sockaddr *, u32, u32);
>  extern struct nfs_client *nfs_find_client_next(struct nfs_client *);
>  extern struct nfs_server *nfs_create_server(
>  					const struct nfs_parsed_mount_data *,

Re: [PATCH 1/3] NFS add minorversion to nfs_find_client search

[William A. (Andy) Adamson]

On Wed, Nov 17, 2010 at 6:10 PM, Trond Myklebust
<trond.myklebust@fys.uio.no> wrote:
> On Tue, 2010-11-16 at 22:36 -0500, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> The v4.0 and v4.1 callback threads share a pg_authenticate method.
>> Minorversions do not share an nfs_client.
>>
>> Respond with an rpc auth error (SVC_DENIED) if the nfs_client matching the
>> server address, nfs version, and minorversion is not found.
>
> No. We should simply drop the request.

See comments below.

>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>>  fs/nfs/callback.c      |   19 +++++++++++++------
>>  fs/nfs/callback.h      |    4 ++--
>>  fs/nfs/callback_proc.c |    8 ++++----
>>  fs/nfs/client.c        |   16 +++++++++++-----
>>  fs/nfs/internal.h      |    2 +-
>>  5 files changed, 31 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
>> index aeec017..962c5de 100644
>> --- a/fs/nfs/callback.c
>> +++ b/fs/nfs/callback.c
>> @@ -17,9 +17,7 @@
>>  #include <linux/freezer.h>
>>  #include <linux/kthread.h>
>>  #include <linux/sunrpc/svcauth_gss.h>
>> -#if defined(CONFIG_NFS_V4_1)
>>  #include <linux/sunrpc/bc_xprt.h>
>> -#endif
>>
>>  #include <net/inet_sock.h>
>>
>> @@ -346,19 +344,28 @@ static int check_gss_callback_principal(struct nfs_client *clp,
>>       return SVC_OK;
>>  }
>>
>> +/*
>> + * Lookup the nfs_client that corresponds to this backchannel request.
>> + *
>> + * We only support NFSv4.1 callbacks on the fore channel connection, so
>> + * the svc_is_backchannel test indicates which minorversion nfs_client we are
>> + * searching for.
>> + */
>>  static int nfs_callback_authenticate(struct svc_rqst *rqstp)
>>  {
>>       struct nfs_client *clp;
>>       RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]);
>> +     u32 minorversion = svc_is_backchannel(rqstp) ? 1 : 0;
>
> Hmm... Nope. minorversion == CB_COMPOUND4args->minorversion.

Nope. We are in the pg_authenticate method called from
svc_process_common in the RPC layer. We know nothing in the NFS layer.
We won't know what CB_COMPOUND4args says until after decode.

What's ugly is that the session information really belongs in the RPC layer.

The other way I was thinking of coding this is to not share the
svc_program for v4 and v4.1 callback services. Each svc_program can
then declare it's own pg_authenticate method which will know if it's
v4.0 or v4.1.

>
>>       int ret = SVC_OK;
>>
>>       /* Don't talk to strangers */
>> -     clp = nfs_find_client(svc_addr(rqstp), 4);
>> +     clp = nfs_find_client(svc_addr(rqstp), 4, minorversion);
>
> Why do we need this at all in the NFSv4.1 case? Unlike minor version ==
> 0, we _know_ that it arrived on a socket that is associated to a
> specific session. Can't we find a way to pass that information down to
> the callback server thread?

As long as we only support a single session to a server, and a single
back channel using the fore channel connection, doesn't using the
minorversion has the same effect? I believe there is only one
nfs_client struct associated with the <rqstp->rq_addr, nfsversion,
minorversion> tuple.  By adding the minorversion, nfs_find_client now
uses the same criteria as nfs_match_client which decides when to
create a new nfs_client or use an existing one.

Knowing which session is associated with the callback thread won't do
us any good here where we have yet to decode the request session from
CB_SEQUENCE.

>
>>       if (clp == NULL)
>> -             return SVC_DROP;
>> +             return SVC_DENIED;
>
> Nope. SVC_DROP is correct. We shouldn't reply to unsolicited callbacks
> at all...

Should nfsd also do the same? In nfsd's pg_authenticate routine
svcauth_gss_set_client, if the auth domain is not found - which I
think is the same as not finding a matching nfs_client in the callback
server, SVC_DENIED is returned.   The other nfsd pg_authenticate
routine, svcauth_unix_set_client,  also returns SVC_DENIED when the
ip_map_cache or lookup fails. So, nfsd replys with an rpc auth error
to unsolicited requests.

>
>>
>> -     dprintk("%s: %s NFSv4 callback!\n", __func__,
>> -                     svc_print_addr(rqstp, buf, sizeof(buf)));
>> +     dprintk("%s: %s NFSv4 callback! minorversion %d\n", __func__,
>> +                     svc_print_addr(rqstp, buf, sizeof(buf)),
>> +                     clp->cl_minorversion);
>>
>>       switch (rqstp->rq_authop->flavour) {
>>               case RPC_AUTH_NULL:
>> diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
>> index b16dd1f..b69bec5 100644
>> --- a/fs/nfs/callback.h
>> +++ b/fs/nfs/callback.h
>> @@ -177,7 +177,7 @@ static inline void put_session_client(struct nfs4_session *session)
>>  static inline struct nfs_client *
>>  find_client_from_cps(struct cb_process_state *cps, struct sockaddr *addr)
>>  {
>> -     return cps->session ? cps->session->clp : nfs_find_client(addr, 4);
>> +     return cps->session ? cps->session->clp : nfs_find_client(addr, 4, 0);
>>  }
>
> This is ugly. If we can save session info in 'struct cb_process_state',
> then why can't we save the NFSv4.0 client too?

We can.  It will only be used if both CB_GETATTR and CB_RECALL appear
in the same CB_COMPOUND, and there is no ordering. But I agree, we
should save the nfs_client and dereference it in
nfs4_callback_compound at the end of processing.

>
>>
>>  #else /* CONFIG_NFS_V4_1 */
>> @@ -189,7 +189,7 @@ static inline void nfs_client_return_layouts(struct nfs_client *clp)
>>  static inline struct nfs_client *
>>  find_client_from_cps(struct cb_process_state *cps, struct sockaddr *addr)
>>  {
>> -     return nfs_find_client(addr, 4);
>> +     return nfs_find_client(addr, 4, 0);
>>  }
>>
>>  static inline void put_session_client(struct nfs4_session *session)
>> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
>> index b4c68e9..69d085a 100644
>> --- a/fs/nfs/callback_proc.c
>> +++ b/fs/nfs/callback_proc.c
>> @@ -505,13 +505,13 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
>>   * Returns NULL if there are no connections with sessions, or if no session
>>   * matches the one of interest.
>>   */
>> - static struct nfs_client *find_client_with_session(
>> -     const struct sockaddr *addr, u32 nfsversion,
>> -     struct nfs4_sessionid *sessionid)
>> +static struct nfs_client *
>> +find_client_with_session(const struct sockaddr *addr, u32 nfsversion,
>> +                      struct nfs4_sessionid *sessionid)
>>  {
>>       struct nfs_client *clp;
>>
>> -     clp = nfs_find_client(addr, 4);
>> +     clp = nfs_find_client(addr, 4, 1);
>
> We pass 'u32 nfsversion' as an argument, yet we hand code the
> nfs_find_client version and minor version arguments?

Yes - we should not pass the nfsversion.

>
>>       if (clp == NULL)
>>               return NULL;
>>
>> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
>> index dbf43e7..ba7712c 100644
>> --- a/fs/nfs/client.c
>> +++ b/fs/nfs/client.c
>> @@ -371,7 +371,8 @@ EXPORT_SYMBOL(nfs_sockaddr_cmp);
>>   * Find a client by IP address and protocol version
>>   * - returns NULL if no such client
>>   */
>> -struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion)
>> +struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion,
>> +                                u32 minorversion)
>>  {
>>       struct nfs_client *clp;
>>
>> @@ -385,7 +386,8 @@ struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion)
>>                       continue;
>>
>>               /* Different NFS versions cannot share the same nfs_client */
>> -             if (clp->rpc_ops->version != nfsversion)
>> +             if (clp->rpc_ops->version != nfsversion ||
>> +                 clp->cl_minorversion != minorversion)
>>                       continue;
>>
>>               /* Match only the IP address, not the port number */
>> @@ -401,13 +403,16 @@ struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion)
>>  }
>>
>>  /*
>> - * Find a client by IP address and protocol version
>> - * - returns NULL if no such client
>> + * Callback service RPC layer pg_authenticate method.
>> + *
>> + * Find a client by IP address, protocol version, and minorversion.
>> + * Returns NULL if no such client
>>   */
>>  struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
>>  {
>>       struct sockaddr *sap = (struct sockaddr *)&clp->cl_addr;
>>       u32 nfsvers = clp->rpc_ops->version;
>> +     u32 minorversion = clp->cl_minorversion;
>>
>>       spin_lock(&nfs_client_lock);
>>       list_for_each_entry_continue(clp, &nfs_client_list, cl_share_link) {
>> @@ -418,7 +423,8 @@ struct nfs_client *nfs_find_client_next(struct nfs_client *clp)
>>                       continue;
>>
>>               /* Different NFS versions cannot share the same nfs_client */
>> -             if (clp->rpc_ops->version != nfsvers)
>> +             if (clp->rpc_ops->version != nfsvers ||
>> +                 clp->cl_minorversion != minorversion)
>>                       continue;
>>
>>               /* Match only the IP address, not the port number */
>> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
>> index 0a9ea58..d89aded 100644
>> --- a/fs/nfs/internal.h
>> +++ b/fs/nfs/internal.h
>> @@ -131,7 +131,7 @@ extern void nfs_umount(const struct nfs_mount_request *info);
>>  extern struct rpc_program nfs_program;
>>
>>  extern void nfs_put_client(struct nfs_client *);
>> -extern struct nfs_client *nfs_find_client(const struct sockaddr *, u32);
>> +extern struct nfs_client *nfs_find_client(const struct sockaddr *, u32, u32);
>>  extern struct nfs_client *nfs_find_client_next(struct nfs_client *);
>>  extern struct nfs_server *nfs_create_server(
>>                                       const struct nfs_parsed_mount_data *,
>
>
> --
> 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
>

Re: [PATCH 1/3] NFS add minorversion to nfs_find_client search

[Trond Myklebust]

On Thu, 2010-11-18 at 09:11 -0500, William A. (Andy) Adamson wrote:
> On Wed, Nov 17, 2010 at 6:10 PM, Trond Myklebust
> <trond.myklebust@fys.uio.no> wrote:
> > On Tue, 2010-11-16 at 22:36 -0500, andros@netapp.com wrote:
> >>  static int nfs_callback_authenticate(struct svc_rqst *rqstp)
> >>  {
> >>       struct nfs_client *clp;
> >>       RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]);
> >> +     u32 minorversion = svc_is_backchannel(rqstp) ? 1 : 0;
> >
> > Hmm... Nope. minorversion == CB_COMPOUND4args->minorversion.
> 
> Nope. We are in the pg_authenticate method called from
> svc_process_common in the RPC layer. We know nothing in the NFS layer.
> We won't know what CB_COMPOUND4args says until after decode.

My point is we shouldn't call it 'minorversion', 'cos it isn't...

> What's ugly is that the session information really belongs in the RPC layer.
> 
> The other way I was thinking of coding this is to not share the
> svc_program for v4 and v4.1 callback services. Each svc_program can
> then declare it's own pg_authenticate method which will know if it's
> v4.0 or v4.1.

As I said below: you _know_ which socket this came from, so you know if
it is a session backchannel or not. That's all you care about here.

> >
> >>       int ret = SVC_OK;
> >>
> >>       /* Don't talk to strangers */
> >> -     clp = nfs_find_client(svc_addr(rqstp), 4);
> >> +     clp = nfs_find_client(svc_addr(rqstp), 4, minorversion);
> >
> > Why do we need this at all in the NFSv4.1 case? Unlike minor version ==
> > 0, we _know_ that it arrived on a socket that is associated to a
> > specific session. Can't we find a way to pass that information down to
> > the callback server thread?
> 
> As long as we only support a single session to a server, and a single
> back channel using the fore channel connection, doesn't using the
> minorversion has the same effect? I believe there is only one
> nfs_client struct associated with the <rqstp->rq_addr, nfsversion,
> minorversion> tuple.  By adding the minorversion, nfs_find_client now
> uses the same criteria as nfs_match_client which decides when to
> create a new nfs_client or use an existing one.

The 'as long as we only support' bit is exactly the problem. We
shouldn't assume that we have 1 session per server because in the
long-term that isn't going to be the case.

> Knowing which session is associated with the callback thread won't do
> us any good here where we have yet to decode the request session from
> CB_SEQUENCE.

Then we shouldn't be caring too much about whether or not the server is
talking minor version 0 or 1 here. Do it when we know more about the
CB_COMPOUND.

> >
> >>       if (clp == NULL)
> >> -             return SVC_DROP;
> >> +             return SVC_DENIED;
> >
> > Nope. SVC_DROP is correct. We shouldn't reply to unsolicited callbacks
> > at all...
> 
> Should nfsd also do the same? In nfsd's pg_authenticate routine
> svcauth_gss_set_client, if the auth domain is not found - which I
> think is the same as not finding a matching nfs_client in the callback
> server, SVC_DENIED is returned.   The other nfsd pg_authenticate
> routine, svcauth_unix_set_client,  also returns SVC_DENIED when the
> ip_map_cache or lookup fails. So, nfsd replys with an rpc auth error
> to unsolicited requests.

I can see why the NFS server would care to let the client quickly
whether or not the RPC request is denied, but why do we care on the
backchannel case? If a server is sending us callbacks, and we don't
recognise that server, why should we waste computing and networking
cycles by replying at all?

Trond

Re: [PATCH 1/3] NFS add minorversion to nfs_find_client search

[J. Bruce Fields]

On Thu, Nov 18, 2010 at 09:46:02AM -0500, Trond Myklebust wrote:
> I can see why the NFS server would care to let the client quickly
> whether or not the RPC request is denied, but why do we care on the
> backchannel case? If a server is sending us callbacks, and we don't
> recognise that server, why should we waste computing and networking
> cycles by replying at all?

Agreed.  I have a hard time seeing a real benefit to returning an error
here.

Also, note the only reason to use pg_authenticate is if you want to
return an rpc-level error.

Not that it's a problem to do the work here in pg_authenticate if you
want to, but if it's easier to just allow the request through and let
the nfs layer decide what to do with it, that shouldn't be a problem
either.

--b.

Re: [PATCH 0/3] NFSv4 callback pg_authenticate fix

[J. Bruce Fields]

On Tue, Nov 16, 2010 at 10:36:27PM -0500, andros@netapp.com wrote:
> The back channel server svc_process_common RPC layer pg_authenticate call
> [nfs_callback_authenticate] is shared by both the NFSv4.0 and the NFSv4.1
> callback threads. It authenticates the incoming request by finding (and
> referencing) an nfs_client struct based on the incoming request address
> and the NFS version (4). This is akin to the NFS server which authenticates
> requests by matching the server address to the exports file client list.
> 
> Since there is no minorversion in the search, it may find the wrong
> nfs_client struct.

What really matters to a sessions client is which connection the request
came over--if a server attempts to open a new connection back to a
client and send a 4.1 callback over it, it'd probably be best if the
client just refused that callback, even if it's from the right IP
address.

Would it be better to match cl_rpcclient->cl_xprt against rqstp->rq_xprt
somehow?  (Or maybe just check that the port number matches as well as
the ip address?)

--b.

> For the nfsv4.0 callback service thread, this means it
> could find an NFSv4.1 nfs_client. For the NFSv4.1 callback service thread, it
> could find an NFSv4.0 instead of v4.1, or find an NFSv4.1 nfs_client with the
> wrong session.
> 
> The nfs_client is dereferenced at the end of pg_authenticate. Another
> nfs_find_client call is done in the per operation  dispatcher routines
> for NFSv4.0 and in the cb_sequence operation dispatcher routine for NFSv4.1
> after decoding.
> 
> This means the callback server could start processing a callback, passing
> the pg_authenticate test, and have the nfs_client struct freed between the
> pg_authenticate call and the dispatcher operation call. Or, it could have
> found the wrong nfs_client in the pg_authenticate call.
> 
> The current code has this behaviour: If the nfs_client is not found in
> pg_authenticate, the request is simply dropped (SVC_DROP). If an nfs_client
> is not found in the dispatcher routines NFS4ERR_BADSESSION is returned for
> v4.1 requests and NFS4ERR_BADHANDLE for v4.0 requests.
> 
> The first patch fixes some of these issues. It adds a minorversion to the
> nfs_find_client routine which means that the NFSv4.0 back channel service always
> finds the correct nfs_client struct. The NFSv4.1 back channel service requires
> the sessionid from the CB_SEQUENCE operation to guarantee that the correct
> nfs_client struct is being used.
> 
> The minorversion is assigned by checking for the existance of the
> bc_xprt field in the svc_rqst. This method of determining minorversion will
> work until we support stand alone back channels for NFSv4.1.
> 
> The SVC_DROP return in pg_authenticate is changed to SVC_DENIED which sends
> an rpc auth error back to the caller. This matches nfsd which returns
> SVC_DENIED if there is no matching client address in the /etc/exports file.
> SVC_DROP is used for a memory allocation error.
> 
> 0001-NFS-add-minorversion-to-nfs_find_client-search.patch
> 0002-SQUASHME-pnfs-submit-fix-highest-backchannel-slot-us.patch
> 
> Both NFSv4.0 CB_RECALL and NFSv4.1 CB_LAYOUT_RECALL have been tested.
> 
> This third patch is for comment only. It compiles but has not been tested.
> It returns SVC_DENIED if the dispatcher routines fail to find an
> nfs_client struct - replacing the NFS4ERR_BADSESSION and NFS4ERR_BADHANDLE
> currently returned. I'm not fully convinced that this is the behavior we want,
> comments appreciated.
> 
> 0003-NFS-return-an-rpc-auth-error-on-back-channel.patch
> 
> -->Andy
> 
> --
> 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