[PATCH 00/27] Remaining IPv6 NSM patches for 2.6.29

[PATCH 00/27] Remaining IPv6 NSM patches for 2.6.29

[Chuck Lever]

Hi Bruce-

A little light reading for the weekend.

These are all the IPv6-related patches for NSM I have.  I'll post the
NFSD patches next week.

The first six patches convert the NSM client's upcalls to use xdr_stream
based XDR encoders and decoders.  I figure we won't be back in this area
for a while, but eventually we will want all the client XDR functions to
be based on xdr_stream.  So while this is still fresh in my mind, I did
the conversion.

The next thirteen patches reorganize the kernel's NSM implementation to
allow us to replace the current contents of NSMPROC_MON's "priv"
argument with something not based on an IP address.

Then one patch that does that replacement.

This is followed by four clean up patches, and the final three patches
take the wraps off IPv6 support for lockd.

---

Chuck Lever (27):
      lockd: Enable NLM use of AF_INET6
      NLM: Rewrite IPv4 privileged requester's check
      NLM: nlm_privileged_requester() doesn't recognize mapped loopback address
      NSM: Move nsm_create()
      NSM: Move nsm_use_hostnames to mon.c
      NSM: Move nsm_addr() to fs/lockd/mon.c
      NSM: Remove include/linux/lockd/sm_inter.h
      NSM: Replace IP address as our nlm_reboot lookup key
      NSM: More clean up of nsm_get_handle()
      NSM: Refactor nsm_handle creation into a helper function
      NLM: Remove "create" argument from nsm_find()
      NLM: Call nsm_reboot_lookup() instead of nsm_find()
      NSM: Add nsm_lookup() function
      NLM: Decode "priv" argument of NLMPROC_SM_NOTIFY as an opaque
      NLM: Change nlm_host_rebooted() to take a single nlm_reboot argument
      NSM: Encode the new "priv" cookie for NSMPROC_MON requests
      NSM: Generate NSMPROC_MON's "priv" argument when nsm_handle is created
      NSM: Remove !nsm check from nsm_release()
      NSM: Remove NULL pointer check from nsm_find()
      NSM: Add dprintk() calls in nsm_find and nsm_release
      NSM: Move nsm_find() to fs/lockd/mon.c
      NSM: Remove unused old-style XDR functions
      NSM: Switch over to the new-style XDR functions
      NSM: Add xdr_stream-based XDR encoders
      NSM: Add xdr_stream-based XDR encoders
      NSM: Move NSM program and procedure numbers to fs/lockd/mon.c
      NSM: Move NSM-related XDR data structures to lockd's xdr.h


 fs/lockd/clntproc.c            |    1 
 fs/lockd/host.c                |  168 +-------------
 fs/lockd/mon.c                 |  491 ++++++++++++++++++++++++++++++++--------
 fs/lockd/svc.c                 |   15 +
 fs/lockd/svc4proc.c            |   13 -
 fs/lockd/svcproc.c             |   13 -
 fs/lockd/svcsubs.c             |    1 
 fs/lockd/xdr.c                 |    5 
 fs/lockd/xdr4.c                |    5 
 include/linux/lockd/lockd.h    |   39 ++-
 include/linux/lockd/sm_inter.h |   46 ----
 include/linux/lockd/xdr.h      |   15 +
 12 files changed, 457 insertions(+), 355 deletions(-)
 delete mode 100644 include/linux/lockd/sm_inter.h

-- 
Chuck Lever

[PATCH 01/27] NSM: Move NSM-related XDR data structures to lockd's xdr.h

[Chuck Lever]

Clean up: NSM's XDR data structures are used only in fs/lockd/mon.c,
so move them there.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c                 |   14 ++++++++++++++
 include/linux/lockd/sm_inter.h |   20 --------------------
 2 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 3bb71e1..8130883 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -18,6 +18,20 @@
 
 #define NLMDBG_FACILITY		NLMDBG_MONITOR
 
+struct nsm_args {
+	__be32			addr;		/* remote address */
+	u32			prog;		/* RPC callback info */
+	u32			vers;
+	u32			proc;
+
+	char			*mon_name;
+};
+
+struct nsm_res {
+	u32			status;
+	u32			state;
+};
+
 static struct rpc_clnt *	nsm_create(void);
 
 static struct rpc_program	nsm_program;
diff --git a/include/linux/lockd/sm_inter.h b/include/linux/lockd/sm_inter.h
index 896a5e3..dd9d8a5 100644
--- a/include/linux/lockd/sm_inter.h
+++ b/include/linux/lockd/sm_inter.h
@@ -21,26 +21,6 @@
 #define SM_MAXSTRLEN	1024
 #define SM_PRIV_SIZE	16
 
-/*
- * Arguments for all calls to statd
- */
-struct nsm_args {
-	__be32		addr;		/* remote address */
-	u32		prog;		/* RPC callback info */
-	u32		vers;
-	u32		proc;
-
-	char *		mon_name;
-};
-
-/*
- * Result returned by statd
- */
-struct nsm_res {
-	u32		status;
-	u32		state;
-};
-
 extern int	nsm_local_state;
 
 #endif /* LINUX_LOCKD_SM_INTER_H */

[PATCH 02/27] NSM: Move NSM program and procedure numbers to fs/lockd/mon.c

[Chuck Lever]

Clean up: Move the RPC program and procedure numbers for NSM into the
one source file that needs them: fs/lockd/mon.c.

And, as with NLM, NFS, and rpcbind calls, use NSMPROC_FOO instead of
SM_FOO for NSM procedure numbers.

Finally, make a couple of comments more precise: what is referred to
here as SM_NOTIFY is really the NLM (lockd) NLMPROC_SM_NOTIFY downcall,
not NSMPROC_NOTIFY.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c                 |   42 ++++++++++++++++++++++++++--------------
 include/linux/lockd/sm_inter.h |    9 ---------
 2 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 8130883..0fc9836 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -17,6 +17,18 @@
 
 
 #define NLMDBG_FACILITY		NLMDBG_MONITOR
+#define NSM_PROGRAM		100024
+#define NSM_VERSION		1
+
+enum {
+	NSMPROC_NULL,
+	NSMPROC_STAT,
+	NSMPROC_MON,
+	NSMPROC_UNMON,
+	NSMPROC_UNMON_ALL,
+	NSMPROC_SIMU_CRASH,
+	NSMPROC_NOTIFY,
+};
 
 struct nsm_args {
 	__be32			addr;		/* remote address */
@@ -42,7 +54,7 @@ static struct rpc_program	nsm_program;
 int				nsm_local_state;
 
 /*
- * Common procedure for SM_MON/SM_UNMON calls
+ * Common procedure for NSMPROC_MON/NSMPROC_UNMON calls
  */
 static int
 nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
@@ -111,7 +123,7 @@ int nsm_monitor(const struct nlm_host *host)
 	 */
 	nsm->sm_mon_name = nsm_use_hostnames ? nsm->sm_name : nsm->sm_addrbuf;
 
-	status = nsm_mon_unmon(nsm, SM_MON, &res);
+	status = nsm_mon_unmon(nsm, NSMPROC_MON, &res);
 	if (res.status != 0)
 		status = -EIO;
 	if (status < 0)
@@ -139,7 +151,7 @@ void nsm_unmonitor(const struct nlm_host *host)
 	 && nsm->sm_monitored && !nsm->sm_sticky) {
 		dprintk("lockd: nsm_unmonitor(%s)\n", nsm->sm_name);
 
-		status = nsm_mon_unmon(nsm, SM_UNMON, &res);
+		status = nsm_mon_unmon(nsm, NSMPROC_UNMON, &res);
 		if (res.status != 0)
 			status = -EIO;
 		if (status < 0)
@@ -167,7 +179,7 @@ nsm_create(void)
 		.addrsize	= sizeof(sin),
 		.servername	= "localhost",
 		.program	= &nsm_program,
-		.version	= SM_VERSION,
+		.version	= NSM_VERSION,
 		.authflavor	= RPC_AUTH_NULL,
 	};
 
@@ -201,7 +213,7 @@ static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
 /*
  * The "my_id" argument specifies the hostname and RPC procedure
  * to be called when the status manager receives notification
- * (via the SM_NOTIFY call) that the state of host "mon_name"
+ * (via the NLMPROC_SM_NOTIFY call) that the state of host "mon_name"
  * has changed.
  */
 static __be32 *xdr_encode_my_id(__be32 *p, struct nsm_args *argp)
@@ -219,7 +231,7 @@ static __be32 *xdr_encode_my_id(__be32 *p, struct nsm_args *argp)
 
 /*
  * The "mon_id" argument specifies the non-private arguments
- * of an SM_MON or SM_UNMON call.
+ * of an NSMPROC_MON or NSMPROC_UNMON call.
  */
 static __be32 *xdr_encode_mon_id(__be32 *p, struct nsm_args *argp)
 {
@@ -232,8 +244,8 @@ static __be32 *xdr_encode_mon_id(__be32 *p, struct nsm_args *argp)
 
 /*
  * The "priv" argument may contain private information required
- * by the SM_MON call. This information will be supplied in the
- * SM_NOTIFY call.
+ * by the NSMPROC_MON call. This information will be supplied in the
+ * NLMPROC_SM_NOTIFY call.
  *
  * Linux provides the raw IP address of the monitored host,
  * left in network byte order.
@@ -300,22 +312,22 @@ xdr_decode_stat(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
 #define SM_unmonres_sz	1
 
 static struct rpc_procinfo	nsm_procedures[] = {
-[SM_MON] = {
-		.p_proc		= SM_MON,
+[NSMPROC_MON] = {
+		.p_proc		= NSMPROC_MON,
 		.p_encode	= (kxdrproc_t) xdr_encode_mon,
 		.p_decode	= (kxdrproc_t) xdr_decode_stat_res,
 		.p_arglen	= SM_mon_sz,
 		.p_replen	= SM_monres_sz,
-		.p_statidx	= SM_MON,
+		.p_statidx	= NSMPROC_MON,
 		.p_name		= "MONITOR",
 	},
-[SM_UNMON] = {
-		.p_proc		= SM_UNMON,
+[NSMPROC_UNMON] = {
+		.p_proc		= NSMPROC_UNMON,
 		.p_encode	= (kxdrproc_t) xdr_encode_unmon,
 		.p_decode	= (kxdrproc_t) xdr_decode_stat,
 		.p_arglen	= SM_mon_id_sz,
 		.p_replen	= SM_unmonres_sz,
-		.p_statidx	= SM_UNMON,
+		.p_statidx	= NSMPROC_UNMON,
 		.p_name		= "UNMONITOR",
 	},
 };
@@ -334,7 +346,7 @@ static struct rpc_stat		nsm_stats;
 
 static struct rpc_program	nsm_program = {
 		.name		= "statd",
-		.number		= SM_PROGRAM,
+		.number		= NSM_PROGRAM,
 		.nrvers		= ARRAY_SIZE(nsm_version),
 		.version	= nsm_version,
 		.stats		= &nsm_stats
diff --git a/include/linux/lockd/sm_inter.h b/include/linux/lockd/sm_inter.h
index dd9d8a5..116bf38 100644
--- a/include/linux/lockd/sm_inter.h
+++ b/include/linux/lockd/sm_inter.h
@@ -9,15 +9,6 @@
 #ifndef LINUX_LOCKD_SM_INTER_H
 #define LINUX_LOCKD_SM_INTER_H
 
-#define SM_PROGRAM	100024
-#define SM_VERSION	1
-#define SM_STAT		1
-#define SM_MON		2
-#define SM_UNMON	3
-#define SM_UNMON_ALL	4
-#define SM_SIMU_CRASH	5
-#define SM_NOTIFY	6
-
 #define SM_MAXSTRLEN	1024
 #define SM_PRIV_SIZE	16
 

Re: [PATCH 02/27] NSM: Move NSM program and procedure numbers to fs/lockd/mon.c

[J. Bruce Fields]

On Fri, Dec 05, 2008 at 07:02:07PM -0500, Chuck Lever wrote:
> Clean up: Move the RPC program and procedure numbers for NSM into the
> one source file that needs them: fs/lockd/mon.c.
> 
> And, as with NLM, NFS, and rpcbind calls, use NSMPROC_FOO instead of
> SM_FOO for NSM procedure numbers.
> 
> Finally, make a couple of comments more precise: what is referred to
> here as SM_NOTIFY is really the NLM (lockd) NLMPROC_SM_NOTIFY downcall,
> not NSMPROC_NOTIFY.

Yep, the notify downcalls are confusing to sort out; anything to help
clarify is useful.

Applied these first two patches, thanks.

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/mon.c                 |   42 ++++++++++++++++++++++++++--------------
>  include/linux/lockd/sm_inter.h |    9 ---------
>  2 files changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index 8130883..0fc9836 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -17,6 +17,18 @@
>  
>  
>  #define NLMDBG_FACILITY		NLMDBG_MONITOR
> +#define NSM_PROGRAM		100024
> +#define NSM_VERSION		1
> +
> +enum {
> +	NSMPROC_NULL,
> +	NSMPROC_STAT,
> +	NSMPROC_MON,
> +	NSMPROC_UNMON,
> +	NSMPROC_UNMON_ALL,
> +	NSMPROC_SIMU_CRASH,
> +	NSMPROC_NOTIFY,
> +};
>  
>  struct nsm_args {
>  	__be32			addr;		/* remote address */
> @@ -42,7 +54,7 @@ static struct rpc_program	nsm_program;
>  int				nsm_local_state;
>  
>  /*
> - * Common procedure for SM_MON/SM_UNMON calls
> + * Common procedure for NSMPROC_MON/NSMPROC_UNMON calls
>   */
>  static int
>  nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
> @@ -111,7 +123,7 @@ int nsm_monitor(const struct nlm_host *host)
>  	 */
>  	nsm->sm_mon_name = nsm_use_hostnames ? nsm->sm_name : nsm->sm_addrbuf;
>  
> -	status = nsm_mon_unmon(nsm, SM_MON, &res);
> +	status = nsm_mon_unmon(nsm, NSMPROC_MON, &res);
>  	if (res.status != 0)
>  		status = -EIO;
>  	if (status < 0)
> @@ -139,7 +151,7 @@ void nsm_unmonitor(const struct nlm_host *host)
>  	 && nsm->sm_monitored && !nsm->sm_sticky) {
>  		dprintk("lockd: nsm_unmonitor(%s)\n", nsm->sm_name);
>  
> -		status = nsm_mon_unmon(nsm, SM_UNMON, &res);
> +		status = nsm_mon_unmon(nsm, NSMPROC_UNMON, &res);
>  		if (res.status != 0)
>  			status = -EIO;
>  		if (status < 0)
> @@ -167,7 +179,7 @@ nsm_create(void)
>  		.addrsize	= sizeof(sin),
>  		.servername	= "localhost",
>  		.program	= &nsm_program,
> -		.version	= SM_VERSION,
> +		.version	= NSM_VERSION,
>  		.authflavor	= RPC_AUTH_NULL,
>  	};
>  
> @@ -201,7 +213,7 @@ static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
>  /*
>   * The "my_id" argument specifies the hostname and RPC procedure
>   * to be called when the status manager receives notification
> - * (via the SM_NOTIFY call) that the state of host "mon_name"
> + * (via the NLMPROC_SM_NOTIFY call) that the state of host "mon_name"
>   * has changed.
>   */
>  static __be32 *xdr_encode_my_id(__be32 *p, struct nsm_args *argp)
> @@ -219,7 +231,7 @@ static __be32 *xdr_encode_my_id(__be32 *p, struct nsm_args *argp)
>  
>  /*
>   * The "mon_id" argument specifies the non-private arguments
> - * of an SM_MON or SM_UNMON call.
> + * of an NSMPROC_MON or NSMPROC_UNMON call.
>   */
>  static __be32 *xdr_encode_mon_id(__be32 *p, struct nsm_args *argp)
>  {
> @@ -232,8 +244,8 @@ static __be32 *xdr_encode_mon_id(__be32 *p, struct nsm_args *argp)
>  
>  /*
>   * The "priv" argument may contain private information required
> - * by the SM_MON call. This information will be supplied in the
> - * SM_NOTIFY call.
> + * by the NSMPROC_MON call. This information will be supplied in the
> + * NLMPROC_SM_NOTIFY call.
>   *
>   * Linux provides the raw IP address of the monitored host,
>   * left in network byte order.
> @@ -300,22 +312,22 @@ xdr_decode_stat(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
>  #define SM_unmonres_sz	1
>  
>  static struct rpc_procinfo	nsm_procedures[] = {
> -[SM_MON] = {
> -		.p_proc		= SM_MON,
> +[NSMPROC_MON] = {
> +		.p_proc		= NSMPROC_MON,
>  		.p_encode	= (kxdrproc_t) xdr_encode_mon,
>  		.p_decode	= (kxdrproc_t) xdr_decode_stat_res,
>  		.p_arglen	= SM_mon_sz,
>  		.p_replen	= SM_monres_sz,
> -		.p_statidx	= SM_MON,
> +		.p_statidx	= NSMPROC_MON,
>  		.p_name		= "MONITOR",
>  	},
> -[SM_UNMON] = {
> -		.p_proc		= SM_UNMON,
> +[NSMPROC_UNMON] = {
> +		.p_proc		= NSMPROC_UNMON,
>  		.p_encode	= (kxdrproc_t) xdr_encode_unmon,
>  		.p_decode	= (kxdrproc_t) xdr_decode_stat,
>  		.p_arglen	= SM_mon_id_sz,
>  		.p_replen	= SM_unmonres_sz,
> -		.p_statidx	= SM_UNMON,
> +		.p_statidx	= NSMPROC_UNMON,
>  		.p_name		= "UNMONITOR",
>  	},
>  };
> @@ -334,7 +346,7 @@ static struct rpc_stat		nsm_stats;
>  
>  static struct rpc_program	nsm_program = {
>  		.name		= "statd",
> -		.number		= SM_PROGRAM,
> +		.number		= NSM_PROGRAM,
>  		.nrvers		= ARRAY_SIZE(nsm_version),
>  		.version	= nsm_version,
>  		.stats		= &nsm_stats
> diff --git a/include/linux/lockd/sm_inter.h b/include/linux/lockd/sm_inter.h
> index dd9d8a5..116bf38 100644
> --- a/include/linux/lockd/sm_inter.h
> +++ b/include/linux/lockd/sm_inter.h
> @@ -9,15 +9,6 @@
>  #ifndef LINUX_LOCKD_SM_INTER_H
>  #define LINUX_LOCKD_SM_INTER_H
>  
> -#define SM_PROGRAM	100024
> -#define SM_VERSION	1
> -#define SM_STAT		1
> -#define SM_MON		2
> -#define SM_UNMON	3
> -#define SM_UNMON_ALL	4
> -#define SM_SIMU_CRASH	5
> -#define SM_NOTIFY	6
> -
>  #define SM_MAXSTRLEN	1024
>  #define SM_PRIV_SIZE	16
>  
> 

[PATCH 03/27] NSM: Add xdr_stream-based XDR encoders

[Chuck Lever]

Introduce xdr_stream-based XDR encoder functions.  These are more
careful about preventing RPC buffer overflows.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 82 insertions(+), 0 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 0fc9836..cf85c0f 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -202,6 +202,20 @@ static __be32 *xdr_encode_nsm_string(__be32 *p, char *string)
 	return xdr_encode_opaque(p, string, len);
 }
 
+static int encode_nsm_string(struct xdr_stream *xdr, const char *string)
+{
+	const u32 len = strlen(string);
+	__be32 *p;
+
+	if (unlikely(len > SM_MAXSTRLEN))
+		return -EIO;
+	p = xdr_reserve_space(xdr, sizeof(u32) + len);
+	if (unlikely(p == NULL))
+		return -EIO;
+	xdr_encode_opaque(p, string, len);
+	return 0;
+}
+
 /*
  * "mon_name" specifies the host to be monitored.
  */
@@ -210,6 +224,11 @@ static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
 	return xdr_encode_nsm_string(p, argp->mon_name);
 }
 
+static int encode_mon_name(struct xdr_stream *xdr, const struct nsm_args *argp)
+{
+	return encode_nsm_string(xdr, argp->mon_name);
+}
+
 /*
  * The "my_id" argument specifies the hostname and RPC procedure
  * to be called when the status manager receives notification
@@ -229,6 +248,23 @@ static __be32 *xdr_encode_my_id(__be32 *p, struct nsm_args *argp)
 	return p;
 }
 
+static int encode_my_id(struct xdr_stream *xdr, const struct nsm_args *argp)
+{
+	int status;
+	__be32 *p;
+
+	status = encode_nsm_string(xdr, utsname()->nodename);
+	if (unlikely(status != 0))
+		return status;
+	p = xdr_reserve_space(xdr, 3 * sizeof(u32));
+	if (unlikely(p == NULL))
+		return -EIO;
+	*p++ = htonl(argp->prog);
+	*p++ = htonl(argp->vers);
+	*p++ = htonl(argp->proc);
+	return 0;
+}
+
 /*
  * The "mon_id" argument specifies the non-private arguments
  * of an NSMPROC_MON or NSMPROC_UNMON call.
@@ -242,6 +278,16 @@ static __be32 *xdr_encode_mon_id(__be32 *p, struct nsm_args *argp)
 	return xdr_encode_my_id(p, argp);
 }
 
+static int encode_mon_id(struct xdr_stream *xdr, const struct nsm_args *argp)
+{
+	int status;
+
+	status = encode_mon_name(xdr, argp);
+	if (unlikely(status != 0))
+		return status;
+	return encode_my_id(xdr, argp);
+}
+
 /*
  * The "priv" argument may contain private information required
  * by the NSMPROC_MON call. This information will be supplied in the
@@ -260,6 +306,20 @@ static __be32 *xdr_encode_priv(__be32 *p, struct nsm_args *argp)
 	return p;
 }
 
+static int encode_priv(struct xdr_stream *xdr, const struct nsm_args *argp)
+{
+	__be32 *p;
+
+	p = xdr_reserve_space(xdr, SM_PRIV_SIZE);
+	if (unlikely(p == NULL))
+		return -EIO;
+	*p++ = argp->addr;
+	*p++ = 0;
+	*p++ = 0;
+	*p++ = 0;
+	return 0;
+}
+
 static int
 xdr_encode_mon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args *argp)
 {
@@ -275,6 +335,19 @@ xdr_encode_mon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args *argp)
 	return 0;
 }
 
+static int xdr_enc_mon(struct rpc_rqst *req, __be32 *p,
+		       const struct nsm_args *argp)
+{
+	struct xdr_stream xdr;
+	int status;
+
+	xdr_init_encode(&xdr, &req->rq_snd_buf, p);
+	status = encode_mon_id(&xdr, argp);
+	if (unlikely(status))
+		return status;
+	return encode_priv(&xdr, argp);
+}
+
 static int
 xdr_encode_unmon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args *argp)
 {
@@ -285,6 +358,15 @@ xdr_encode_unmon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args *argp)
 	return 0;
 }
 
+static int xdr_enc_unmon(struct rpc_rqst *req, __be32 *p,
+			 const struct nsm_args *argp)
+{
+	struct xdr_stream xdr;
+
+	xdr_init_encode(&xdr, &req->rq_snd_buf, p);
+	return encode_mon_id(&xdr, argp);
+}
+
 static int
 xdr_decode_stat_res(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
 {

Re: [PATCH 03/27] NSM: Add xdr_stream-based XDR encoders

[J. Bruce Fields]

On Fri, Dec 05, 2008 at 07:02:15PM -0500, Chuck Lever wrote:
> Introduce xdr_stream-based XDR encoder functions.  These are more
> careful about preventing RPC buffer overflows.

Looks fine, thanks; applied, with a superficial patch reorganization:
for once this is a patch I think is too small.

My two rules of thumb:

	- Callers for new code should be added at the same time as the
	  new code.

	  (Otherwise it's not possible to judge the single patch on its
	  own, because you don't know whether the new (temporarily dead)
	  code is correct until you see it called.)

	- When replacing code, I'd rather see the new code added in the
	  same patch that removes the old code.

	  (Again, it's easier to judge the single patch on its own if
	  you see the side-by-side old-to-new comparison.)

Neither rule is an absolute.

In this case I think squashing the following four patches gives a
reasonable result; I've applied the below (literally just the
composition of the four patches) instead of 3--7.

(If we were to split this up more finely, I'd consider doing encoders
separately from decoders, and/or SM_MON separately from SM_UNMON.)

--b.

commit fc88c50d16bbc43ab6a90107acbf45a0654d57ee
Author: Chuck Lever <chuck.lever@oracle.com>
Date:   Fri Dec 5 19:02:15 2008 -0500

    NSM: move to xdr_stream-based XDR encoders and decoders
    
    Introduce xdr_stream-based XDR encoder and decoder functions, which are
    more careful about preventing RPC buffer overflows.
    
    Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 0fc9836..81e1cc1 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -193,21 +193,26 @@ nsm_create(void)
  * Status Monitor wire protocol.
  */
 
-static __be32 *xdr_encode_nsm_string(__be32 *p, char *string)
+static int encode_nsm_string(struct xdr_stream *xdr, const char *string)
 {
-	size_t len = strlen(string);
-
-	if (len > SM_MAXSTRLEN)
-		len = SM_MAXSTRLEN;
-	return xdr_encode_opaque(p, string, len);
+	const u32 len = strlen(string);
+	__be32 *p;
+
+	if (unlikely(len > SM_MAXSTRLEN))
+		return -EIO;
+	p = xdr_reserve_space(xdr, sizeof(u32) + len);
+	if (unlikely(p == NULL))
+		return -EIO;
+	xdr_encode_opaque(p, string, len);
+	return 0;
 }
 
 /*
  * "mon_name" specifies the host to be monitored.
  */
-static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
+static int encode_mon_name(struct xdr_stream *xdr, const struct nsm_args *argp)
 {
-	return xdr_encode_nsm_string(p, argp->mon_name);
+	return encode_nsm_string(xdr, argp->mon_name);
 }
 
 /*
@@ -216,30 +221,35 @@ static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
  * (via the NLMPROC_SM_NOTIFY call) that the state of host "mon_name"
  * has changed.
  */
-static __be32 *xdr_encode_my_id(__be32 *p, struct nsm_args *argp)
+static int encode_my_id(struct xdr_stream *xdr, const struct nsm_args *argp)
 {
-	p = xdr_encode_nsm_string(p, utsname()->nodename);
-	if (!p)
-		return ERR_PTR(-EIO);
-
+	int status;
+	__be32 *p;
+
+	status = encode_nsm_string(xdr, utsname()->nodename);
+	if (unlikely(status != 0))
+		return status;
+	p = xdr_reserve_space(xdr, 3 * sizeof(u32));
+	if (unlikely(p == NULL))
+		return -EIO;
 	*p++ = htonl(argp->prog);
 	*p++ = htonl(argp->vers);
 	*p++ = htonl(argp->proc);
-
-	return p;
+	return 0;
 }
 
 /*
  * The "mon_id" argument specifies the non-private arguments
  * of an NSMPROC_MON or NSMPROC_UNMON call.
  */
-static __be32 *xdr_encode_mon_id(__be32 *p, struct nsm_args *argp)
+static int encode_mon_id(struct xdr_stream *xdr, const struct nsm_args *argp)
 {
-	p = xdr_encode_mon_name(p, argp);
-	if (!p)
-		return ERR_PTR(-EIO);
+	int status;
 
-	return xdr_encode_my_id(p, argp);
+	status = encode_mon_name(xdr, argp);
+	if (unlikely(status != 0))
+		return status;
+	return encode_my_id(xdr, argp);
 }
 
 /*
@@ -250,55 +260,71 @@ static __be32 *xdr_encode_mon_id(__be32 *p, struct nsm_args *argp)
  * Linux provides the raw IP address of the monitored host,
  * left in network byte order.
  */
-static __be32 *xdr_encode_priv(__be32 *p, struct nsm_args *argp)
+static int encode_priv(struct xdr_stream *xdr, const struct nsm_args *argp)
 {
+	__be32 *p;
+
+	p = xdr_reserve_space(xdr, SM_PRIV_SIZE);
+	if (unlikely(p == NULL))
+		return -EIO;
 	*p++ = argp->addr;
 	*p++ = 0;
 	*p++ = 0;
 	*p++ = 0;
-
-	return p;
+	return 0;
 }
 
-static int
-xdr_encode_mon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args *argp)
+static int xdr_enc_mon(struct rpc_rqst *req, __be32 *p,
+		       const struct nsm_args *argp)
 {
-	p = xdr_encode_mon_id(p, argp);
-	if (IS_ERR(p))
-		return PTR_ERR(p);
-
-	p = xdr_encode_priv(p, argp);
-	if (IS_ERR(p))
-		return PTR_ERR(p);
+	struct xdr_stream xdr;
+	int status;
 
-	rqstp->rq_slen = xdr_adjust_iovec(rqstp->rq_svec, p);
-	return 0;
+	xdr_init_encode(&xdr, &req->rq_snd_buf, p);
+	status = encode_mon_id(&xdr, argp);
+	if (unlikely(status))
+		return status;
+	return encode_priv(&xdr, argp);
 }
 
-static int
-xdr_encode_unmon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args *argp)
+static int xdr_enc_unmon(struct rpc_rqst *req, __be32 *p,
+			 const struct nsm_args *argp)
 {
-	p = xdr_encode_mon_id(p, argp);
-	if (IS_ERR(p))
-		return PTR_ERR(p);
-	rqstp->rq_slen = xdr_adjust_iovec(rqstp->rq_svec, p);
-	return 0;
+	struct xdr_stream xdr;
+
+	xdr_init_encode(&xdr, &req->rq_snd_buf, p);
+	return encode_mon_id(&xdr, argp);
 }
 
-static int
-xdr_decode_stat_res(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
+static int xdr_dec_stat_res(struct rpc_rqst *rqstp, __be32 *p,
+			    struct nsm_res *resp)
 {
+	struct xdr_stream xdr;
+
+	xdr_init_decode(&xdr, &rqstp->rq_rcv_buf, p);
+	p = xdr_inline_decode(&xdr, 2 * sizeof(u32));
+	if (unlikely(p == NULL))
+		return -EIO;
 	resp->status = ntohl(*p++);
-	resp->state = ntohl(*p++);
-	dprintk("nsm: xdr_decode_stat_res status %d state %d\n",
+	resp->state = ntohl(*p);
+
+	dprintk("lockd: xdr_dec_stat_res status %d state %d\n",
 			resp->status, resp->state);
 	return 0;
 }
 
-static int
-xdr_decode_stat(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
+static int xdr_dec_stat(struct rpc_rqst *rqstp, __be32 *p,
+			struct nsm_res *resp)
 {
-	resp->state = ntohl(*p++);
+	struct xdr_stream xdr;
+
+	xdr_init_decode(&xdr, &rqstp->rq_rcv_buf, p);
+	p = xdr_inline_decode(&xdr, sizeof(u32));
+	if (unlikely(p == NULL))
+		return -EIO;
+	resp->state = ntohl(*p);
+
+	dprintk("lockd: xdr_dec_stat state %d\n", resp->state);
 	return 0;
 }
 
@@ -314,8 +340,8 @@ xdr_decode_stat(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
 static struct rpc_procinfo	nsm_procedures[] = {
 [NSMPROC_MON] = {
 		.p_proc		= NSMPROC_MON,
-		.p_encode	= (kxdrproc_t) xdr_encode_mon,
-		.p_decode	= (kxdrproc_t) xdr_decode_stat_res,
+		.p_encode	= (kxdrproc_t)xdr_enc_mon,
+		.p_decode	= (kxdrproc_t)xdr_dec_stat_res,
 		.p_arglen	= SM_mon_sz,
 		.p_replen	= SM_monres_sz,
 		.p_statidx	= NSMPROC_MON,
@@ -323,8 +349,8 @@ static struct rpc_procinfo	nsm_procedures[] = {
 	},
 [NSMPROC_UNMON] = {
 		.p_proc		= NSMPROC_UNMON,
-		.p_encode	= (kxdrproc_t) xdr_encode_unmon,
-		.p_decode	= (kxdrproc_t) xdr_decode_stat,
+		.p_encode	= (kxdrproc_t)xdr_enc_unmon,
+		.p_decode	= (kxdrproc_t)xdr_dec_stat,
 		.p_arglen	= SM_mon_id_sz,
 		.p_replen	= SM_unmonres_sz,
 		.p_statidx	= NSMPROC_UNMON,

Re: [PATCH 03/27] NSM: Add xdr_stream-based XDR encoders

[J. Bruce Fields]

On Wed, Dec 10, 2008 at 03:35:33PM -0500, bfields wrote:
> On Fri, Dec 05, 2008 at 07:02:15PM -0500, Chuck Lever wrote:
> > Introduce xdr_stream-based XDR encoder functions.  These are more
> > careful about preventing RPC buffer overflows.
> 
> Looks fine, thanks; applied, with a superficial patch reorganization:
> for once this is a patch I think is too small.
> 
> My two rules of thumb:
> 
> 	- Callers for new code should be added at the same time as the
> 	  new code.
> 
> 	  (Otherwise it's not possible to judge the single patch on its
> 	  own, because you don't know whether the new (temporarily dead)
> 	  code is correct until you see it called.)
> 
> 	- When replacing code, I'd rather see the new code added in the
> 	  same patch that removes the old code.
> 
> 	  (Again, it's easier to judge the single patch on its own if
> 	  you see the side-by-side old-to-new comparison.)
> 
> Neither rule is an absolute.
> 
> In this case I think squashing the following four patches gives a
> reasonable result; I've applied the below (literally just the
> composition of the four patches) instead of 3--7.

(Oops, I meant 3--6.)

--b.

> 
> (If we were to split this up more finely, I'd consider doing encoders
> separately from decoders, and/or SM_MON separately from SM_UNMON.)
> 
> --b.
> 
> commit fc88c50d16bbc43ab6a90107acbf45a0654d57ee
> Author: Chuck Lever <chuck.lever@oracle.com>
> Date:   Fri Dec 5 19:02:15 2008 -0500
> 
>     NSM: move to xdr_stream-based XDR encoders and decoders
>     
>     Introduce xdr_stream-based XDR encoder and decoder functions, which are
>     more careful about preventing RPC buffer overflows.
>     
>     Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>     Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> 
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index 0fc9836..81e1cc1 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -193,21 +193,26 @@ nsm_create(void)
>   * Status Monitor wire protocol.
>   */
>  
> -static __be32 *xdr_encode_nsm_string(__be32 *p, char *string)
> +static int encode_nsm_string(struct xdr_stream *xdr, const char *string)
>  {
> -	size_t len = strlen(string);
> -
> -	if (len > SM_MAXSTRLEN)
> -		len = SM_MAXSTRLEN;
> -	return xdr_encode_opaque(p, string, len);
> +	const u32 len = strlen(string);
> +	__be32 *p;
> +
> +	if (unlikely(len > SM_MAXSTRLEN))
> +		return -EIO;
> +	p = xdr_reserve_space(xdr, sizeof(u32) + len);
> +	if (unlikely(p == NULL))
> +		return -EIO;
> +	xdr_encode_opaque(p, string, len);
> +	return 0;
>  }
>  
>  /*
>   * "mon_name" specifies the host to be monitored.
>   */
> -static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
> +static int encode_mon_name(struct xdr_stream *xdr, const struct nsm_args *argp)
>  {
> -	return xdr_encode_nsm_string(p, argp->mon_name);
> +	return encode_nsm_string(xdr, argp->mon_name);
>  }
>  
>  /*
> @@ -216,30 +221,35 @@ static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
>   * (via the NLMPROC_SM_NOTIFY call) that the state of host "mon_name"
>   * has changed.
>   */
> -static __be32 *xdr_encode_my_id(__be32 *p, struct nsm_args *argp)
> +static int encode_my_id(struct xdr_stream *xdr, const struct nsm_args *argp)
>  {
> -	p = xdr_encode_nsm_string(p, utsname()->nodename);
> -	if (!p)
> -		return ERR_PTR(-EIO);
> -
> +	int status;
> +	__be32 *p;
> +
> +	status = encode_nsm_string(xdr, utsname()->nodename);
> +	if (unlikely(status != 0))
> +		return status;
> +	p = xdr_reserve_space(xdr, 3 * sizeof(u32));
> +	if (unlikely(p == NULL))
> +		return -EIO;
>  	*p++ = htonl(argp->prog);
>  	*p++ = htonl(argp->vers);
>  	*p++ = htonl(argp->proc);
> -
> -	return p;
> +	return 0;
>  }
>  
>  /*
>   * The "mon_id" argument specifies the non-private arguments
>   * of an NSMPROC_MON or NSMPROC_UNMON call.
>   */
> -static __be32 *xdr_encode_mon_id(__be32 *p, struct nsm_args *argp)
> +static int encode_mon_id(struct xdr_stream *xdr, const struct nsm_args *argp)
>  {
> -	p = xdr_encode_mon_name(p, argp);
> -	if (!p)
> -		return ERR_PTR(-EIO);
> +	int status;
>  
> -	return xdr_encode_my_id(p, argp);
> +	status = encode_mon_name(xdr, argp);
> +	if (unlikely(status != 0))
> +		return status;
> +	return encode_my_id(xdr, argp);
>  }
>  
>  /*
> @@ -250,55 +260,71 @@ static __be32 *xdr_encode_mon_id(__be32 *p, struct nsm_args *argp)
>   * Linux provides the raw IP address of the monitored host,
>   * left in network byte order.
>   */
> -static __be32 *xdr_encode_priv(__be32 *p, struct nsm_args *argp)
> +static int encode_priv(struct xdr_stream *xdr, const struct nsm_args *argp)
>  {
> +	__be32 *p;
> +
> +	p = xdr_reserve_space(xdr, SM_PRIV_SIZE);
> +	if (unlikely(p == NULL))
> +		return -EIO;
>  	*p++ = argp->addr;
>  	*p++ = 0;
>  	*p++ = 0;
>  	*p++ = 0;
> -
> -	return p;
> +	return 0;
>  }
>  
> -static int
> -xdr_encode_mon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args *argp)
> +static int xdr_enc_mon(struct rpc_rqst *req, __be32 *p,
> +		       const struct nsm_args *argp)
>  {
> -	p = xdr_encode_mon_id(p, argp);
> -	if (IS_ERR(p))
> -		return PTR_ERR(p);
> -
> -	p = xdr_encode_priv(p, argp);
> -	if (IS_ERR(p))
> -		return PTR_ERR(p);
> +	struct xdr_stream xdr;
> +	int status;
>  
> -	rqstp->rq_slen = xdr_adjust_iovec(rqstp->rq_svec, p);
> -	return 0;
> +	xdr_init_encode(&xdr, &req->rq_snd_buf, p);
> +	status = encode_mon_id(&xdr, argp);
> +	if (unlikely(status))
> +		return status;
> +	return encode_priv(&xdr, argp);
>  }
>  
> -static int
> -xdr_encode_unmon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args *argp)
> +static int xdr_enc_unmon(struct rpc_rqst *req, __be32 *p,
> +			 const struct nsm_args *argp)
>  {
> -	p = xdr_encode_mon_id(p, argp);
> -	if (IS_ERR(p))
> -		return PTR_ERR(p);
> -	rqstp->rq_slen = xdr_adjust_iovec(rqstp->rq_svec, p);
> -	return 0;
> +	struct xdr_stream xdr;
> +
> +	xdr_init_encode(&xdr, &req->rq_snd_buf, p);
> +	return encode_mon_id(&xdr, argp);
>  }
>  
> -static int
> -xdr_decode_stat_res(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
> +static int xdr_dec_stat_res(struct rpc_rqst *rqstp, __be32 *p,
> +			    struct nsm_res *resp)
>  {
> +	struct xdr_stream xdr;
> +
> +	xdr_init_decode(&xdr, &rqstp->rq_rcv_buf, p);
> +	p = xdr_inline_decode(&xdr, 2 * sizeof(u32));
> +	if (unlikely(p == NULL))
> +		return -EIO;
>  	resp->status = ntohl(*p++);
> -	resp->state = ntohl(*p++);
> -	dprintk("nsm: xdr_decode_stat_res status %d state %d\n",
> +	resp->state = ntohl(*p);
> +
> +	dprintk("lockd: xdr_dec_stat_res status %d state %d\n",
>  			resp->status, resp->state);
>  	return 0;
>  }
>  
> -static int
> -xdr_decode_stat(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
> +static int xdr_dec_stat(struct rpc_rqst *rqstp, __be32 *p,
> +			struct nsm_res *resp)
>  {
> -	resp->state = ntohl(*p++);
> +	struct xdr_stream xdr;
> +
> +	xdr_init_decode(&xdr, &rqstp->rq_rcv_buf, p);
> +	p = xdr_inline_decode(&xdr, sizeof(u32));
> +	if (unlikely(p == NULL))
> +		return -EIO;
> +	resp->state = ntohl(*p);
> +
> +	dprintk("lockd: xdr_dec_stat state %d\n", resp->state);
>  	return 0;
>  }
>  
> @@ -314,8 +340,8 @@ xdr_decode_stat(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
>  static struct rpc_procinfo	nsm_procedures[] = {
>  [NSMPROC_MON] = {
>  		.p_proc		= NSMPROC_MON,
> -		.p_encode	= (kxdrproc_t) xdr_encode_mon,
> -		.p_decode	= (kxdrproc_t) xdr_decode_stat_res,
> +		.p_encode	= (kxdrproc_t)xdr_enc_mon,
> +		.p_decode	= (kxdrproc_t)xdr_dec_stat_res,
>  		.p_arglen	= SM_mon_sz,
>  		.p_replen	= SM_monres_sz,
>  		.p_statidx	= NSMPROC_MON,
> @@ -323,8 +349,8 @@ static struct rpc_procinfo	nsm_procedures[] = {
>  	},
>  [NSMPROC_UNMON] = {
>  		.p_proc		= NSMPROC_UNMON,
> -		.p_encode	= (kxdrproc_t) xdr_encode_unmon,
> -		.p_decode	= (kxdrproc_t) xdr_decode_stat,
> +		.p_encode	= (kxdrproc_t)xdr_enc_unmon,
> +		.p_decode	= (kxdrproc_t)xdr_dec_stat,
>  		.p_arglen	= SM_mon_id_sz,
>  		.p_replen	= SM_unmonres_sz,
>  		.p_statidx	= NSMPROC_UNMON,

Re: [PATCH 03/27] NSM: Add xdr_stream-based XDR encoders

[Chuck Lever]

On Dec 10, 2008, at Dec 10, 2008, 3:35 PM, J. Bruce Fields wrote:
> On Fri, Dec 05, 2008 at 07:02:15PM -0500, Chuck Lever wrote:
>> Introduce xdr_stream-based XDR encoder functions.  These are more
>> careful about preventing RPC buffer overflows.
>
> Looks fine, thanks; applied, with a superficial patch reorganization:
> for once this is a patch I think is too small.
>
> My two rules of thumb:
>
> 	- Callers for new code should be added at the same time as the
> 	  new code.
>
> 	  (Otherwise it's not possible to judge the single patch on its
> 	  own, because you don't know whether the new (temporarily dead)
> 	  code is correct until you see it called.)
>
> 	- When replacing code, I'd rather see the new code added in the
> 	  same patch that removes the old code.
>
> 	  (Again, it's easier to judge the single patch on its own if
> 	  you see the side-by-side old-to-new comparison.)

OK... FYI, I broke it up this way because often when we do both in the  
same patch, the diff is nearly impossible to read.

The kernel patch rules I'm familiar with require that each patch  
should not break the build or kernel operation, and should be as  
atomic as possible.

> Neither rule is an absolute.
>
> In this case I think squashing the following four patches gives a
> reasonable result; I've applied the below (literally just the
> composition of the four patches) instead of 3--7.

This looks OK at first glance.

In general making this kind of alteration means I can't do an  
automated merge when I sync up with 2.6.29.  I will have to check over  
these changes carefully to see that what I sent you is exactly what  
was applied, and delete them each separately.

This is an important reason I prefer to make this kind of change  
myself and resend the patches -- it's one more insulator against  
operator mistakes and bugs in our tools.  I wish there were a better  
way to handle this case, but such are the tools we have been provided.

> (If we were to split this up more finely, I'd consider doing encoders
> separately from decoders, and/or SM_MON separately from SM_UNMON.)
>
> --b.
>
> commit fc88c50d16bbc43ab6a90107acbf45a0654d57ee
> Author: Chuck Lever <chuck.lever@oracle.com>
> Date:   Fri Dec 5 19:02:15 2008 -0500
>
>    NSM: move to xdr_stream-based XDR encoders and decoders
>
>    Introduce xdr_stream-based XDR encoder and decoder functions,  
> which are
>    more careful about preventing RPC buffer overflows.
>
>    Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
>
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index 0fc9836..81e1cc1 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -193,21 +193,26 @@ nsm_create(void)
>  * Status Monitor wire protocol.
>  */
>
> -static __be32 *xdr_encode_nsm_string(__be32 *p, char *string)
> +static int encode_nsm_string(struct xdr_stream *xdr, const char  
> *string)
> {
> -	size_t len = strlen(string);
> -
> -	if (len > SM_MAXSTRLEN)
> -		len = SM_MAXSTRLEN;
> -	return xdr_encode_opaque(p, string, len);
> +	const u32 len = strlen(string);
> +	__be32 *p;
> +
> +	if (unlikely(len > SM_MAXSTRLEN))
> +		return -EIO;
> +	p = xdr_reserve_space(xdr, sizeof(u32) + len);
> +	if (unlikely(p == NULL))
> +		return -EIO;
> +	xdr_encode_opaque(p, string, len);
> +	return 0;
> }
>
> /*
>  * "mon_name" specifies the host to be monitored.
>  */
> -static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
> +static int encode_mon_name(struct xdr_stream *xdr, const struct  
> nsm_args *argp)
> {
> -	return xdr_encode_nsm_string(p, argp->mon_name);
> +	return encode_nsm_string(xdr, argp->mon_name);
> }
>
> /*
> @@ -216,30 +221,35 @@ static __be32 *xdr_encode_mon_name(__be32 *p,  
> struct nsm_args *argp)
>  * (via the NLMPROC_SM_NOTIFY call) that the state of host "mon_name"
>  * has changed.
>  */
> -static __be32 *xdr_encode_my_id(__be32 *p, struct nsm_args *argp)
> +static int encode_my_id(struct xdr_stream *xdr, const struct  
> nsm_args *argp)
> {
> -	p = xdr_encode_nsm_string(p, utsname()->nodename);
> -	if (!p)
> -		return ERR_PTR(-EIO);
> -
> +	int status;
> +	__be32 *p;
> +
> +	status = encode_nsm_string(xdr, utsname()->nodename);
> +	if (unlikely(status != 0))
> +		return status;
> +	p = xdr_reserve_space(xdr, 3 * sizeof(u32));
> +	if (unlikely(p == NULL))
> +		return -EIO;
> 	*p++ = htonl(argp->prog);
> 	*p++ = htonl(argp->vers);
> 	*p++ = htonl(argp->proc);
> -
> -	return p;
> +	return 0;
> }
>
> /*
>  * The "mon_id" argument specifies the non-private arguments
>  * of an NSMPROC_MON or NSMPROC_UNMON call.
>  */
> -static __be32 *xdr_encode_mon_id(__be32 *p, struct nsm_args *argp)
> +static int encode_mon_id(struct xdr_stream *xdr, const struct  
> nsm_args *argp)
> {
> -	p = xdr_encode_mon_name(p, argp);
> -	if (!p)
> -		return ERR_PTR(-EIO);
> +	int status;
>
> -	return xdr_encode_my_id(p, argp);
> +	status = encode_mon_name(xdr, argp);
> +	if (unlikely(status != 0))
> +		return status;
> +	return encode_my_id(xdr, argp);
> }
>
> /*
> @@ -250,55 +260,71 @@ static __be32 *xdr_encode_mon_id(__be32 *p,  
> struct nsm_args *argp)
>  * Linux provides the raw IP address of the monitored host,
>  * left in network byte order.
>  */
> -static __be32 *xdr_encode_priv(__be32 *p, struct nsm_args *argp)
> +static int encode_priv(struct xdr_stream *xdr, const struct  
> nsm_args *argp)
> {
> +	__be32 *p;
> +
> +	p = xdr_reserve_space(xdr, SM_PRIV_SIZE);
> +	if (unlikely(p == NULL))
> +		return -EIO;
> 	*p++ = argp->addr;
> 	*p++ = 0;
> 	*p++ = 0;
> 	*p++ = 0;
> -
> -	return p;
> +	return 0;
> }
>
> -static int
> -xdr_encode_mon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args  
> *argp)
> +static int xdr_enc_mon(struct rpc_rqst *req, __be32 *p,
> +		       const struct nsm_args *argp)
> {
> -	p = xdr_encode_mon_id(p, argp);
> -	if (IS_ERR(p))
> -		return PTR_ERR(p);
> -
> -	p = xdr_encode_priv(p, argp);
> -	if (IS_ERR(p))
> -		return PTR_ERR(p);
> +	struct xdr_stream xdr;
> +	int status;
>
> -	rqstp->rq_slen = xdr_adjust_iovec(rqstp->rq_svec, p);
> -	return 0;
> +	xdr_init_encode(&xdr, &req->rq_snd_buf, p);
> +	status = encode_mon_id(&xdr, argp);
> +	if (unlikely(status))
> +		return status;
> +	return encode_priv(&xdr, argp);
> }
>
> -static int
> -xdr_encode_unmon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args  
> *argp)
> +static int xdr_enc_unmon(struct rpc_rqst *req, __be32 *p,
> +			 const struct nsm_args *argp)
> {
> -	p = xdr_encode_mon_id(p, argp);
> -	if (IS_ERR(p))
> -		return PTR_ERR(p);
> -	rqstp->rq_slen = xdr_adjust_iovec(rqstp->rq_svec, p);
> -	return 0;
> +	struct xdr_stream xdr;
> +
> +	xdr_init_encode(&xdr, &req->rq_snd_buf, p);
> +	return encode_mon_id(&xdr, argp);
> }
>
> -static int
> -xdr_decode_stat_res(struct rpc_rqst *rqstp, __be32 *p, struct  
> nsm_res *resp)
> +static int xdr_dec_stat_res(struct rpc_rqst *rqstp, __be32 *p,
> +			    struct nsm_res *resp)
> {
> +	struct xdr_stream xdr;
> +
> +	xdr_init_decode(&xdr, &rqstp->rq_rcv_buf, p);
> +	p = xdr_inline_decode(&xdr, 2 * sizeof(u32));
> +	if (unlikely(p == NULL))
> +		return -EIO;
> 	resp->status = ntohl(*p++);
> -	resp->state = ntohl(*p++);
> -	dprintk("nsm: xdr_decode_stat_res status %d state %d\n",
> +	resp->state = ntohl(*p);
> +
> +	dprintk("lockd: xdr_dec_stat_res status %d state %d\n",
> 			resp->status, resp->state);
> 	return 0;
> }
>
> -static int
> -xdr_decode_stat(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res  
> *resp)
> +static int xdr_dec_stat(struct rpc_rqst *rqstp, __be32 *p,
> +			struct nsm_res *resp)
> {
> -	resp->state = ntohl(*p++);
> +	struct xdr_stream xdr;
> +
> +	xdr_init_decode(&xdr, &rqstp->rq_rcv_buf, p);
> +	p = xdr_inline_decode(&xdr, sizeof(u32));
> +	if (unlikely(p == NULL))
> +		return -EIO;
> +	resp->state = ntohl(*p);
> +
> +	dprintk("lockd: xdr_dec_stat state %d\n", resp->state);
> 	return 0;
> }
>
> @@ -314,8 +340,8 @@ xdr_decode_stat(struct rpc_rqst *rqstp, __be32  
> *p, struct nsm_res *resp)
> static struct rpc_procinfo	nsm_procedures[] = {
> [NSMPROC_MON] = {
> 		.p_proc		= NSMPROC_MON,
> -		.p_encode	= (kxdrproc_t) xdr_encode_mon,
> -		.p_decode	= (kxdrproc_t) xdr_decode_stat_res,
> +		.p_encode	= (kxdrproc_t)xdr_enc_mon,
> +		.p_decode	= (kxdrproc_t)xdr_dec_stat_res,
> 		.p_arglen	= SM_mon_sz,
> 		.p_replen	= SM_monres_sz,
> 		.p_statidx	= NSMPROC_MON,
> @@ -323,8 +349,8 @@ static struct rpc_procinfo	nsm_procedures[] = {
> 	},
> [NSMPROC_UNMON] = {
> 		.p_proc		= NSMPROC_UNMON,
> -		.p_encode	= (kxdrproc_t) xdr_encode_unmon,
> -		.p_decode	= (kxdrproc_t) xdr_decode_stat,
> +		.p_encode	= (kxdrproc_t)xdr_enc_unmon,
> +		.p_decode	= (kxdrproc_t)xdr_dec_stat,
> 		.p_arglen	= SM_mon_id_sz,
> 		.p_replen	= SM_unmonres_sz,
> 		.p_statidx	= NSMPROC_UNMON,
> --
> 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

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 03/27] NSM: Add xdr_stream-based XDR encoders

[J. Bruce Fields]

On Wed, Dec 10, 2008 at 03:54:43PM -0500, Chuck Lever wrote:
> On Dec 10, 2008, at Dec 10, 2008, 3:35 PM, J. Bruce Fields wrote:
>> On Fri, Dec 05, 2008 at 07:02:15PM -0500, Chuck Lever wrote:
>>> Introduce xdr_stream-based XDR encoder functions.  These are more
>>> careful about preventing RPC buffer overflows.
>>
>> Looks fine, thanks; applied, with a superficial patch reorganization:
>> for once this is a patch I think is too small.
>>
>> My two rules of thumb:
>>
>> 	- Callers for new code should be added at the same time as the
>> 	  new code.
>>
>> 	  (Otherwise it's not possible to judge the single patch on its
>> 	  own, because you don't know whether the new (temporarily dead)
>> 	  code is correct until you see it called.)
>>
>> 	- When replacing code, I'd rather see the new code added in the
>> 	  same patch that removes the old code.
>>
>> 	  (Again, it's easier to judge the single patch on its own if
>> 	  you see the side-by-side old-to-new comparison.)
>
> OK... FYI, I broke it up this way because often when we do both in the  
> same patch, the diff is nearly impossible to read.
>
> The kernel patch rules I'm familiar with require that each patch should 
> not break the build or kernel operation, and should be as atomic as 
> possible.
>
>> Neither rule is an absolute.
>>
>> In this case I think squashing the following four patches gives a
>> reasonable result; I've applied the below (literally just the
>> composition of the four patches) instead of 3--7.
>
> This looks OK at first glance.
>
> In general making this kind of alteration means I can't do an automated 
> merge when I sync up with 2.6.29.  I will have to check over these 
> changes carefully to see that what I sent you is exactly what was 
> applied, and delete them each separately.
>
> This is an important reason I prefer to make this kind of change myself 
> and resend the patches -- it's one more insulator against operator 
> mistakes and bugs in our tools.  I wish there were a better way to handle 
> this case, but such are the tools we have been provided.

Hm.  Since the end result is exactly the same, this should be
mechanically verifiable, but yeah, I'm not sure which (if any) git tool
will do that in one go.

--b.

>
>> (If we were to split this up more finely, I'd consider doing encoders
>> separately from decoders, and/or SM_MON separately from SM_UNMON.)
>>
>> --b.
>>
>> commit fc88c50d16bbc43ab6a90107acbf45a0654d57ee
>> Author: Chuck Lever <chuck.lever@oracle.com>
>> Date:   Fri Dec 5 19:02:15 2008 -0500
>>
>>    NSM: move to xdr_stream-based XDR encoders and decoders
>>
>>    Introduce xdr_stream-based XDR encoder and decoder functions, which 
>> are
>>    more careful about preventing RPC buffer overflows.
>>
>>    Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
>>
>> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
>> index 0fc9836..81e1cc1 100644
>> --- a/fs/lockd/mon.c
>> +++ b/fs/lockd/mon.c
>> @@ -193,21 +193,26 @@ nsm_create(void)
>>  * Status Monitor wire protocol.
>>  */
>>
>> -static __be32 *xdr_encode_nsm_string(__be32 *p, char *string)
>> +static int encode_nsm_string(struct xdr_stream *xdr, const char  
>> *string)
>> {
>> -	size_t len = strlen(string);
>> -
>> -	if (len > SM_MAXSTRLEN)
>> -		len = SM_MAXSTRLEN;
>> -	return xdr_encode_opaque(p, string, len);
>> +	const u32 len = strlen(string);
>> +	__be32 *p;
>> +
>> +	if (unlikely(len > SM_MAXSTRLEN))
>> +		return -EIO;
>> +	p = xdr_reserve_space(xdr, sizeof(u32) + len);
>> +	if (unlikely(p == NULL))
>> +		return -EIO;
>> +	xdr_encode_opaque(p, string, len);
>> +	return 0;
>> }
>>
>> /*
>>  * "mon_name" specifies the host to be monitored.
>>  */
>> -static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
>> +static int encode_mon_name(struct xdr_stream *xdr, const struct  
>> nsm_args *argp)
>> {
>> -	return xdr_encode_nsm_string(p, argp->mon_name);
>> +	return encode_nsm_string(xdr, argp->mon_name);
>> }
>>
>> /*
>> @@ -216,30 +221,35 @@ static __be32 *xdr_encode_mon_name(__be32 *p,  
>> struct nsm_args *argp)
>>  * (via the NLMPROC_SM_NOTIFY call) that the state of host "mon_name"
>>  * has changed.
>>  */
>> -static __be32 *xdr_encode_my_id(__be32 *p, struct nsm_args *argp)
>> +static int encode_my_id(struct xdr_stream *xdr, const struct nsm_args 
>> *argp)
>> {
>> -	p = xdr_encode_nsm_string(p, utsname()->nodename);
>> -	if (!p)
>> -		return ERR_PTR(-EIO);
>> -
>> +	int status;
>> +	__be32 *p;
>> +
>> +	status = encode_nsm_string(xdr, utsname()->nodename);
>> +	if (unlikely(status != 0))
>> +		return status;
>> +	p = xdr_reserve_space(xdr, 3 * sizeof(u32));
>> +	if (unlikely(p == NULL))
>> +		return -EIO;
>> 	*p++ = htonl(argp->prog);
>> 	*p++ = htonl(argp->vers);
>> 	*p++ = htonl(argp->proc);
>> -
>> -	return p;
>> +	return 0;
>> }
>>
>> /*
>>  * The "mon_id" argument specifies the non-private arguments
>>  * of an NSMPROC_MON or NSMPROC_UNMON call.
>>  */
>> -static __be32 *xdr_encode_mon_id(__be32 *p, struct nsm_args *argp)
>> +static int encode_mon_id(struct xdr_stream *xdr, const struct  
>> nsm_args *argp)
>> {
>> -	p = xdr_encode_mon_name(p, argp);
>> -	if (!p)
>> -		return ERR_PTR(-EIO);
>> +	int status;
>>
>> -	return xdr_encode_my_id(p, argp);
>> +	status = encode_mon_name(xdr, argp);
>> +	if (unlikely(status != 0))
>> +		return status;
>> +	return encode_my_id(xdr, argp);
>> }
>>
>> /*
>> @@ -250,55 +260,71 @@ static __be32 *xdr_encode_mon_id(__be32 *p,  
>> struct nsm_args *argp)
>>  * Linux provides the raw IP address of the monitored host,
>>  * left in network byte order.
>>  */
>> -static __be32 *xdr_encode_priv(__be32 *p, struct nsm_args *argp)
>> +static int encode_priv(struct xdr_stream *xdr, const struct nsm_args 
>> *argp)
>> {
>> +	__be32 *p;
>> +
>> +	p = xdr_reserve_space(xdr, SM_PRIV_SIZE);
>> +	if (unlikely(p == NULL))
>> +		return -EIO;
>> 	*p++ = argp->addr;
>> 	*p++ = 0;
>> 	*p++ = 0;
>> 	*p++ = 0;
>> -
>> -	return p;
>> +	return 0;
>> }
>>
>> -static int
>> -xdr_encode_mon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args  
>> *argp)
>> +static int xdr_enc_mon(struct rpc_rqst *req, __be32 *p,
>> +		       const struct nsm_args *argp)
>> {
>> -	p = xdr_encode_mon_id(p, argp);
>> -	if (IS_ERR(p))
>> -		return PTR_ERR(p);
>> -
>> -	p = xdr_encode_priv(p, argp);
>> -	if (IS_ERR(p))
>> -		return PTR_ERR(p);
>> +	struct xdr_stream xdr;
>> +	int status;
>>
>> -	rqstp->rq_slen = xdr_adjust_iovec(rqstp->rq_svec, p);
>> -	return 0;
>> +	xdr_init_encode(&xdr, &req->rq_snd_buf, p);
>> +	status = encode_mon_id(&xdr, argp);
>> +	if (unlikely(status))
>> +		return status;
>> +	return encode_priv(&xdr, argp);
>> }
>>
>> -static int
>> -xdr_encode_unmon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args  
>> *argp)
>> +static int xdr_enc_unmon(struct rpc_rqst *req, __be32 *p,
>> +			 const struct nsm_args *argp)
>> {
>> -	p = xdr_encode_mon_id(p, argp);
>> -	if (IS_ERR(p))
>> -		return PTR_ERR(p);
>> -	rqstp->rq_slen = xdr_adjust_iovec(rqstp->rq_svec, p);
>> -	return 0;
>> +	struct xdr_stream xdr;
>> +
>> +	xdr_init_encode(&xdr, &req->rq_snd_buf, p);
>> +	return encode_mon_id(&xdr, argp);
>> }
>>
>> -static int
>> -xdr_decode_stat_res(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res 
>> *resp)
>> +static int xdr_dec_stat_res(struct rpc_rqst *rqstp, __be32 *p,
>> +			    struct nsm_res *resp)
>> {
>> +	struct xdr_stream xdr;
>> +
>> +	xdr_init_decode(&xdr, &rqstp->rq_rcv_buf, p);
>> +	p = xdr_inline_decode(&xdr, 2 * sizeof(u32));
>> +	if (unlikely(p == NULL))
>> +		return -EIO;
>> 	resp->status = ntohl(*p++);
>> -	resp->state = ntohl(*p++);
>> -	dprintk("nsm: xdr_decode_stat_res status %d state %d\n",
>> +	resp->state = ntohl(*p);
>> +
>> +	dprintk("lockd: xdr_dec_stat_res status %d state %d\n",
>> 			resp->status, resp->state);
>> 	return 0;
>> }
>>
>> -static int
>> -xdr_decode_stat(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res  
>> *resp)
>> +static int xdr_dec_stat(struct rpc_rqst *rqstp, __be32 *p,
>> +			struct nsm_res *resp)
>> {
>> -	resp->state = ntohl(*p++);
>> +	struct xdr_stream xdr;
>> +
>> +	xdr_init_decode(&xdr, &rqstp->rq_rcv_buf, p);
>> +	p = xdr_inline_decode(&xdr, sizeof(u32));
>> +	if (unlikely(p == NULL))
>> +		return -EIO;
>> +	resp->state = ntohl(*p);
>> +
>> +	dprintk("lockd: xdr_dec_stat state %d\n", resp->state);
>> 	return 0;
>> }
>>
>> @@ -314,8 +340,8 @@ xdr_decode_stat(struct rpc_rqst *rqstp, __be32 *p, 
>> struct nsm_res *resp)
>> static struct rpc_procinfo	nsm_procedures[] = {
>> [NSMPROC_MON] = {
>> 		.p_proc		= NSMPROC_MON,
>> -		.p_encode	= (kxdrproc_t) xdr_encode_mon,
>> -		.p_decode	= (kxdrproc_t) xdr_decode_stat_res,
>> +		.p_encode	= (kxdrproc_t)xdr_enc_mon,
>> +		.p_decode	= (kxdrproc_t)xdr_dec_stat_res,
>> 		.p_arglen	= SM_mon_sz,
>> 		.p_replen	= SM_monres_sz,
>> 		.p_statidx	= NSMPROC_MON,
>> @@ -323,8 +349,8 @@ static struct rpc_procinfo	nsm_procedures[] = {
>> 	},
>> [NSMPROC_UNMON] = {
>> 		.p_proc		= NSMPROC_UNMON,
>> -		.p_encode	= (kxdrproc_t) xdr_encode_unmon,
>> -		.p_decode	= (kxdrproc_t) xdr_decode_stat,
>> +		.p_encode	= (kxdrproc_t)xdr_enc_unmon,
>> +		.p_decode	= (kxdrproc_t)xdr_dec_stat,
>> 		.p_arglen	= SM_mon_id_sz,
>> 		.p_replen	= SM_unmonres_sz,
>> 		.p_statidx	= NSMPROC_UNMON,
>> --
>> 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
>
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
>
>
>
>

[PATCH 04/27] NSM: Add xdr_stream-based XDR encoders

[Chuck Lever]

Introduce xdr_stream-based XDR decoder functions.  These are more
careful about preventing RPC buffer overflows.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index cf85c0f..0f8e029 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -377,6 +377,23 @@ xdr_decode_stat_res(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
 	return 0;
 }
 
+static int xdr_dec_stat_res(struct rpc_rqst *rqstp, __be32 *p,
+			    struct nsm_res *resp)
+{
+	struct xdr_stream xdr;
+
+	xdr_init_decode(&xdr, &rqstp->rq_rcv_buf, p);
+	p = xdr_inline_decode(&xdr, 2 * sizeof(u32));
+	if (unlikely(p == NULL))
+		return -EIO;
+	resp->status = ntohl(*p++);
+	resp->state = ntohl(*p);
+
+	dprintk("lockd: xdr_dec_stat_res status %d state %d\n",
+			resp->status, resp->state);
+	return 0;
+}
+
 static int
 xdr_decode_stat(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
 {
@@ -384,6 +401,21 @@ xdr_decode_stat(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
 	return 0;
 }
 
+static int xdr_dec_stat(struct rpc_rqst *rqstp, __be32 *p,
+			struct nsm_res *resp)
+{
+	struct xdr_stream xdr;
+
+	xdr_init_decode(&xdr, &rqstp->rq_rcv_buf, p);
+	p = xdr_inline_decode(&xdr, sizeof(u32));
+	if (unlikely(p == NULL))
+		return -EIO;
+	resp->state = ntohl(*p);
+
+	dprintk("lockd: xdr_dec_stat state %d\n", resp->state);
+	return 0;
+}
+
 #define SM_my_name_sz	(1+XDR_QUADLEN(SM_MAXSTRLEN))
 #define SM_my_id_sz	(SM_my_name_sz+3)
 #define SM_mon_name_sz	(1+XDR_QUADLEN(SM_MAXSTRLEN))

[PATCH 05/27] NSM: Switch over to the new-style XDR functions

[Chuck Lever]

Use the new xdr_stream-based XDR encoder and decoder functions instead of the
old-style functions.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 0f8e029..67cce9f 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -428,8 +428,8 @@ static int xdr_dec_stat(struct rpc_rqst *rqstp, __be32 *p,
 static struct rpc_procinfo	nsm_procedures[] = {
 [NSMPROC_MON] = {
 		.p_proc		= NSMPROC_MON,
-		.p_encode	= (kxdrproc_t) xdr_encode_mon,
-		.p_decode	= (kxdrproc_t) xdr_decode_stat_res,
+		.p_encode	= (kxdrproc_t)xdr_enc_mon,
+		.p_decode	= (kxdrproc_t)xdr_dec_stat_res,
 		.p_arglen	= SM_mon_sz,
 		.p_replen	= SM_monres_sz,
 		.p_statidx	= NSMPROC_MON,
@@ -437,8 +437,8 @@ static struct rpc_procinfo	nsm_procedures[] = {
 	},
 [NSMPROC_UNMON] = {
 		.p_proc		= NSMPROC_UNMON,
-		.p_encode	= (kxdrproc_t) xdr_encode_unmon,
-		.p_decode	= (kxdrproc_t) xdr_decode_stat,
+		.p_encode	= (kxdrproc_t)xdr_enc_unmon,
+		.p_decode	= (kxdrproc_t)xdr_dec_stat,
 		.p_arglen	= SM_mon_id_sz,
 		.p_replen	= SM_unmonres_sz,
 		.p_statidx	= NSMPROC_UNMON,

[PATCH 06/27] NSM: Remove unused old-style XDR functions

[Chuck Lever]

Now that they are no longer used, remove the old-style XDR encoder and
decoder functions.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c |   88 --------------------------------------------------------
 1 files changed, 0 insertions(+), 88 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 67cce9f..81e1cc1 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -193,15 +193,6 @@ nsm_create(void)
  * Status Monitor wire protocol.
  */
 
-static __be32 *xdr_encode_nsm_string(__be32 *p, char *string)
-{
-	size_t len = strlen(string);
-
-	if (len > SM_MAXSTRLEN)
-		len = SM_MAXSTRLEN;
-	return xdr_encode_opaque(p, string, len);
-}
-
 static int encode_nsm_string(struct xdr_stream *xdr, const char *string)
 {
 	const u32 len = strlen(string);
@@ -219,11 +210,6 @@ static int encode_nsm_string(struct xdr_stream *xdr, const char *string)
 /*
  * "mon_name" specifies the host to be monitored.
  */
-static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
-{
-	return xdr_encode_nsm_string(p, argp->mon_name);
-}
-
 static int encode_mon_name(struct xdr_stream *xdr, const struct nsm_args *argp)
 {
 	return encode_nsm_string(xdr, argp->mon_name);
@@ -235,19 +221,6 @@ static int encode_mon_name(struct xdr_stream *xdr, const struct nsm_args *argp)
  * (via the NLMPROC_SM_NOTIFY call) that the state of host "mon_name"
  * has changed.
  */
-static __be32 *xdr_encode_my_id(__be32 *p, struct nsm_args *argp)
-{
-	p = xdr_encode_nsm_string(p, utsname()->nodename);
-	if (!p)
-		return ERR_PTR(-EIO);
-
-	*p++ = htonl(argp->prog);
-	*p++ = htonl(argp->vers);
-	*p++ = htonl(argp->proc);
-
-	return p;
-}
-
 static int encode_my_id(struct xdr_stream *xdr, const struct nsm_args *argp)
 {
 	int status;
@@ -269,15 +242,6 @@ static int encode_my_id(struct xdr_stream *xdr, const struct nsm_args *argp)
  * The "mon_id" argument specifies the non-private arguments
  * of an NSMPROC_MON or NSMPROC_UNMON call.
  */
-static __be32 *xdr_encode_mon_id(__be32 *p, struct nsm_args *argp)
-{
-	p = xdr_encode_mon_name(p, argp);
-	if (!p)
-		return ERR_PTR(-EIO);
-
-	return xdr_encode_my_id(p, argp);
-}
-
 static int encode_mon_id(struct xdr_stream *xdr, const struct nsm_args *argp)
 {
 	int status;
@@ -296,16 +260,6 @@ static int encode_mon_id(struct xdr_stream *xdr, const struct nsm_args *argp)
  * Linux provides the raw IP address of the monitored host,
  * left in network byte order.
  */
-static __be32 *xdr_encode_priv(__be32 *p, struct nsm_args *argp)
-{
-	*p++ = argp->addr;
-	*p++ = 0;
-	*p++ = 0;
-	*p++ = 0;
-
-	return p;
-}
-
 static int encode_priv(struct xdr_stream *xdr, const struct nsm_args *argp)
 {
 	__be32 *p;
@@ -320,21 +274,6 @@ static int encode_priv(struct xdr_stream *xdr, const struct nsm_args *argp)
 	return 0;
 }
 
-static int
-xdr_encode_mon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args *argp)
-{
-	p = xdr_encode_mon_id(p, argp);
-	if (IS_ERR(p))
-		return PTR_ERR(p);
-
-	p = xdr_encode_priv(p, argp);
-	if (IS_ERR(p))
-		return PTR_ERR(p);
-
-	rqstp->rq_slen = xdr_adjust_iovec(rqstp->rq_svec, p);
-	return 0;
-}
-
 static int xdr_enc_mon(struct rpc_rqst *req, __be32 *p,
 		       const struct nsm_args *argp)
 {
@@ -348,16 +287,6 @@ static int xdr_enc_mon(struct rpc_rqst *req, __be32 *p,
 	return encode_priv(&xdr, argp);
 }
 
-static int
-xdr_encode_unmon(struct rpc_rqst *rqstp, __be32 *p, struct nsm_args *argp)
-{
-	p = xdr_encode_mon_id(p, argp);
-	if (IS_ERR(p))
-		return PTR_ERR(p);
-	rqstp->rq_slen = xdr_adjust_iovec(rqstp->rq_svec, p);
-	return 0;
-}
-
 static int xdr_enc_unmon(struct rpc_rqst *req, __be32 *p,
 			 const struct nsm_args *argp)
 {
@@ -367,16 +296,6 @@ static int xdr_enc_unmon(struct rpc_rqst *req, __be32 *p,
 	return encode_mon_id(&xdr, argp);
 }
 
-static int
-xdr_decode_stat_res(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
-{
-	resp->status = ntohl(*p++);
-	resp->state = ntohl(*p++);
-	dprintk("nsm: xdr_decode_stat_res status %d state %d\n",
-			resp->status, resp->state);
-	return 0;
-}
-
 static int xdr_dec_stat_res(struct rpc_rqst *rqstp, __be32 *p,
 			    struct nsm_res *resp)
 {
@@ -394,13 +313,6 @@ static int xdr_dec_stat_res(struct rpc_rqst *rqstp, __be32 *p,
 	return 0;
 }
 
-static int
-xdr_decode_stat(struct rpc_rqst *rqstp, __be32 *p, struct nsm_res *resp)
-{
-	resp->state = ntohl(*p++);
-	return 0;
-}
-
 static int xdr_dec_stat(struct rpc_rqst *rqstp, __be32 *p,
 			struct nsm_res *resp)
 {

[PATCH 07/27] NSM: Move nsm_find() to fs/lockd/mon.c

[Chuck Lever]

The nsm_find() function sets up fresh nsm_handle entries.  This is
where we will store the "priv" cookie used to lookup nsm_handles during
reboot recovery.  The cookie will be constructed when nsm_find()
creates a new nsm_handle.

As much as possible, I would like to keep everything that handles a
"priv" cookie in fs/lockd/mon.c so that all the smarts are in one
source file.  That organization should make it pretty simple to see how
all this works.

To me, it makes more sense than the current arrangement to keep
nsm_find() with nsm_monitor() and nsm_unmonitor().

So, start reorganizing by moving nsm_find() into fs/lockd/mon.c.  The
nsm_release() function comes along too, since it shares the nsm_lock
global variable.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/host.c             |  129 -----------------------------------------
 fs/lockd/mon.c              |  134 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/lockd/lockd.h |    6 ++
 3 files changed, 140 insertions(+), 129 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index 316241a..dbdeaa8 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -32,12 +32,6 @@ static int			nrhosts;
 static DEFINE_MUTEX(nlm_host_mutex);
 
 static void			nlm_gc_hosts(void);
-static struct nsm_handle	*nsm_find(const struct sockaddr *sap,
-						const size_t salen,
-						const char *hostname,
-						const size_t hostname_len,
-						const int create);
-static void			nsm_release(struct nsm_handle *nsm);
 
 struct nlm_lookup_host_info {
 	const int		server;		/* search for server|client */
@@ -106,44 +100,6 @@ static void nlm_clear_port(struct sockaddr *sap)
 	}
 }
 
-static void nlm_display_ipv4_address(const struct sockaddr *sap, char *buf,
-				     const size_t len)
-{
-	const struct sockaddr_in *sin = (struct sockaddr_in *)sap;
-	snprintf(buf, len, NIPQUAD_FMT, NIPQUAD(sin->sin_addr.s_addr));
-}
-
-static void nlm_display_ipv6_address(const struct sockaddr *sap, char *buf,
-				     const size_t len)
-{
-	const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
-
-	if (ipv6_addr_v4mapped(&sin6->sin6_addr))
-		snprintf(buf, len, NIPQUAD_FMT,
-				NIPQUAD(sin6->sin6_addr.s6_addr32[3]));
-	else if (sin6->sin6_scope_id != 0)
-		snprintf(buf, len, NIP6_FMT "%%%u", NIP6(sin6->sin6_addr),
-				sin6->sin6_scope_id);
-	else
-		snprintf(buf, len, NIP6_FMT, NIP6(sin6->sin6_addr));
-}
-
-static void nlm_display_address(const struct sockaddr *sap,
-				char *buf, const size_t len)
-{
-	switch (sap->sa_family) {
-	case AF_INET:
-		nlm_display_ipv4_address(sap, buf, len);
-		break;
-	case AF_INET6:
-		nlm_display_ipv6_address(sap, buf, len);
-		break;
-	default:
-		snprintf(buf, len, "unsupported address family");
-		break;
-	}
-}
-
 /*
  * Common host lookup routine for server & client
  */
@@ -636,88 +592,3 @@ nlm_gc_hosts(void)
 
 	next_gc = jiffies + NLM_HOST_COLLECT;
 }
-
-
-/*
- * Manage NSM handles
- */
-static LIST_HEAD(nsm_handles);
-static DEFINE_SPINLOCK(nsm_lock);
-
-static struct nsm_handle *nsm_find(const struct sockaddr *sap,
-				   const size_t salen,
-				   const char *hostname,
-				   const size_t hostname_len,
-				   const int create)
-{
-	struct nsm_handle *nsm = NULL;
-	struct nsm_handle *pos;
-
-	if (!sap)
-		return NULL;
-
-	if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
-		if (printk_ratelimit()) {
-			printk(KERN_WARNING "Invalid hostname \"%.*s\" "
-					    "in NFS lock request\n",
-				(int)hostname_len, hostname);
-		}
-		return NULL;
-	}
-
-retry:
-	spin_lock(&nsm_lock);
-	list_for_each_entry(pos, &nsm_handles, sm_link) {
-
-		if (hostname && nsm_use_hostnames) {
-			if (strlen(pos->sm_name) != hostname_len
-			 || memcmp(pos->sm_name, hostname, hostname_len))
-				continue;
-		} else if (!nlm_cmp_addr(nsm_addr(pos), sap))
-			continue;
-		atomic_inc(&pos->sm_count);
-		kfree(nsm);
-		nsm = pos;
-		goto found;
-	}
-	if (nsm) {
-		list_add(&nsm->sm_link, &nsm_handles);
-		goto found;
-	}
-	spin_unlock(&nsm_lock);
-
-	if (!create)
-		return NULL;
-
-	nsm = kzalloc(sizeof(*nsm) + hostname_len + 1, GFP_KERNEL);
-	if (nsm == NULL)
-		return NULL;
-
-	memcpy(nsm_addr(nsm), sap, salen);
-	nsm->sm_addrlen = salen;
-	nsm->sm_name = (char *) (nsm + 1);
-	memcpy(nsm->sm_name, hostname, hostname_len);
-	nsm->sm_name[hostname_len] = '\0';
-	nlm_display_address((struct sockaddr *)&nsm->sm_addr,
-				nsm->sm_addrbuf, sizeof(nsm->sm_addrbuf));
-	atomic_set(&nsm->sm_count, 1);
-	goto retry;
-
-found:
-	spin_unlock(&nsm_lock);
-	return nsm;
-}
-
-/*
- * Release an NSM handle
- */
-static void nsm_release(struct nsm_handle *nsm)
-{
-	if (!nsm)
-		return;
-	if (atomic_dec_and_lock(&nsm->sm_count, &nsm_lock)) {
-		list_del(&nsm->sm_link);
-		spin_unlock(&nsm_lock);
-		kfree(nsm);
-	}
-}
diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 81e1cc1..d5bd847 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -47,12 +47,52 @@ struct nsm_res {
 static struct rpc_clnt *	nsm_create(void);
 
 static struct rpc_program	nsm_program;
+static				LIST_HEAD(nsm_handles);
+static				DEFINE_SPINLOCK(nsm_lock);
 
 /*
  * Local NSM state
  */
 int				nsm_local_state;
 
+static void nsm_display_ipv4_address(const struct sockaddr *sap, char *buf,
+				     const size_t len)
+{
+	const struct sockaddr_in *sin = (struct sockaddr_in *)sap;
+	snprintf(buf, len, NIPQUAD_FMT, NIPQUAD(sin->sin_addr.s_addr));
+}
+
+static void nsm_display_ipv6_address(const struct sockaddr *sap, char *buf,
+				     const size_t len)
+{
+	const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
+
+	if (ipv6_addr_v4mapped(&sin6->sin6_addr))
+		snprintf(buf, len, NIPQUAD_FMT,
+				NIPQUAD(sin6->sin6_addr.s6_addr32[3]));
+	else if (sin6->sin6_scope_id != 0)
+		snprintf(buf, len, NIP6_FMT "%%%u", NIP6(sin6->sin6_addr),
+				sin6->sin6_scope_id);
+	else
+		snprintf(buf, len, NIP6_FMT, NIP6(sin6->sin6_addr));
+}
+
+static void nsm_display_address(const struct sockaddr *sap,
+				char *buf, const size_t len)
+{
+	switch (sap->sa_family) {
+	case AF_INET:
+		nsm_display_ipv4_address(sap, buf, len);
+		break;
+	case AF_INET6:
+		nsm_display_ipv6_address(sap, buf, len);
+		break;
+	default:
+		snprintf(buf, len, "unsupported address family");
+		break;
+	}
+}
+
 /*
  * Common procedure for NSMPROC_MON/NSMPROC_UNMON calls
  */
@@ -162,6 +202,100 @@ void nsm_unmonitor(const struct nlm_host *host)
 	}
 }
 
+/**
+ * nsm_find - Find or create a cached nsm_handle
+ * @sap: pointer to socket address of handle to find
+ * @salen: length of socket address
+ * @hostname: pointer to C string containing hostname to find
+ * @hostname_len: length of C string
+ * @create: one means create new handle if not found in cache
+ *
+ * Behavior is modulated by the global nsm_use_hostnames variable
+ * and by the @create argument.
+ *
+ * Returns a cached nsm_handle after bumping its ref count, or if
+ * @create is set, returns a fresh nsm_handle if a handle that
+ * matches @sap and/or @hostname cannot be found in the handle cache.
+ * Returns NULL if an error occurs.
+ */
+struct nsm_handle *nsm_find(const struct sockaddr *sap, const size_t salen,
+			    const char *hostname, const size_t hostname_len,
+			    const int create)
+{
+	struct nsm_handle *nsm = NULL;
+	struct nsm_handle *pos;
+
+	if (!sap)
+		return NULL;
+
+	if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
+		if (printk_ratelimit()) {
+			printk(KERN_WARNING "Invalid hostname \"%.*s\" "
+					    "in NFS lock request\n",
+				(int)hostname_len, hostname);
+		}
+		return NULL;
+	}
+
+retry:
+	spin_lock(&nsm_lock);
+	list_for_each_entry(pos, &nsm_handles, sm_link) {
+
+		if (hostname && nsm_use_hostnames) {
+			if (strlen(pos->sm_name) != hostname_len
+			 || memcmp(pos->sm_name, hostname, hostname_len))
+				continue;
+		} else if (!nlm_cmp_addr(nsm_addr(pos), sap))
+			continue;
+		atomic_inc(&pos->sm_count);
+		kfree(nsm);
+		nsm = pos;
+		goto found;
+	}
+	if (nsm) {
+		list_add(&nsm->sm_link, &nsm_handles);
+		goto found;
+	}
+	spin_unlock(&nsm_lock);
+
+	if (!create)
+		return NULL;
+
+	nsm = kzalloc(sizeof(*nsm) + hostname_len + 1, GFP_KERNEL);
+	if (nsm == NULL)
+		return NULL;
+
+	memcpy(nsm_addr(nsm), sap, salen);
+	nsm->sm_addrlen = salen;
+	nsm->sm_name = (char *) (nsm + 1);
+	memcpy(nsm->sm_name, hostname, hostname_len);
+	nsm->sm_name[hostname_len] = '\0';
+	nsm_display_address((struct sockaddr *)&nsm->sm_addr,
+				nsm->sm_addrbuf, sizeof(nsm->sm_addrbuf));
+	atomic_set(&nsm->sm_count, 1);
+	goto retry;
+
+found:
+	spin_unlock(&nsm_lock);
+	return nsm;
+}
+
+/**
+ * nsm_release - Release an NSM handle
+ * @nsm: pointer to handle to be released
+ *
+ */
+void nsm_release(struct nsm_handle *nsm)
+{
+	if (!nsm)
+		return;
+	if (atomic_dec_and_lock(&nsm->sm_count, &nsm_lock)) {
+		list_del(&nsm->sm_link);
+		spin_unlock(&nsm_lock);
+		kfree(nsm);
+	}
+}
+
 /*
  * Create NSM client for the local host
  */
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 38344bf..8d71536 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -247,6 +247,12 @@ extern void	  nlm_host_rebooted(const struct sockaddr_in *, const char *,
 int		  nsm_monitor(const struct nlm_host *host);
 void		  nsm_unmonitor(const struct nlm_host *host);
 
+struct nsm_handle *nsm_find(const struct sockaddr *sap, const size_t salen,
+					const char *hostname,
+					const size_t hostname_len,
+					const int create);
+void		  nsm_release(struct nsm_handle *nsm);
+
 /*
  * This is used in garbage collection and resource reclaim
  * A return value != 0 means destroy the lock/block/share

Re: [PATCH 07/27] NSM: Move nsm_find() to fs/lockd/mon.c

[J. Bruce Fields]

On Fri, Dec 05, 2008 at 07:02:45PM -0500, Chuck Lever wrote:
> The nsm_find() function sets up fresh nsm_handle entries.  This is
> where we will store the "priv" cookie used to lookup nsm_handles during
> reboot recovery.  The cookie will be constructed when nsm_find()
> creates a new nsm_handle.
> 
> As much as possible, I would like to keep everything that handles a
> "priv" cookie in fs/lockd/mon.c so that all the smarts are in one
> source file.  That organization should make it pretty simple to see how
> all this works.
> 
> To me, it makes more sense than the current arrangement to keep
> nsm_find() with nsm_monitor() and nsm_unmonitor().
> 
> So, start reorganizing by moving nsm_find() into fs/lockd/mon.c.  The
> nsm_release() function comes along too, since it shares the nsm_lock
> global variable.

OK, haven't thought that organization through myself, but it sounds
sensible, so I'll trust your judgement; applied.--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/host.c             |  129 -----------------------------------------
>  fs/lockd/mon.c              |  134 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/lockd/lockd.h |    6 ++
>  3 files changed, 140 insertions(+), 129 deletions(-)
> 
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index 316241a..dbdeaa8 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -32,12 +32,6 @@ static int			nrhosts;
>  static DEFINE_MUTEX(nlm_host_mutex);
>  
>  static void			nlm_gc_hosts(void);
> -static struct nsm_handle	*nsm_find(const struct sockaddr *sap,
> -						const size_t salen,
> -						const char *hostname,
> -						const size_t hostname_len,
> -						const int create);
> -static void			nsm_release(struct nsm_handle *nsm);
>  
>  struct nlm_lookup_host_info {
>  	const int		server;		/* search for server|client */
> @@ -106,44 +100,6 @@ static void nlm_clear_port(struct sockaddr *sap)
>  	}
>  }
>  
> -static void nlm_display_ipv4_address(const struct sockaddr *sap, char *buf,
> -				     const size_t len)
> -{
> -	const struct sockaddr_in *sin = (struct sockaddr_in *)sap;
> -	snprintf(buf, len, NIPQUAD_FMT, NIPQUAD(sin->sin_addr.s_addr));
> -}
> -
> -static void nlm_display_ipv6_address(const struct sockaddr *sap, char *buf,
> -				     const size_t len)
> -{
> -	const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
> -
> -	if (ipv6_addr_v4mapped(&sin6->sin6_addr))
> -		snprintf(buf, len, NIPQUAD_FMT,
> -				NIPQUAD(sin6->sin6_addr.s6_addr32[3]));
> -	else if (sin6->sin6_scope_id != 0)
> -		snprintf(buf, len, NIP6_FMT "%%%u", NIP6(sin6->sin6_addr),
> -				sin6->sin6_scope_id);
> -	else
> -		snprintf(buf, len, NIP6_FMT, NIP6(sin6->sin6_addr));
> -}
> -
> -static void nlm_display_address(const struct sockaddr *sap,
> -				char *buf, const size_t len)
> -{
> -	switch (sap->sa_family) {
> -	case AF_INET:
> -		nlm_display_ipv4_address(sap, buf, len);
> -		break;
> -	case AF_INET6:
> -		nlm_display_ipv6_address(sap, buf, len);
> -		break;
> -	default:
> -		snprintf(buf, len, "unsupported address family");
> -		break;
> -	}
> -}
> -
>  /*
>   * Common host lookup routine for server & client
>   */
> @@ -636,88 +592,3 @@ nlm_gc_hosts(void)
>  
>  	next_gc = jiffies + NLM_HOST_COLLECT;
>  }
> -
> -
> -/*
> - * Manage NSM handles
> - */
> -static LIST_HEAD(nsm_handles);
> -static DEFINE_SPINLOCK(nsm_lock);
> -
> -static struct nsm_handle *nsm_find(const struct sockaddr *sap,
> -				   const size_t salen,
> -				   const char *hostname,
> -				   const size_t hostname_len,
> -				   const int create)
> -{
> -	struct nsm_handle *nsm = NULL;
> -	struct nsm_handle *pos;
> -
> -	if (!sap)
> -		return NULL;
> -
> -	if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
> -		if (printk_ratelimit()) {
> -			printk(KERN_WARNING "Invalid hostname \"%.*s\" "
> -					    "in NFS lock request\n",
> -				(int)hostname_len, hostname);
> -		}
> -		return NULL;
> -	}
> -
> -retry:
> -	spin_lock(&nsm_lock);
> -	list_for_each_entry(pos, &nsm_handles, sm_link) {
> -
> -		if (hostname && nsm_use_hostnames) {
> -			if (strlen(pos->sm_name) != hostname_len
> -			 || memcmp(pos->sm_name, hostname, hostname_len))
> -				continue;
> -		} else if (!nlm_cmp_addr(nsm_addr(pos), sap))
> -			continue;
> -		atomic_inc(&pos->sm_count);
> -		kfree(nsm);
> -		nsm = pos;
> -		goto found;
> -	}
> -	if (nsm) {
> -		list_add(&nsm->sm_link, &nsm_handles);
> -		goto found;
> -	}
> -	spin_unlock(&nsm_lock);
> -
> -	if (!create)
> -		return NULL;
> -
> -	nsm = kzalloc(sizeof(*nsm) + hostname_len + 1, GFP_KERNEL);
> -	if (nsm == NULL)
> -		return NULL;
> -
> -	memcpy(nsm_addr(nsm), sap, salen);
> -	nsm->sm_addrlen = salen;
> -	nsm->sm_name = (char *) (nsm + 1);
> -	memcpy(nsm->sm_name, hostname, hostname_len);
> -	nsm->sm_name[hostname_len] = '\0';
> -	nlm_display_address((struct sockaddr *)&nsm->sm_addr,
> -				nsm->sm_addrbuf, sizeof(nsm->sm_addrbuf));
> -	atomic_set(&nsm->sm_count, 1);
> -	goto retry;
> -
> -found:
> -	spin_unlock(&nsm_lock);
> -	return nsm;
> -}
> -
> -/*
> - * Release an NSM handle
> - */
> -static void nsm_release(struct nsm_handle *nsm)
> -{
> -	if (!nsm)
> -		return;
> -	if (atomic_dec_and_lock(&nsm->sm_count, &nsm_lock)) {
> -		list_del(&nsm->sm_link);
> -		spin_unlock(&nsm_lock);
> -		kfree(nsm);
> -	}
> -}
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index 81e1cc1..d5bd847 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -47,12 +47,52 @@ struct nsm_res {
>  static struct rpc_clnt *	nsm_create(void);
>  
>  static struct rpc_program	nsm_program;
> +static				LIST_HEAD(nsm_handles);
> +static				DEFINE_SPINLOCK(nsm_lock);
>  
>  /*
>   * Local NSM state
>   */
>  int				nsm_local_state;
>  
> +static void nsm_display_ipv4_address(const struct sockaddr *sap, char *buf,
> +				     const size_t len)
> +{
> +	const struct sockaddr_in *sin = (struct sockaddr_in *)sap;
> +	snprintf(buf, len, NIPQUAD_FMT, NIPQUAD(sin->sin_addr.s_addr));
> +}
> +
> +static void nsm_display_ipv6_address(const struct sockaddr *sap, char *buf,
> +				     const size_t len)
> +{
> +	const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
> +
> +	if (ipv6_addr_v4mapped(&sin6->sin6_addr))
> +		snprintf(buf, len, NIPQUAD_FMT,
> +				NIPQUAD(sin6->sin6_addr.s6_addr32[3]));
> +	else if (sin6->sin6_scope_id != 0)
> +		snprintf(buf, len, NIP6_FMT "%%%u", NIP6(sin6->sin6_addr),
> +				sin6->sin6_scope_id);
> +	else
> +		snprintf(buf, len, NIP6_FMT, NIP6(sin6->sin6_addr));
> +}
> +
> +static void nsm_display_address(const struct sockaddr *sap,
> +				char *buf, const size_t len)
> +{
> +	switch (sap->sa_family) {
> +	case AF_INET:
> +		nsm_display_ipv4_address(sap, buf, len);
> +		break;
> +	case AF_INET6:
> +		nsm_display_ipv6_address(sap, buf, len);
> +		break;
> +	default:
> +		snprintf(buf, len, "unsupported address family");
> +		break;
> +	}
> +}
> +
>  /*
>   * Common procedure for NSMPROC_MON/NSMPROC_UNMON calls
>   */
> @@ -162,6 +202,100 @@ void nsm_unmonitor(const struct nlm_host *host)
>  	}
>  }
>  
> +/**
> + * nsm_find - Find or create a cached nsm_handle
> + * @sap: pointer to socket address of handle to find
> + * @salen: length of socket address
> + * @hostname: pointer to C string containing hostname to find
> + * @hostname_len: length of C string
> + * @create: one means create new handle if not found in cache
> + *
> + * Behavior is modulated by the global nsm_use_hostnames variable
> + * and by the @create argument.
> + *
> + * Returns a cached nsm_handle after bumping its ref count, or if
> + * @create is set, returns a fresh nsm_handle if a handle that
> + * matches @sap and/or @hostname cannot be found in the handle cache.
> + * Returns NULL if an error occurs.
> + */
> +struct nsm_handle *nsm_find(const struct sockaddr *sap, const size_t salen,
> +			    const char *hostname, const size_t hostname_len,
> +			    const int create)
> +{
> +	struct nsm_handle *nsm = NULL;
> +	struct nsm_handle *pos;
> +
> +	if (!sap)
> +		return NULL;
> +
> +	if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
> +		if (printk_ratelimit()) {
> +			printk(KERN_WARNING "Invalid hostname \"%.*s\" "
> +					    "in NFS lock request\n",
> +				(int)hostname_len, hostname);
> +		}
> +		return NULL;
> +	}
> +
> +retry:
> +	spin_lock(&nsm_lock);
> +	list_for_each_entry(pos, &nsm_handles, sm_link) {
> +
> +		if (hostname && nsm_use_hostnames) {
> +			if (strlen(pos->sm_name) != hostname_len
> +			 || memcmp(pos->sm_name, hostname, hostname_len))
> +				continue;
> +		} else if (!nlm_cmp_addr(nsm_addr(pos), sap))
> +			continue;
> +		atomic_inc(&pos->sm_count);
> +		kfree(nsm);
> +		nsm = pos;
> +		goto found;
> +	}
> +	if (nsm) {
> +		list_add(&nsm->sm_link, &nsm_handles);
> +		goto found;
> +	}
> +	spin_unlock(&nsm_lock);
> +
> +	if (!create)
> +		return NULL;
> +
> +	nsm = kzalloc(sizeof(*nsm) + hostname_len + 1, GFP_KERNEL);
> +	if (nsm == NULL)
> +		return NULL;
> +
> +	memcpy(nsm_addr(nsm), sap, salen);
> +	nsm->sm_addrlen = salen;
> +	nsm->sm_name = (char *) (nsm + 1);
> +	memcpy(nsm->sm_name, hostname, hostname_len);
> +	nsm->sm_name[hostname_len] = '\0';
> +	nsm_display_address((struct sockaddr *)&nsm->sm_addr,
> +				nsm->sm_addrbuf, sizeof(nsm->sm_addrbuf));
> +	atomic_set(&nsm->sm_count, 1);
> +	goto retry;
> +
> +found:
> +	spin_unlock(&nsm_lock);
> +	return nsm;
> +}
> +
> +/**
> + * nsm_release - Release an NSM handle
> + * @nsm: pointer to handle to be released
> + *
> + */
> +void nsm_release(struct nsm_handle *nsm)
> +{
> +	if (!nsm)
> +		return;
> +	if (atomic_dec_and_lock(&nsm->sm_count, &nsm_lock)) {
> +		list_del(&nsm->sm_link);
> +		spin_unlock(&nsm_lock);
> +		kfree(nsm);
> +	}
> +}
> +
>  /*
>   * Create NSM client for the local host
>   */
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 38344bf..8d71536 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -247,6 +247,12 @@ extern void	  nlm_host_rebooted(const struct sockaddr_in *, const char *,
>  int		  nsm_monitor(const struct nlm_host *host);
>  void		  nsm_unmonitor(const struct nlm_host *host);
>  
> +struct nsm_handle *nsm_find(const struct sockaddr *sap, const size_t salen,
> +					const char *hostname,
> +					const size_t hostname_len,
> +					const int create);
> +void		  nsm_release(struct nsm_handle *nsm);
> +
>  /*
>   * This is used in garbage collection and resource reclaim
>   * A return value != 0 means destroy the lock/block/share
> 

Re: [PATCH 07/27] NSM: Move nsm_find() to fs/lockd/mon.c

[Chuck Lever]

On Dec 10, 2008, at Dec 10, 2008, 3:58 PM, J. Bruce Fields wrote:
> On Fri, Dec 05, 2008 at 07:02:45PM -0500, Chuck Lever wrote:
>> The nsm_find() function sets up fresh nsm_handle entries.  This is
>> where we will store the "priv" cookie used to lookup nsm_handles  
>> during
>> reboot recovery.  The cookie will be constructed when nsm_find()
>> creates a new nsm_handle.
>>
>> As much as possible, I would like to keep everything that handles a
>> "priv" cookie in fs/lockd/mon.c so that all the smarts are in one
>> source file.  That organization should make it pretty simple to see  
>> how
>> all this works.
>>
>> To me, it makes more sense than the current arrangement to keep
>> nsm_find() with nsm_monitor() and nsm_unmonitor().
>>
>> So, start reorganizing by moving nsm_find() into fs/lockd/mon.c.  The
>> nsm_release() function comes along too, since it shares the nsm_lock
>> global variable.
>
> OK, haven't thought that organization through myself, but it sounds
> sensible, so I'll trust your judgement; applied.--b.

My thinking is that an nsm_handle is a different object than an  
nlm_host.  nlm_hosts are managed in fs/lockd/host.c, and nsm_handles  
will be managed in fs/lockd/mon.c.  The data structures and methods  
for each will be in their own separate source files.

It's possible that fs/lockd/mon.c should only care about the upcalls,  
as it does today.  However, if we leave nsm_find() (and later,  
nsm_reboot_lookup()) in fs/lockd/host.c, then both mon.c and host.c  
have to care about what's in nsm_private.  I would rather collect all  
of that local knowledge in a single source file.

We can talk about it more f2f tomorrow if you have questions, or have  
thought of better ways to organize this.

>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/lockd/host.c             |  129  
>> -----------------------------------------
>> fs/lockd/mon.c              |  134 +++++++++++++++++++++++++++++++++ 
>> ++++++++++
>> include/linux/lockd/lockd.h |    6 ++
>> 3 files changed, 140 insertions(+), 129 deletions(-)
>>
>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>> index 316241a..dbdeaa8 100644
>> --- a/fs/lockd/host.c
>> +++ b/fs/lockd/host.c
>> @@ -32,12 +32,6 @@ static int			nrhosts;
>> static DEFINE_MUTEX(nlm_host_mutex);
>>
>> static void			nlm_gc_hosts(void);
>> -static struct nsm_handle	*nsm_find(const struct sockaddr *sap,
>> -						const size_t salen,
>> -						const char *hostname,
>> -						const size_t hostname_len,
>> -						const int create);
>> -static void			nsm_release(struct nsm_handle *nsm);
>>
>> struct nlm_lookup_host_info {
>> 	const int		server;		/* search for server|client */
>> @@ -106,44 +100,6 @@ static void nlm_clear_port(struct sockaddr *sap)
>> 	}
>> }
>>
>> -static void nlm_display_ipv4_address(const struct sockaddr *sap,  
>> char *buf,
>> -				     const size_t len)
>> -{
>> -	const struct sockaddr_in *sin = (struct sockaddr_in *)sap;
>> -	snprintf(buf, len, NIPQUAD_FMT, NIPQUAD(sin->sin_addr.s_addr));
>> -}
>> -
>> -static void nlm_display_ipv6_address(const struct sockaddr *sap,  
>> char *buf,
>> -				     const size_t len)
>> -{
>> -	const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
>> -
>> -	if (ipv6_addr_v4mapped(&sin6->sin6_addr))
>> -		snprintf(buf, len, NIPQUAD_FMT,
>> -				NIPQUAD(sin6->sin6_addr.s6_addr32[3]));
>> -	else if (sin6->sin6_scope_id != 0)
>> -		snprintf(buf, len, NIP6_FMT "%%%u", NIP6(sin6->sin6_addr),
>> -				sin6->sin6_scope_id);
>> -	else
>> -		snprintf(buf, len, NIP6_FMT, NIP6(sin6->sin6_addr));
>> -}
>> -
>> -static void nlm_display_address(const struct sockaddr *sap,
>> -				char *buf, const size_t len)
>> -{
>> -	switch (sap->sa_family) {
>> -	case AF_INET:
>> -		nlm_display_ipv4_address(sap, buf, len);
>> -		break;
>> -	case AF_INET6:
>> -		nlm_display_ipv6_address(sap, buf, len);
>> -		break;
>> -	default:
>> -		snprintf(buf, len, "unsupported address family");
>> -		break;
>> -	}
>> -}
>> -
>> /*
>>  * Common host lookup routine for server & client
>>  */
>> @@ -636,88 +592,3 @@ nlm_gc_hosts(void)
>>
>> 	next_gc = jiffies + NLM_HOST_COLLECT;
>> }
>> -
>> -
>> -/*
>> - * Manage NSM handles
>> - */
>> -static LIST_HEAD(nsm_handles);
>> -static DEFINE_SPINLOCK(nsm_lock);
>> -
>> -static struct nsm_handle *nsm_find(const struct sockaddr *sap,
>> -				   const size_t salen,
>> -				   const char *hostname,
>> -				   const size_t hostname_len,
>> -				   const int create)
>> -{
>> -	struct nsm_handle *nsm = NULL;
>> -	struct nsm_handle *pos;
>> -
>> -	if (!sap)
>> -		return NULL;
>> -
>> -	if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
>> -		if (printk_ratelimit()) {
>> -			printk(KERN_WARNING "Invalid hostname \"%.*s\" "
>> -					    "in NFS lock request\n",
>> -				(int)hostname_len, hostname);
>> -		}
>> -		return NULL;
>> -	}
>> -
>> -retry:
>> -	spin_lock(&nsm_lock);
>> -	list_for_each_entry(pos, &nsm_handles, sm_link) {
>> -
>> -		if (hostname && nsm_use_hostnames) {
>> -			if (strlen(pos->sm_name) != hostname_len
>> -			 || memcmp(pos->sm_name, hostname, hostname_len))
>> -				continue;
>> -		} else if (!nlm_cmp_addr(nsm_addr(pos), sap))
>> -			continue;
>> -		atomic_inc(&pos->sm_count);
>> -		kfree(nsm);
>> -		nsm = pos;
>> -		goto found;
>> -	}
>> -	if (nsm) {
>> -		list_add(&nsm->sm_link, &nsm_handles);
>> -		goto found;
>> -	}
>> -	spin_unlock(&nsm_lock);
>> -
>> -	if (!create)
>> -		return NULL;
>> -
>> -	nsm = kzalloc(sizeof(*nsm) + hostname_len + 1, GFP_KERNEL);
>> -	if (nsm == NULL)
>> -		return NULL;
>> -
>> -	memcpy(nsm_addr(nsm), sap, salen);
>> -	nsm->sm_addrlen = salen;
>> -	nsm->sm_name = (char *) (nsm + 1);
>> -	memcpy(nsm->sm_name, hostname, hostname_len);
>> -	nsm->sm_name[hostname_len] = '\0';
>> -	nlm_display_address((struct sockaddr *)&nsm->sm_addr,
>> -				nsm->sm_addrbuf, sizeof(nsm->sm_addrbuf));
>> -	atomic_set(&nsm->sm_count, 1);
>> -	goto retry;
>> -
>> -found:
>> -	spin_unlock(&nsm_lock);
>> -	return nsm;
>> -}
>> -
>> -/*
>> - * Release an NSM handle
>> - */
>> -static void nsm_release(struct nsm_handle *nsm)
>> -{
>> -	if (!nsm)
>> -		return;
>> -	if (atomic_dec_and_lock(&nsm->sm_count, &nsm_lock)) {
>> -		list_del(&nsm->sm_link);
>> -		spin_unlock(&nsm_lock);
>> -		kfree(nsm);
>> -	}
>> -}
>> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
>> index 81e1cc1..d5bd847 100644
>> --- a/fs/lockd/mon.c
>> +++ b/fs/lockd/mon.c
>> @@ -47,12 +47,52 @@ struct nsm_res {
>> static struct rpc_clnt *	nsm_create(void);
>>
>> static struct rpc_program	nsm_program;
>> +static				LIST_HEAD(nsm_handles);
>> +static				DEFINE_SPINLOCK(nsm_lock);
>>
>> /*
>>  * Local NSM state
>>  */
>> int				nsm_local_state;
>>
>> +static void nsm_display_ipv4_address(const struct sockaddr *sap,  
>> char *buf,
>> +				     const size_t len)
>> +{
>> +	const struct sockaddr_in *sin = (struct sockaddr_in *)sap;
>> +	snprintf(buf, len, NIPQUAD_FMT, NIPQUAD(sin->sin_addr.s_addr));
>> +}
>> +
>> +static void nsm_display_ipv6_address(const struct sockaddr *sap,  
>> char *buf,
>> +				     const size_t len)
>> +{
>> +	const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
>> +
>> +	if (ipv6_addr_v4mapped(&sin6->sin6_addr))
>> +		snprintf(buf, len, NIPQUAD_FMT,
>> +				NIPQUAD(sin6->sin6_addr.s6_addr32[3]));
>> +	else if (sin6->sin6_scope_id != 0)
>> +		snprintf(buf, len, NIP6_FMT "%%%u", NIP6(sin6->sin6_addr),
>> +				sin6->sin6_scope_id);
>> +	else
>> +		snprintf(buf, len, NIP6_FMT, NIP6(sin6->sin6_addr));
>> +}
>> +
>> +static void nsm_display_address(const struct sockaddr *sap,
>> +				char *buf, const size_t len)
>> +{
>> +	switch (sap->sa_family) {
>> +	case AF_INET:
>> +		nsm_display_ipv4_address(sap, buf, len);
>> +		break;
>> +	case AF_INET6:
>> +		nsm_display_ipv6_address(sap, buf, len);
>> +		break;
>> +	default:
>> +		snprintf(buf, len, "unsupported address family");
>> +		break;
>> +	}
>> +}
>> +
>> /*
>>  * Common procedure for NSMPROC_MON/NSMPROC_UNMON calls
>>  */
>> @@ -162,6 +202,100 @@ void nsm_unmonitor(const struct nlm_host *host)
>> 	}
>> }
>>
>> +/**
>> + * nsm_find - Find or create a cached nsm_handle
>> + * @sap: pointer to socket address of handle to find
>> + * @salen: length of socket address
>> + * @hostname: pointer to C string containing hostname to find
>> + * @hostname_len: length of C string
>> + * @create: one means create new handle if not found in cache
>> + *
>> + * Behavior is modulated by the global nsm_use_hostnames variable
>> + * and by the @create argument.
>> + *
>> + * Returns a cached nsm_handle after bumping its ref count, or if
>> + * @create is set, returns a fresh nsm_handle if a handle that
>> + * matches @sap and/or @hostname cannot be found in the handle  
>> cache.
>> + * Returns NULL if an error occurs.
>> + */
>> +struct nsm_handle *nsm_find(const struct sockaddr *sap, const  
>> size_t salen,
>> +			    const char *hostname, const size_t hostname_len,
>> +			    const int create)
>> +{
>> +	struct nsm_handle *nsm = NULL;
>> +	struct nsm_handle *pos;
>> +
>> +	if (!sap)
>> +		return NULL;
>> +
>> +	if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
>> +		if (printk_ratelimit()) {
>> +			printk(KERN_WARNING "Invalid hostname \"%.*s\" "
>> +					    "in NFS lock request\n",
>> +				(int)hostname_len, hostname);
>> +		}
>> +		return NULL;
>> +	}
>> +
>> +retry:
>> +	spin_lock(&nsm_lock);
>> +	list_for_each_entry(pos, &nsm_handles, sm_link) {
>> +
>> +		if (hostname && nsm_use_hostnames) {
>> +			if (strlen(pos->sm_name) != hostname_len
>> +			 || memcmp(pos->sm_name, hostname, hostname_len))
>> +				continue;
>> +		} else if (!nlm_cmp_addr(nsm_addr(pos), sap))
>> +			continue;
>> +		atomic_inc(&pos->sm_count);
>> +		kfree(nsm);
>> +		nsm = pos;
>> +		goto found;
>> +	}
>> +	if (nsm) {
>> +		list_add(&nsm->sm_link, &nsm_handles);
>> +		goto found;
>> +	}
>> +	spin_unlock(&nsm_lock);
>> +
>> +	if (!create)
>> +		return NULL;
>> +
>> +	nsm = kzalloc(sizeof(*nsm) + hostname_len + 1, GFP_KERNEL);
>> +	if (nsm == NULL)
>> +		return NULL;
>> +
>> +	memcpy(nsm_addr(nsm), sap, salen);
>> +	nsm->sm_addrlen = salen;
>> +	nsm->sm_name = (char *) (nsm + 1);
>> +	memcpy(nsm->sm_name, hostname, hostname_len);
>> +	nsm->sm_name[hostname_len] = '\0';
>> +	nsm_display_address((struct sockaddr *)&nsm->sm_addr,
>> +				nsm->sm_addrbuf, sizeof(nsm->sm_addrbuf));
>> +	atomic_set(&nsm->sm_count, 1);
>> +	goto retry;
>> +
>> +found:
>> +	spin_unlock(&nsm_lock);
>> +	return nsm;
>> +}
>> +
>> +/**
>> + * nsm_release - Release an NSM handle
>> + * @nsm: pointer to handle to be released
>> + *
>> + */
>> +void nsm_release(struct nsm_handle *nsm)
>> +{
>> +	if (!nsm)
>> +		return;
>> +	if (atomic_dec_and_lock(&nsm->sm_count, &nsm_lock)) {
>> +		list_del(&nsm->sm_link);
>> +		spin_unlock(&nsm_lock);
>> +		kfree(nsm);
>> +	}
>> +}
>> +
>> /*
>>  * Create NSM client for the local host
>>  */
>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/ 
>> lockd.h
>> index 38344bf..8d71536 100644
>> --- a/include/linux/lockd/lockd.h
>> +++ b/include/linux/lockd/lockd.h
>> @@ -247,6 +247,12 @@ extern void	  nlm_host_rebooted(const struct  
>> sockaddr_in *, const char *,
>> int		  nsm_monitor(const struct nlm_host *host);
>> void		  nsm_unmonitor(const struct nlm_host *host);
>>
>> +struct nsm_handle *nsm_find(const struct sockaddr *sap, const  
>> size_t salen,
>> +					const char *hostname,
>> +					const size_t hostname_len,
>> +					const int create);
>> +void		  nsm_release(struct nsm_handle *nsm);
>> +
>> /*
>>  * This is used in garbage collection and resource reclaim
>>  * A return value != 0 means destroy the lock/block/share
>>

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

[PATCH 08/27] NSM: Add dprintk() calls in nsm_find and nsm_release

[Chuck Lever]

Introduce some dprintk() calls in fs/lockd/mon.c that are enabled by
the NLMDBG_MONITOR flag.  These report when we find, create, and
release nsm_handles.

Since printk() can sleep, these are placed outside the nsm_lock
spinlock.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index d5bd847..8628d31 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -248,14 +248,22 @@ retry:
 		} else if (!nlm_cmp_addr(nsm_addr(pos), sap))
 			continue;
 		atomic_inc(&pos->sm_count);
+		spin_unlock(&nsm_lock);
 		kfree(nsm);
-		nsm = pos;
-		goto found;
+		dprintk("lockd: found nsm_handle for %s (%s), cnt %d\n",
+				pos->sm_name, pos->sm_addrbuf,
+				atomic_read(&pos->sm_count));
+		return pos;
 	}
+
 	if (nsm) {
 		list_add(&nsm->sm_link, &nsm_handles);
-		goto found;
+		spin_unlock(&nsm_lock);
+		dprintk("lockd: created nsm_handle for %s (%s)\n",
+				nsm->sm_name, nsm->sm_addrbuf);
+		return nsm;
 	}
+
 	spin_unlock(&nsm_lock);
 
 	if (!create)
@@ -274,10 +282,6 @@ retry:
 				nsm->sm_addrbuf, sizeof(nsm->sm_addrbuf));
 	atomic_set(&nsm->sm_count, 1);
 	goto retry;
-
-found:
-	spin_unlock(&nsm_lock);
-	return nsm;
 }
 
 /**
@@ -292,6 +296,9 @@ void nsm_release(struct nsm_handle *nsm)
 	if (atomic_dec_and_lock(&nsm->sm_count, &nsm_lock)) {
 		list_del(&nsm->sm_link);
 		spin_unlock(&nsm_lock);
+
+		dprintk("lockd: destroyed nsm_handle for %s (%s)\n",
+				nsm->sm_name, nsm->sm_addrbuf);
 		kfree(nsm);
 	}
 }

Re: [PATCH 08/27] NSM: Add dprintk() calls in nsm_find and nsm_release

[J. Bruce Fields]

On Fri, Dec 05, 2008 at 07:02:53PM -0500, Chuck Lever wrote:
> Introduce some dprintk() calls in fs/lockd/mon.c that are enabled by
> the NLMDBG_MONITOR flag.  These report when we find, create, and
> release nsm_handles.
> 
> Since printk() can sleep, these are placed outside the nsm_lock
> spinlock.

Nope, printk() is designed to be callable from anywhere.  (This is
documented e.g. in kernel/printk.c:printk().)  I've applied the
following; look OK?

--b.

commit dfe441d67b96543eb8cdebd5323c3239e5460213
Author: Chuck Lever <chuck.lever@oracle.com>
Date:   Fri Dec 5 19:02:53 2008 -0500

    NSM: Add dprintk() calls in nsm_find and nsm_release
    
    Introduce some dprintk() calls in fs/lockd/mon.c that are enabled by
    the NLMDBG_MONITOR flag.  These report when we find, create, and
    release nsm_handles.
    
    Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index d5bd847..c332c50 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -250,10 +250,15 @@ retry:
 		atomic_inc(&pos->sm_count);
 		kfree(nsm);
 		nsm = pos;
+		dprintk("lockd: found nsm_handle for %s (%s), cnt %d\n",
+				pos->sm_name, pos->sm_addrbuf,
+				atomic_read(&pos->sm_count));
 		goto found;
 	}
 	if (nsm) {
 		list_add(&nsm->sm_link, &nsm_handles);
+		dprintk("lockd: created nsm_handle for %s (%s)\n",
+				nsm->sm_name, nsm->sm_addrbuf);
 		goto found;
 	}
 	spin_unlock(&nsm_lock);
@@ -292,6 +297,8 @@ void nsm_release(struct nsm_handle *nsm)
 	if (atomic_dec_and_lock(&nsm->sm_count, &nsm_lock)) {
 		list_del(&nsm->sm_link);
 		spin_unlock(&nsm_lock);
+		dprintk("lockd: destroyed nsm_handle for %s (%s)\n",
+				nsm->sm_name, nsm->sm_addrbuf);
 		kfree(nsm);
 	}
 }

Re: [PATCH 08/27] NSM: Add dprintk() calls in nsm_find and nsm_release

[Chuck Lever]

On Dec 10, 2008, at Dec 10, 2008, 4:05 PM, J. Bruce Fields wrote:

> On Fri, Dec 05, 2008 at 07:02:53PM -0500, Chuck Lever wrote:
>> Introduce some dprintk() calls in fs/lockd/mon.c that are enabled by
>> the NLMDBG_MONITOR flag.  These report when we find, create, and
>> release nsm_handles.
>>
>> Since printk() can sleep, these are placed outside the nsm_lock
>> spinlock.
>
> Nope, printk() is designed to be callable from anywhere.  (This is
> documented e.g. in kernel/printk.c:printk().)  I've applied the
> following; look OK?

That's fine, but it will make applying later patches more difficult.

I can fix this up in my repo and resend subsequent patches if you  
like, or you can just delete that last sentence in the description,  
and apply this patch as is.

> --b.
>
> commit dfe441d67b96543eb8cdebd5323c3239e5460213
> Author: Chuck Lever <chuck.lever@oracle.com>
> Date:   Fri Dec 5 19:02:53 2008 -0500
>
>    NSM: Add dprintk() calls in nsm_find and nsm_release
>
>    Introduce some dprintk() calls in fs/lockd/mon.c that are enabled  
> by
>    the NLMDBG_MONITOR flag.  These report when we find, create, and
>    release nsm_handles.
>
>    Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
>
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index d5bd847..c332c50 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -250,10 +250,15 @@ retry:
> 		atomic_inc(&pos->sm_count);
> 		kfree(nsm);
> 		nsm = pos;
> +		dprintk("lockd: found nsm_handle for %s (%s), cnt %d\n",
> +				pos->sm_name, pos->sm_addrbuf,
> +				atomic_read(&pos->sm_count));
> 		goto found;
> 	}
> 	if (nsm) {
> 		list_add(&nsm->sm_link, &nsm_handles);
> +		dprintk("lockd: created nsm_handle for %s (%s)\n",
> +				nsm->sm_name, nsm->sm_addrbuf);
> 		goto found;
> 	}
> 	spin_unlock(&nsm_lock);
> @@ -292,6 +297,8 @@ void nsm_release(struct nsm_handle *nsm)
> 	if (atomic_dec_and_lock(&nsm->sm_count, &nsm_lock)) {
> 		list_del(&nsm->sm_link);
> 		spin_unlock(&nsm_lock);
> +		dprintk("lockd: destroyed nsm_handle for %s (%s)\n",
> +				nsm->sm_name, nsm->sm_addrbuf);
> 		kfree(nsm);
> 	}
> }

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 08/27] NSM: Add dprintk() calls in nsm_find and nsm_release

[J. Bruce Fields]

On Wed, Dec 10, 2008 at 04:10:41PM -0500, Chuck Lever wrote:
>
> On Dec 10, 2008, at Dec 10, 2008, 4:05 PM, J. Bruce Fields wrote:
>
>> On Fri, Dec 05, 2008 at 07:02:53PM -0500, Chuck Lever wrote:
>>> Introduce some dprintk() calls in fs/lockd/mon.c that are enabled by
>>> the NLMDBG_MONITOR flag.  These report when we find, create, and
>>> release nsm_handles.
>>>
>>> Since printk() can sleep, these are placed outside the nsm_lock
>>> spinlock.
>>
>> Nope, printk() is designed to be callable from anywhere.  (This is
>> documented e.g. in kernel/printk.c:printk().)  I've applied the
>> following; look OK?
>
> That's fine, but it will make applying later patches more difficult.
>
> I can fix this up in my repo and resend subsequent patches if you like, 
> or you can just delete that last sentence in the description, and apply 
> this patch as is.

Yeah, already applied, figuring it'd save a round trip.  I think you'd
told me before you preferred fixing up yourself and resending, and I'd
forgotten; I'll try to remember that next time.

--b.

[PATCH 09/27] NSM: Remove NULL pointer check from nsm_find()

[Chuck Lever]

The nsm_find() function should never be called with a NULL IP address
pointer.  If it is, that's a bug.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 8628d31..f3ae631 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -225,9 +225,6 @@ struct nsm_handle *nsm_find(const struct sockaddr *sap, const size_t salen,
 	struct nsm_handle *nsm = NULL;
 	struct nsm_handle *pos;
 
-	if (!sap)
-		return NULL;
-
 	if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
 		if (printk_ratelimit()) {
 			printk(KERN_WARNING "Invalid hostname \"%.*s\" "

[PATCH 10/27] NSM: Remove !nsm check from nsm_release()

[Chuck Lever]

The nsm_release() function should never be called with a NULL handle
point.  If it is, that's a bug.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index f3ae631..c2f4607 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -288,8 +288,6 @@ retry:
  */
 void nsm_release(struct nsm_handle *nsm)
 {
-	if (!nsm)
-		return;
 	if (atomic_dec_and_lock(&nsm->sm_count, &nsm_lock)) {
 		list_del(&nsm->sm_link);
 		spin_unlock(&nsm_lock);

Re: [PATCH 10/27] NSM: Remove !nsm check from nsm_release()

[J. Bruce Fields]

On Fri, Dec 05, 2008 at 07:03:09PM -0500, Chuck Lever wrote:
> The nsm_release() function should never be called with a NULL handle
> point.  If it is, that's a bug.

OK, both applied.--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/mon.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index f3ae631..c2f4607 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -288,8 +288,6 @@ retry:
>   */
>  void nsm_release(struct nsm_handle *nsm)
>  {
> -	if (!nsm)
> -		return;
>  	if (atomic_dec_and_lock(&nsm->sm_count, &nsm_lock)) {
>  		list_del(&nsm->sm_link);
>  		spin_unlock(&nsm_lock);
> 

[PATCH 11/27] NSM: Generate NSMPROC_MON's "priv" argument when nsm_handle is created

[Chuck Lever]

Introduce a new data type, used by both the in-kernel NLM and NSM
implementations, that is used to manage the opaque "priv" argument
for the NSMPROC_MON and NLMPROC_SM_NOTIFY calls.

Construct the "priv" cookie when the nsm_handle is created.

The nsm_init_private() function may look a little strange, but it is
roughly equivalent to how the XDR encoder formed the "priv" argument.
It's going to go away soon.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c                 |   16 ++++++++++++++++
 include/linux/lockd/lockd.h    |    1 +
 include/linux/lockd/sm_inter.h |    1 -
 include/linux/lockd/xdr.h      |    6 ++++++
 4 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index c2f4607..a6504f4 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -202,6 +202,21 @@ void nsm_unmonitor(const struct nlm_host *host)
 	}
 }
 
+/*
+ * Construct a unique cookie to match this nsm_handle to this monitored
+ * host.  It is passed to the local rpc.statd via NSMPROC_MON, and
+ * returned via NLMPROC_SM_NOTIFY, in the "priv" field of these
+ * requests.
+ *
+ * Linux provides the raw IP address of the monitored host,
+ * left in network byte order.
+ */
+static void nsm_init_private(struct nsm_handle *nsm)
+{
+	__be32 *p = (__be32 *)&nsm->sm_priv.data;
+	*p = nsm_addr_in(nsm)->sin_addr.s_addr;
+}
+
 /**
  * nsm_find - Find or create a cached nsm_handle
  * @sap: pointer to socket address of handle to find
@@ -275,6 +290,7 @@ retry:
 	nsm->sm_name = (char *) (nsm + 1);
 	memcpy(nsm->sm_name, hostname, hostname_len);
 	nsm->sm_name[hostname_len] = '\0';
+	nsm_init_private(nsm);
 	nsm_display_address((struct sockaddr *)&nsm->sm_addr,
 				nsm->sm_addrbuf, sizeof(nsm->sm_addrbuf));
 	atomic_set(&nsm->sm_count, 1);
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 8d71536..194fa8a 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -85,6 +85,7 @@ struct nsm_handle {
 	size_t			sm_addrlen;
 	unsigned int		sm_monitored : 1,
 				sm_sticky : 1;	/* don't unmonitor */
+	struct nsm_private	sm_priv;
 	char			sm_addrbuf[NSM_ADDRBUF];
 };
 
diff --git a/include/linux/lockd/sm_inter.h b/include/linux/lockd/sm_inter.h
index 116bf38..5cef5a7 100644
--- a/include/linux/lockd/sm_inter.h
+++ b/include/linux/lockd/sm_inter.h
@@ -10,7 +10,6 @@
 #define LINUX_LOCKD_SM_INTER_H
 
 #define SM_MAXSTRLEN	1024
-#define SM_PRIV_SIZE	16
 
 extern int	nsm_local_state;
 
diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
index d6b3a80..6b51992 100644
--- a/include/linux/lockd/xdr.h
+++ b/include/linux/lockd/xdr.h
@@ -13,6 +13,12 @@
 #include <linux/nfs.h>
 #include <linux/sunrpc/xdr.h>
 
+#define SM_PRIV_SIZE		16
+
+struct nsm_private {
+	unsigned char		data[SM_PRIV_SIZE];
+};
+
 struct svc_rqst;
 
 #define NLM_MAXCOOKIELEN    	32

[PATCH 12/27] NSM: Encode the new "priv" cookie for NSMPROC_MON requests

[Chuck Lever]

Pass the new "priv" cookie to NSMPROC_MON's XDR encoder, instead of
creating the "priv" argument in the encoder at call time.

This patch should not cause a behavioral change: the contents of the
cookie remain the same for the time being.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index a6504f4..e0a799d 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -31,7 +31,7 @@ enum {
 };
 
 struct nsm_args {
-	__be32			addr;		/* remote address */
+	struct nsm_private	*priv;
 	u32			prog;		/* RPC callback info */
 	u32			vers;
 	u32			proc;
@@ -102,7 +102,7 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
 	struct rpc_clnt	*clnt;
 	int		status;
 	struct nsm_args args = {
-		.addr		= nsm_addr_in(nsm)->sin_addr.s_addr,
+		.priv		= &nsm->sm_priv,
 		.prog		= NLM_PROGRAM,
 		.vers		= 3,
 		.proc		= NLMPROC_NSM_NOTIFY,
@@ -408,9 +408,6 @@ static int encode_mon_id(struct xdr_stream *xdr, const struct nsm_args *argp)
  * The "priv" argument may contain private information required
  * by the NSMPROC_MON call. This information will be supplied in the
  * NLMPROC_SM_NOTIFY call.
- *
- * Linux provides the raw IP address of the monitored host,
- * left in network byte order.
  */
 static int encode_priv(struct xdr_stream *xdr, const struct nsm_args *argp)
 {
@@ -419,10 +416,7 @@ static int encode_priv(struct xdr_stream *xdr, const struct nsm_args *argp)
 	p = xdr_reserve_space(xdr, SM_PRIV_SIZE);
 	if (unlikely(p == NULL))
 		return -EIO;
-	*p++ = argp->addr;
-	*p++ = 0;
-	*p++ = 0;
-	*p++ = 0;
+	xdr_encode_opaque_fixed(p, argp->priv->data, SM_PRIV_SIZE);
 	return 0;
 }
 

[PATCH 13/27] NLM: Change nlm_host_rebooted() to take a single nlm_reboot argument

[Chuck Lever]

Pass the nlm_reboot data structure directly from the NLMPROC_SM_NOTIFY
XDR decoders to nlm_host_rebooted().  This eliminates some packing and
unpacking of the NLMPROC_SM_NOTIFY results, and prepares for passing
these results, including the "priv" cookie, directly to a lookup
routine in fs/lockd/mon.c.

This patch changes code organization but should not cause any
behavioral change.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/host.c             |   31 +++++++++++++++++--------------
 fs/lockd/svc4proc.c         |   11 +----------
 fs/lockd/svcproc.c          |   11 +----------
 include/linux/lockd/lockd.h |    3 +--
 4 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index dbdeaa8..ed10338 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -444,31 +444,34 @@ void nlm_release_host(struct nlm_host *host)
 	}
 }
 
-/*
- * We were notified that the host indicated by address &sin
- * has rebooted.
- * Release all resources held by that peer.
+/**
+ * nlm_host_rebooted - Release all resources held by rebooted host
+ * @info: pointer to decoded results of NLM_SM_NOTIFY call
+ *
+ * We were notified that the specified host has rebooted.  Release
+ * all resources held by that peer.
  */
-void nlm_host_rebooted(const struct sockaddr_in *sin,
-				const char *hostname,
-				unsigned int hostname_len,
-				u32 new_state)
+void nlm_host_rebooted(const struct nlm_reboot *info)
 {
+	const struct sockaddr_in sin = {
+		.sin_family		= AF_INET,
+		.sin_addr.s_addr	= info->addr,
+	};
 	struct hlist_head *chain;
 	struct hlist_node *pos;
 	struct nsm_handle *nsm;
 	struct nlm_host	*host;
 
-	nsm = nsm_find((struct sockaddr *)sin, sizeof(*sin),
-			hostname, hostname_len, 0);
+	nsm = nsm_find((struct sockaddr *)&sin, sizeof(sin),
+			info->mon, info->len, 0);
 	if (nsm == NULL) {
 		dprintk("lockd: never saw rebooted peer '%.*s' before\n",
-				hostname_len, hostname);
+				info->len, info->mon);
 		return;
 	}
 
 	dprintk("lockd: nlm_host_rebooted(%.*s, %s)\n",
-			hostname_len, hostname, nsm->sm_addrbuf);
+			info->len, info->mon, nsm->sm_addrbuf);
 
 	/* When reclaiming locks on this peer, make sure that
 	 * we set up a new notification */
@@ -483,8 +486,8 @@ again:	mutex_lock(&nlm_host_mutex);
 	for (chain = nlm_hosts; chain < nlm_hosts + NLM_HOST_NRHASH; ++chain) {
 		hlist_for_each_entry(host, pos, chain, h_hash) {
 			if (host->h_nsmhandle == nsm
-			 && host->h_nsmstate != new_state) {
-				host->h_nsmstate = new_state;
+			 && host->h_nsmstate != info->state) {
+				host->h_nsmstate = info->state;
 				host->h_state++;
 
 				nlm_get_host(host);
diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 4dfdcbc..bb79a53 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -419,8 +419,6 @@ static __be32
 nlm4svc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
 					      void	        *resp)
 {
-	struct sockaddr_in	saddr;
-
 	dprintk("lockd: SM_NOTIFY     called\n");
 
 	if (!nlm_privileged_requester(rqstp)) {
@@ -430,14 +428,7 @@ nlm4svc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
 		return rpc_system_err;
 	}
 
-	/* Obtain the host pointer for this NFS server and try to
-	 * reclaim all locks we hold on this server.
-	 */
-	memset(&saddr, 0, sizeof(saddr));
-	saddr.sin_family = AF_INET;
-	saddr.sin_addr.s_addr = argp->addr;
-	nlm_host_rebooted(&saddr, argp->mon, argp->len, argp->state);
-
+	nlm_host_rebooted(argp);
 	return rpc_success;
 }
 
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index 3ca89e2..e44310c 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -451,8 +451,6 @@ static __be32
 nlmsvc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
 					      void	        *resp)
 {
-	struct sockaddr_in	saddr;
-
 	dprintk("lockd: SM_NOTIFY     called\n");
 
 	if (!nlm_privileged_requester(rqstp)) {
@@ -462,14 +460,7 @@ nlmsvc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
 		return rpc_system_err;
 	}
 
-	/* Obtain the host pointer for this NFS server and try to
-	 * reclaim all locks we hold on this server.
-	 */
-	memset(&saddr, 0, sizeof(saddr));
-	saddr.sin_family = AF_INET;
-	saddr.sin_addr.s_addr = argp->addr;
-	nlm_host_rebooted(&saddr, argp->mon, argp->len, argp->state);
-
+	nlm_host_rebooted(argp);
 	return rpc_success;
 }
 
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 194fa8a..2a3533e 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -239,8 +239,7 @@ void		  nlm_rebind_host(struct nlm_host *);
 struct nlm_host * nlm_get_host(struct nlm_host *);
 void		  nlm_release_host(struct nlm_host *);
 void		  nlm_shutdown_hosts(void);
-extern void	  nlm_host_rebooted(const struct sockaddr_in *, const char *,
-					unsigned int, u32);
+void		  nlm_host_rebooted(const struct nlm_reboot *);
 
 /*
  * Host monitoring

[PATCH 14/27] NLM: Decode "priv" argument of NLMPROC_SM_NOTIFY as an opaque

[Chuck Lever]

The NLM XDR decoders for the NLMPROC_SM_NOTIFY procedure should treat
their "priv" argument truly as an opaque, as defined by the protocol,
and let the upper layers figure out what is in it.

This will make it easier to modify the contents and interpretation of
the "priv" argument, and keep knowledge about what's in "priv" local
to fs/lockd/mon.c.

For now, the NLM and NSM implementations should behave exactly as they
did before.

The formation of the address of the rebooted host in
nlm_host_rebooted() may look a little strange, but it is the inverse
of how nsm_init_private() forms the private cookie.  Plus, it's
going away soon anyway.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/host.c           |    3 ++-
 fs/lockd/xdr.c            |    4 ++--
 fs/lockd/xdr4.c           |    4 ++--
 include/linux/lockd/xdr.h |    8 ++++----
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index ed10338..dc41e46 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -453,9 +453,10 @@ void nlm_release_host(struct nlm_host *host)
  */
 void nlm_host_rebooted(const struct nlm_reboot *info)
 {
+	__be32 *p = (__be32 *)&info->priv.data;
 	const struct sockaddr_in sin = {
 		.sin_family		= AF_INET,
-		.sin_addr.s_addr	= info->addr,
+		.sin_addr.s_addr	= *p,
 	};
 	struct hlist_head *chain;
 	struct hlist_node *pos;
diff --git a/fs/lockd/xdr.c b/fs/lockd/xdr.c
index 1f22629..4cc7d01 100644
--- a/fs/lockd/xdr.c
+++ b/fs/lockd/xdr.c
@@ -349,8 +349,8 @@ nlmsvc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct nlm_reboot *argp)
 	if (!(p = xdr_decode_string_inplace(p, &argp->mon, &argp->len, SM_MAXSTRLEN)))
 		return 0;
 	argp->state = ntohl(*p++);
-	/* Preserve the address in network byte order */
-	argp->addr = *p++;
+	memcpy(&argp->priv.data, p, sizeof(argp->priv.data));
+	p += XDR_QUADLEN(SM_PRIV_SIZE);
 	return xdr_argsize_check(rqstp, p);
 }
 
diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
index 50c493a..61d1714 100644
--- a/fs/lockd/xdr4.c
+++ b/fs/lockd/xdr4.c
@@ -356,8 +356,8 @@ nlm4svc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct nlm_reboot *argp
 	if (!(p = xdr_decode_string_inplace(p, &argp->mon, &argp->len, SM_MAXSTRLEN)))
 		return 0;
 	argp->state = ntohl(*p++);
-	/* Preserve the address in network byte order */
-	argp->addr  = *p++;
+	memcpy(&argp->priv.data, p, sizeof(argp->priv.data));
+	p += XDR_QUADLEN(SM_PRIV_SIZE);
 	return xdr_argsize_check(rqstp, p);
 }
 
diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
index 6b51992..6338866 100644
--- a/include/linux/lockd/xdr.h
+++ b/include/linux/lockd/xdr.h
@@ -83,10 +83,10 @@ struct nlm_res {
  * statd callback when client has rebooted
  */
 struct nlm_reboot {
-	char *		mon;
-	unsigned int	len;
-	u32		state;
-	__be32		addr;
+	char			*mon;
+	unsigned int		len;
+	u32			state;
+	struct nsm_private	priv;
 };
 
 /*

Re: [PATCH 14/27] NLM: Decode "priv" argument of NLMPROC_SM_NOTIFY as an opaque

[J. Bruce Fields]

On Fri, Dec 05, 2008 at 07:03:39PM -0500, Chuck Lever wrote:
> The NLM XDR decoders for the NLMPROC_SM_NOTIFY procedure should treat
> their "priv" argument truly as an opaque, as defined by the protocol,
> and let the upper layers figure out what is in it.
> 
> This will make it easier to modify the contents and interpretation of
> the "priv" argument, and keep knowledge about what's in "priv" local
> to fs/lockd/mon.c.
> 
> For now, the NLM and NSM implementations should behave exactly as they
> did before.
> 
> The formation of the address of the rebooted host in
> nlm_host_rebooted() may look a little strange, but it is the inverse
> of how nsm_init_private() forms the private cookie.  Plus, it's
> going away soon anyway.

OK, applied these four.

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/host.c           |    3 ++-
>  fs/lockd/xdr.c            |    4 ++--
>  fs/lockd/xdr4.c           |    4 ++--
>  include/linux/lockd/xdr.h |    8 ++++----
>  4 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index ed10338..dc41e46 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -453,9 +453,10 @@ void nlm_release_host(struct nlm_host *host)
>   */
>  void nlm_host_rebooted(const struct nlm_reboot *info)
>  {
> +	__be32 *p = (__be32 *)&info->priv.data;
>  	const struct sockaddr_in sin = {
>  		.sin_family		= AF_INET,
> -		.sin_addr.s_addr	= info->addr,
> +		.sin_addr.s_addr	= *p,
>  	};
>  	struct hlist_head *chain;
>  	struct hlist_node *pos;
> diff --git a/fs/lockd/xdr.c b/fs/lockd/xdr.c
> index 1f22629..4cc7d01 100644
> --- a/fs/lockd/xdr.c
> +++ b/fs/lockd/xdr.c
> @@ -349,8 +349,8 @@ nlmsvc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct nlm_reboot *argp)
>  	if (!(p = xdr_decode_string_inplace(p, &argp->mon, &argp->len, SM_MAXSTRLEN)))
>  		return 0;
>  	argp->state = ntohl(*p++);
> -	/* Preserve the address in network byte order */
> -	argp->addr = *p++;
> +	memcpy(&argp->priv.data, p, sizeof(argp->priv.data));
> +	p += XDR_QUADLEN(SM_PRIV_SIZE);
>  	return xdr_argsize_check(rqstp, p);
>  }
>  
> diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
> index 50c493a..61d1714 100644
> --- a/fs/lockd/xdr4.c
> +++ b/fs/lockd/xdr4.c
> @@ -356,8 +356,8 @@ nlm4svc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct nlm_reboot *argp
>  	if (!(p = xdr_decode_string_inplace(p, &argp->mon, &argp->len, SM_MAXSTRLEN)))
>  		return 0;
>  	argp->state = ntohl(*p++);
> -	/* Preserve the address in network byte order */
> -	argp->addr  = *p++;
> +	memcpy(&argp->priv.data, p, sizeof(argp->priv.data));
> +	p += XDR_QUADLEN(SM_PRIV_SIZE);
>  	return xdr_argsize_check(rqstp, p);
>  }
>  
> diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
> index 6b51992..6338866 100644
> --- a/include/linux/lockd/xdr.h
> +++ b/include/linux/lockd/xdr.h
> @@ -83,10 +83,10 @@ struct nlm_res {
>   * statd callback when client has rebooted
>   */
>  struct nlm_reboot {
> -	char *		mon;
> -	unsigned int	len;
> -	u32		state;
> -	__be32		addr;
> +	char			*mon;
> +	unsigned int		len;
> +	u32			state;
> +	struct nsm_private	priv;
>  };
>  
>  /*
> 

[PATCH 15/27] NSM: Add nsm_lookup() function

[Chuck Lever]

Introduce a new API to fs/lockd/mon.c that allows nlm_host_rebooted()
to lookup up nsm_handles via the contents of an nlm_reboot struct.

The new function is equivalent to calling nsm_find() with @create set
to zero, but it takes a struct nlm_reboot instead of separate
arguments.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c              |   64 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/lockd/lockd.h |    1 +
 2 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index e0a799d..65fb904 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -202,6 +202,29 @@ void nsm_unmonitor(const struct nlm_host *host)
 	}
 }
 
+static struct nsm_handle *nsm_lookup_hostname(const char *hostname,
+					      const size_t len)
+{
+	struct nsm_handle *nsm;
+
+	list_for_each_entry(nsm, &nsm_handles, sm_link)
+		if (strlen(nsm->sm_name) == len &&
+		    memcmp(nsm->sm_name, hostname, len) == 0)
+			return nsm;
+	return NULL;
+}
+
+static struct nsm_handle *nsm_lookup_priv(const struct nsm_private *priv)
+{
+	struct nsm_handle *nsm;
+
+	list_for_each_entry(nsm, &nsm_handles, sm_link)
+		if (memcmp(nsm->sm_priv.data, priv->data,
+					sizeof(priv->data)) == 0)
+			return nsm;
+	return NULL;
+}
+
 /*
  * Construct a unique cookie to match this nsm_handle to this monitored
  * host.  It is passed to the local rpc.statd via NSMPROC_MON, and
@@ -298,6 +321,47 @@ retry:
 }
 
 /**
+ * nsm_reboot_lookup - match NLMPROC_SM_NOTIFY arguments to an nsm_handle
+ * @info: pointer to NLMPROC_SM_NOTIFY arguments
+ *
+ * Returns a matching nsm_handle if found in the nsm cache; the returned
+ * nsm_handle's reference count is bumped and sm_monitored is cleared.
+ * Otherwise returns NULL if some error occurred.
+ */
+struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info)
+{
+	struct nsm_handle *cached;
+
+	spin_lock(&nsm_lock);
+
+	if (nsm_use_hostnames && info->mon != NULL)
+		cached = nsm_lookup_hostname(info->mon, info->len);
+	else
+		cached = nsm_lookup_priv(&info->priv);
+
+	if (unlikely(cached == NULL)) {
+		spin_unlock(&nsm_lock);
+		dprintk("lockd: never saw rebooted peer '%.*s' before\n",
+				info->len, info->mon);
+		return cached;
+	}
+
+	atomic_inc(&cached->sm_count);
+	spin_unlock(&nsm_lock);
+
+	/*
+	 * During subsequent lock activity, force a fresh
+	 * notification to be set up for this host.
+	 */
+	cached->sm_monitored = 0;
+
+	dprintk("lockd: host %s (%s) rebooted, cnt %d\n",
+			cached->sm_name, cached->sm_addrbuf,
+			atomic_read(&cached->sm_count));
+	return cached;
+}
+
+/**
  * nsm_release - Release an NSM handle
  * @nsm: pointer to handle to be released
  *
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 2a3533e..5e3ad92 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -251,6 +251,7 @@ struct nsm_handle *nsm_find(const struct sockaddr *sap, const size_t salen,
 					const char *hostname,
 					const size_t hostname_len,
 					const int create);
+struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info);
 void		  nsm_release(struct nsm_handle *nsm);
 
 /*

Re: [PATCH 15/27] NSM: Add nsm_lookup() function

[J. Bruce Fields]

On Fri, Dec 05, 2008 at 07:03:46PM -0500, Chuck Lever wrote:
> Introduce a new API to fs/lockd/mon.c that allows nlm_host_rebooted()
> to lookup up nsm_handles via the contents of an nlm_reboot struct.
> 
> The new function is equivalent to calling nsm_find() with @create set
> to zero, but it takes a struct nlm_reboot instead of separate
> arguments.

For next time: would prefer this combined with the following patch (on
the try-to-introduce-new-code-with-its-users principal), but OK for now;
applied.--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/mon.c              |   64 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/lockd/lockd.h |    1 +
>  2 files changed, 65 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index e0a799d..65fb904 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -202,6 +202,29 @@ void nsm_unmonitor(const struct nlm_host *host)
>  	}
>  }
>  
> +static struct nsm_handle *nsm_lookup_hostname(const char *hostname,
> +					      const size_t len)
> +{
> +	struct nsm_handle *nsm;
> +
> +	list_for_each_entry(nsm, &nsm_handles, sm_link)
> +		if (strlen(nsm->sm_name) == len &&
> +		    memcmp(nsm->sm_name, hostname, len) == 0)
> +			return nsm;
> +	return NULL;
> +}
> +
> +static struct nsm_handle *nsm_lookup_priv(const struct nsm_private *priv)
> +{
> +	struct nsm_handle *nsm;
> +
> +	list_for_each_entry(nsm, &nsm_handles, sm_link)
> +		if (memcmp(nsm->sm_priv.data, priv->data,
> +					sizeof(priv->data)) == 0)
> +			return nsm;
> +	return NULL;
> +}
> +
>  /*
>   * Construct a unique cookie to match this nsm_handle to this monitored
>   * host.  It is passed to the local rpc.statd via NSMPROC_MON, and
> @@ -298,6 +321,47 @@ retry:
>  }
>  
>  /**
> + * nsm_reboot_lookup - match NLMPROC_SM_NOTIFY arguments to an nsm_handle
> + * @info: pointer to NLMPROC_SM_NOTIFY arguments
> + *
> + * Returns a matching nsm_handle if found in the nsm cache; the returned
> + * nsm_handle's reference count is bumped and sm_monitored is cleared.
> + * Otherwise returns NULL if some error occurred.
> + */
> +struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info)
> +{
> +	struct nsm_handle *cached;
> +
> +	spin_lock(&nsm_lock);
> +
> +	if (nsm_use_hostnames && info->mon != NULL)
> +		cached = nsm_lookup_hostname(info->mon, info->len);
> +	else
> +		cached = nsm_lookup_priv(&info->priv);
> +
> +	if (unlikely(cached == NULL)) {
> +		spin_unlock(&nsm_lock);
> +		dprintk("lockd: never saw rebooted peer '%.*s' before\n",
> +				info->len, info->mon);
> +		return cached;
> +	}
> +
> +	atomic_inc(&cached->sm_count);
> +	spin_unlock(&nsm_lock);
> +
> +	/*
> +	 * During subsequent lock activity, force a fresh
> +	 * notification to be set up for this host.
> +	 */
> +	cached->sm_monitored = 0;
> +
> +	dprintk("lockd: host %s (%s) rebooted, cnt %d\n",
> +			cached->sm_name, cached->sm_addrbuf,
> +			atomic_read(&cached->sm_count));
> +	return cached;
> +}
> +
> +/**
>   * nsm_release - Release an NSM handle
>   * @nsm: pointer to handle to be released
>   *
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 2a3533e..5e3ad92 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -251,6 +251,7 @@ struct nsm_handle *nsm_find(const struct sockaddr *sap, const size_t salen,
>  					const char *hostname,
>  					const size_t hostname_len,
>  					const int create);
> +struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info);
>  void		  nsm_release(struct nsm_handle *nsm);
>  
>  /*
> 

[PATCH 16/27] NLM: Call nsm_reboot_lookup() instead of nsm_find()

[Chuck Lever]

Invoke the newly introduced nsm_reboot_lookup() function in
nlm_host_rebooted() instead of nsm_find().

This introduces just one behavioral change: debugging messages
produced during reboot notification will now appear when the
NLMDBG_MONITOR flag is set, but not when the NLMDBG_HOSTCACHE flag
is set.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/host.c |   20 ++------------------
 1 files changed, 2 insertions(+), 18 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index dc41e46..230de93 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -453,30 +453,14 @@ void nlm_release_host(struct nlm_host *host)
  */
 void nlm_host_rebooted(const struct nlm_reboot *info)
 {
-	__be32 *p = (__be32 *)&info->priv.data;
-	const struct sockaddr_in sin = {
-		.sin_family		= AF_INET,
-		.sin_addr.s_addr	= *p,
-	};
 	struct hlist_head *chain;
 	struct hlist_node *pos;
 	struct nsm_handle *nsm;
 	struct nlm_host	*host;
 
-	nsm = nsm_find((struct sockaddr *)&sin, sizeof(sin),
-			info->mon, info->len, 0);
-	if (nsm == NULL) {
-		dprintk("lockd: never saw rebooted peer '%.*s' before\n",
-				info->len, info->mon);
+	nsm = nsm_reboot_lookup(info);
+	if (unlikely(nsm == NULL))
 		return;
-	}
-
-	dprintk("lockd: nlm_host_rebooted(%.*s, %s)\n",
-			info->len, info->mon, nsm->sm_addrbuf);
-
-	/* When reclaiming locks on this peer, make sure that
-	 * we set up a new notification */
-	nsm->sm_monitored = 0;
 
 	/* Mark all hosts tied to this NSM state as having rebooted.
 	 * We run the loop repeatedly, because we drop the host table

[PATCH 17/27] NLM: Remove "create" argument from nsm_find()

[Chuck Lever]

Clean up: nsm_find() now has only one caller, and that caller
unconditionally sets the @create argument. Thus the @create
argument is no longer needed.

Since nsm_find() now has a more specific purpose, pick a more
appropriate name for it.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/host.c             |    4 ++--
 fs/lockd/mon.c              |   23 +++++++++--------------
 include/linux/lockd/lockd.h |    6 +++---
 3 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index 230de93..e5a65df 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -159,8 +159,8 @@ static struct nlm_host *nlm_lookup_host(struct nlm_lookup_host_info *ni)
 		atomic_inc(&nsm->sm_count);
 	else {
 		host = NULL;
-		nsm = nsm_find(ni->sap, ni->salen,
-				ni->hostname, ni->hostname_len, 1);
+		nsm = nsm_get_handle(ni->sap, ni->salen,
+					ni->hostname, ni->hostname_len);
 		if (!nsm) {
 			dprintk("lockd: nlm_lookup_host failed; "
 				"no nsm handle\n");
diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 65fb904..4267a57 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -241,24 +241,22 @@ static void nsm_init_private(struct nsm_handle *nsm)
 }
 
 /**
- * nsm_find - Find or create a cached nsm_handle
+ * nsm_get_handle - Find or create a cached nsm_handle
  * @sap: pointer to socket address of handle to find
  * @salen: length of socket address
  * @hostname: pointer to C string containing hostname to find
  * @hostname_len: length of C string
- * @create: one means create new handle if not found in cache
  *
- * Behavior is modulated by the global nsm_use_hostnames variable
- * and by the @create argument.
+ * Behavior is modulated by the global nsm_use_hostnames variable.
  *
- * Returns a cached nsm_handle after bumping its ref count, or if
- * @create is set, returns a fresh nsm_handle if a handle that
- * matches @sap and/or @hostname cannot be found in the handle cache.
- * Returns NULL if an error occurs.
+ * Returns a cached nsm_handle after bumping its ref count, or
+ * returns a fresh nsm_handle if a handle that matches @sap and/or
+ * @hostname cannot be found in the handle cache.  Returns NULL if
+ * an error occurs.
  */
-struct nsm_handle *nsm_find(const struct sockaddr *sap, const size_t salen,
-			    const char *hostname, const size_t hostname_len,
-			    const int create)
+struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
+				  const size_t salen, const char *hostname,
+				  const size_t hostname_len)
 {
 	struct nsm_handle *nsm = NULL;
 	struct nsm_handle *pos;
@@ -301,9 +299,6 @@ retry:
 
 	spin_unlock(&nsm_lock);
 
-	if (!create)
-		return NULL;
-
 	nsm = kzalloc(sizeof(*nsm) + hostname_len + 1, GFP_KERNEL);
 	if (nsm == NULL)
 		return NULL;
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 5e3ad92..1ccd49e 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -247,10 +247,10 @@ void		  nlm_host_rebooted(const struct nlm_reboot *);
 int		  nsm_monitor(const struct nlm_host *host);
 void		  nsm_unmonitor(const struct nlm_host *host);
 
-struct nsm_handle *nsm_find(const struct sockaddr *sap, const size_t salen,
+struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
+					const size_t salen,
 					const char *hostname,
-					const size_t hostname_len,
-					const int create);
+					const size_t hostname_len);
 struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info);
 void		  nsm_release(struct nsm_handle *nsm);
 

Re: [PATCH 17/27] NLM: Remove "create" argument from nsm_find()

[J. Bruce Fields]

On Fri, Dec 05, 2008 at 07:04:01PM -0500, Chuck Lever wrote:
> Clean up: nsm_find() now has only one caller, and that caller
> unconditionally sets the @create argument. Thus the @create
> argument is no longer needed.
> 
> Since nsm_find() now has a more specific purpose, pick a more
> appropriate name for it.

Thanks, applied.--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/host.c             |    4 ++--
>  fs/lockd/mon.c              |   23 +++++++++--------------
>  include/linux/lockd/lockd.h |    6 +++---
>  3 files changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index 230de93..e5a65df 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -159,8 +159,8 @@ static struct nlm_host *nlm_lookup_host(struct nlm_lookup_host_info *ni)
>  		atomic_inc(&nsm->sm_count);
>  	else {
>  		host = NULL;
> -		nsm = nsm_find(ni->sap, ni->salen,
> -				ni->hostname, ni->hostname_len, 1);
> +		nsm = nsm_get_handle(ni->sap, ni->salen,
> +					ni->hostname, ni->hostname_len);
>  		if (!nsm) {
>  			dprintk("lockd: nlm_lookup_host failed; "
>  				"no nsm handle\n");
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index 65fb904..4267a57 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -241,24 +241,22 @@ static void nsm_init_private(struct nsm_handle *nsm)
>  }
>  
>  /**
> - * nsm_find - Find or create a cached nsm_handle
> + * nsm_get_handle - Find or create a cached nsm_handle
>   * @sap: pointer to socket address of handle to find
>   * @salen: length of socket address
>   * @hostname: pointer to C string containing hostname to find
>   * @hostname_len: length of C string
> - * @create: one means create new handle if not found in cache
>   *
> - * Behavior is modulated by the global nsm_use_hostnames variable
> - * and by the @create argument.
> + * Behavior is modulated by the global nsm_use_hostnames variable.
>   *
> - * Returns a cached nsm_handle after bumping its ref count, or if
> - * @create is set, returns a fresh nsm_handle if a handle that
> - * matches @sap and/or @hostname cannot be found in the handle cache.
> - * Returns NULL if an error occurs.
> + * Returns a cached nsm_handle after bumping its ref count, or
> + * returns a fresh nsm_handle if a handle that matches @sap and/or
> + * @hostname cannot be found in the handle cache.  Returns NULL if
> + * an error occurs.
>   */
> -struct nsm_handle *nsm_find(const struct sockaddr *sap, const size_t salen,
> -			    const char *hostname, const size_t hostname_len,
> -			    const int create)
> +struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
> +				  const size_t salen, const char *hostname,
> +				  const size_t hostname_len)
>  {
>  	struct nsm_handle *nsm = NULL;
>  	struct nsm_handle *pos;
> @@ -301,9 +299,6 @@ retry:
>  
>  	spin_unlock(&nsm_lock);
>  
> -	if (!create)
> -		return NULL;
> -
>  	nsm = kzalloc(sizeof(*nsm) + hostname_len + 1, GFP_KERNEL);
>  	if (nsm == NULL)
>  		return NULL;
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 5e3ad92..1ccd49e 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -247,10 +247,10 @@ void		  nlm_host_rebooted(const struct nlm_reboot *);
>  int		  nsm_monitor(const struct nlm_host *host);
>  void		  nsm_unmonitor(const struct nlm_host *host);
>  
> -struct nsm_handle *nsm_find(const struct sockaddr *sap, const size_t salen,
> +struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
> +					const size_t salen,
>  					const char *hostname,
> -					const size_t hostname_len,
> -					const int create);
> +					const size_t hostname_len);
>  struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info);
>  void		  nsm_release(struct nsm_handle *nsm);
>  
> 

[PATCH 18/27] NSM: Refactor nsm_handle creation into a helper function

[Chuck Lever]

Clean up.

We're about to get rid of the "goto retry" in nsm_get_handle().
To facilitate this, move the nsm_handle initialization logic to
a helper function.

Fields are initialized in increasing address order to make efficient
use of CPU caches.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c |   41 ++++++++++++++++++++++++++++-------------
 1 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 4267a57..24857a8 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -240,6 +240,30 @@ static void nsm_init_private(struct nsm_handle *nsm)
 	*p = nsm_addr_in(nsm)->sin_addr.s_addr;
 }
 
+static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
+					    const size_t salen,
+					    const char *hostname,
+					    const size_t hostname_len)
+{
+	struct nsm_handle *new;
+
+	new = kzalloc(sizeof(*new) + hostname_len + 1, GFP_KERNEL);
+	if (unlikely(new == NULL))
+		return NULL;
+
+	atomic_set(&new->sm_count, 1);
+	new->sm_name = (char *)(new + 1);
+	memcpy(nsm_addr(new), sap, salen);
+	new->sm_addrlen = salen;
+	nsm_init_private(new);
+	nsm_display_address((const struct sockaddr *)&new->sm_addr,
+				new->sm_addrbuf, sizeof(new->sm_addrbuf));
+	memcpy(new->sm_name, hostname, hostname_len);
+	new->sm_name[hostname_len] = '\0';
+
+	return new;
+}
+
 /**
  * nsm_get_handle - Find or create a cached nsm_handle
  * @sap: pointer to socket address of handle to find
@@ -299,20 +323,11 @@ retry:
 
 	spin_unlock(&nsm_lock);
 
-	nsm = kzalloc(sizeof(*nsm) + hostname_len + 1, GFP_KERNEL);
-	if (nsm == NULL)
-		return NULL;
+	nsm = nsm_create_handle(sap, salen, hostname, hostname_len);
+	if (nsm != NULL)
+		goto retry;
 
-	memcpy(nsm_addr(nsm), sap, salen);
-	nsm->sm_addrlen = salen;
-	nsm->sm_name = (char *) (nsm + 1);
-	memcpy(nsm->sm_name, hostname, hostname_len);
-	nsm->sm_name[hostname_len] = '\0';
-	nsm_init_private(nsm);
-	nsm_display_address((struct sockaddr *)&nsm->sm_addr,
-				nsm->sm_addrbuf, sizeof(nsm->sm_addrbuf));
-	atomic_set(&nsm->sm_count, 1);
-	goto retry;
+	return NULL;
 }
 
 /**

Re: [PATCH 18/27] NSM: Refactor nsm_handle creation into a helper function

[J. Bruce Fields]

On Fri, Dec 05, 2008 at 07:04:09PM -0500, Chuck Lever wrote:
> Clean up.

The nsm_create_handle() thing is fine, but

> 
> We're about to get rid of the "goto retry" in nsm_get_handle().

I'm not that interested in removing the "goto retry".  I realize tastes
differ here, but I don't see a great improvement.  The:

	retry:
		look for something
		oops, not there, allocate a new one
		goto retry

pattern is pretty common and I'm comfortable with it.

There's also a few conflicts that result from my dropping the changes
that move the dprintk's out from under the spinlock, and I'm getting
lazy....  Could you humor me here and rebase these last 10 patches on my
latest for-2.6.29:

	git://linux-nfs.org/~bfields/linux.git for-2.6.29

without futzing with this goto?

>From a quick skim the rest of the patches they seem reasonable.  If you
have more 2.6.29 patches, this'd be a good time to send them along as
well.

--b.

> To facilitate this, move the nsm_handle initialization logic to
> a helper function.
> 
> Fields are initialized in increasing address order to make efficient
> use of CPU caches.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/mon.c |   41 ++++++++++++++++++++++++++++-------------
>  1 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index 4267a57..24857a8 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -240,6 +240,30 @@ static void nsm_init_private(struct nsm_handle *nsm)
>  	*p = nsm_addr_in(nsm)->sin_addr.s_addr;
>  }
>  
> +static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
> +					    const size_t salen,
> +					    const char *hostname,
> +					    const size_t hostname_len)
> +{
> +	struct nsm_handle *new;
> +
> +	new = kzalloc(sizeof(*new) + hostname_len + 1, GFP_KERNEL);
> +	if (unlikely(new == NULL))
> +		return NULL;
> +
> +	atomic_set(&new->sm_count, 1);
> +	new->sm_name = (char *)(new + 1);
> +	memcpy(nsm_addr(new), sap, salen);
> +	new->sm_addrlen = salen;
> +	nsm_init_private(new);
> +	nsm_display_address((const struct sockaddr *)&new->sm_addr,
> +				new->sm_addrbuf, sizeof(new->sm_addrbuf));
> +	memcpy(new->sm_name, hostname, hostname_len);
> +	new->sm_name[hostname_len] = '\0';
> +
> +	return new;
> +}
> +
>  /**
>   * nsm_get_handle - Find or create a cached nsm_handle
>   * @sap: pointer to socket address of handle to find
> @@ -299,20 +323,11 @@ retry:
>  
>  	spin_unlock(&nsm_lock);
>  
> -	nsm = kzalloc(sizeof(*nsm) + hostname_len + 1, GFP_KERNEL);
> -	if (nsm == NULL)
> -		return NULL;
> +	nsm = nsm_create_handle(sap, salen, hostname, hostname_len);
> +	if (nsm != NULL)
> +		goto retry;
>  
> -	memcpy(nsm_addr(nsm), sap, salen);
> -	nsm->sm_addrlen = salen;
> -	nsm->sm_name = (char *) (nsm + 1);
> -	memcpy(nsm->sm_name, hostname, hostname_len);
> -	nsm->sm_name[hostname_len] = '\0';
> -	nsm_init_private(nsm);
> -	nsm_display_address((struct sockaddr *)&nsm->sm_addr,
> -				nsm->sm_addrbuf, sizeof(nsm->sm_addrbuf));
> -	atomic_set(&nsm->sm_count, 1);
> -	goto retry;
> +	return NULL;
>  }
>  
>  /**
> 

Re: [PATCH 18/27] NSM: Refactor nsm_handle creation into a helper function

[Chuck Lever]

On Dec 10, 2008, at Dec 10, 2008, 6:28 PM, J. Bruce Fields wrote:
> On Fri, Dec 05, 2008 at 07:04:09PM -0500, Chuck Lever wrote:
>> Clean up.
>
> The nsm_create_handle() thing is fine, but
>
>>
>> We're about to get rid of the "goto retry" in nsm_get_handle().
>
> I'm not that interested in removing the "goto retry".  I realize  
> tastes
> differ here, but I don't see a great improvement.  The:
>
> 	retry:
> 		look for something
> 		oops, not there, allocate a new one
> 		goto retry
>
> pattern is pretty common and I'm comfortable with it.

Would we need the retry at all if we replaced nsm_lock with a mutex?   
nlm_lookup_host() holds its mutex across a kzalloc() call.  We could  
do the same here, and it would make this much more straightforward.

Is there any real need for the atomic_dec_and_lock in nsm_release(),  
for example?  That's not exactly a performance-critical code path.

> There's also a few conflicts that result from my dropping the changes
> that move the dprintk's out from under the spinlock, and I'm getting
> lazy....  Could you humor me here and rebase these last 10 patches  
> on my
> latest for-2.6.29:
>
> 	git://linux-nfs.org/~bfields/linux.git for-2.6.29
>
> without futzing with this goto?
>
> From a quick skim the rest of the patches they seem reasonable.  If  
> you
> have more 2.6.29 patches, this'd be a good time to send them along as
> well.

>> To facilitate this, move the nsm_handle initialization logic to
>> a helper function.
>>
>> Fields are initialized in increasing address order to make efficient
>> use of CPU caches.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/lockd/mon.c |   41 ++++++++++++++++++++++++++++-------------
>> 1 files changed, 28 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
>> index 4267a57..24857a8 100644
>> --- a/fs/lockd/mon.c
>> +++ b/fs/lockd/mon.c
>> @@ -240,6 +240,30 @@ static void nsm_init_private(struct nsm_handle  
>> *nsm)
>> 	*p = nsm_addr_in(nsm)->sin_addr.s_addr;
>> }
>>
>> +static struct nsm_handle *nsm_create_handle(const struct sockaddr  
>> *sap,
>> +					    const size_t salen,
>> +					    const char *hostname,
>> +					    const size_t hostname_len)
>> +{
>> +	struct nsm_handle *new;
>> +
>> +	new = kzalloc(sizeof(*new) + hostname_len + 1, GFP_KERNEL);
>> +	if (unlikely(new == NULL))
>> +		return NULL;
>> +
>> +	atomic_set(&new->sm_count, 1);
>> +	new->sm_name = (char *)(new + 1);
>> +	memcpy(nsm_addr(new), sap, salen);
>> +	new->sm_addrlen = salen;
>> +	nsm_init_private(new);
>> +	nsm_display_address((const struct sockaddr *)&new->sm_addr,
>> +				new->sm_addrbuf, sizeof(new->sm_addrbuf));
>> +	memcpy(new->sm_name, hostname, hostname_len);
>> +	new->sm_name[hostname_len] = '\0';
>> +
>> +	return new;
>> +}
>> +
>> /**
>>  * nsm_get_handle - Find or create a cached nsm_handle
>>  * @sap: pointer to socket address of handle to find
>> @@ -299,20 +323,11 @@ retry:
>>
>> 	spin_unlock(&nsm_lock);
>>
>> -	nsm = kzalloc(sizeof(*nsm) + hostname_len + 1, GFP_KERNEL);
>> -	if (nsm == NULL)
>> -		return NULL;
>> +	nsm = nsm_create_handle(sap, salen, hostname, hostname_len);
>> +	if (nsm != NULL)
>> +		goto retry;
>>
>> -	memcpy(nsm_addr(nsm), sap, salen);
>> -	nsm->sm_addrlen = salen;
>> -	nsm->sm_name = (char *) (nsm + 1);
>> -	memcpy(nsm->sm_name, hostname, hostname_len);
>> -	nsm->sm_name[hostname_len] = '\0';
>> -	nsm_init_private(nsm);
>> -	nsm_display_address((struct sockaddr *)&nsm->sm_addr,
>> -				nsm->sm_addrbuf, sizeof(nsm->sm_addrbuf));
>> -	atomic_set(&nsm->sm_count, 1);
>> -	goto retry;
>> +	return NULL;
>> }
>>
>> /**
>>

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 18/27] NSM: Refactor nsm_handle creation into a helper function

[J. Bruce Fields]

On Thu, Dec 11, 2008 at 12:09:49PM -0500, Chuck Lever wrote:
> On Dec 10, 2008, at Dec 10, 2008, 6:28 PM, J. Bruce Fields wrote:
>> On Fri, Dec 05, 2008 at 07:04:09PM -0500, Chuck Lever wrote:
>>> Clean up.
>>
>> The nsm_create_handle() thing is fine, but
>>
>>>
>>> We're about to get rid of the "goto retry" in nsm_get_handle().
>>
>> I'm not that interested in removing the "goto retry".  I realize  
>> tastes
>> differ here, but I don't see a great improvement.  The:
>>
>> 	retry:
>> 		look for something
>> 		oops, not there, allocate a new one
>> 		goto retry
>>
>> pattern is pretty common and I'm comfortable with it.
>
> Would we need the retry at all if we replaced nsm_lock with a mutex?   
> nlm_lookup_host() holds its mutex across a kzalloc() call.  We could do 
> the same here, and it would make this much more straightforward.
>
> Is there any real need for the atomic_dec_and_lock in nsm_release(), for 
> example?  That's not exactly a performance-critical code path.

Perhaps not, but I can't see any harm in leaving this as is.

--b.

[PATCH 19/27] NSM: More clean up of nsm_get_handle()

[Chuck Lever]

Clean up: refactor nsm_get_handle() so it is organized the same way that
nsm_reboot_lookup() is, and get rid of the "goto retry".

There is an additional micro-optimization here.  This change moves the
"hostname & nsm_use_hostnames" test out of the list_for_each_entry()
clause in nsm_get_handle(), since it is loop-invariant.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c |   69 +++++++++++++++++++++++++++++++-------------------------
 1 files changed, 38 insertions(+), 31 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 24857a8..a5f26f3 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -214,6 +214,16 @@ static struct nsm_handle *nsm_lookup_hostname(const char *hostname,
 	return NULL;
 }
 
+static struct nsm_handle *nsm_lookup_addr(const struct sockaddr *sap)
+{
+	struct nsm_handle *nsm;
+
+	list_for_each_entry(nsm, &nsm_handles, sm_link)
+		if (nlm_cmp_addr(nsm_addr(nsm), sap))
+			return nsm;
+	return NULL;
+}
+
 static struct nsm_handle *nsm_lookup_priv(const struct nsm_private *priv)
 {
 	struct nsm_handle *nsm;
@@ -282,8 +292,7 @@ struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
 				  const size_t salen, const char *hostname,
 				  const size_t hostname_len)
 {
-	struct nsm_handle *nsm = NULL;
-	struct nsm_handle *pos;
+	struct nsm_handle *cached, *new = NULL;
 
 	if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
 		if (printk_ratelimit()) {
@@ -294,39 +303,37 @@ struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
 		return NULL;
 	}
 
-retry:
-	spin_lock(&nsm_lock);
-	list_for_each_entry(pos, &nsm_handles, sm_link) {
-
-		if (hostname && nsm_use_hostnames) {
-			if (strlen(pos->sm_name) != hostname_len
-			 || memcmp(pos->sm_name, hostname, hostname_len))
-				continue;
-		} else if (!nlm_cmp_addr(nsm_addr(pos), sap))
-			continue;
-		atomic_inc(&pos->sm_count);
-		spin_unlock(&nsm_lock);
-		kfree(nsm);
-		dprintk("lockd: found nsm_handle for %s (%s), cnt %d\n",
-				pos->sm_name, pos->sm_addrbuf,
-				atomic_read(&pos->sm_count));
-		return pos;
-	}
+	do {
+		spin_lock(&nsm_lock);
 
-	if (nsm) {
-		list_add(&nsm->sm_link, &nsm_handles);
-		spin_unlock(&nsm_lock);
-		dprintk("lockd: created nsm_handle for %s (%s)\n",
-				nsm->sm_name, nsm->sm_addrbuf);
-		return nsm;
-	}
+		if (nsm_use_hostnames && hostname != NULL)
+			cached = nsm_lookup_hostname(hostname, hostname_len);
+		else
+			cached = nsm_lookup_addr(sap);
+
+		if (cached != NULL) {
+			atomic_inc(&cached->sm_count);
+			spin_unlock(&nsm_lock);
+			kfree(new);
+			dprintk("lockd: found nsm_handle for %s (%s), "
+					"cnt %d\n", cached->sm_name,
+					cached->sm_addrbuf,
+					atomic_read(&cached->sm_count));
+			return cached;
+		}
 
-	spin_unlock(&nsm_lock);
+		if (new != NULL) {
+			list_add(&new->sm_link, &nsm_handles);
+			spin_unlock(&nsm_lock);
+			dprintk("lockd: created nsm_handle for %s (%s)\n",
+					new->sm_name, new->sm_addrbuf);
+			return new;
+		}
 
-	nsm = nsm_create_handle(sap, salen, hostname, hostname_len);
-	if (nsm != NULL)
-		goto retry;
+		spin_unlock(&nsm_lock);
 
+		new = nsm_create_handle(sap, salen, hostname, hostname_len);
+	} while (new != NULL);
 	return NULL;
 }
 

[PATCH 20/27] NSM: Replace IP address as our nlm_reboot lookup key

[Chuck Lever]

NLM provides file locking services for NFS files.  Part of this service
includes a second protocol, known as NSM, which is a reboot
notification service.  NLM uses this service to determine when to
reclaim locks or enter a grace period after a client or server reboots.

The NLM service (implemented by lockd in the Linux kernel) contacts
the local NSM service (implemented by rpc.statd in Linux user space)
via NSM protocol upcalls to register a callback when a particular
remote peer reboots.

To match the callback to the correct remote peer, the NLM service
constructs a cookie that it passes in the request.  The NSM service
passes that cookie back to the NLM service when it is notified that
the given remote peer has indeed rebooted.

Currently on Linux, the cookie is the raw 32-bit IPv4 address of the
remote peer.  To support IPv6 addresses, which are larger, we could
use all 16 bytes of the cookie to represent a full IPv6 address,
although we still can't represent an IPv6 address with a scope ID in
just 16 bytes.

Instead, to avoid the need for future changes to support additional
address types, we'll use a manufactured value for the cookie, and use
that to find the corresponding nsm_handle struct in the kernel during
the NLMPROC_SM_NOTIFY callback.

This should provide complete support in the kernel's NSM
implementation for IPv6 hosts, while remaining backwards compatible
with older rpc.statd implementations.

Note we also deal with another case where nsm_use_hostnames can change
while there are outstanding notifications, possibly resulting in the
loss of reboot notifications.  After this patch, the priv cookie is
always used to lookup rebooted hosts in the kernel.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c |   39 ++++++++++++++++++++++++++++++---------
 1 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index a5f26f3..4113ed1 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -241,13 +241,38 @@ static struct nsm_handle *nsm_lookup_priv(const struct nsm_private *priv)
  * returned via NLMPROC_SM_NOTIFY, in the "priv" field of these
  * requests.
  *
- * Linux provides the raw IP address of the monitored host,
- * left in network byte order.
+ * These cookies are not required to last across reboots, but they
+ * must be unique for each nsm_handle during the same boot.
+ * Uniqueness is guaranteed by using the memory address of the
+ * nsm_handle data structure.  Such memory addresses are only reused
+ * if the nsm_handle is destroyed by an nsm_release().
+ *
+ * A time stamp is added in case rpc.statd returns a stale cookie.
+ * That would be a bug in rpc.statd, but it would result in some
+ * client losing its locks inappropriately, which we would like to
+ * avoid.
+ *
+ * For safety, the cookie returned via NLM_SM_NOTIFY is treated as
+ * an opaque -- the address is not used directly to access the
+ * associated nsm_handle.  This also means it would be simple to
+ * change the cookie generator again at some later point without
+ * having to mess with the nsm_handle lookup code in
+ * nsm_reboot_lookup().
+ *
+ * The cookies are exposed only to local user space via loopback.
+ * They do not appear on the physical network.  If we want greater
+ * security for some reason, nsm_init_private() could perform a
+ * one-way hash to obscure the contents of the cookie.
  */
 static void nsm_init_private(struct nsm_handle *nsm)
 {
-	__be32 *p = (__be32 *)&nsm->sm_priv.data;
-	*p = nsm_addr_in(nsm)->sin_addr.s_addr;
+	u64 *p = (u64 *)&nsm->sm_priv.data;
+	struct timeval tv;
+
+	do_gettimeofday(&tv);
+
+	*p++ = (unsigned long)nsm;
+	*p = timeval_to_ns(&tv);
 }
 
 static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
@@ -351,11 +376,7 @@ struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info)
 
 	spin_lock(&nsm_lock);
 
-	if (nsm_use_hostnames && info->mon != NULL)
-		cached = nsm_lookup_hostname(info->mon, info->len);
-	else
-		cached = nsm_lookup_priv(&info->priv);
-
+	cached = nsm_lookup_priv(&info->priv);
 	if (unlikely(cached == NULL)) {
 		spin_unlock(&nsm_lock);
 		dprintk("lockd: never saw rebooted peer '%.*s' before\n",

Re: [PATCH 20/27] NSM: Replace IP address as our nlm_reboot lookup key

[J. Bruce Fields]

On Fri, Dec 05, 2008 at 07:04:24PM -0500, Chuck Lever wrote:
> NLM provides file locking services for NFS files.  Part of this service
> includes a second protocol, known as NSM, which is a reboot
> notification service.  NLM uses this service to determine when to
> reclaim locks or enter a grace period after a client or server reboots.
> 
> The NLM service (implemented by lockd in the Linux kernel) contacts
> the local NSM service (implemented by rpc.statd in Linux user space)
> via NSM protocol upcalls to register a callback when a particular
> remote peer reboots.
> 
> To match the callback to the correct remote peer, the NLM service
> constructs a cookie that it passes in the request.  The NSM service
> passes that cookie back to the NLM service when it is notified that
> the given remote peer has indeed rebooted.
> 
> Currently on Linux, the cookie is the raw 32-bit IPv4 address of the
> remote peer.  To support IPv6 addresses, which are larger, we could
> use all 16 bytes of the cookie to represent a full IPv6 address,
> although we still can't represent an IPv6 address with a scope ID in
> just 16 bytes.
> 
> Instead, to avoid the need for future changes to support additional
> address types, we'll use a manufactured value for the cookie, and use
> that to find the corresponding nsm_handle struct in the kernel during
> the NLMPROC_SM_NOTIFY callback.
> 
> This should provide complete support in the kernel's NSM
> implementation for IPv6 hosts, while remaining backwards compatible
> with older rpc.statd implementations.
> 
> Note we also deal with another case where nsm_use_hostnames can change
> while there are outstanding notifications, possibly resulting in the
> loss of reboot notifications.  After this patch, the priv cookie is
> always used to lookup rebooted hosts in the kernel.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/mon.c |   39 ++++++++++++++++++++++++++++++---------
>  1 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index a5f26f3..4113ed1 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -241,13 +241,38 @@ static struct nsm_handle *nsm_lookup_priv(const struct nsm_private *priv)
>   * returned via NLMPROC_SM_NOTIFY, in the "priv" field of these
>   * requests.
>   *
> - * Linux provides the raw IP address of the monitored host,
> - * left in network byte order.
> + * These cookies are not required to last across reboots, but they
> + * must be unique for each nsm_handle during the same boot.
> + * Uniqueness is guaranteed by using the memory address of the
> + * nsm_handle data structure.  Such memory addresses are only reused
> + * if the nsm_handle is destroyed by an nsm_release().
> + *
> + * A time stamp is added in case rpc.statd returns a stale cookie.
> + * That would be a bug in rpc.statd, but it would result in some
> + * client losing its locks inappropriately, which we would like to
> + * avoid.

I forget what the arguments were against using a simple counter here.

> + *
> + * For safety, the cookie returned via NLM_SM_NOTIFY is treated as
> + * an opaque -- the address is not used directly to access the
> + * associated nsm_handle.  This also means it would be simple to
> + * change the cookie generator again at some later point without
> + * having to mess with the nsm_handle lookup code in
> + * nsm_reboot_lookup().
> + *
> + * The cookies are exposed only to local user space via loopback.
> + * They do not appear on the physical network.  If we want greater
> + * security for some reason, nsm_init_private() could perform a
> + * one-way hash to obscure the contents of the cookie.
>   */
>  static void nsm_init_private(struct nsm_handle *nsm)
>  {
> -	__be32 *p = (__be32 *)&nsm->sm_priv.data;
> -	*p = nsm_addr_in(nsm)->sin_addr.s_addr;
> +	u64 *p = (u64 *)&nsm->sm_priv.data;
> +	struct timeval tv;
> +
> +	do_gettimeofday(&tv);

I see there's a note on do_gettimeofday advising callers to use
getnstimeofday instead.

--b.

> +
> +	*p++ = (unsigned long)nsm;
> +	*p = timeval_to_ns(&tv);
>  }
>  
>  static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
> @@ -351,11 +376,7 @@ struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info)
>  
>  	spin_lock(&nsm_lock);
>  
> -	if (nsm_use_hostnames && info->mon != NULL)
> -		cached = nsm_lookup_hostname(info->mon, info->len);
> -	else
> -		cached = nsm_lookup_priv(&info->priv);
> -
> +	cached = nsm_lookup_priv(&info->priv);
>  	if (unlikely(cached == NULL)) {
>  		spin_unlock(&nsm_lock);
>  		dprintk("lockd: never saw rebooted peer '%.*s' before\n",
> 

Re: [PATCH 20/27] NSM: Replace IP address as our nlm_reboot lookup key

[Chuck Lever]

On Dec 10, 2008, at Dec 10, 2008, 6:43 PM, J. Bruce Fields wrote:
> On Fri, Dec 05, 2008 at 07:04:24PM -0500, Chuck Lever wrote:
>> NLM provides file locking services for NFS files.  Part of this  
>> service
>> includes a second protocol, known as NSM, which is a reboot
>> notification service.  NLM uses this service to determine when to
>> reclaim locks or enter a grace period after a client or server  
>> reboots.
>>
>> The NLM service (implemented by lockd in the Linux kernel) contacts
>> the local NSM service (implemented by rpc.statd in Linux user space)
>> via NSM protocol upcalls to register a callback when a particular
>> remote peer reboots.
>>
>> To match the callback to the correct remote peer, the NLM service
>> constructs a cookie that it passes in the request.  The NSM service
>> passes that cookie back to the NLM service when it is notified that
>> the given remote peer has indeed rebooted.
>>
>> Currently on Linux, the cookie is the raw 32-bit IPv4 address of the
>> remote peer.  To support IPv6 addresses, which are larger, we could
>> use all 16 bytes of the cookie to represent a full IPv6 address,
>> although we still can't represent an IPv6 address with a scope ID in
>> just 16 bytes.
>>
>> Instead, to avoid the need for future changes to support additional
>> address types, we'll use a manufactured value for the cookie, and use
>> that to find the corresponding nsm_handle struct in the kernel during
>> the NLMPROC_SM_NOTIFY callback.
>>
>> This should provide complete support in the kernel's NSM
>> implementation for IPv6 hosts, while remaining backwards compatible
>> with older rpc.statd implementations.
>>
>> Note we also deal with another case where nsm_use_hostnames can  
>> change
>> while there are outstanding notifications, possibly resulting in the
>> loss of reboot notifications.  After this patch, the priv cookie is
>> always used to lookup rebooted hosts in the kernel.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/lockd/mon.c |   39 ++++++++++++++++++++++++++++++---------
>> 1 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
>> index a5f26f3..4113ed1 100644
>> --- a/fs/lockd/mon.c
>> +++ b/fs/lockd/mon.c
>> @@ -241,13 +241,38 @@ static struct nsm_handle  
>> *nsm_lookup_priv(const struct nsm_private *priv)
>>  * returned via NLMPROC_SM_NOTIFY, in the "priv" field of these
>>  * requests.
>>  *
>> - * Linux provides the raw IP address of the monitored host,
>> - * left in network byte order.
>> + * These cookies are not required to last across reboots, but they
>> + * must be unique for each nsm_handle during the same boot.
>> + * Uniqueness is guaranteed by using the memory address of the
>> + * nsm_handle data structure.  Such memory addresses are only reused
>> + * if the nsm_handle is destroyed by an nsm_release().
>> + *
>> + * A time stamp is added in case rpc.statd returns a stale cookie.
>> + * That would be a bug in rpc.statd, but it would result in some
>> + * client losing its locks inappropriately, which we would like to
>> + * avoid.
>
> I forget what the arguments were against using a simple counter here.

Using a simple counter would replay the cookies after every reboot,  
exposing us to bugs in user space which would cause the kernel to  
think the wrong host rebooted.

There is also a best practice for network protocols that advise  
against using a simple counter for anything.

Using the memory address and a timestamp is an efficient way to get a  
good guarantee of uniqueness without having to store any state across  
reboots.

>> + *
>> + * For safety, the cookie returned via NLM_SM_NOTIFY is treated as
>> + * an opaque -- the address is not used directly to access the
>> + * associated nsm_handle.  This also means it would be simple to
>> + * change the cookie generator again at some later point without
>> + * having to mess with the nsm_handle lookup code in
>> + * nsm_reboot_lookup().
>> + *
>> + * The cookies are exposed only to local user space via loopback.
>> + * They do not appear on the physical network.  If we want greater
>> + * security for some reason, nsm_init_private() could perform a
>> + * one-way hash to obscure the contents of the cookie.
>>  */
>> static void nsm_init_private(struct nsm_handle *nsm)
>> {
>> -	__be32 *p = (__be32 *)&nsm->sm_priv.data;
>> -	*p = nsm_addr_in(nsm)->sin_addr.s_addr;
>> +	u64 *p = (u64 *)&nsm->sm_priv.data;
>> +	struct timeval tv;
>> +
>> +	do_gettimeofday(&tv);
>
> I see there's a note on do_gettimeofday advising callers to use
> getnstimeofday instead.

Sorry, there are a number of architecture-specific do_gettimeofday()  
functions that don't have that note.  I must have missed the generic  
one.

> --b.
>
>> +
>> +	*p++ = (unsigned long)nsm;
>> +	*p = timeval_to_ns(&tv);
>> }
>>
>> static struct nsm_handle *nsm_create_handle(const struct sockaddr  
>> *sap,
>> @@ -351,11 +376,7 @@ struct nsm_handle *nsm_reboot_lookup(const  
>> struct nlm_reboot *info)
>>
>> 	spin_lock(&nsm_lock);
>>
>> -	if (nsm_use_hostnames && info->mon != NULL)
>> -		cached = nsm_lookup_hostname(info->mon, info->len);
>> -	else
>> -		cached = nsm_lookup_priv(&info->priv);
>> -
>> +	cached = nsm_lookup_priv(&info->priv);
>> 	if (unlikely(cached == NULL)) {
>> 		spin_unlock(&nsm_lock);
>> 		dprintk("lockd: never saw rebooted peer '%.*s' before\n",
>>

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

[PATCH 21/27] NSM: Remove include/linux/lockd/sm_inter.h

[Chuck Lever]

Clean up: The include/linux/lockd/sm_inter.h header is nearly empty
now.  Remove it.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/clntproc.c            |    1 -
 fs/lockd/host.c                |    1 -
 fs/lockd/mon.c                 |    2 --
 fs/lockd/svc.c                 |    1 -
 fs/lockd/svc4proc.c            |    2 --
 fs/lockd/svcproc.c             |    2 --
 fs/lockd/svcsubs.c             |    1 -
 fs/lockd/xdr.c                 |    1 -
 fs/lockd/xdr4.c                |    1 -
 include/linux/lockd/lockd.h    |    1 +
 include/linux/lockd/sm_inter.h |   16 ----------------
 include/linux/lockd/xdr.h      |    1 +
 12 files changed, 2 insertions(+), 28 deletions(-)
 delete mode 100644 include/linux/lockd/sm_inter.h

diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index 5ce42e0..dd79570 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -16,7 +16,6 @@
 #include <linux/sunrpc/clnt.h>
 #include <linux/sunrpc/svc.h>
 #include <linux/lockd/lockd.h>
-#include <linux/lockd/sm_inter.h>
 
 #define NLMDBG_FACILITY		NLMDBG_CLIENT
 #define NLMCLNT_GRACE_WAIT	(5*HZ)
diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index e5a65df..99d737b 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -15,7 +15,6 @@
 #include <linux/sunrpc/clnt.h>
 #include <linux/sunrpc/svc.h>
 #include <linux/lockd/lockd.h>
-#include <linux/lockd/sm_inter.h>
 #include <linux/mutex.h>
 
 #include <net/ipv6.h>
diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 4113ed1..97011ee 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -13,8 +13,6 @@
 #include <linux/sunrpc/xprtsock.h>
 #include <linux/sunrpc/svc.h>
 #include <linux/lockd/lockd.h>
-#include <linux/lockd/sm_inter.h>
-
 
 #define NLMDBG_FACILITY		NLMDBG_MONITOR
 #define NSM_PROGRAM		100024
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index c631a83..50de426 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -35,7 +35,6 @@
 #include <linux/sunrpc/svcsock.h>
 #include <net/ip.h>
 #include <linux/lockd/lockd.h>
-#include <linux/lockd/sm_inter.h>
 #include <linux/nfs.h>
 
 #define NLMDBG_FACILITY		NLMDBG_SVC
diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index bb79a53..1725037 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -16,8 +16,6 @@
 #include <linux/nfsd/nfsd.h>
 #include <linux/lockd/lockd.h>
 #include <linux/lockd/share.h>
-#include <linux/lockd/sm_inter.h>
-
 
 #define NLMDBG_FACILITY		NLMDBG_CLIENT
 
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index e44310c..3688e55 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -16,8 +16,6 @@
 #include <linux/nfsd/nfsd.h>
 #include <linux/lockd/lockd.h>
 #include <linux/lockd/share.h>
-#include <linux/lockd/sm_inter.h>
-
 
 #define NLMDBG_FACILITY		NLMDBG_CLIENT
 
diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index 34c2766..9e4d6aa 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -17,7 +17,6 @@
 #include <linux/nfsd/export.h>
 #include <linux/lockd/lockd.h>
 #include <linux/lockd/share.h>
-#include <linux/lockd/sm_inter.h>
 #include <linux/module.h>
 #include <linux/mount.h>
 
diff --git a/fs/lockd/xdr.c b/fs/lockd/xdr.c
index 4cc7d01..0336f2b 100644
--- a/fs/lockd/xdr.c
+++ b/fs/lockd/xdr.c
@@ -16,7 +16,6 @@
 #include <linux/sunrpc/svc.h>
 #include <linux/sunrpc/stats.h>
 #include <linux/lockd/lockd.h>
-#include <linux/lockd/sm_inter.h>
 
 #define NLMDBG_FACILITY		NLMDBG_XDR
 
diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
index 61d1714..e1d5286 100644
--- a/fs/lockd/xdr4.c
+++ b/fs/lockd/xdr4.c
@@ -17,7 +17,6 @@
 #include <linux/sunrpc/svc.h>
 #include <linux/sunrpc/stats.h>
 #include <linux/lockd/lockd.h>
-#include <linux/lockd/sm_inter.h>
 
 #define NLMDBG_FACILITY		NLMDBG_XDR
 
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 1ccd49e..8b57467 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -205,6 +205,7 @@ extern struct svc_procedure	nlmsvc_procedures4[];
 extern int			nlmsvc_grace_period;
 extern unsigned long		nlmsvc_timeout;
 extern int			nsm_use_hostnames;
+extern int			nsm_local_state;
 
 /*
  * Lockd client functions
diff --git a/include/linux/lockd/sm_inter.h b/include/linux/lockd/sm_inter.h
deleted file mode 100644
index 5cef5a7..0000000
--- a/include/linux/lockd/sm_inter.h
+++ /dev/null
@@ -1,16 +0,0 @@
-/*
- * linux/include/linux/lockd/sm_inter.h
- *
- * Declarations for the kernel statd client.
- *
- * Copyright (C) 1996, Olaf Kirch <okir-pn4DOG8n3UYbFoVRYvo4fw@public.gmane.org>
- */
-
-#ifndef LINUX_LOCKD_SM_INTER_H
-#define LINUX_LOCKD_SM_INTER_H
-
-#define SM_MAXSTRLEN	1024
-
-extern int	nsm_local_state;
-
-#endif /* LINUX_LOCKD_SM_INTER_H */
diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
index 6338866..7dc5b6c 100644
--- a/include/linux/lockd/xdr.h
+++ b/include/linux/lockd/xdr.h
@@ -13,6 +13,7 @@
 #include <linux/nfs.h>
 #include <linux/sunrpc/xdr.h>
 
+#define SM_MAXSTRLEN		1024
 #define SM_PRIV_SIZE		16
 
 struct nsm_private {

[PATCH 22/27] NSM: Move nsm_addr() to fs/lockd/mon.c

[Chuck Lever]

Clean up: nsm_addr_in() is no longer used, and nsm_addr() is used only in
fs/lockd/mon.c, so move it there.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c              |    5 +++++
 include/linux/lockd/lockd.h |   10 ----------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 97011ee..2e0ec9f 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -53,6 +53,11 @@ static				DEFINE_SPINLOCK(nsm_lock);
  */
 int				nsm_local_state;
 
+static inline struct sockaddr *nsm_addr(const struct nsm_handle *nsm)
+{
+	return (struct sockaddr *)&nsm->sm_addr;
+}
+
 static void nsm_display_ipv4_address(const struct sockaddr *sap, char *buf,
 				     const size_t len)
 {
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 8b57467..6ab0449 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -112,16 +112,6 @@ static inline struct sockaddr *nlm_srcaddr(const struct nlm_host *host)
 	return (struct sockaddr *)&host->h_srcaddr;
 }
 
-static inline struct sockaddr_in *nsm_addr_in(const struct nsm_handle *handle)
-{
-	return (struct sockaddr_in *)&handle->sm_addr;
-}
-
-static inline struct sockaddr *nsm_addr(const struct nsm_handle *handle)
-{
-	return (struct sockaddr *)&handle->sm_addr;
-}
-
 /*
  * Map an fl_owner_t into a unique 32-bit "pid"
  */

[PATCH 23/27] NSM: Move nsm_use_hostnames to mon.c

[Chuck Lever]

Clean up.

Treat the nsm_use_hostnames global variable like nsm_local_state.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c |    3 ++-
 fs/lockd/svc.c |    1 -
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 2e0ec9f..38b8d51 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -51,7 +51,8 @@ static				DEFINE_SPINLOCK(nsm_lock);
 /*
  * Local NSM state
  */
-int				nsm_local_state;
+int	__read_mostly		nsm_local_state;
+int	__read_mostly		nsm_use_hostnames;
 
 static inline struct sockaddr *nsm_addr(const struct nsm_handle *nsm)
 {
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 50de426..8283b0e 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -59,7 +59,6 @@ unsigned long			nlmsvc_timeout;
 static unsigned long		nlm_grace_period;
 static unsigned long		nlm_timeout = LOCKD_DFLT_TIMEO;
 static int			nlm_udpport, nlm_tcpport;
-int				nsm_use_hostnames = 0;
 
 /*
  * Constants needed for the sysctl interface.

[PATCH 24/27] NSM: Move nsm_create()

[Chuck Lever]

Clean up: one last thing... relocate nsm_create() to eliminate the forward
declaration and group it near the only function that actually uses it.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/lockd/mon.c |   51 ++++++++++++++++++++-------------------------------
 1 files changed, 20 insertions(+), 31 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 38b8d51..e86ba16 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -42,8 +42,6 @@ struct nsm_res {
 	u32			state;
 };
 
-static struct rpc_clnt *	nsm_create(void);
-
 static struct rpc_program	nsm_program;
 static				LIST_HEAD(nsm_handles);
 static				DEFINE_SPINLOCK(nsm_lock);
@@ -97,11 +95,26 @@ static void nsm_display_address(const struct sockaddr *sap,
 	}
 }
 
-/*
- * Common procedure for NSMPROC_MON/NSMPROC_UNMON calls
- */
-static int
-nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
+static struct rpc_clnt *nsm_create(void)
+{
+	struct sockaddr_in sin = {
+		.sin_family		= AF_INET,
+		.sin_addr.s_addr	= htonl(INADDR_LOOPBACK),
+	};
+	struct rpc_create_args args = {
+		.protocol		= XPRT_TRANSPORT_UDP,
+		.address		= (struct sockaddr *)&sin,
+		.addrsize		= sizeof(sin),
+		.servername		= "rpc.statd",
+		.program		= &nsm_program,
+		.version		= NSM_VERSION,
+		.authflavor		= RPC_AUTH_NULL,
+	};
+
+	return rpc_create(&args);
+}
+
+static int nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
 {
 	struct rpc_clnt	*clnt;
 	int		status;
@@ -421,30 +434,6 @@ void nsm_release(struct nsm_handle *nsm)
 }
 
 /*
- * Create NSM client for the local host
- */
-static struct rpc_clnt *
-nsm_create(void)
-{
-	struct sockaddr_in	sin = {
-		.sin_family	= AF_INET,
-		.sin_addr.s_addr = htonl(INADDR_LOOPBACK),
-		.sin_port	= 0,
-	};
-	struct rpc_create_args args = {
-		.protocol	= XPRT_TRANSPORT_UDP,
-		.address	= (struct sockaddr *)&sin,
-		.addrsize	= sizeof(sin),
-		.servername	= "localhost",
-		.program	= &nsm_program,
-		.version	= NSM_VERSION,
-		.authflavor	= RPC_AUTH_NULL,
-	};
-
-	return rpc_create(&args);
-}
-
-/*
  * XDR functions for NSM.
  *
  * See http://www.opengroup.org/ for details on the Network

[PATCH 25/27] NLM: nlm_privileged_requester() doesn't recognize mapped loopback address

[Chuck Lever]

Commit b85e4676 added the nlm_privileged_requester() helper to check
whether an RPC request was sent from a local privileged caller.  It
recognizes IPv4 privileged callers (from "127.0.0.1"), and IPv6
privileged callers (from "::1").

However, IPV6_ADDR_LOOPBACK is not set for the mapped IPv4 loopback
address (::ffff:7f00:0001), so the test breaks when the kernel's RPC
service is IPv6-enabled but user space is calling via the IPv4
loopback address.  This is actually the most common case for IPv6-
enabled RPC services on Linux.

Rewrite the IPv6 check to handle the mapped IPv4 loopback address as
well as a normal IPv6 loopback address.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 include/linux/lockd/lockd.h |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 6ab0449..80d7e8a 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -299,8 +299,14 @@ static inline int __nlm_privileged_request4(const struct sockaddr *sap)
 static inline int __nlm_privileged_request6(const struct sockaddr *sap)
 {
 	const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
-	return (ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LOOPBACK) &&
-			(ntohs(sin6->sin6_port) < 1024);
+
+	if (ntohs(sin6->sin6_port) > 1023)
+		return 0;
+
+	if (ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_MAPPED)
+		return ipv4_is_loopback(sin6->sin6_addr.s6_addr32[3]);
+
+	return ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LOOPBACK;
 }
 #else	/* defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) */
 static inline int __nlm_privileged_request6(const struct sockaddr *sap)

[PATCH 26/27] NLM: Rewrite IPv4 privileged requester's check

[Chuck Lever]

Clean up.

For consistency, rewrite the IPv4 check to match the same style as the
new IPv6 check.  Note that ipv4_is_loopback() is somewhat broader in
its interpretation of what is a loopback address than simply
"127.0.0.1".

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 include/linux/lockd/lockd.h |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 80d7e8a..aa6fe70 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -291,8 +291,11 @@ static inline struct inode *nlmsvc_file_inode(struct nlm_file *file)
 static inline int __nlm_privileged_request4(const struct sockaddr *sap)
 {
 	const struct sockaddr_in *sin = (struct sockaddr_in *)sap;
-	return (sin->sin_addr.s_addr == htonl(INADDR_LOOPBACK)) &&
-			(ntohs(sin->sin_port) < 1024);
+
+	if (ntohs(sin->sin_port) > 1023)
+		return 0;
+
+	return ipv4_is_loopback(sin->sin_addr.s_addr);
 }
 
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)

[PATCH 27/27] lockd: Enable NLM use of AF_INET6

[Chuck Lever]

If the kernel is configured to support IPv6 and the RPC server can register
services via rpcbindv4, we are all set to enable IPv6 support for lockd.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: Aime Le Rouzic <aime.le-rouzic@bull.net>
---

 fs/lockd/svc.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 8283b0e..6d289f3 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -53,6 +53,17 @@ static struct svc_rqst		*nlmsvc_rqst;
 unsigned long			nlmsvc_timeout;
 
 /*
+ * If the kernel has IPv6 support available, always listen for
+ * both AF_INET and AF_INET6 requests.
+ */
+#if (defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)) && \
+	defined(CONFIG_SUNRPC_REGISTER_V4)
+static const sa_family_t	nlmsvc_family = AF_INET6;
+#else	/* (CONFIG_IPV6 || CONFIG_IPV6_MODULE) && CONFIG_SUNRPC_REGISTER_V4 */
+static const sa_family_t	nlmsvc_family = AF_INET;
+#endif	/* (CONFIG_IPV6 || CONFIG_IPV6_MODULE) && CONFIG_SUNRPC_REGISTER_V4 */
+
+/*
  * These can be set at insmod time (useful for NFS as root filesystem),
  * and also changed through the sysctl interface.  -- Jamie Lokier, Aug 2003
  */
@@ -249,7 +260,7 @@ int lockd_up(void)
 			"lockd_up: no pid, %d users??\n", nlmsvc_users);
 
 	error = -ENOMEM;
-	serv = svc_create(&nlmsvc_program, LOCKD_BUFSIZE, AF_INET, NULL);
+	serv = svc_create(&nlmsvc_program, LOCKD_BUFSIZE, nlmsvc_family, NULL);
 	if (!serv) {
 		printk(KERN_WARNING "lockd_up: create service failed\n");
 		goto out;