[PATCH 00/10] Next series of IPv6 patches for lockd

[PATCH 00/10] Next series of IPv6 patches for lockd

[Chuck Lever]

Hi Bruce-

Here are ten more IPv6 patches for lockd, as requested.  There are just
a handful of remaining minor IPv6 patches to finish enabling IPv6 in
lockd.  These can likely wait until we have a complete IPv6 user space
implementation.

Please consider these for 2.6.28.

---

Chuck Lever (10):
      lockd: Use "unsigned short" for lockd_up() "proto" argument
      lockd: IPv6 support for SM_MON / SM_UNMON
      lockd: struct nlm_reboot should contain a full socket address
      lockd: Remove unused fields in the nlm_reboot structure
      lockd: Add helper to sanity check incoming NOTIFY requests
      lockd: Adjust signature of nlm_host_rebooted to handle non-AF_INET
      lockd: change nlmclnt_grant() to take a "struct sockaddr *"
      lockd: Adjust nlmsvc_lookup_host() to accomodate AF_INET6 addresses
      lockd: Adjust nlmclnt_lookup_host() signature to accomodate non-AF_INET
      lockd: Support non-AF_INET addresses in nlm_lookup_host()


 fs/lockd/clntlock.c            |   10 +-
 fs/lockd/host.c                |  189 ++++++++++++++++++++++++++++------------
 fs/lockd/mon.c                 |   76 ++++++++++++----
 fs/lockd/svc.c                 |    5 -
 fs/lockd/svc4proc.c            |   18 +---
 fs/lockd/svcproc.c             |   18 +---
 fs/lockd/xdr.c                 |   30 ++++++
 fs/lockd/xdr4.c                |   29 +++++-
 include/linux/lockd/bind.h     |    2 
 include/linux/lockd/lockd.h    |   63 ++++++++++++-
 include/linux/lockd/sm_inter.h |    2 
 include/linux/lockd/xdr.h      |    5 -
 12 files changed, 315 insertions(+), 132 deletions(-)

-- 
Chuck Lever

[PATCH 01/10] lockd: Support non-AF_INET addresses in nlm_lookup_host()

[Chuck Lever]

Make nlm_lookup_host() agnostic to address families.  As a clean up, convert
nlm_lookup_host() to use a single structure instead of nearly a dozen separate
arguments.

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

 fs/lockd/host.c |   88 +++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 56 insertions(+), 32 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index be8f19d..c9d25d1 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -38,6 +38,21 @@ static struct nsm_handle	*nsm_find(const struct sockaddr *sap,
 						const size_t hostname_len,
 						const int create);
 
+#define NLM_SERVER	(0)
+#define NLM_CLIENT	(1)
+
+struct nlm_lookup_host_info {
+	const int		peer;		/* search for server|client */
+	const struct sockaddr	*sap;		/* address to search for */
+	const size_t		salen;		/* it's length */
+	const unsigned short	protocol;	/* transport to search for*/
+	const u32		version;	/* NLM version to search for */
+	const char		*hostname;	/* remote's hostname */
+	const size_t		hostname_len;	/* it's length */
+	const struct sockaddr	*src_sap;	/* our address (optional) */
+	const size_t		src_len;	/* it's length */
+};
+
 /*
  * Hash function must work well on big- and little-endian platforms
  */
@@ -121,23 +136,13 @@ static void nlm_display_address(const struct sockaddr *sap,
 /*
  * Common host lookup routine for server & client
  */
-static struct nlm_host *nlm_lookup_host(int server,
-					const struct sockaddr_in *sin,
-					int proto, u32 version,
-					const char *hostname,
-					unsigned int hostname_len,
-					const struct sockaddr_in *ssin)
+static struct nlm_host *nlm_lookup_host(struct nlm_lookup_host_info *ni)
 {
 	struct hlist_head *chain;
 	struct hlist_node *pos;
 	struct nlm_host	*host;
 	struct nsm_handle *nsm = NULL;
 
-	dprintk("lockd: nlm_lookup_host(proto=%d, vers=%u,"
-			" my role is %s, hostname=%.*s)\n",
-			proto, version, server ? "server" : "client",
-			hostname_len, hostname ? hostname : "<none>");
-
 	mutex_lock(&nlm_host_mutex);
 
 	if (time_after_eq(jiffies, next_gc))
@@ -150,22 +155,22 @@ static struct nlm_host *nlm_lookup_host(int server,
 	 * different NLM rpc_clients into one single nlm_host object.
 	 * This would allow us to have one nlm_host per address.
 	 */
-	chain = &nlm_hosts[nlm_hash_address((struct sockaddr *)sin)];
+	chain = &nlm_hosts[nlm_hash_address(ni->sap)];
 	hlist_for_each_entry(host, pos, chain, h_hash) {
-		if (!nlm_cmp_addr(nlm_addr(host), (struct sockaddr *)sin))
+		if (!nlm_cmp_addr(nlm_addr(host), ni->sap))
 			continue;
 
 		/* See if we have an NSM handle for this client */
 		if (!nsm)
 			nsm = host->h_nsmhandle;
 
-		if (host->h_proto != proto)
+		if (host->h_proto != ni->protocol)
 			continue;
-		if (host->h_version != version)
+		if (host->h_version != ni->version)
 			continue;
-		if (host->h_server != server)
+		if (host->h_server != ni->peer)
 			continue;
-		if (!nlm_cmp_addr(nlm_srcaddr(host), (struct sockaddr *)ssin))
+		if (!nlm_cmp_addr(nlm_srcaddr(host), ni->src_sap))
 			continue;
 
 		/* Move to head of hash chain. */
@@ -186,8 +191,8 @@ static struct nlm_host *nlm_lookup_host(int server,
 		atomic_inc(&nsm->sm_count);
 	else {
 		host = NULL;
-		nsm = nsm_find((struct sockaddr *)sin, sizeof(*sin),
-				hostname, hostname_len, 1);
+		nsm = nsm_find(ni->sap, ni->salen,
+					ni->hostname, ni->hostname_len, 1);
 		if (!nsm) {
 			dprintk("lockd: nlm_lookup_host failed; "
 				"no nsm handle\n");
@@ -202,12 +207,12 @@ static struct nlm_host *nlm_lookup_host(int server,
 		goto out;
 	}
 	host->h_name	   = nsm->sm_name;
-	memcpy(nlm_addr(host), sin, sizeof(*sin));
-	host->h_addrlen = sizeof(*sin);
+	memcpy(nlm_addr(host), ni->sap, ni->salen);
+	host->h_addrlen = ni->salen;
 	nlm_clear_port(nlm_addr(host));
-	memcpy(nlm_srcaddr(host), ssin, sizeof(*ssin));
-	host->h_version    = version;
-	host->h_proto      = proto;
+	memcpy(nlm_srcaddr(host), ni->src_sap, ni->src_len);
+	host->h_version    = ni->version;
+	host->h_proto      = ni->protocol;
 	host->h_rpcclnt    = NULL;
 	mutex_init(&host->h_mutex);
 	host->h_nextrebind = jiffies + NLM_HOST_REBIND;
@@ -218,7 +223,7 @@ static struct nlm_host *nlm_lookup_host(int server,
 	host->h_state      = 0;			/* pseudo NSM state */
 	host->h_nsmstate   = 0;			/* real NSM state */
 	host->h_nsmhandle  = nsm;
-	host->h_server	   = server;
+	host->h_server	   = ni->peer;
 	hlist_add_head(&host->h_hash, chain);
 	INIT_LIST_HEAD(&host->h_lockowners);
 	spin_lock_init(&host->h_lock);
@@ -270,12 +275,22 @@ struct nlm_host *nlmclnt_lookup_host(const struct sockaddr_in *sin,
 				     const char *hostname,
 				     unsigned int hostname_len)
 {
-	const struct sockaddr_in source = {
-		.sin_family	= AF_UNSPEC,
+	const struct sockaddr source = {
+		.sa_family	= AF_UNSPEC,
+	};
+	struct nlm_lookup_host_info ni = {
+		.peer		= NLM_SERVER,
+		.sap		= (struct sockaddr *)sin,
+		.salen		= sizeof(*sin),
+		.protocol	= proto,
+		.version	= version,
+		.hostname	= hostname,
+		.hostname_len	= hostname_len,
+		.src_sap	= &source,
+		.src_len	= sizeof(source),
 	};
 
-	return nlm_lookup_host(0, sin, proto, version,
-			       hostname, hostname_len, &source);
+	return nlm_lookup_host(&ni);
 }
 
 /*
@@ -289,10 +304,19 @@ nlmsvc_lookup_host(struct svc_rqst *rqstp,
 		.sin_family	= AF_INET,
 		.sin_addr	= rqstp->rq_daddr.addr,
 	};
+	struct nlm_lookup_host_info ni = {
+		.peer		= NLM_CLIENT,
+		.sap		= svc_addr(rqstp),
+		.salen		= rqstp->rq_addrlen,
+		.protocol	= rqstp->rq_prot,
+		.version	= rqstp->rq_vers,
+		.hostname	= hostname,
+		.hostname_len	= hostname_len,
+		.src_sap	= (struct sockaddr *)&source,
+		.src_len	= sizeof(source),
+	};
 
-	return nlm_lookup_host(1, svc_addr_in(rqstp),
-			       rqstp->rq_prot, rqstp->rq_vers,
-			       hostname, hostname_len, &source);
+	return nlm_lookup_host(&ni);
 }
 
 /*

Re: [PATCH 01/10] lockd: Support non-AF_INET addresses in nlm_lookup_host()

[J. Bruce Fields]

On Wed, Sep 17, 2008 at 11:17:21AM -0500, Chuck Lever wrote:
> Make nlm_lookup_host() agnostic to address families.  As a clean up, convert
> nlm_lookup_host() to use a single structure instead of nearly a dozen separate
> arguments.

Could you do those two steps as two separate patches, and resend?

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/host.c |   88 +++++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 56 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index be8f19d..c9d25d1 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -38,6 +38,21 @@ static struct nsm_handle	*nsm_find(const struct sockaddr *sap,
>  						const size_t hostname_len,
>  						const int create);
>  
> +#define NLM_SERVER	(0)
> +#define NLM_CLIENT	(1)
> +
> +struct nlm_lookup_host_info {
> +	const int		peer;		/* search for server|client */
> +	const struct sockaddr	*sap;		/* address to search for */
> +	const size_t		salen;		/* it's length */
> +	const unsigned short	protocol;	/* transport to search for*/
> +	const u32		version;	/* NLM version to search for */
> +	const char		*hostname;	/* remote's hostname */
> +	const size_t		hostname_len;	/* it's length */
> +	const struct sockaddr	*src_sap;	/* our address (optional) */
> +	const size_t		src_len;	/* it's length */
> +};
> +
>  /*
>   * Hash function must work well on big- and little-endian platforms
>   */
> @@ -121,23 +136,13 @@ static void nlm_display_address(const struct sockaddr *sap,
>  /*
>   * Common host lookup routine for server & client
>   */
> -static struct nlm_host *nlm_lookup_host(int server,
> -					const struct sockaddr_in *sin,
> -					int proto, u32 version,
> -					const char *hostname,
> -					unsigned int hostname_len,
> -					const struct sockaddr_in *ssin)
> +static struct nlm_host *nlm_lookup_host(struct nlm_lookup_host_info *ni)
>  {
>  	struct hlist_head *chain;
>  	struct hlist_node *pos;
>  	struct nlm_host	*host;
>  	struct nsm_handle *nsm = NULL;
>  
> -	dprintk("lockd: nlm_lookup_host(proto=%d, vers=%u,"
> -			" my role is %s, hostname=%.*s)\n",
> -			proto, version, server ? "server" : "client",
> -			hostname_len, hostname ? hostname : "<none>");
> -
>  	mutex_lock(&nlm_host_mutex);
>  
>  	if (time_after_eq(jiffies, next_gc))
> @@ -150,22 +155,22 @@ static struct nlm_host *nlm_lookup_host(int server,
>  	 * different NLM rpc_clients into one single nlm_host object.
>  	 * This would allow us to have one nlm_host per address.
>  	 */
> -	chain = &nlm_hosts[nlm_hash_address((struct sockaddr *)sin)];
> +	chain = &nlm_hosts[nlm_hash_address(ni->sap)];
>  	hlist_for_each_entry(host, pos, chain, h_hash) {
> -		if (!nlm_cmp_addr(nlm_addr(host), (struct sockaddr *)sin))
> +		if (!nlm_cmp_addr(nlm_addr(host), ni->sap))
>  			continue;
>  
>  		/* See if we have an NSM handle for this client */
>  		if (!nsm)
>  			nsm = host->h_nsmhandle;
>  
> -		if (host->h_proto != proto)
> +		if (host->h_proto != ni->protocol)
>  			continue;
> -		if (host->h_version != version)
> +		if (host->h_version != ni->version)
>  			continue;
> -		if (host->h_server != server)
> +		if (host->h_server != ni->peer)
>  			continue;
> -		if (!nlm_cmp_addr(nlm_srcaddr(host), (struct sockaddr *)ssin))
> +		if (!nlm_cmp_addr(nlm_srcaddr(host), ni->src_sap))
>  			continue;
>  
>  		/* Move to head of hash chain. */
> @@ -186,8 +191,8 @@ static struct nlm_host *nlm_lookup_host(int server,
>  		atomic_inc(&nsm->sm_count);
>  	else {
>  		host = NULL;
> -		nsm = nsm_find((struct sockaddr *)sin, sizeof(*sin),
> -				hostname, hostname_len, 1);
> +		nsm = nsm_find(ni->sap, ni->salen,
> +					ni->hostname, ni->hostname_len, 1);
>  		if (!nsm) {
>  			dprintk("lockd: nlm_lookup_host failed; "
>  				"no nsm handle\n");
> @@ -202,12 +207,12 @@ static struct nlm_host *nlm_lookup_host(int server,
>  		goto out;
>  	}
>  	host->h_name	   = nsm->sm_name;
> -	memcpy(nlm_addr(host), sin, sizeof(*sin));
> -	host->h_addrlen = sizeof(*sin);
> +	memcpy(nlm_addr(host), ni->sap, ni->salen);
> +	host->h_addrlen = ni->salen;
>  	nlm_clear_port(nlm_addr(host));
> -	memcpy(nlm_srcaddr(host), ssin, sizeof(*ssin));
> -	host->h_version    = version;
> -	host->h_proto      = proto;
> +	memcpy(nlm_srcaddr(host), ni->src_sap, ni->src_len);
> +	host->h_version    = ni->version;
> +	host->h_proto      = ni->protocol;
>  	host->h_rpcclnt    = NULL;
>  	mutex_init(&host->h_mutex);
>  	host->h_nextrebind = jiffies + NLM_HOST_REBIND;
> @@ -218,7 +223,7 @@ static struct nlm_host *nlm_lookup_host(int server,
>  	host->h_state      = 0;			/* pseudo NSM state */
>  	host->h_nsmstate   = 0;			/* real NSM state */
>  	host->h_nsmhandle  = nsm;
> -	host->h_server	   = server;
> +	host->h_server	   = ni->peer;
>  	hlist_add_head(&host->h_hash, chain);
>  	INIT_LIST_HEAD(&host->h_lockowners);
>  	spin_lock_init(&host->h_lock);
> @@ -270,12 +275,22 @@ struct nlm_host *nlmclnt_lookup_host(const struct sockaddr_in *sin,
>  				     const char *hostname,
>  				     unsigned int hostname_len)
>  {
> -	const struct sockaddr_in source = {
> -		.sin_family	= AF_UNSPEC,
> +	const struct sockaddr source = {
> +		.sa_family	= AF_UNSPEC,
> +	};
> +	struct nlm_lookup_host_info ni = {
> +		.peer		= NLM_SERVER,
> +		.sap		= (struct sockaddr *)sin,
> +		.salen		= sizeof(*sin),
> +		.protocol	= proto,
> +		.version	= version,
> +		.hostname	= hostname,
> +		.hostname_len	= hostname_len,
> +		.src_sap	= &source,
> +		.src_len	= sizeof(source),
>  	};
>  
> -	return nlm_lookup_host(0, sin, proto, version,
> -			       hostname, hostname_len, &source);
> +	return nlm_lookup_host(&ni);
>  }
>  
>  /*
> @@ -289,10 +304,19 @@ nlmsvc_lookup_host(struct svc_rqst *rqstp,
>  		.sin_family	= AF_INET,
>  		.sin_addr	= rqstp->rq_daddr.addr,
>  	};
> +	struct nlm_lookup_host_info ni = {
> +		.peer		= NLM_CLIENT,
> +		.sap		= svc_addr(rqstp),
> +		.salen		= rqstp->rq_addrlen,
> +		.protocol	= rqstp->rq_prot,
> +		.version	= rqstp->rq_vers,
> +		.hostname	= hostname,
> +		.hostname_len	= hostname_len,
> +		.src_sap	= (struct sockaddr *)&source,
> +		.src_len	= sizeof(source),
> +	};
>  
> -	return nlm_lookup_host(1, svc_addr_in(rqstp),
> -			       rqstp->rq_prot, rqstp->rq_vers,
> -			       hostname, hostname_len, &source);
> +	return nlm_lookup_host(&ni);
>  }
>  
>  /*
> 

Re: [PATCH 01/10] lockd: Support non-AF_INET addresses in nlm_lookup_host()

[Chuck Lever]

On Sep 26, 2008, at Sep 26, 2008, 5:53 PM, J. Bruce Fields wrote:
> On Wed, Sep 17, 2008 at 11:17:21AM -0500, Chuck Lever wrote:
>> Make nlm_lookup_host() agnostic to address families.  As a clean  
>> up, convert
>> nlm_lookup_host() to use a single structure instead of nearly a  
>> dozen separate
>> arguments.
>
> Could you do those two steps as two separate patches, and resend?

I tried that before, but it didn't make sense as two patches.

I can try it again.

>
>
> --b.
>
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/lockd/host.c |   88 ++++++++++++++++++++++++++++++++++ 
>> +--------------------
>> 1 files changed, 56 insertions(+), 32 deletions(-)
>>
>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>> index be8f19d..c9d25d1 100644
>> --- a/fs/lockd/host.c
>> +++ b/fs/lockd/host.c
>> @@ -38,6 +38,21 @@ static struct nsm_handle	*nsm_find(const struct  
>> sockaddr *sap,
>> 						const size_t hostname_len,
>> 						const int create);
>>
>> +#define NLM_SERVER	(0)
>> +#define NLM_CLIENT	(1)
>> +
>> +struct nlm_lookup_host_info {
>> +	const int		peer;		/* search for server|client */
>> +	const struct sockaddr	*sap;		/* address to search for */
>> +	const size_t		salen;		/* it's length */
>> +	const unsigned short	protocol;	/* transport to search for*/
>> +	const u32		version;	/* NLM version to search for */
>> +	const char		*hostname;	/* remote's hostname */
>> +	const size_t		hostname_len;	/* it's length */
>> +	const struct sockaddr	*src_sap;	/* our address (optional) */
>> +	const size_t		src_len;	/* it's length */
>> +};
>> +
>> /*
>>  * Hash function must work well on big- and little-endian platforms
>>  */
>> @@ -121,23 +136,13 @@ static void nlm_display_address(const struct  
>> sockaddr *sap,
>> /*
>>  * Common host lookup routine for server & client
>>  */
>> -static struct nlm_host *nlm_lookup_host(int server,
>> -					const struct sockaddr_in *sin,
>> -					int proto, u32 version,
>> -					const char *hostname,
>> -					unsigned int hostname_len,
>> -					const struct sockaddr_in *ssin)
>> +static struct nlm_host *nlm_lookup_host(struct  
>> nlm_lookup_host_info *ni)
>> {
>> 	struct hlist_head *chain;
>> 	struct hlist_node *pos;
>> 	struct nlm_host	*host;
>> 	struct nsm_handle *nsm = NULL;
>>
>> -	dprintk("lockd: nlm_lookup_host(proto=%d, vers=%u,"
>> -			" my role is %s, hostname=%.*s)\n",
>> -			proto, version, server ? "server" : "client",
>> -			hostname_len, hostname ? hostname : "<none>");
>> -
>> 	mutex_lock(&nlm_host_mutex);
>>
>> 	if (time_after_eq(jiffies, next_gc))
>> @@ -150,22 +155,22 @@ static struct nlm_host *nlm_lookup_host(int  
>> server,
>> 	 * different NLM rpc_clients into one single nlm_host object.
>> 	 * This would allow us to have one nlm_host per address.
>> 	 */
>> -	chain = &nlm_hosts[nlm_hash_address((struct sockaddr *)sin)];
>> +	chain = &nlm_hosts[nlm_hash_address(ni->sap)];
>> 	hlist_for_each_entry(host, pos, chain, h_hash) {
>> -		if (!nlm_cmp_addr(nlm_addr(host), (struct sockaddr *)sin))
>> +		if (!nlm_cmp_addr(nlm_addr(host), ni->sap))
>> 			continue;
>>
>> 		/* See if we have an NSM handle for this client */
>> 		if (!nsm)
>> 			nsm = host->h_nsmhandle;
>>
>> -		if (host->h_proto != proto)
>> +		if (host->h_proto != ni->protocol)
>> 			continue;
>> -		if (host->h_version != version)
>> +		if (host->h_version != ni->version)
>> 			continue;
>> -		if (host->h_server != server)
>> +		if (host->h_server != ni->peer)
>> 			continue;
>> -		if (!nlm_cmp_addr(nlm_srcaddr(host), (struct sockaddr *)ssin))
>> +		if (!nlm_cmp_addr(nlm_srcaddr(host), ni->src_sap))
>> 			continue;
>>
>> 		/* Move to head of hash chain. */
>> @@ -186,8 +191,8 @@ static struct nlm_host *nlm_lookup_host(int  
>> server,
>> 		atomic_inc(&nsm->sm_count);
>> 	else {
>> 		host = NULL;
>> -		nsm = nsm_find((struct sockaddr *)sin, sizeof(*sin),
>> -				hostname, hostname_len, 1);
>> +		nsm = nsm_find(ni->sap, ni->salen,
>> +					ni->hostname, ni->hostname_len, 1);
>> 		if (!nsm) {
>> 			dprintk("lockd: nlm_lookup_host failed; "
>> 				"no nsm handle\n");
>> @@ -202,12 +207,12 @@ static struct nlm_host *nlm_lookup_host(int  
>> server,
>> 		goto out;
>> 	}
>> 	host->h_name	   = nsm->sm_name;
>> -	memcpy(nlm_addr(host), sin, sizeof(*sin));
>> -	host->h_addrlen = sizeof(*sin);
>> +	memcpy(nlm_addr(host), ni->sap, ni->salen);
>> +	host->h_addrlen = ni->salen;
>> 	nlm_clear_port(nlm_addr(host));
>> -	memcpy(nlm_srcaddr(host), ssin, sizeof(*ssin));
>> -	host->h_version    = version;
>> -	host->h_proto      = proto;
>> +	memcpy(nlm_srcaddr(host), ni->src_sap, ni->src_len);
>> +	host->h_version    = ni->version;
>> +	host->h_proto      = ni->protocol;
>> 	host->h_rpcclnt    = NULL;
>> 	mutex_init(&host->h_mutex);
>> 	host->h_nextrebind = jiffies + NLM_HOST_REBIND;
>> @@ -218,7 +223,7 @@ static struct nlm_host *nlm_lookup_host(int  
>> server,
>> 	host->h_state      = 0;			/* pseudo NSM state */
>> 	host->h_nsmstate   = 0;			/* real NSM state */
>> 	host->h_nsmhandle  = nsm;
>> -	host->h_server	   = server;
>> +	host->h_server	   = ni->peer;
>> 	hlist_add_head(&host->h_hash, chain);
>> 	INIT_LIST_HEAD(&host->h_lockowners);
>> 	spin_lock_init(&host->h_lock);
>> @@ -270,12 +275,22 @@ struct nlm_host *nlmclnt_lookup_host(const  
>> struct sockaddr_in *sin,
>> 				     const char *hostname,
>> 				     unsigned int hostname_len)
>> {
>> -	const struct sockaddr_in source = {
>> -		.sin_family	= AF_UNSPEC,
>> +	const struct sockaddr source = {
>> +		.sa_family	= AF_UNSPEC,
>> +	};
>> +	struct nlm_lookup_host_info ni = {
>> +		.peer		= NLM_SERVER,
>> +		.sap		= (struct sockaddr *)sin,
>> +		.salen		= sizeof(*sin),
>> +		.protocol	= proto,
>> +		.version	= version,
>> +		.hostname	= hostname,
>> +		.hostname_len	= hostname_len,
>> +		.src_sap	= &source,
>> +		.src_len	= sizeof(source),
>> 	};
>>
>> -	return nlm_lookup_host(0, sin, proto, version,
>> -			       hostname, hostname_len, &source);
>> +	return nlm_lookup_host(&ni);
>> }
>>
>> /*
>> @@ -289,10 +304,19 @@ nlmsvc_lookup_host(struct svc_rqst *rqstp,
>> 		.sin_family	= AF_INET,
>> 		.sin_addr	= rqstp->rq_daddr.addr,
>> 	};
>> +	struct nlm_lookup_host_info ni = {
>> +		.peer		= NLM_CLIENT,
>> +		.sap		= svc_addr(rqstp),
>> +		.salen		= rqstp->rq_addrlen,
>> +		.protocol	= rqstp->rq_prot,
>> +		.version	= rqstp->rq_vers,
>> +		.hostname	= hostname,
>> +		.hostname_len	= hostname_len,
>> +		.src_sap	= (struct sockaddr *)&source,
>> +		.src_len	= sizeof(source),
>> +	};
>>
>> -	return nlm_lookup_host(1, svc_addr_in(rqstp),
>> -			       rqstp->rq_prot, rqstp->rq_vers,
>> -			       hostname, hostname_len, &source);
>> +	return nlm_lookup_host(&ni);
>> }
>>
>> /*
>>

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

Re: [PATCH 01/10] lockd: Support non-AF_INET addresses in nlm_lookup_host()

[J. Bruce Fields]

On Wed, Oct 01, 2008 at 11:50:27AM -0400, Chuck Lever wrote:
> On Sep 26, 2008, at Sep 26, 2008, 5:53 PM, J. Bruce Fields wrote:
>> On Wed, Sep 17, 2008 at 11:17:21AM -0500, Chuck Lever wrote:
>>> Make nlm_lookup_host() agnostic to address families.  As a clean up, 
>>> convert
>>> nlm_lookup_host() to use a single structure instead of nearly a  
>>> dozen separate
>>> arguments.
>>
>> Could you do those two steps as two separate patches, and resend?
>
> I tried that before, but it didn't make sense as two patches.
>
> I can try it again.

It should be very straightforward; I'd do the cleanup first, so:

	1. Bundle up nlm_lookup_host arguments into a single structure.
	2. Fix the address fields in that structure.

And it'd make change #2 trivial to verify.

--b.

>
>>
>>
>> --b.
>>
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> fs/lockd/host.c |   88 ++++++++++++++++++++++++++++++++++ 
>>> +--------------------
>>> 1 files changed, 56 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>>> index be8f19d..c9d25d1 100644
>>> --- a/fs/lockd/host.c
>>> +++ b/fs/lockd/host.c
>>> @@ -38,6 +38,21 @@ static struct nsm_handle	*nsm_find(const struct  
>>> sockaddr *sap,
>>> 						const size_t hostname_len,
>>> 						const int create);
>>>
>>> +#define NLM_SERVER	(0)
>>> +#define NLM_CLIENT	(1)
>>> +
>>> +struct nlm_lookup_host_info {
>>> +	const int		peer;		/* search for server|client */
>>> +	const struct sockaddr	*sap;		/* address to search for */
>>> +	const size_t		salen;		/* it's length */
>>> +	const unsigned short	protocol;	/* transport to search for*/
>>> +	const u32		version;	/* NLM version to search for */
>>> +	const char		*hostname;	/* remote's hostname */
>>> +	const size_t		hostname_len;	/* it's length */
>>> +	const struct sockaddr	*src_sap;	/* our address (optional) */
>>> +	const size_t		src_len;	/* it's length */
>>> +};
>>> +
>>> /*
>>>  * Hash function must work well on big- and little-endian platforms
>>>  */
>>> @@ -121,23 +136,13 @@ static void nlm_display_address(const struct  
>>> sockaddr *sap,
>>> /*
>>>  * Common host lookup routine for server & client
>>>  */
>>> -static struct nlm_host *nlm_lookup_host(int server,
>>> -					const struct sockaddr_in *sin,
>>> -					int proto, u32 version,
>>> -					const char *hostname,
>>> -					unsigned int hostname_len,
>>> -					const struct sockaddr_in *ssin)
>>> +static struct nlm_host *nlm_lookup_host(struct nlm_lookup_host_info 
>>> *ni)
>>> {
>>> 	struct hlist_head *chain;
>>> 	struct hlist_node *pos;
>>> 	struct nlm_host	*host;
>>> 	struct nsm_handle *nsm = NULL;
>>>
>>> -	dprintk("lockd: nlm_lookup_host(proto=%d, vers=%u,"
>>> -			" my role is %s, hostname=%.*s)\n",
>>> -			proto, version, server ? "server" : "client",
>>> -			hostname_len, hostname ? hostname : "<none>");
>>> -
>>> 	mutex_lock(&nlm_host_mutex);
>>>
>>> 	if (time_after_eq(jiffies, next_gc))
>>> @@ -150,22 +155,22 @@ static struct nlm_host *nlm_lookup_host(int  
>>> server,
>>> 	 * different NLM rpc_clients into one single nlm_host object.
>>> 	 * This would allow us to have one nlm_host per address.
>>> 	 */
>>> -	chain = &nlm_hosts[nlm_hash_address((struct sockaddr *)sin)];
>>> +	chain = &nlm_hosts[nlm_hash_address(ni->sap)];
>>> 	hlist_for_each_entry(host, pos, chain, h_hash) {
>>> -		if (!nlm_cmp_addr(nlm_addr(host), (struct sockaddr *)sin))
>>> +		if (!nlm_cmp_addr(nlm_addr(host), ni->sap))
>>> 			continue;
>>>
>>> 		/* See if we have an NSM handle for this client */
>>> 		if (!nsm)
>>> 			nsm = host->h_nsmhandle;
>>>
>>> -		if (host->h_proto != proto)
>>> +		if (host->h_proto != ni->protocol)
>>> 			continue;
>>> -		if (host->h_version != version)
>>> +		if (host->h_version != ni->version)
>>> 			continue;
>>> -		if (host->h_server != server)
>>> +		if (host->h_server != ni->peer)
>>> 			continue;
>>> -		if (!nlm_cmp_addr(nlm_srcaddr(host), (struct sockaddr *)ssin))
>>> +		if (!nlm_cmp_addr(nlm_srcaddr(host), ni->src_sap))
>>> 			continue;
>>>
>>> 		/* Move to head of hash chain. */
>>> @@ -186,8 +191,8 @@ static struct nlm_host *nlm_lookup_host(int  
>>> server,
>>> 		atomic_inc(&nsm->sm_count);
>>> 	else {
>>> 		host = NULL;
>>> -		nsm = nsm_find((struct sockaddr *)sin, sizeof(*sin),
>>> -				hostname, hostname_len, 1);
>>> +		nsm = nsm_find(ni->sap, ni->salen,
>>> +					ni->hostname, ni->hostname_len, 1);
>>> 		if (!nsm) {
>>> 			dprintk("lockd: nlm_lookup_host failed; "
>>> 				"no nsm handle\n");
>>> @@ -202,12 +207,12 @@ static struct nlm_host *nlm_lookup_host(int  
>>> server,
>>> 		goto out;
>>> 	}
>>> 	host->h_name	   = nsm->sm_name;
>>> -	memcpy(nlm_addr(host), sin, sizeof(*sin));
>>> -	host->h_addrlen = sizeof(*sin);
>>> +	memcpy(nlm_addr(host), ni->sap, ni->salen);
>>> +	host->h_addrlen = ni->salen;
>>> 	nlm_clear_port(nlm_addr(host));
>>> -	memcpy(nlm_srcaddr(host), ssin, sizeof(*ssin));
>>> -	host->h_version    = version;
>>> -	host->h_proto      = proto;
>>> +	memcpy(nlm_srcaddr(host), ni->src_sap, ni->src_len);
>>> +	host->h_version    = ni->version;
>>> +	host->h_proto      = ni->protocol;
>>> 	host->h_rpcclnt    = NULL;
>>> 	mutex_init(&host->h_mutex);
>>> 	host->h_nextrebind = jiffies + NLM_HOST_REBIND;
>>> @@ -218,7 +223,7 @@ static struct nlm_host *nlm_lookup_host(int  
>>> server,
>>> 	host->h_state      = 0;			/* pseudo NSM state */
>>> 	host->h_nsmstate   = 0;			/* real NSM state */
>>> 	host->h_nsmhandle  = nsm;
>>> -	host->h_server	   = server;
>>> +	host->h_server	   = ni->peer;
>>> 	hlist_add_head(&host->h_hash, chain);
>>> 	INIT_LIST_HEAD(&host->h_lockowners);
>>> 	spin_lock_init(&host->h_lock);
>>> @@ -270,12 +275,22 @@ struct nlm_host *nlmclnt_lookup_host(const  
>>> struct sockaddr_in *sin,
>>> 				     const char *hostname,
>>> 				     unsigned int hostname_len)
>>> {
>>> -	const struct sockaddr_in source = {
>>> -		.sin_family	= AF_UNSPEC,
>>> +	const struct sockaddr source = {
>>> +		.sa_family	= AF_UNSPEC,
>>> +	};
>>> +	struct nlm_lookup_host_info ni = {
>>> +		.peer		= NLM_SERVER,
>>> +		.sap		= (struct sockaddr *)sin,
>>> +		.salen		= sizeof(*sin),
>>> +		.protocol	= proto,
>>> +		.version	= version,
>>> +		.hostname	= hostname,
>>> +		.hostname_len	= hostname_len,
>>> +		.src_sap	= &source,
>>> +		.src_len	= sizeof(source),
>>> 	};
>>>
>>> -	return nlm_lookup_host(0, sin, proto, version,
>>> -			       hostname, hostname_len, &source);
>>> +	return nlm_lookup_host(&ni);
>>> }
>>>
>>> /*
>>> @@ -289,10 +304,19 @@ nlmsvc_lookup_host(struct svc_rqst *rqstp,
>>> 		.sin_family	= AF_INET,
>>> 		.sin_addr	= rqstp->rq_daddr.addr,
>>> 	};
>>> +	struct nlm_lookup_host_info ni = {
>>> +		.peer		= NLM_CLIENT,
>>> +		.sap		= svc_addr(rqstp),
>>> +		.salen		= rqstp->rq_addrlen,
>>> +		.protocol	= rqstp->rq_prot,
>>> +		.version	= rqstp->rq_vers,
>>> +		.hostname	= hostname,
>>> +		.hostname_len	= hostname_len,
>>> +		.src_sap	= (struct sockaddr *)&source,
>>> +		.src_len	= sizeof(source),
>>> +	};
>>>
>>> -	return nlm_lookup_host(1, svc_addr_in(rqstp),
>>> -			       rqstp->rq_prot, rqstp->rq_vers,
>>> -			       hostname, hostname_len, &source);
>>> +	return nlm_lookup_host(&ni);
>>> }
>>>
>>> /*
>>>
>
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
>
>
>
>

[PATCH 02/10] lockd: Adjust nlmclnt_lookup_host() signature to accomodate non-AF_INET

[Chuck Lever]

Pass a struct sockaddr * and a length to nlmclnt_lookup_host() to
accomodate non-AF_INET family addresses.

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

 fs/lockd/clntlock.c         |    5 ++---
 fs/lockd/host.c             |   34 ++++++++++++++++++++++++----------
 include/linux/lockd/lockd.h |    9 +++++----
 3 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index 237224a..9eaf306 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -58,10 +58,9 @@ struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init)
 	if (status < 0)
 		return ERR_PTR(status);
 
-	host = nlmclnt_lookup_host((struct sockaddr_in *)nlm_init->address,
+	host = nlmclnt_lookup_host(nlm_init->address, nlm_init->addrlen,
 				   nlm_init->protocol, nlm_version,
-				   nlm_init->hostname,
-				   strlen(nlm_init->hostname));
+				   nlm_init->hostname);
 	if (host == NULL) {
 		lockd_down();
 		return ERR_PTR(-ENOLCK);
diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index c9d25d1..b9c87e1 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -267,29 +267,43 @@ nlm_destroy_host(struct nlm_host *host)
 	kfree(host);
 }
 
-/*
- * Find an NLM server handle in the cache. If there is none, create it.
+/**
+ * nlmclnt_lookup_host - Find an NLM host handle matching a remote server
+ * @sap: network address of server
+ * @salen: length of server address
+ * @protocol: transport protocol to use
+ * @version: NLM protocol version
+ * @hostname: '\0'-terminated hostname of server
+ *
+ * Returns an nlm_host structure that matches the passed-in
+ * [server address, transport protocol, NLM version, server hostname].
+ * If one doesn't already exist in the host cache, a new handle is
+ * created and returned.
  */
-struct nlm_host *nlmclnt_lookup_host(const struct sockaddr_in *sin,
-				     int proto, u32 version,
-				     const char *hostname,
-				     unsigned int hostname_len)
+struct nlm_host *nlmclnt_lookup_host(const struct sockaddr *sap,
+				     const size_t salen,
+				     const unsigned short protocol,
+				     const u32 version, const char *hostname)
 {
 	const struct sockaddr source = {
 		.sa_family	= AF_UNSPEC,
 	};
 	struct nlm_lookup_host_info ni = {
 		.peer		= NLM_SERVER,
-		.sap		= (struct sockaddr *)sin,
-		.salen		= sizeof(*sin),
-		.protocol	= proto,
+		.sap		= sap,
+		.salen		= salen,
+		.protocol	= protocol,
 		.version	= version,
 		.hostname	= hostname,
-		.hostname_len	= hostname_len,
+		.hostname_len	= strlen(hostname),
 		.src_sap	= &source,
 		.src_len	= sizeof(source),
 	};
 
+	dprintk("lockd: nlmclnt_lookup_host(host='%s', vers=%u, proto=%s)\n",
+			(hostname ? hostname : "<none>"), version,
+			(protocol == IPPROTO_UDP ? "udp" : "tcp"));
+
 	return nlm_lookup_host(&ni);
 }
 
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index ec8af11..693d57b 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -215,10 +215,11 @@ void		  nlmclnt_next_cookie(struct nlm_cookie *);
 /*
  * Host cache
  */
-struct nlm_host  *nlmclnt_lookup_host(const struct sockaddr_in *sin,
-					int proto, u32 version,
-					const char *hostname,
-					unsigned int hostname_len);
+struct nlm_host  *nlmclnt_lookup_host(const struct sockaddr *sap,
+					const size_t salen,
+					const unsigned short protocol,
+					const u32 version,
+					const char *hostname);
 struct nlm_host  *nlmsvc_lookup_host(struct svc_rqst *, const char *,
 					unsigned int);
 struct rpc_clnt * nlm_bind_host(struct nlm_host *);

Re: [PATCH 02/10] lockd: Adjust nlmclnt_lookup_host() signature to accomodate non-AF_INET

[J. Bruce Fields]

On Wed, Sep 17, 2008 at 11:17:28AM -0500, Chuck Lever wrote:
> Pass a struct sockaddr * and a length to nlmclnt_lookup_host() to
> accomodate non-AF_INET family addresses.

I'd prefer separate patches here too, though since the two changes
(null-terminated hostname and sockaddr) are a bit easier to separate
visually, it doesn't bother me as much here....

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/clntlock.c         |    5 ++---
>  fs/lockd/host.c             |   34 ++++++++++++++++++++++++----------
>  include/linux/lockd/lockd.h |    9 +++++----
>  3 files changed, 31 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
> index 237224a..9eaf306 100644
> --- a/fs/lockd/clntlock.c
> +++ b/fs/lockd/clntlock.c
> @@ -58,10 +58,9 @@ struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init)
>  	if (status < 0)
>  		return ERR_PTR(status);
>  
> -	host = nlmclnt_lookup_host((struct sockaddr_in *)nlm_init->address,
> +	host = nlmclnt_lookup_host(nlm_init->address, nlm_init->addrlen,
>  				   nlm_init->protocol, nlm_version,
> -				   nlm_init->hostname,
> -				   strlen(nlm_init->hostname));
> +				   nlm_init->hostname);
>  	if (host == NULL) {
>  		lockd_down();
>  		return ERR_PTR(-ENOLCK);
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index c9d25d1..b9c87e1 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -267,29 +267,43 @@ nlm_destroy_host(struct nlm_host *host)
>  	kfree(host);
>  }
>  
> -/*
> - * Find an NLM server handle in the cache. If there is none, create it.
> +/**
> + * nlmclnt_lookup_host - Find an NLM host handle matching a remote server
> + * @sap: network address of server
> + * @salen: length of server address
> + * @protocol: transport protocol to use
> + * @version: NLM protocol version
> + * @hostname: '\0'-terminated hostname of server
> + *
> + * Returns an nlm_host structure that matches the passed-in
> + * [server address, transport protocol, NLM version, server hostname].
> + * If one doesn't already exist in the host cache, a new handle is
> + * created and returned.
>   */
> -struct nlm_host *nlmclnt_lookup_host(const struct sockaddr_in *sin,
> -				     int proto, u32 version,
> -				     const char *hostname,
> -				     unsigned int hostname_len)
> +struct nlm_host *nlmclnt_lookup_host(const struct sockaddr *sap,
> +				     const size_t salen,
> +				     const unsigned short protocol,
> +				     const u32 version, const char *hostname)
>  {
>  	const struct sockaddr source = {
>  		.sa_family	= AF_UNSPEC,
>  	};
>  	struct nlm_lookup_host_info ni = {
>  		.peer		= NLM_SERVER,
> -		.sap		= (struct sockaddr *)sin,
> -		.salen		= sizeof(*sin),
> -		.protocol	= proto,
> +		.sap		= sap,
> +		.salen		= salen,
> +		.protocol	= protocol,
>  		.version	= version,
>  		.hostname	= hostname,
> -		.hostname_len	= hostname_len,
> +		.hostname_len	= strlen(hostname),
>  		.src_sap	= &source,
>  		.src_len	= sizeof(source),
>  	};
>  
> +	dprintk("lockd: nlmclnt_lookup_host(host='%s', vers=%u, proto=%s)\n",
> +			(hostname ? hostname : "<none>"), version,
> +			(protocol == IPPROTO_UDP ? "udp" : "tcp"));
> +
>  	return nlm_lookup_host(&ni);
>  }
>  
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index ec8af11..693d57b 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -215,10 +215,11 @@ void		  nlmclnt_next_cookie(struct nlm_cookie *);
>  /*
>   * Host cache
>   */
> -struct nlm_host  *nlmclnt_lookup_host(const struct sockaddr_in *sin,
> -					int proto, u32 version,
> -					const char *hostname,
> -					unsigned int hostname_len);
> +struct nlm_host  *nlmclnt_lookup_host(const struct sockaddr *sap,
> +					const size_t salen,
> +					const unsigned short protocol,
> +					const u32 version,
> +					const char *hostname);
>  struct nlm_host  *nlmsvc_lookup_host(struct svc_rqst *, const char *,
>  					unsigned int);
>  struct rpc_clnt * nlm_bind_host(struct nlm_host *);
> 

Re: [PATCH 02/10] lockd: Adjust nlmclnt_lookup_host() signature to accomodate non-AF_INET

[Chuck Lever]

On Sep 26, 2008, at Sep 26, 2008, 6:02 PM, J. Bruce Fields wrote:
> On Wed, Sep 17, 2008 at 11:17:28AM -0500, Chuck Lever wrote:
>> Pass a struct sockaddr * and a length to nlmclnt_lookup_host() to
>> accomodate non-AF_INET family addresses.
>
> I'd prefer separate patches here too, though since the two changes
> (null-terminated hostname and sockaddr) are a bit easier to separate
> visually, it doesn't bother me as much here....

I'm not sure I understand what you want here.  The "NUL-terminated"  
stuff is merely documenting what is already the case, and eliminating  
an unneeded argument.  Maybe I should just add a note about it to the  
patch description?


>
>
> --b.
>
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/lockd/clntlock.c         |    5 ++---
>> fs/lockd/host.c             |   34 ++++++++++++++++++++++++----------
>> include/linux/lockd/lockd.h |    9 +++++----
>> 3 files changed, 31 insertions(+), 17 deletions(-)
>>
>> diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
>> index 237224a..9eaf306 100644
>> --- a/fs/lockd/clntlock.c
>> +++ b/fs/lockd/clntlock.c
>> @@ -58,10 +58,9 @@ struct nlm_host *nlmclnt_init(const struct  
>> nlmclnt_initdata *nlm_init)
>> 	if (status < 0)
>> 		return ERR_PTR(status);
>>
>> -	host = nlmclnt_lookup_host((struct sockaddr_in *)nlm_init->address,
>> +	host = nlmclnt_lookup_host(nlm_init->address, nlm_init->addrlen,
>> 				   nlm_init->protocol, nlm_version,
>> -				   nlm_init->hostname,
>> -				   strlen(nlm_init->hostname));
>> +				   nlm_init->hostname);
>> 	if (host == NULL) {
>> 		lockd_down();
>> 		return ERR_PTR(-ENOLCK);
>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>> index c9d25d1..b9c87e1 100644
>> --- a/fs/lockd/host.c
>> +++ b/fs/lockd/host.c
>> @@ -267,29 +267,43 @@ nlm_destroy_host(struct nlm_host *host)
>> 	kfree(host);
>> }
>>
>> -/*
>> - * Find an NLM server handle in the cache. If there is none,  
>> create it.
>> +/**
>> + * nlmclnt_lookup_host - Find an NLM host handle matching a remote  
>> server
>> + * @sap: network address of server
>> + * @salen: length of server address
>> + * @protocol: transport protocol to use
>> + * @version: NLM protocol version
>> + * @hostname: '\0'-terminated hostname of server
>> + *
>> + * Returns an nlm_host structure that matches the passed-in
>> + * [server address, transport protocol, NLM version, server  
>> hostname].
>> + * If one doesn't already exist in the host cache, a new handle is
>> + * created and returned.
>>  */
>> -struct nlm_host *nlmclnt_lookup_host(const struct sockaddr_in *sin,
>> -				     int proto, u32 version,
>> -				     const char *hostname,
>> -				     unsigned int hostname_len)
>> +struct nlm_host *nlmclnt_lookup_host(const struct sockaddr *sap,
>> +				     const size_t salen,
>> +				     const unsigned short protocol,
>> +				     const u32 version, const char *hostname)
>> {
>> 	const struct sockaddr source = {
>> 		.sa_family	= AF_UNSPEC,
>> 	};
>> 	struct nlm_lookup_host_info ni = {
>> 		.peer		= NLM_SERVER,
>> -		.sap		= (struct sockaddr *)sin,
>> -		.salen		= sizeof(*sin),
>> -		.protocol	= proto,
>> +		.sap		= sap,
>> +		.salen		= salen,
>> +		.protocol	= protocol,
>> 		.version	= version,
>> 		.hostname	= hostname,
>> -		.hostname_len	= hostname_len,
>> +		.hostname_len	= strlen(hostname),
>> 		.src_sap	= &source,
>> 		.src_len	= sizeof(source),
>> 	};
>>
>> +	dprintk("lockd: nlmclnt_lookup_host(host='%s', vers=%u, proto=%s) 
>> \n",
>> +			(hostname ? hostname : "<none>"), version,
>> +			(protocol == IPPROTO_UDP ? "udp" : "tcp"));
>> +
>> 	return nlm_lookup_host(&ni);
>> }
>>
>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/ 
>> lockd.h
>> index ec8af11..693d57b 100644
>> --- a/include/linux/lockd/lockd.h
>> +++ b/include/linux/lockd/lockd.h
>> @@ -215,10 +215,11 @@ void		  nlmclnt_next_cookie(struct nlm_cookie  
>> *);
>> /*
>>  * Host cache
>>  */
>> -struct nlm_host  *nlmclnt_lookup_host(const struct sockaddr_in *sin,
>> -					int proto, u32 version,
>> -					const char *hostname,
>> -					unsigned int hostname_len);
>> +struct nlm_host  *nlmclnt_lookup_host(const struct sockaddr *sap,
>> +					const size_t salen,
>> +					const unsigned short protocol,
>> +					const u32 version,
>> +					const char *hostname);
>> struct nlm_host  *nlmsvc_lookup_host(struct svc_rqst *, const char *,
>> 					unsigned int);
>> struct rpc_clnt * nlm_bind_host(struct nlm_host *);
>>
> --
> 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 02/10] lockd: Adjust nlmclnt_lookup_host() signature to accomodate non-AF_INET

[J. Bruce Fields]

On Wed, Oct 01, 2008 at 11:52:58AM -0400, Chuck Lever wrote:
> On Sep 26, 2008, at Sep 26, 2008, 6:02 PM, J. Bruce Fields wrote:
>> On Wed, Sep 17, 2008 at 11:17:28AM -0500, Chuck Lever wrote:
>>> Pass a struct sockaddr * and a length to nlmclnt_lookup_host() to
>>> accomodate non-AF_INET family addresses.
>>
>> I'd prefer separate patches here too, though since the two changes
>> (null-terminated hostname and sockaddr) are a bit easier to separate
>> visually, it doesn't bother me as much here....
>
> I'm not sure I understand what you want here.  The "NUL-terminated"  
> stuff is merely documenting what is already the case, and eliminating an 
> unneeded argument.

Yes, I understand, and I'm fine with that--it's just that I'd personally
normally stick those two in a separate patch.

It's not a big deal.

--b.


> Maybe I should just add a note about it to the patch 
> description?
>
>
>>
>>
>> --b.
>>
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> fs/lockd/clntlock.c         |    5 ++---
>>> fs/lockd/host.c             |   34 ++++++++++++++++++++++++----------
>>> include/linux/lockd/lockd.h |    9 +++++----
>>> 3 files changed, 31 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
>>> index 237224a..9eaf306 100644
>>> --- a/fs/lockd/clntlock.c
>>> +++ b/fs/lockd/clntlock.c
>>> @@ -58,10 +58,9 @@ struct nlm_host *nlmclnt_init(const struct  
>>> nlmclnt_initdata *nlm_init)
>>> 	if (status < 0)
>>> 		return ERR_PTR(status);
>>>
>>> -	host = nlmclnt_lookup_host((struct sockaddr_in *)nlm_init->address,
>>> +	host = nlmclnt_lookup_host(nlm_init->address, nlm_init->addrlen,
>>> 				   nlm_init->protocol, nlm_version,
>>> -				   nlm_init->hostname,
>>> -				   strlen(nlm_init->hostname));
>>> +				   nlm_init->hostname);
>>> 	if (host == NULL) {
>>> 		lockd_down();
>>> 		return ERR_PTR(-ENOLCK);
>>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>>> index c9d25d1..b9c87e1 100644
>>> --- a/fs/lockd/host.c
>>> +++ b/fs/lockd/host.c
>>> @@ -267,29 +267,43 @@ nlm_destroy_host(struct nlm_host *host)
>>> 	kfree(host);
>>> }
>>>
>>> -/*
>>> - * Find an NLM server handle in the cache. If there is none, create 
>>> it.
>>> +/**
>>> + * nlmclnt_lookup_host - Find an NLM host handle matching a remote  
>>> server
>>> + * @sap: network address of server
>>> + * @salen: length of server address
>>> + * @protocol: transport protocol to use
>>> + * @version: NLM protocol version
>>> + * @hostname: '\0'-terminated hostname of server
>>> + *
>>> + * Returns an nlm_host structure that matches the passed-in
>>> + * [server address, transport protocol, NLM version, server  
>>> hostname].
>>> + * If one doesn't already exist in the host cache, a new handle is
>>> + * created and returned.
>>>  */
>>> -struct nlm_host *nlmclnt_lookup_host(const struct sockaddr_in *sin,
>>> -				     int proto, u32 version,
>>> -				     const char *hostname,
>>> -				     unsigned int hostname_len)
>>> +struct nlm_host *nlmclnt_lookup_host(const struct sockaddr *sap,
>>> +				     const size_t salen,
>>> +				     const unsigned short protocol,
>>> +				     const u32 version, const char *hostname)
>>> {
>>> 	const struct sockaddr source = {
>>> 		.sa_family	= AF_UNSPEC,
>>> 	};
>>> 	struct nlm_lookup_host_info ni = {
>>> 		.peer		= NLM_SERVER,
>>> -		.sap		= (struct sockaddr *)sin,
>>> -		.salen		= sizeof(*sin),
>>> -		.protocol	= proto,
>>> +		.sap		= sap,
>>> +		.salen		= salen,
>>> +		.protocol	= protocol,
>>> 		.version	= version,
>>> 		.hostname	= hostname,
>>> -		.hostname_len	= hostname_len,
>>> +		.hostname_len	= strlen(hostname),
>>> 		.src_sap	= &source,
>>> 		.src_len	= sizeof(source),
>>> 	};
>>>
>>> +	dprintk("lockd: nlmclnt_lookup_host(host='%s', vers=%u, proto=%s) 
>>> \n",
>>> +			(hostname ? hostname : "<none>"), version,
>>> +			(protocol == IPPROTO_UDP ? "udp" : "tcp"));
>>> +
>>> 	return nlm_lookup_host(&ni);
>>> }
>>>
>>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/ 
>>> lockd.h
>>> index ec8af11..693d57b 100644
>>> --- a/include/linux/lockd/lockd.h
>>> +++ b/include/linux/lockd/lockd.h
>>> @@ -215,10 +215,11 @@ void		  nlmclnt_next_cookie(struct nlm_cookie  
>>> *);
>>> /*
>>>  * Host cache
>>>  */
>>> -struct nlm_host  *nlmclnt_lookup_host(const struct sockaddr_in *sin,
>>> -					int proto, u32 version,
>>> -					const char *hostname,
>>> -					unsigned int hostname_len);
>>> +struct nlm_host  *nlmclnt_lookup_host(const struct sockaddr *sap,
>>> +					const size_t salen,
>>> +					const unsigned short protocol,
>>> +					const u32 version,
>>> +					const char *hostname);
>>> struct nlm_host  *nlmsvc_lookup_host(struct svc_rqst *, const char *,
>>> 					unsigned int);
>>> struct rpc_clnt * nlm_bind_host(struct nlm_host *);
>>>
>> --
>> 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 03/10] lockd: Adjust nlmsvc_lookup_host() to accomodate AF_INET6 addresses

[Chuck Lever]

Fix up nlmsvc_lookup_host() to pass AF_INET6 source addresses to
nlm_lookup_host().

To keep stack usage down, we use address-family-specific sockaddr_in
and sockaddr_in6 structures instead of sockaddr_storage where
appropriate.  The server-side supports only AF_INET and AF_INET6.

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

 fs/lockd/host.c             |   51 +++++++++++++++++++++++++++++++++++--------
 include/linux/lockd/lockd.h |    5 +++-
 2 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index b9c87e1..cfaa9a2 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -307,29 +307,60 @@ struct nlm_host *nlmclnt_lookup_host(const struct sockaddr *sap,
 	return nlm_lookup_host(&ni);
 }
 
-/*
- * Find an NLM client handle in the cache. If there is none, create it.
+/**
+ * nlmsvc_lookup_host - Find an NLM host handle matching a remote client
+ * @rqstp: incoming NLM request
+ * @hostname: name of client host
+ * @hostname_len: length of client hostname
+ *
+ * Returns an nlm_host structure that matches the [client address,
+ * transport protocol, NLM version, client hostname] of the passed-in
+ * NLM request.  If one doesn't already exist in the host cache, a
+ * new handle is created and returned.
+ *
+ * Manufacture a specific source address in case the local system
+ * services clients from multiple IP addresses.  The family of the
+ * address in rq_daddr is guaranteed to be the same as the family of
+ * the address in rq_addr, so it's safe to use the same family and
+ * length for our manufactured source address.
  */
-struct nlm_host *
-nlmsvc_lookup_host(struct svc_rqst *rqstp,
-			const char *hostname, unsigned int hostname_len)
+struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
+				    const char *hostname,
+				    const size_t hostname_len)
 {
-	const struct sockaddr_in source = {
-		.sin_family	= AF_INET,
-		.sin_addr	= rqstp->rq_daddr.addr,
+	const struct sockaddr *sap = svc_addr(rqstp);
+	struct sockaddr_storage source = {
+		.ss_family	= sap->sa_family,
 	};
+	struct sockaddr_in *sin = (struct sockaddr_in *)&source;
+	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&source;
 	struct nlm_lookup_host_info ni = {
 		.peer		= NLM_CLIENT,
-		.sap		= svc_addr(rqstp),
+		.sap		= sap,
 		.salen		= rqstp->rq_addrlen,
 		.protocol	= rqstp->rq_prot,
 		.version	= rqstp->rq_vers,
 		.hostname	= hostname,
 		.hostname_len	= hostname_len,
 		.src_sap	= (struct sockaddr *)&source,
-		.src_len	= sizeof(source),
+		.src_len	= rqstp->rq_addrlen,
 	};
 
+	dprintk("lockd: %s(host='%.*s', vers=%u, proto=%s)\n", __func__,
+			(int)hostname_len, hostname, rqstp->rq_vers,
+			(rqstp->rq_prot == IPPROTO_UDP ? "udp" : "tcp"));
+
+	switch (sap->sa_family) {
+	case AF_INET:
+		sin->sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr;
+		break;
+	case AF_INET6:
+		ipv6_addr_copy(&sin6->sin6_addr, &rqstp->rq_daddr.addr6);
+		break;
+	default:
+		return NULL;
+	}
+
 	return nlm_lookup_host(&ni);
 }
 
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 693d57b..2e13c0b 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -220,8 +220,9 @@ struct nlm_host  *nlmclnt_lookup_host(const struct sockaddr *sap,
 					const unsigned short protocol,
 					const u32 version,
 					const char *hostname);
-struct nlm_host  *nlmsvc_lookup_host(struct svc_rqst *, const char *,
-					unsigned int);
+struct nlm_host  *nlmsvc_lookup_host(const struct svc_rqst *rqstp,
+					const char *hostname,
+					const size_t hostname_len);
 struct rpc_clnt * nlm_bind_host(struct nlm_host *);
 void		  nlm_rebind_host(struct nlm_host *);
 struct nlm_host * nlm_get_host(struct nlm_host *);

Re: [PATCH 03/10] lockd: Adjust nlmsvc_lookup_host() to accomodate AF_INET6 addresses

[J. Bruce Fields]

On Wed, Sep 17, 2008 at 11:17:35AM -0500, Chuck Lever wrote:
> Fix up nlmsvc_lookup_host() to pass AF_INET6 source addresses to
> nlm_lookup_host().
> 
> To keep stack usage down, we use address-family-specific sockaddr_in
> and sockaddr_in6 structures instead of sockaddr_storage where
> appropriate.  The server-side supports only AF_INET and AF_INET6.

Looks like there still is one sockaddr_storage on the stack.  That's 128
bytes.  Sounds doable, OK.

> +/**
> + * nlmsvc_lookup_host - Find an NLM host handle matching a remote client
> + * @rqstp: incoming NLM request
> + * @hostname: name of client host
> + * @hostname_len: length of client hostname
> + *
> + * Returns an nlm_host structure that matches the [client address,
> + * transport protocol, NLM version, client hostname] of the passed-in
> + * NLM request.  If one doesn't already exist in the host cache, a
> + * new handle is created and returned.
> + *
> + * Manufacture a specific source address in case the local system
> + * services clients from multiple IP addresses.

"Manufacture" makes it sound like we're faking one up somehow, but we're
actually just copying it from the svc_rqst, right?  Maybe make that
"store the specific source address..." instead?

--b.

Re: [PATCH 03/10] lockd: Adjust nlmsvc_lookup_host() to accomodate AF_INET6 addresses

[Chuck Lever]

On Sep 26, 2008, at Sep 26, 2008, 6:19 PM, J. Bruce Fields wrote:
> On Wed, Sep 17, 2008 at 11:17:35AM -0500, Chuck Lever wrote:
>> Fix up nlmsvc_lookup_host() to pass AF_INET6 source addresses to
>> nlm_lookup_host().
>>
>> To keep stack usage down, we use address-family-specific sockaddr_in
>> and sockaddr_in6 structures instead of sockaddr_storage where
>> appropriate.  The server-side supports only AF_INET and AF_INET6.
>
> Looks like there still is one sockaddr_storage on the stack.  That's  
> 128
> bytes.  Sounds doable, OK.
>
>> +/**
>> + * nlmsvc_lookup_host - Find an NLM host handle matching a remote  
>> client
>> + * @rqstp: incoming NLM request
>> + * @hostname: name of client host
>> + * @hostname_len: length of client hostname
>> + *
>> + * Returns an nlm_host structure that matches the [client address,
>> + * transport protocol, NLM version, client hostname] of the passed- 
>> in
>> + * NLM request.  If one doesn't already exist in the host cache, a
>> + * new handle is created and returned.
>> + *
>> + * Manufacture a specific source address in case the local system
>> + * services clients from multiple IP addresses.
>
> "Manufacture" makes it sound like we're faking one up somehow, but  
> we're
> actually just copying it from the svc_rqst, right?  Maybe make that
> "store the specific source address..." instead?

We really are faking it up, in a sense.  The svc_rqst does not store  
an address family for our source address, so we use an assumption  
(that our source address will always have the same family as the  
sender's address) to construct a full sockaddr.

How about "Construct a sockaddr for a specific source address, in case  
the
local system has multiple network addresses."

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

Re: [PATCH 03/10] lockd: Adjust nlmsvc_lookup_host() to accomodate AF_INET6 addresses

[J. Bruce Fields]

On Wed, Oct 01, 2008 at 11:59:08AM -0400, Chuck Lever wrote:
> On Sep 26, 2008, at Sep 26, 2008, 6:19 PM, J. Bruce Fields wrote:
>> On Wed, Sep 17, 2008 at 11:17:35AM -0500, Chuck Lever wrote:
>>> Fix up nlmsvc_lookup_host() to pass AF_INET6 source addresses to
>>> nlm_lookup_host().
>>>
>>> To keep stack usage down, we use address-family-specific sockaddr_in
>>> and sockaddr_in6 structures instead of sockaddr_storage where
>>> appropriate.  The server-side supports only AF_INET and AF_INET6.
>>
>> Looks like there still is one sockaddr_storage on the stack.  That's  
>> 128
>> bytes.  Sounds doable, OK.
>>
>>> +/**
>>> + * nlmsvc_lookup_host - Find an NLM host handle matching a remote  
>>> client
>>> + * @rqstp: incoming NLM request
>>> + * @hostname: name of client host
>>> + * @hostname_len: length of client hostname
>>> + *
>>> + * Returns an nlm_host structure that matches the [client address,
>>> + * transport protocol, NLM version, client hostname] of the passed- 
>>> in
>>> + * NLM request.  If one doesn't already exist in the host cache, a
>>> + * new handle is created and returned.
>>> + *
>>> + * Manufacture a specific source address in case the local system
>>> + * services clients from multiple IP addresses.
>>
>> "Manufacture" makes it sound like we're faking one up somehow, but  
>> we're
>> actually just copying it from the svc_rqst, right?  Maybe make that
>> "store the specific source address..." instead?
>
> We really are faking it up, in a sense.  The svc_rqst does not store an 
> address family for our source address, so we use an assumption (that our 
> source address will always have the same family as the sender's address) 
> to construct a full sockaddr.

OK, got it.

> How about "Construct a sockaddr for a specific source address, in case  
> the
> local system has multiple network addresses."

The existing comment's fine.

--b.

[PATCH 04/10] lockd: change nlmclnt_grant() to take a "struct sockaddr *"

[Chuck Lever]

Adjust the signature and callers of nlmclnt_grant() to pass a "struct
sockaddr *" instead of a "struct sockaddr_in *" in order to support IPv6
addresses.

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

 fs/lockd/clntlock.c         |    5 ++---
 fs/lockd/svc4proc.c         |    2 +-
 fs/lockd/svcproc.c          |    2 +-
 include/linux/lockd/lockd.h |    3 ++-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index 9eaf306..2976bf0 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -141,7 +141,7 @@ int nlmclnt_block(struct nlm_wait *block, struct nlm_rqst *req, long timeout)
 /*
  * The server lockd has called us back to tell us the lock was granted
  */
-__be32 nlmclnt_grant(const struct sockaddr_in *addr, const struct nlm_lock *lock)
+__be32 nlmclnt_grant(const struct sockaddr *addr, const struct nlm_lock *lock)
 {
 	const struct file_lock *fl = &lock->fl;
 	const struct nfs_fh *fh = &lock->fh;
@@ -165,8 +165,7 @@ __be32 nlmclnt_grant(const struct sockaddr_in *addr, const struct nlm_lock *lock
 		 */
 		if (fl_blocked->fl_u.nfs_fl.owner->pid != lock->svid)
 			continue;
-		if (!nlm_cmp_addr(nlm_addr(block->b_host),
-					(struct sockaddr *)addr))
+		if (!nlm_cmp_addr(nlm_addr(block->b_host), addr))
 			continue;
 		if (nfs_compare_fh(NFS_FH(fl_blocked->fl_file->f_path.dentry->d_inode) ,fh) != 0)
 			continue;
diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 4a714f6..89eb6f9 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -231,7 +231,7 @@ nlm4svc_proc_granted(struct svc_rqst *rqstp, struct nlm_args *argp,
 	resp->cookie = argp->cookie;
 
 	dprintk("lockd: GRANTED       called\n");
-	resp->status = nlmclnt_grant(svc_addr_in(rqstp), &argp->lock);
+	resp->status = nlmclnt_grant(svc_addr(rqstp), &argp->lock);
 	dprintk("lockd: GRANTED       status %d\n", ntohl(resp->status));
 	return rpc_success;
 }
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index 76262c1..361aac2 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -261,7 +261,7 @@ nlmsvc_proc_granted(struct svc_rqst *rqstp, struct nlm_args *argp,
 	resp->cookie = argp->cookie;
 
 	dprintk("lockd: GRANTED       called\n");
-	resp->status = nlmclnt_grant(svc_addr_in(rqstp), &argp->lock);
+	resp->status = nlmclnt_grant(svc_addr(rqstp), &argp->lock);
 	dprintk("lockd: GRANTED       status %d\n", ntohl(resp->status));
 	return rpc_success;
 }
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 2e13c0b..165ef7a 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -207,7 +207,8 @@ int		  nlm_async_reply(struct nlm_rqst *, u32, const struct rpc_call_ops *);
 struct nlm_wait * nlmclnt_prepare_block(struct nlm_host *host, struct file_lock *fl);
 void		  nlmclnt_finish_block(struct nlm_wait *block);
 int		  nlmclnt_block(struct nlm_wait *block, struct nlm_rqst *req, long timeout);
-__be32		  nlmclnt_grant(const struct sockaddr_in *addr, const struct nlm_lock *);
+__be32		  nlmclnt_grant(const struct sockaddr *addr,
+				const struct nlm_lock *lock);
 void		  nlmclnt_recovery(struct nlm_host *);
 int		  nlmclnt_reclaim(struct nlm_host *, struct file_lock *);
 void		  nlmclnt_next_cookie(struct nlm_cookie *);

Re: [PATCH 04/10] lockd: change nlmclnt_grant() to take a "struct sockaddr *"

[J. Bruce Fields]

On Wed, Sep 17, 2008 at 11:17:42AM -0500, Chuck Lever wrote:
> Adjust the signature and callers of nlmclnt_grant() to pass a "struct
> sockaddr *" instead of a "struct sockaddr_in *" in order to support IPv6
> addresses.

OK!--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/clntlock.c         |    5 ++---
>  fs/lockd/svc4proc.c         |    2 +-
>  fs/lockd/svcproc.c          |    2 +-
>  include/linux/lockd/lockd.h |    3 ++-
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
> index 9eaf306..2976bf0 100644
> --- a/fs/lockd/clntlock.c
> +++ b/fs/lockd/clntlock.c
> @@ -141,7 +141,7 @@ int nlmclnt_block(struct nlm_wait *block, struct nlm_rqst *req, long timeout)
>  /*
>   * The server lockd has called us back to tell us the lock was granted
>   */
> -__be32 nlmclnt_grant(const struct sockaddr_in *addr, const struct nlm_lock *lock)
> +__be32 nlmclnt_grant(const struct sockaddr *addr, const struct nlm_lock *lock)
>  {
>  	const struct file_lock *fl = &lock->fl;
>  	const struct nfs_fh *fh = &lock->fh;
> @@ -165,8 +165,7 @@ __be32 nlmclnt_grant(const struct sockaddr_in *addr, const struct nlm_lock *lock
>  		 */
>  		if (fl_blocked->fl_u.nfs_fl.owner->pid != lock->svid)
>  			continue;
> -		if (!nlm_cmp_addr(nlm_addr(block->b_host),
> -					(struct sockaddr *)addr))
> +		if (!nlm_cmp_addr(nlm_addr(block->b_host), addr))
>  			continue;
>  		if (nfs_compare_fh(NFS_FH(fl_blocked->fl_file->f_path.dentry->d_inode) ,fh) != 0)
>  			continue;
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index 4a714f6..89eb6f9 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -231,7 +231,7 @@ nlm4svc_proc_granted(struct svc_rqst *rqstp, struct nlm_args *argp,
>  	resp->cookie = argp->cookie;
>  
>  	dprintk("lockd: GRANTED       called\n");
> -	resp->status = nlmclnt_grant(svc_addr_in(rqstp), &argp->lock);
> +	resp->status = nlmclnt_grant(svc_addr(rqstp), &argp->lock);
>  	dprintk("lockd: GRANTED       status %d\n", ntohl(resp->status));
>  	return rpc_success;
>  }
> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> index 76262c1..361aac2 100644
> --- a/fs/lockd/svcproc.c
> +++ b/fs/lockd/svcproc.c
> @@ -261,7 +261,7 @@ nlmsvc_proc_granted(struct svc_rqst *rqstp, struct nlm_args *argp,
>  	resp->cookie = argp->cookie;
>  
>  	dprintk("lockd: GRANTED       called\n");
> -	resp->status = nlmclnt_grant(svc_addr_in(rqstp), &argp->lock);
> +	resp->status = nlmclnt_grant(svc_addr(rqstp), &argp->lock);
>  	dprintk("lockd: GRANTED       status %d\n", ntohl(resp->status));
>  	return rpc_success;
>  }
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 2e13c0b..165ef7a 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -207,7 +207,8 @@ int		  nlm_async_reply(struct nlm_rqst *, u32, const struct rpc_call_ops *);
>  struct nlm_wait * nlmclnt_prepare_block(struct nlm_host *host, struct file_lock *fl);
>  void		  nlmclnt_finish_block(struct nlm_wait *block);
>  int		  nlmclnt_block(struct nlm_wait *block, struct nlm_rqst *req, long timeout);
> -__be32		  nlmclnt_grant(const struct sockaddr_in *addr, const struct nlm_lock *);
> +__be32		  nlmclnt_grant(const struct sockaddr *addr,
> +				const struct nlm_lock *lock);
>  void		  nlmclnt_recovery(struct nlm_host *);
>  int		  nlmclnt_reclaim(struct nlm_host *, struct file_lock *);
>  void		  nlmclnt_next_cookie(struct nlm_cookie *);
> 

[PATCH 05/10] lockd: Adjust signature of nlm_host_rebooted to handle non-AF_INET

[Chuck Lever]

Pass a struct sockaddr * and length to nlm_host_rebooted() to accomodate
non-AF_INET addresses.

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

 fs/lockd/host.c             |   28 ++++++++++++++++------------
 fs/lockd/svc4proc.c         |    4 +++-
 fs/lockd/svcproc.c          |    4 +++-
 include/linux/lockd/lockd.h |    7 +++++--
 4 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index cfaa9a2..d893c58 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -473,31 +473,35 @@ 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
+ * @sap: network address of peer that just rebooted
+ * @salen: length of peer's network address
+ * @hostname: hostname of peer that just rebooted
+ * @hostname_len: length of peer's hostname
+ * @new_state: peer's new nsmstate
+ *
+ * We were notified that the host indicated by address "sap"
+ * 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 sockaddr *sap, const size_t salen,
+		       const char *hostname, const size_t hostname_len,
+		       const u32 new_state)
 {
 	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(sap, salen, hostname, hostname_len, 0);
 	if (nsm == NULL) {
 		dprintk("lockd: never saw rebooted peer '%.*s' before\n",
-				hostname_len, hostname);
+				(int)hostname_len, hostname);
 		return;
 	}
 
 	dprintk("lockd: nlm_host_rebooted(%.*s, %s)\n",
-			hostname_len, hostname, nsm->sm_addrbuf);
+			(int)hostname_len, hostname, nsm->sm_addrbuf);
 
 	/* When reclaiming locks on this peer, make sure that
 	 * we set up a new notification */
diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 89eb6f9..9e1c751 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -447,8 +447,10 @@ nlm4svc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
 	 * 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((struct sockaddr *)&saddr, sizeof(saddr),
+			  argp->mon, argp->len, argp->state);
 
 	return rpc_success;
 }
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index 361aac2..fcb7998 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -479,8 +479,10 @@ nlmsvc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
 	 * 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((struct sockaddr *)&saddr, sizeof(saddr),
+			  argp->mon, argp->len, argp->state);
 
 	return rpc_success;
 }
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 165ef7a..075095f 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -229,8 +229,11 @@ 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);
+extern void	  nlm_host_rebooted(const struct sockaddr *sap,
+					const size_t salen,
+					const char *hostname,
+					const size_t hostname_len,
+					const u32 new_state);
 void		  nsm_release(struct nsm_handle *);
 
 

Re: [PATCH 05/10] lockd: Adjust signature of nlm_host_rebooted to handle non-AF_INET

[J. Bruce Fields]

On Wed, Sep 17, 2008 at 11:17:50AM -0500, Chuck Lever wrote:
> Pass a struct sockaddr * and length to nlm_host_rebooted() to accomodate
> non-AF_INET addresses.

OK, thanks.

> +/**
> + * nlm_host_rebooted - Release all resources held by rebooted host
> + * @sap: network address of peer that just rebooted
> + * @salen: length of peer's network address
> + * @hostname: hostname of peer that just rebooted
> + * @hostname_len: length of peer's hostname
> + * @new_state: peer's new nsmstate
> + *
> + * We were notified that the host indicated by address "sap"
> + * 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 sockaddr *sap, const size_t salen,
> +		       const char *hostname, const size_t hostname_len,
> +		       const u32 new_state)
>  {
>  	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(sap, salen, hostname, hostname_len, 0);
>  	if (nsm == NULL) {
>  		dprintk("lockd: never saw rebooted peer '%.*s' before\n",
> -				hostname_len, hostname);
> +				(int)hostname_len, hostname);

This seems unrelated to the current patch.  And there's been no change
to the signedness of hostname_len.  Do we care?

--b.

>  		return;
>  	}
>  
>  	dprintk("lockd: nlm_host_rebooted(%.*s, %s)\n",
> -			hostname_len, hostname, nsm->sm_addrbuf);
> +			(int)hostname_len, hostname, nsm->sm_addrbuf);
>  
>  	/* When reclaiming locks on this peer, make sure that
>  	 * we set up a new notification */
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index 89eb6f9..9e1c751 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -447,8 +447,10 @@ nlm4svc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
>  	 * 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((struct sockaddr *)&saddr, sizeof(saddr),
> +			  argp->mon, argp->len, argp->state);
>  
>  	return rpc_success;
>  }
> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> index 361aac2..fcb7998 100644
> --- a/fs/lockd/svcproc.c
> +++ b/fs/lockd/svcproc.c
> @@ -479,8 +479,10 @@ nlmsvc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
>  	 * 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((struct sockaddr *)&saddr, sizeof(saddr),
> +			  argp->mon, argp->len, argp->state);
>  
>  	return rpc_success;
>  }
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 165ef7a..075095f 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -229,8 +229,11 @@ 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);
> +extern void	  nlm_host_rebooted(const struct sockaddr *sap,
> +					const size_t salen,
> +					const char *hostname,
> +					const size_t hostname_len,
> +					const u32 new_state);
>  void		  nsm_release(struct nsm_handle *);
>  
>  
> 

[PATCH 06/10] lockd: Add helper to sanity check incoming NOTIFY requests

[Chuck Lever]

The NLM performs a silly test to see that incoming NOTIFY requests are
relatively secure.  Make sure the test works for both AF_INET and AF_INET6
addresses.

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

 fs/lockd/svc4proc.c         |    6 ++----
 fs/lockd/svcproc.c          |    6 ++----
 include/linux/lockd/lockd.h |   41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 9e1c751..6a5ef9f 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -432,11 +432,9 @@ nlm4svc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
 {
 	struct sockaddr_in	saddr;
 
-	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
-
 	dprintk("lockd: SM_NOTIFY     called\n");
-	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
-	 || ntohs(saddr.sin_port) >= 1024) {
+
+	if (!nlm_privileged_requester(rqstp)) {
 		char buf[RPC_MAX_ADDRBUFLEN];
 		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
 				svc_print_addr(rqstp, buf, sizeof(buf)));
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index fcb7998..62fcfdb 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -464,11 +464,9 @@ nlmsvc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
 {
 	struct sockaddr_in	saddr;
 
-	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
-
 	dprintk("lockd: SM_NOTIFY     called\n");
-	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
-	 || ntohs(saddr.sin_port) >= 1024) {
+
+	if (!nlm_privileged_requester(rqstp)) {
 		char buf[RPC_MAX_ADDRBUFLEN];
 		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
 				svc_print_addr(rqstp, buf, sizeof(buf)));
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 075095f..409eab4 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -280,6 +280,47 @@ static inline struct inode *nlmsvc_file_inode(struct nlm_file *file)
 	return file->f_file->f_path.dentry->d_inode;
 }
 
+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 defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+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);
+}
+#else	/* defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) */
+static inline int __nlm_privileged_request6(const struct sockaddr *sap)
+{
+	return 0;
+}
+#endif	/* defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) */
+
+/*
+ * Ensure incoming requests are suitably "secure"
+ *
+ * Return TRUE if sender is local and is connecting via a privileged port;
+ * otherwise return FALSE.
+ */
+static inline int nlm_privileged_requester(const struct svc_rqst *rqstp)
+{
+	const struct sockaddr *sap = svc_addr(rqstp);
+
+	switch (sap->sa_family) {
+	case AF_INET:
+		return __nlm_privileged_request4(sap);
+	case AF_INET6:
+		return __nlm_privileged_request6(sap);
+	default:
+		return 0;
+	}
+}
+
 static inline int __nlm_cmp_addr4(const struct sockaddr *sap1,
 				  const struct sockaddr *sap2)
 {

Re: [PATCH 06/10] lockd: Add helper to sanity check incoming NOTIFY requests

[J. Bruce Fields]

On Wed, Sep 17, 2008 at 11:17:57AM -0500, Chuck Lever wrote:
> The NLM performs a silly test to see that incoming NOTIFY requests are
> relatively secure.  Make sure the test works for both AF_INET and AF_INET6
> addresses.

Makes sense.  (Why's the test silly?  If it prevents local users from
telling lockd to drop a client's locks, that seems good.)

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/svc4proc.c         |    6 ++----
>  fs/lockd/svcproc.c          |    6 ++----
>  include/linux/lockd/lockd.h |   41 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index 9e1c751..6a5ef9f 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -432,11 +432,9 @@ nlm4svc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
>  {
>  	struct sockaddr_in	saddr;
>  
> -	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
> -
>  	dprintk("lockd: SM_NOTIFY     called\n");
> -	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
> -	 || ntohs(saddr.sin_port) >= 1024) {
> +
> +	if (!nlm_privileged_requester(rqstp)) {
>  		char buf[RPC_MAX_ADDRBUFLEN];
>  		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
>  				svc_print_addr(rqstp, buf, sizeof(buf)));
> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> index fcb7998..62fcfdb 100644
> --- a/fs/lockd/svcproc.c
> +++ b/fs/lockd/svcproc.c
> @@ -464,11 +464,9 @@ nlmsvc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
>  {
>  	struct sockaddr_in	saddr;
>  
> -	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
> -
>  	dprintk("lockd: SM_NOTIFY     called\n");
> -	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
> -	 || ntohs(saddr.sin_port) >= 1024) {
> +
> +	if (!nlm_privileged_requester(rqstp)) {
>  		char buf[RPC_MAX_ADDRBUFLEN];
>  		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
>  				svc_print_addr(rqstp, buf, sizeof(buf)));
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 075095f..409eab4 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -280,6 +280,47 @@ static inline struct inode *nlmsvc_file_inode(struct nlm_file *file)
>  	return file->f_file->f_path.dentry->d_inode;
>  }
>  
> +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 defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> +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);
> +}
> +#else	/* defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) */
> +static inline int __nlm_privileged_request6(const struct sockaddr *sap)
> +{
> +	return 0;
> +}
> +#endif	/* defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) */
> +
> +/*
> + * Ensure incoming requests are suitably "secure"
> + *
> + * Return TRUE if sender is local and is connecting via a privileged port;
> + * otherwise return FALSE.
> + */
> +static inline int nlm_privileged_requester(const struct svc_rqst *rqstp)
> +{
> +	const struct sockaddr *sap = svc_addr(rqstp);
> +
> +	switch (sap->sa_family) {
> +	case AF_INET:
> +		return __nlm_privileged_request4(sap);
> +	case AF_INET6:
> +		return __nlm_privileged_request6(sap);
> +	default:
> +		return 0;
> +	}
> +}
> +
>  static inline int __nlm_cmp_addr4(const struct sockaddr *sap1,
>  				  const struct sockaddr *sap2)
>  {
> 

Re: [PATCH 06/10] lockd: Add helper to sanity check incoming NOTIFY requests

[Chuck Lever]

On Sep 26, 2008, at Sep 26, 2008, 6:43 PM, J. Bruce Fields wrote:
> On Wed, Sep 17, 2008 at 11:17:57AM -0500, Chuck Lever wrote:
>> The NLM performs a silly test to see that incoming NOTIFY requests  
>> are
>> relatively secure.  Make sure the test works for both AF_INET and  
>> AF_INET6
>> addresses.
>
> Makes sense.  (Why's the test silly?  If it prevents local users from
> telling lockd to drop a client's locks, that seems good.)

I was referring to the port range part of the test.  Anyone who wants  
real security will not rely on the port value, but will use SSL or  
third-party authentication like Kerberos.

>
>
> --b.
>
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/lockd/svc4proc.c         |    6 ++----
>> fs/lockd/svcproc.c          |    6 ++----
>> include/linux/lockd/lockd.h |   41 +++++++++++++++++++++++++++++++++ 
>> ++++++++
>> 3 files changed, 45 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
>> index 9e1c751..6a5ef9f 100644
>> --- a/fs/lockd/svc4proc.c
>> +++ b/fs/lockd/svc4proc.c
>> @@ -432,11 +432,9 @@ nlm4svc_proc_sm_notify(struct svc_rqst *rqstp,  
>> struct nlm_reboot *argp,
>> {
>> 	struct sockaddr_in	saddr;
>>
>> -	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
>> -
>> 	dprintk("lockd: SM_NOTIFY     called\n");
>> -	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
>> -	 || ntohs(saddr.sin_port) >= 1024) {
>> +
>> +	if (!nlm_privileged_requester(rqstp)) {
>> 		char buf[RPC_MAX_ADDRBUFLEN];
>> 		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
>> 				svc_print_addr(rqstp, buf, sizeof(buf)));
>> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
>> index fcb7998..62fcfdb 100644
>> --- a/fs/lockd/svcproc.c
>> +++ b/fs/lockd/svcproc.c
>> @@ -464,11 +464,9 @@ nlmsvc_proc_sm_notify(struct svc_rqst *rqstp,  
>> struct nlm_reboot *argp,
>> {
>> 	struct sockaddr_in	saddr;
>>
>> -	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
>> -
>> 	dprintk("lockd: SM_NOTIFY     called\n");
>> -	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
>> -	 || ntohs(saddr.sin_port) >= 1024) {
>> +
>> +	if (!nlm_privileged_requester(rqstp)) {
>> 		char buf[RPC_MAX_ADDRBUFLEN];
>> 		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
>> 				svc_print_addr(rqstp, buf, sizeof(buf)));
>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/ 
>> lockd.h
>> index 075095f..409eab4 100644
>> --- a/include/linux/lockd/lockd.h
>> +++ b/include/linux/lockd/lockd.h
>> @@ -280,6 +280,47 @@ static inline struct inode  
>> *nlmsvc_file_inode(struct nlm_file *file)
>> 	return file->f_file->f_path.dentry->d_inode;
>> }
>>
>> +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 defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>> +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);
>> +}
>> +#else	/* defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) */
>> +static inline int __nlm_privileged_request6(const struct sockaddr  
>> *sap)
>> +{
>> +	return 0;
>> +}
>> +#endif	/* defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) */
>> +
>> +/*
>> + * Ensure incoming requests are suitably "secure"
>> + *
>> + * Return TRUE if sender is local and is connecting via a  
>> privileged port;
>> + * otherwise return FALSE.
>> + */
>> +static inline int nlm_privileged_requester(const struct svc_rqst  
>> *rqstp)
>> +{
>> +	const struct sockaddr *sap = svc_addr(rqstp);
>> +
>> +	switch (sap->sa_family) {
>> +	case AF_INET:
>> +		return __nlm_privileged_request4(sap);
>> +	case AF_INET6:
>> +		return __nlm_privileged_request6(sap);
>> +	default:
>> +		return 0;
>> +	}
>> +}
>> +
>> static inline int __nlm_cmp_addr4(const struct sockaddr *sap1,
>> 				  const struct sockaddr *sap2)
>> {
>>
> --
> 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 06/10] lockd: Add helper to sanity check incoming NOTIFY requests

[J. Bruce Fields]

On Wed, Oct 01, 2008 at 12:01:30PM -0400, Chuck Lever wrote:
> On Sep 26, 2008, at Sep 26, 2008, 6:43 PM, J. Bruce Fields wrote:
>> On Wed, Sep 17, 2008 at 11:17:57AM -0500, Chuck Lever wrote:
>>> The NLM performs a silly test to see that incoming NOTIFY requests  
>>> are
>>> relatively secure.  Make sure the test works for both AF_INET and  
>>> AF_INET6
>>> addresses.
>>
>> Makes sense.  (Why's the test silly?  If it prevents local users from
>> telling lockd to drop a client's locks, that seems good.)
>
> I was referring to the port range part of the test.  Anyone who wants  
> real security will not rely on the port value, but will use SSL or  
> third-party authentication like Kerberos.

Over the loopback interface?

This is a local call--if the kernel needs kerberos to decide whether a
local process is privileged, something's wrong.

--b.

>
>>
>>
>> --b.
>>
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> fs/lockd/svc4proc.c         |    6 ++----
>>> fs/lockd/svcproc.c          |    6 ++----
>>> include/linux/lockd/lockd.h |   41 +++++++++++++++++++++++++++++++++ 
>>> ++++++++
>>> 3 files changed, 45 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
>>> index 9e1c751..6a5ef9f 100644
>>> --- a/fs/lockd/svc4proc.c
>>> +++ b/fs/lockd/svc4proc.c
>>> @@ -432,11 +432,9 @@ nlm4svc_proc_sm_notify(struct svc_rqst *rqstp,  
>>> struct nlm_reboot *argp,
>>> {
>>> 	struct sockaddr_in	saddr;
>>>
>>> -	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
>>> -
>>> 	dprintk("lockd: SM_NOTIFY     called\n");
>>> -	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
>>> -	 || ntohs(saddr.sin_port) >= 1024) {
>>> +
>>> +	if (!nlm_privileged_requester(rqstp)) {
>>> 		char buf[RPC_MAX_ADDRBUFLEN];
>>> 		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
>>> 				svc_print_addr(rqstp, buf, sizeof(buf)));
>>> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
>>> index fcb7998..62fcfdb 100644
>>> --- a/fs/lockd/svcproc.c
>>> +++ b/fs/lockd/svcproc.c
>>> @@ -464,11 +464,9 @@ nlmsvc_proc_sm_notify(struct svc_rqst *rqstp,  
>>> struct nlm_reboot *argp,
>>> {
>>> 	struct sockaddr_in	saddr;
>>>
>>> -	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
>>> -
>>> 	dprintk("lockd: SM_NOTIFY     called\n");
>>> -	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
>>> -	 || ntohs(saddr.sin_port) >= 1024) {
>>> +
>>> +	if (!nlm_privileged_requester(rqstp)) {
>>> 		char buf[RPC_MAX_ADDRBUFLEN];
>>> 		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
>>> 				svc_print_addr(rqstp, buf, sizeof(buf)));
>>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/ 
>>> lockd.h
>>> index 075095f..409eab4 100644
>>> --- a/include/linux/lockd/lockd.h
>>> +++ b/include/linux/lockd/lockd.h
>>> @@ -280,6 +280,47 @@ static inline struct inode  
>>> *nlmsvc_file_inode(struct nlm_file *file)
>>> 	return file->f_file->f_path.dentry->d_inode;
>>> }
>>>
>>> +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 defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>>> +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);
>>> +}
>>> +#else	/* defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) */
>>> +static inline int __nlm_privileged_request6(const struct sockaddr  
>>> *sap)
>>> +{
>>> +	return 0;
>>> +}
>>> +#endif	/* defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) */
>>> +
>>> +/*
>>> + * Ensure incoming requests are suitably "secure"
>>> + *
>>> + * Return TRUE if sender is local and is connecting via a  
>>> privileged port;
>>> + * otherwise return FALSE.
>>> + */
>>> +static inline int nlm_privileged_requester(const struct svc_rqst  
>>> *rqstp)
>>> +{
>>> +	const struct sockaddr *sap = svc_addr(rqstp);
>>> +
>>> +	switch (sap->sa_family) {
>>> +	case AF_INET:
>>> +		return __nlm_privileged_request4(sap);
>>> +	case AF_INET6:
>>> +		return __nlm_privileged_request6(sap);
>>> +	default:
>>> +		return 0;
>>> +	}
>>> +}
>>> +
>>> static inline int __nlm_cmp_addr4(const struct sockaddr *sap1,
>>> 				  const struct sockaddr *sap2)
>>> {
>>>
>> --
>> 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 07/10] lockd: Remove unused fields in the nlm_reboot structure

[Chuck Lever]

The nlm_reboot structure is used to store information provided by the
NSM_NOTIFY procedure.  This procedure is not specified by the NLM or NSM
protocols, other than to say that the procedure can be used to transmit
information private to a particular NLM/NSM implementation.

For Linux, the callback arguments include the name of the monitored host,
the new NSM state of the host, and a 16-byte private opaque.

Linux encodes the monitored host's IP address into the opaque field.
Formerly, this field contained a 4-byte in_addr IPv4 address and the NSM
version and transport protocol used to contact the monitored host.
Recently this was changed so that the private field now contains the
4-byte IPv4 address and 12 bytes of zero.

As a clean up, remove the unused fields and the server-side XDR logic that
decodes them.

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

 fs/lockd/xdr.c            |    2 --
 fs/lockd/xdr4.c           |    2 --
 include/linux/lockd/xdr.h |    2 --
 3 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/fs/lockd/xdr.c b/fs/lockd/xdr.c
index 3e459e1..1f22629 100644
--- a/fs/lockd/xdr.c
+++ b/fs/lockd/xdr.c
@@ -351,8 +351,6 @@ nlmsvc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct nlm_reboot *argp)
 	argp->state = ntohl(*p++);
 	/* Preserve the address in network byte order */
 	argp->addr = *p++;
-	argp->vers = *p++;
-	argp->proto = *p++;
 	return xdr_argsize_check(rqstp, p);
 }
 
diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
index 43ff939..50c493a 100644
--- a/fs/lockd/xdr4.c
+++ b/fs/lockd/xdr4.c
@@ -358,8 +358,6 @@ nlm4svc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct nlm_reboot *argp
 	argp->state = ntohl(*p++);
 	/* Preserve the address in network byte order */
 	argp->addr  = *p++;
-	argp->vers  = *p++;
-	argp->proto = *p++;
 	return xdr_argsize_check(rqstp, p);
 }
 
diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
index df18fa0..d6b3a80 100644
--- a/include/linux/lockd/xdr.h
+++ b/include/linux/lockd/xdr.h
@@ -81,8 +81,6 @@ struct nlm_reboot {
 	unsigned int	len;
 	u32		state;
 	__be32		addr;
-	__be32		vers;
-	__be32		proto;
 };
 
 /*

Re: [PATCH 07/10] lockd: Remove unused fields in the nlm_reboot structure

[J. Bruce Fields]

On Wed, Sep 17, 2008 at 11:18:04AM -0500, Chuck Lever wrote:
> The nlm_reboot structure is used to store information provided by the
> NSM_NOTIFY procedure.  This procedure is not specified by the NLM or NSM
> protocols, other than to say that the procedure can be used to transmit
> information private to a particular NLM/NSM implementation.
> 
> For Linux, the callback arguments include the name of the monitored host,
> the new NSM state of the host, and a 16-byte private opaque.
> 
> Linux encodes the monitored host's IP address into the opaque field.
> Formerly, this field contained a 4-byte in_addr IPv4 address and the NSM
> version and transport protocol used to contact the monitored host.
> Recently this was changed so that the private field now contains the
> 4-byte IPv4 address and 12 bytes of zero.
> 
> As a clean up, remove the unused fields and the server-side XDR logic that
> decodes them.

My one concern here:

> diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
> index df18fa0..d6b3a80 100644
> --- a/include/linux/lockd/xdr.h
> +++ b/include/linux/lockd/xdr.h
> @@ -81,8 +81,6 @@ struct nlm_reboot {
>  	unsigned int	len;
>  	u32		state;
>  	__be32		addr;
> -	__be32		vers;
> -	__be32		proto;
>  };

is that sizeof(nlm_reboot) is used to set pc_argsize in the PROC()
macro's at the end of svcproc.c and svc4proc.c.  On a quick glance I can
see any problem that that would cause.  Have you checked this?

--b.

Re: [PATCH 07/10] lockd: Remove unused fields in the nlm_reboot structure

[J. Bruce Fields]

On Fri, Sep 26, 2008 at 06:53:31PM -0400, bfields wrote:
> On Wed, Sep 17, 2008 at 11:18:04AM -0500, Chuck Lever wrote:
> > The nlm_reboot structure is used to store information provided by the
> > NSM_NOTIFY procedure.  This procedure is not specified by the NLM or NSM
> > protocols, other than to say that the procedure can be used to transmit
> > information private to a particular NLM/NSM implementation.
> > 
> > For Linux, the callback arguments include the name of the monitored host,
> > the new NSM state of the host, and a 16-byte private opaque.
> > 
> > Linux encodes the monitored host's IP address into the opaque field.
> > Formerly, this field contained a 4-byte in_addr IPv4 address and the NSM
> > version and transport protocol used to contact the monitored host.
> > Recently this was changed so that the private field now contains the
> > 4-byte IPv4 address and 12 bytes of zero.
> > 
> > As a clean up, remove the unused fields and the server-side XDR logic that
> > decodes them.
> 
> My one concern here:
> 
> > diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
> > index df18fa0..d6b3a80 100644
> > --- a/include/linux/lockd/xdr.h
> > +++ b/include/linux/lockd/xdr.h
> > @@ -81,8 +81,6 @@ struct nlm_reboot {
> >  	unsigned int	len;
> >  	u32		state;
> >  	__be32		addr;
> > -	__be32		vers;
> > -	__be32		proto;
> >  };
> 
> is that sizeof(nlm_reboot) is used to set pc_argsize in the PROC()
> macro's at the end of svcproc.c and svc4proc.c.  On a quick glance I can
> see any problem that that would cause.  Have you checked this?

Never mind, I think I was assuming there was some connection between the
size of this struct and the on-the-wire xdr size, which isn't the case.
OK, this shouldn't matter at all.

--b.

[PATCH 08/10] lockd: struct nlm_reboot should contain a full socket address

[Chuck Lever]

The XDR decoders for the NSM NOTIFY procedure should construct a full
socket address and store it in the nlm_reboot structure.  In addition
to being able to store larger addresses, this means upper layer
routines get an address family tag so they can distinguish between
AF_INET and AF_INET6 addresses.

This also keeps potentially large socket addresses off the stack and
instead in dynamically allocated storage.

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

 fs/lockd/svc4proc.c       |   10 +---------
 fs/lockd/svcproc.c        |   10 +---------
 fs/lockd/xdr.c            |   28 ++++++++++++++++++++++++++--
 fs/lockd/xdr4.c           |   27 +++++++++++++++++++++++++--
 include/linux/lockd/xdr.h |    3 ++-
 5 files changed, 55 insertions(+), 23 deletions(-)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 6a5ef9f..e61c05f 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -430,8 +430,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)) {
@@ -441,13 +439,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((struct sockaddr *)&saddr, sizeof(saddr),
+	nlm_host_rebooted((struct sockaddr *)&argp->addr, argp->addrlen,
 			  argp->mon, argp->len, argp->state);
 
 	return rpc_success;
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index 62fcfdb..86a487a 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -462,8 +462,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)) {
@@ -473,13 +471,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((struct sockaddr *)&saddr, sizeof(saddr),
+	nlm_host_rebooted((struct sockaddr *)&argp->addr, argp->addrlen,
 			  argp->mon, argp->len, argp->state);
 
 	return rpc_success;
diff --git a/fs/lockd/xdr.c b/fs/lockd/xdr.c
index 1f22629..92c5695 100644
--- a/fs/lockd/xdr.c
+++ b/fs/lockd/xdr.c
@@ -9,6 +9,7 @@
 #include <linux/types.h>
 #include <linux/sched.h>
 #include <linux/utsname.h>
+#include <linux/in.h>
 #include <linux/nfs.h>
 
 #include <linux/sunrpc/xdr.h>
@@ -346,11 +347,34 @@ nlmsvc_decode_notify(struct svc_rqst *rqstp, __be32 *p, struct nlm_args *argp)
 int
 nlmsvc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct nlm_reboot *argp)
 {
+	struct sockaddr_in *sin = (struct sockaddr_in *)&argp->addr;
+	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&argp->addr;
+
 	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++;
+
+	/* Decode the 16-byte private field */
+	memset(&argp->addr, 0, sizeof(argp->addr));
+	switch (svc_addr(rqstp)->sa_family) {
+	case AF_INET:
+		/* data in recv buffer is already in network byte order */
+		sin->sin_family = AF_INET;
+		sin->sin_addr.s_addr = *p++;
+		argp->addrlen = sizeof(*sin);
+		break;
+	case AF_INET6:
+		sin6->sin6_family = AF_INET6;
+		sin6->sin6_addr.s6_addr32[0] = *p++;
+		sin6->sin6_addr.s6_addr32[1] = *p++;
+		sin6->sin6_addr.s6_addr32[2] = *p++;
+		sin6->sin6_addr.s6_addr32[3] = *p++;
+		argp->addrlen = sizeof(*sin6);
+		break;
+	default:
+		return -EIO;
+	}
+
 	return xdr_argsize_check(rqstp, p);
 }
 
diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
index 50c493a..2009020 100644
--- a/fs/lockd/xdr4.c
+++ b/fs/lockd/xdr4.c
@@ -353,11 +353,34 @@ nlm4svc_decode_notify(struct svc_rqst *rqstp, __be32 *p, struct nlm_args *argp)
 int
 nlm4svc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct nlm_reboot *argp)
 {
+	struct sockaddr_in *sin = (struct sockaddr_in *)&argp->addr;
+	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&argp->addr;
+
 	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++;
+
+	/* Decode the 16-byte private field */
+	memset(&argp->addr, 0, sizeof(argp->addr));
+	switch (svc_addr(rqstp)->sa_family) {
+	case AF_INET:
+		/* address in recv buffer is already in network byte order */
+		sin->sin_family = AF_INET;
+		sin->sin_addr.s_addr = *p++;
+		argp->addrlen = sizeof(*sin);
+		break;
+	case AF_INET6:
+		sin6->sin6_family = AF_INET6;
+		sin6->sin6_addr.s6_addr32[0] = *p++;
+		sin6->sin6_addr.s6_addr32[1] = *p++;
+		sin6->sin6_addr.s6_addr32[2] = *p++;
+		sin6->sin6_addr.s6_addr32[3] = *p++;
+		argp->addrlen = sizeof(*sin6);
+		break;
+	default:
+		return -EIO;
+	}
+
 	return xdr_argsize_check(rqstp, p);
 }
 
diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
index d6b3a80..6057b7e 100644
--- a/include/linux/lockd/xdr.h
+++ b/include/linux/lockd/xdr.h
@@ -80,7 +80,8 @@ struct nlm_reboot {
 	char *		mon;
 	unsigned int	len;
 	u32		state;
-	__be32		addr;
+	struct sockaddr_storage	addr;
+	size_t		addrlen;
 };
 
 /*

Re: [PATCH 08/10] lockd: struct nlm_reboot should contain a full socket address

[J. Bruce Fields]

On Wed, Sep 17, 2008 at 11:18:11AM -0500, Chuck Lever wrote:
> The XDR decoders for the NSM NOTIFY procedure should construct a full
> socket address and store it in the nlm_reboot structure.  In addition
> to being able to store larger addresses, this means upper layer
> routines get an address family tag so they can distinguish between
> AF_INET and AF_INET6 addresses.
> 
> This also keeps potentially large socket addresses off the stack and
> instead in dynamically allocated storage.

So one way to think of this would be that you're extending the
kernel<->statd interface by using the address family of statd's notify
call to communicate the address family of the host that rebooted.

Do I have that right?

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/svc4proc.c       |   10 +---------
>  fs/lockd/svcproc.c        |   10 +---------
>  fs/lockd/xdr.c            |   28 ++++++++++++++++++++++++++--
>  fs/lockd/xdr4.c           |   27 +++++++++++++++++++++++++--
>  include/linux/lockd/xdr.h |    3 ++-
>  5 files changed, 55 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index 6a5ef9f..e61c05f 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -430,8 +430,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)) {
> @@ -441,13 +439,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((struct sockaddr *)&saddr, sizeof(saddr),
> +	nlm_host_rebooted((struct sockaddr *)&argp->addr, argp->addrlen,
>  			  argp->mon, argp->len, argp->state);
>  
>  	return rpc_success;
> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> index 62fcfdb..86a487a 100644
> --- a/fs/lockd/svcproc.c
> +++ b/fs/lockd/svcproc.c
> @@ -462,8 +462,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)) {
> @@ -473,13 +471,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((struct sockaddr *)&saddr, sizeof(saddr),
> +	nlm_host_rebooted((struct sockaddr *)&argp->addr, argp->addrlen,
>  			  argp->mon, argp->len, argp->state);
>  
>  	return rpc_success;
> diff --git a/fs/lockd/xdr.c b/fs/lockd/xdr.c
> index 1f22629..92c5695 100644
> --- a/fs/lockd/xdr.c
> +++ b/fs/lockd/xdr.c
> @@ -9,6 +9,7 @@
>  #include <linux/types.h>
>  #include <linux/sched.h>
>  #include <linux/utsname.h>
> +#include <linux/in.h>
>  #include <linux/nfs.h>
>  
>  #include <linux/sunrpc/xdr.h>
> @@ -346,11 +347,34 @@ nlmsvc_decode_notify(struct svc_rqst *rqstp, __be32 *p, struct nlm_args *argp)
>  int
>  nlmsvc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct nlm_reboot *argp)
>  {
> +	struct sockaddr_in *sin = (struct sockaddr_in *)&argp->addr;
> +	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&argp->addr;
> +
>  	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++;
> +
> +	/* Decode the 16-byte private field */
> +	memset(&argp->addr, 0, sizeof(argp->addr));
> +	switch (svc_addr(rqstp)->sa_family) {
> +	case AF_INET:
> +		/* data in recv buffer is already in network byte order */
> +		sin->sin_family = AF_INET;
> +		sin->sin_addr.s_addr = *p++;
> +		argp->addrlen = sizeof(*sin);
> +		break;
> +	case AF_INET6:
> +		sin6->sin6_family = AF_INET6;
> +		sin6->sin6_addr.s6_addr32[0] = *p++;
> +		sin6->sin6_addr.s6_addr32[1] = *p++;
> +		sin6->sin6_addr.s6_addr32[2] = *p++;
> +		sin6->sin6_addr.s6_addr32[3] = *p++;
> +		argp->addrlen = sizeof(*sin6);
> +		break;
> +	default:
> +		return -EIO;
> +	}
> +
>  	return xdr_argsize_check(rqstp, p);
>  }
>  
> diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
> index 50c493a..2009020 100644
> --- a/fs/lockd/xdr4.c
> +++ b/fs/lockd/xdr4.c
> @@ -353,11 +353,34 @@ nlm4svc_decode_notify(struct svc_rqst *rqstp, __be32 *p, struct nlm_args *argp)
>  int
>  nlm4svc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct nlm_reboot *argp)
>  {
> +	struct sockaddr_in *sin = (struct sockaddr_in *)&argp->addr;
> +	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&argp->addr;
> +
>  	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++;
> +
> +	/* Decode the 16-byte private field */
> +	memset(&argp->addr, 0, sizeof(argp->addr));
> +	switch (svc_addr(rqstp)->sa_family) {
> +	case AF_INET:
> +		/* address in recv buffer is already in network byte order */
> +		sin->sin_family = AF_INET;
> +		sin->sin_addr.s_addr = *p++;
> +		argp->addrlen = sizeof(*sin);
> +		break;
> +	case AF_INET6:
> +		sin6->sin6_family = AF_INET6;
> +		sin6->sin6_addr.s6_addr32[0] = *p++;
> +		sin6->sin6_addr.s6_addr32[1] = *p++;
> +		sin6->sin6_addr.s6_addr32[2] = *p++;
> +		sin6->sin6_addr.s6_addr32[3] = *p++;
> +		argp->addrlen = sizeof(*sin6);
> +		break;
> +	default:
> +		return -EIO;
> +	}
> +
>  	return xdr_argsize_check(rqstp, p);
>  }
>  
> diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
> index d6b3a80..6057b7e 100644
> --- a/include/linux/lockd/xdr.h
> +++ b/include/linux/lockd/xdr.h
> @@ -80,7 +80,8 @@ struct nlm_reboot {
>  	char *		mon;
>  	unsigned int	len;
>  	u32		state;
> -	__be32		addr;
> +	struct sockaddr_storage	addr;
> +	size_t		addrlen;
>  };
>  
>  /*
> 

Re: [PATCH 08/10] lockd: struct nlm_reboot should contain a full socket address

[Chuck Lever]

On Sep 26, 2008, at Sep 26, 2008, 7:09 PM, J. Bruce Fields wrote:
> On Wed, Sep 17, 2008 at 11:18:11AM -0500, Chuck Lever wrote:
>> The XDR decoders for the NSM NOTIFY procedure should construct a full
>> socket address and store it in the nlm_reboot structure.  In addition
>> to being able to store larger addresses, this means upper layer
>> routines get an address family tag so they can distinguish between
>> AF_INET and AF_INET6 addresses.
>>
>> This also keeps potentially large socket addresses off the stack and
>> instead in dynamically allocated storage.
>
> So one way to think of this would be that you're extending the
> kernel<->statd interface by using the address family of statd's notify
> call to communicate the address family of the host that rebooted.
>
> Do I have that right?

For statd, we're using the same technique that we used when  
constructing the source address in nlmsvc_lookup_host().  There's no  
family tag associated with the address because the 16-byte opaque in  
the on-the-wire format has room only for the sin6_addr part of the  
address.  The notification downcall for IPv6 peers has to come via  
IPv6 loopback for this to work.

>
>
> --b.
>
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/lockd/svc4proc.c       |   10 +---------
>> fs/lockd/svcproc.c        |   10 +---------
>> fs/lockd/xdr.c            |   28 ++++++++++++++++++++++++++--
>> fs/lockd/xdr4.c           |   27 +++++++++++++++++++++++++--
>> include/linux/lockd/xdr.h |    3 ++-
>> 5 files changed, 55 insertions(+), 23 deletions(-)
>>
>> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
>> index 6a5ef9f..e61c05f 100644
>> --- a/fs/lockd/svc4proc.c
>> +++ b/fs/lockd/svc4proc.c
>> @@ -430,8 +430,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)) {
>> @@ -441,13 +439,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((struct sockaddr *)&saddr, sizeof(saddr),
>> +	nlm_host_rebooted((struct sockaddr *)&argp->addr, argp->addrlen,
>> 			  argp->mon, argp->len, argp->state);
>>
>> 	return rpc_success;
>> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
>> index 62fcfdb..86a487a 100644
>> --- a/fs/lockd/svcproc.c
>> +++ b/fs/lockd/svcproc.c
>> @@ -462,8 +462,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)) {
>> @@ -473,13 +471,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((struct sockaddr *)&saddr, sizeof(saddr),
>> +	nlm_host_rebooted((struct sockaddr *)&argp->addr, argp->addrlen,
>> 			  argp->mon, argp->len, argp->state);
>>
>> 	return rpc_success;
>> diff --git a/fs/lockd/xdr.c b/fs/lockd/xdr.c
>> index 1f22629..92c5695 100644
>> --- a/fs/lockd/xdr.c
>> +++ b/fs/lockd/xdr.c
>> @@ -9,6 +9,7 @@
>> #include <linux/types.h>
>> #include <linux/sched.h>
>> #include <linux/utsname.h>
>> +#include <linux/in.h>
>> #include <linux/nfs.h>
>>
>> #include <linux/sunrpc/xdr.h>
>> @@ -346,11 +347,34 @@ nlmsvc_decode_notify(struct svc_rqst *rqstp,  
>> __be32 *p, struct nlm_args *argp)
>> int
>> nlmsvc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct  
>> nlm_reboot *argp)
>> {
>> +	struct sockaddr_in *sin = (struct sockaddr_in *)&argp->addr;
>> +	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&argp->addr;
>> +
>> 	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++;
>> +
>> +	/* Decode the 16-byte private field */
>> +	memset(&argp->addr, 0, sizeof(argp->addr));
>> +	switch (svc_addr(rqstp)->sa_family) {
>> +	case AF_INET:
>> +		/* data in recv buffer is already in network byte order */
>> +		sin->sin_family = AF_INET;
>> +		sin->sin_addr.s_addr = *p++;
>> +		argp->addrlen = sizeof(*sin);
>> +		break;
>> +	case AF_INET6:
>> +		sin6->sin6_family = AF_INET6;
>> +		sin6->sin6_addr.s6_addr32[0] = *p++;
>> +		sin6->sin6_addr.s6_addr32[1] = *p++;
>> +		sin6->sin6_addr.s6_addr32[2] = *p++;
>> +		sin6->sin6_addr.s6_addr32[3] = *p++;
>> +		argp->addrlen = sizeof(*sin6);
>> +		break;
>> +	default:
>> +		return -EIO;
>> +	}
>> +
>> 	return xdr_argsize_check(rqstp, p);
>> }
>>
>> diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
>> index 50c493a..2009020 100644
>> --- a/fs/lockd/xdr4.c
>> +++ b/fs/lockd/xdr4.c
>> @@ -353,11 +353,34 @@ nlm4svc_decode_notify(struct svc_rqst *rqstp,  
>> __be32 *p, struct nlm_args *argp)
>> int
>> nlm4svc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct  
>> nlm_reboot *argp)
>> {
>> +	struct sockaddr_in *sin = (struct sockaddr_in *)&argp->addr;
>> +	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&argp->addr;
>> +
>> 	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++;
>> +
>> +	/* Decode the 16-byte private field */
>> +	memset(&argp->addr, 0, sizeof(argp->addr));
>> +	switch (svc_addr(rqstp)->sa_family) {
>> +	case AF_INET:
>> +		/* address in recv buffer is already in network byte order */
>> +		sin->sin_family = AF_INET;
>> +		sin->sin_addr.s_addr = *p++;
>> +		argp->addrlen = sizeof(*sin);
>> +		break;
>> +	case AF_INET6:
>> +		sin6->sin6_family = AF_INET6;
>> +		sin6->sin6_addr.s6_addr32[0] = *p++;
>> +		sin6->sin6_addr.s6_addr32[1] = *p++;
>> +		sin6->sin6_addr.s6_addr32[2] = *p++;
>> +		sin6->sin6_addr.s6_addr32[3] = *p++;
>> +		argp->addrlen = sizeof(*sin6);
>> +		break;
>> +	default:
>> +		return -EIO;
>> +	}
>> +
>> 	return xdr_argsize_check(rqstp, p);
>> }
>>
>> diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
>> index d6b3a80..6057b7e 100644
>> --- a/include/linux/lockd/xdr.h
>> +++ b/include/linux/lockd/xdr.h
>> @@ -80,7 +80,8 @@ struct nlm_reboot {
>> 	char *		mon;
>> 	unsigned int	len;
>> 	u32		state;
>> -	__be32		addr;
>> +	struct sockaddr_storage	addr;
>> +	size_t		addrlen;
>> };
>>
>> /*
>>
> --
> 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 08/10] lockd: struct nlm_reboot should contain a full socket address

[J. Bruce Fields]

On Wed, Oct 01, 2008 at 12:17:02PM -0400, Chuck Lever wrote:
> On Sep 26, 2008, at Sep 26, 2008, 7:09 PM, J. Bruce Fields wrote:
>> On Wed, Sep 17, 2008 at 11:18:11AM -0500, Chuck Lever wrote:
>>> The XDR decoders for the NSM NOTIFY procedure should construct a full
>>> socket address and store it in the nlm_reboot structure.  In addition
>>> to being able to store larger addresses, this means upper layer
>>> routines get an address family tag so they can distinguish between
>>> AF_INET and AF_INET6 addresses.
>>>
>>> This also keeps potentially large socket addresses off the stack and
>>> instead in dynamically allocated storage.
>>
>> So one way to think of this would be that you're extending the
>> kernel<->statd interface by using the address family of statd's notify
>> call to communicate the address family of the host that rebooted.
>>
>> Do I have that right?
>
> For statd, we're using the same technique that we used when constructing 
> the source address in nlmsvc_lookup_host().  There's no family tag 
> associated with the address because the 16-byte opaque in the on-the-wire 
> format has room only for the sin6_addr part of the address.

OK.  It seems a bit tricky, but I can't see why it doesn't work.

--b.

> The notification downcall for IPv6 peers has to come via IPv6 loopback
> for this to work.
>
>>
>>
>> --b.
>>
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> fs/lockd/svc4proc.c       |   10 +---------
>>> fs/lockd/svcproc.c        |   10 +---------
>>> fs/lockd/xdr.c            |   28 ++++++++++++++++++++++++++--
>>> fs/lockd/xdr4.c           |   27 +++++++++++++++++++++++++--
>>> include/linux/lockd/xdr.h |    3 ++-
>>> 5 files changed, 55 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
>>> index 6a5ef9f..e61c05f 100644
>>> --- a/fs/lockd/svc4proc.c
>>> +++ b/fs/lockd/svc4proc.c
>>> @@ -430,8 +430,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)) {
>>> @@ -441,13 +439,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((struct sockaddr *)&saddr, sizeof(saddr),
>>> +	nlm_host_rebooted((struct sockaddr *)&argp->addr, argp->addrlen,
>>> 			  argp->mon, argp->len, argp->state);
>>>
>>> 	return rpc_success;
>>> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
>>> index 62fcfdb..86a487a 100644
>>> --- a/fs/lockd/svcproc.c
>>> +++ b/fs/lockd/svcproc.c
>>> @@ -462,8 +462,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)) {
>>> @@ -473,13 +471,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((struct sockaddr *)&saddr, sizeof(saddr),
>>> +	nlm_host_rebooted((struct sockaddr *)&argp->addr, argp->addrlen,
>>> 			  argp->mon, argp->len, argp->state);
>>>
>>> 	return rpc_success;
>>> diff --git a/fs/lockd/xdr.c b/fs/lockd/xdr.c
>>> index 1f22629..92c5695 100644
>>> --- a/fs/lockd/xdr.c
>>> +++ b/fs/lockd/xdr.c
>>> @@ -9,6 +9,7 @@
>>> #include <linux/types.h>
>>> #include <linux/sched.h>
>>> #include <linux/utsname.h>
>>> +#include <linux/in.h>
>>> #include <linux/nfs.h>
>>>
>>> #include <linux/sunrpc/xdr.h>
>>> @@ -346,11 +347,34 @@ nlmsvc_decode_notify(struct svc_rqst *rqstp,  
>>> __be32 *p, struct nlm_args *argp)
>>> int
>>> nlmsvc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct  
>>> nlm_reboot *argp)
>>> {
>>> +	struct sockaddr_in *sin = (struct sockaddr_in *)&argp->addr;
>>> +	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&argp->addr;
>>> +
>>> 	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++;
>>> +
>>> +	/* Decode the 16-byte private field */
>>> +	memset(&argp->addr, 0, sizeof(argp->addr));
>>> +	switch (svc_addr(rqstp)->sa_family) {
>>> +	case AF_INET:
>>> +		/* data in recv buffer is already in network byte order */
>>> +		sin->sin_family = AF_INET;
>>> +		sin->sin_addr.s_addr = *p++;
>>> +		argp->addrlen = sizeof(*sin);
>>> +		break;
>>> +	case AF_INET6:
>>> +		sin6->sin6_family = AF_INET6;
>>> +		sin6->sin6_addr.s6_addr32[0] = *p++;
>>> +		sin6->sin6_addr.s6_addr32[1] = *p++;
>>> +		sin6->sin6_addr.s6_addr32[2] = *p++;
>>> +		sin6->sin6_addr.s6_addr32[3] = *p++;
>>> +		argp->addrlen = sizeof(*sin6);
>>> +		break;
>>> +	default:
>>> +		return -EIO;
>>> +	}
>>> +
>>> 	return xdr_argsize_check(rqstp, p);
>>> }
>>>
>>> diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
>>> index 50c493a..2009020 100644
>>> --- a/fs/lockd/xdr4.c
>>> +++ b/fs/lockd/xdr4.c
>>> @@ -353,11 +353,34 @@ nlm4svc_decode_notify(struct svc_rqst *rqstp,  
>>> __be32 *p, struct nlm_args *argp)
>>> int
>>> nlm4svc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct  
>>> nlm_reboot *argp)
>>> {
>>> +	struct sockaddr_in *sin = (struct sockaddr_in *)&argp->addr;
>>> +	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&argp->addr;
>>> +
>>> 	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++;
>>> +
>>> +	/* Decode the 16-byte private field */
>>> +	memset(&argp->addr, 0, sizeof(argp->addr));
>>> +	switch (svc_addr(rqstp)->sa_family) {
>>> +	case AF_INET:
>>> +		/* address in recv buffer is already in network byte order */
>>> +		sin->sin_family = AF_INET;
>>> +		sin->sin_addr.s_addr = *p++;
>>> +		argp->addrlen = sizeof(*sin);
>>> +		break;
>>> +	case AF_INET6:
>>> +		sin6->sin6_family = AF_INET6;
>>> +		sin6->sin6_addr.s6_addr32[0] = *p++;
>>> +		sin6->sin6_addr.s6_addr32[1] = *p++;
>>> +		sin6->sin6_addr.s6_addr32[2] = *p++;
>>> +		sin6->sin6_addr.s6_addr32[3] = *p++;
>>> +		argp->addrlen = sizeof(*sin6);
>>> +		break;
>>> +	default:
>>> +		return -EIO;
>>> +	}
>>> +
>>> 	return xdr_argsize_check(rqstp, p);
>>> }
>>>
>>> diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
>>> index d6b3a80..6057b7e 100644
>>> --- a/include/linux/lockd/xdr.h
>>> +++ b/include/linux/lockd/xdr.h
>>> @@ -80,7 +80,8 @@ struct nlm_reboot {
>>> 	char *		mon;
>>> 	unsigned int	len;
>>> 	u32		state;
>>> -	__be32		addr;
>>> +	struct sockaddr_storage	addr;
>>> +	size_t		addrlen;
>>> };
>>>
>>> /*
>>>
>> --
>> 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 08/10] lockd: struct nlm_reboot should contain a full socket address

[Chuck Lever]

On Oct 1, 2008, at Oct 1, 2008, 2:18 PM, J. Bruce Fields wrote:
> On Wed, Oct 01, 2008 at 12:17:02PM -0400, Chuck Lever wrote:
>> On Sep 26, 2008, at Sep 26, 2008, 7:09 PM, J. Bruce Fields wrote:
>>> On Wed, Sep 17, 2008 at 11:18:11AM -0500, Chuck Lever wrote:
>>>> The XDR decoders for the NSM NOTIFY procedure should construct a  
>>>> full
>>>> socket address and store it in the nlm_reboot structure.  In  
>>>> addition
>>>> to being able to store larger addresses, this means upper layer
>>>> routines get an address family tag so they can distinguish between
>>>> AF_INET and AF_INET6 addresses.
>>>>
>>>> This also keeps potentially large socket addresses off the stack  
>>>> and
>>>> instead in dynamically allocated storage.
>>>
>>> So one way to think of this would be that you're extending the
>>> kernel<->statd interface by using the address family of statd's  
>>> notify
>>> call to communicate the address family of the host that rebooted.
>>>
>>> Do I have that right?
>>
>> For statd, we're using the same technique that we used when  
>> constructing
>> the source address in nlmsvc_lookup_host().  There's no family tag
>> associated with the address because the 16-byte opaque in the on- 
>> the-wire
>> format has room only for the sin6_addr part of the address.
>
> OK.  It seems a bit tricky, but I can't see why it doesn't work.

Right.  I couldn't think of something simpler.

rpc.statd has to continue to work with legacy kernels where the first  
4 bytes of the opaque are a 32-bit sin_addr field and the following 12  
bytes are zero.  Apparently you can have a valid IPv6 address that  
looks like that, but I can't find anything that states this  
conclusively.

The kernel needs to know the address family in order to interpret the  
contents of the opaque and turn them back into a sockaddr so it can be  
matched against an nlm_host.

>> The notification downcall for IPv6 peers has to come via IPv6  
>> loopback
>> for this to work.
>>
>>>
>>>
>>> --b.
>>>
>>>>
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>>
>>>> fs/lockd/svc4proc.c       |   10 +---------
>>>> fs/lockd/svcproc.c        |   10 +---------
>>>> fs/lockd/xdr.c            |   28 ++++++++++++++++++++++++++--
>>>> fs/lockd/xdr4.c           |   27 +++++++++++++++++++++++++--
>>>> include/linux/lockd/xdr.h |    3 ++-
>>>> 5 files changed, 55 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
>>>> index 6a5ef9f..e61c05f 100644
>>>> --- a/fs/lockd/svc4proc.c
>>>> +++ b/fs/lockd/svc4proc.c
>>>> @@ -430,8 +430,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)) {
>>>> @@ -441,13 +439,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((struct sockaddr *)&saddr, sizeof(saddr),
>>>> +	nlm_host_rebooted((struct sockaddr *)&argp->addr, argp->addrlen,
>>>> 			  argp->mon, argp->len, argp->state);
>>>>
>>>> 	return rpc_success;
>>>> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
>>>> index 62fcfdb..86a487a 100644
>>>> --- a/fs/lockd/svcproc.c
>>>> +++ b/fs/lockd/svcproc.c
>>>> @@ -462,8 +462,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)) {
>>>> @@ -473,13 +471,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((struct sockaddr *)&saddr, sizeof(saddr),
>>>> +	nlm_host_rebooted((struct sockaddr *)&argp->addr, argp->addrlen,
>>>> 			  argp->mon, argp->len, argp->state);
>>>>
>>>> 	return rpc_success;
>>>> diff --git a/fs/lockd/xdr.c b/fs/lockd/xdr.c
>>>> index 1f22629..92c5695 100644
>>>> --- a/fs/lockd/xdr.c
>>>> +++ b/fs/lockd/xdr.c
>>>> @@ -9,6 +9,7 @@
>>>> #include <linux/types.h>
>>>> #include <linux/sched.h>
>>>> #include <linux/utsname.h>
>>>> +#include <linux/in.h>
>>>> #include <linux/nfs.h>
>>>>
>>>> #include <linux/sunrpc/xdr.h>
>>>> @@ -346,11 +347,34 @@ nlmsvc_decode_notify(struct svc_rqst *rqstp,
>>>> __be32 *p, struct nlm_args *argp)
>>>> int
>>>> nlmsvc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct
>>>> nlm_reboot *argp)
>>>> {
>>>> +	struct sockaddr_in *sin = (struct sockaddr_in *)&argp->addr;
>>>> +	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&argp->addr;
>>>> +
>>>> 	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++;
>>>> +
>>>> +	/* Decode the 16-byte private field */
>>>> +	memset(&argp->addr, 0, sizeof(argp->addr));
>>>> +	switch (svc_addr(rqstp)->sa_family) {
>>>> +	case AF_INET:
>>>> +		/* data in recv buffer is already in network byte order */
>>>> +		sin->sin_family = AF_INET;
>>>> +		sin->sin_addr.s_addr = *p++;
>>>> +		argp->addrlen = sizeof(*sin);
>>>> +		break;
>>>> +	case AF_INET6:
>>>> +		sin6->sin6_family = AF_INET6;
>>>> +		sin6->sin6_addr.s6_addr32[0] = *p++;
>>>> +		sin6->sin6_addr.s6_addr32[1] = *p++;
>>>> +		sin6->sin6_addr.s6_addr32[2] = *p++;
>>>> +		sin6->sin6_addr.s6_addr32[3] = *p++;
>>>> +		argp->addrlen = sizeof(*sin6);
>>>> +		break;
>>>> +	default:
>>>> +		return -EIO;
>>>> +	}
>>>> +
>>>> 	return xdr_argsize_check(rqstp, p);
>>>> }
>>>>
>>>> diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
>>>> index 50c493a..2009020 100644
>>>> --- a/fs/lockd/xdr4.c
>>>> +++ b/fs/lockd/xdr4.c
>>>> @@ -353,11 +353,34 @@ nlm4svc_decode_notify(struct svc_rqst *rqstp,
>>>> __be32 *p, struct nlm_args *argp)
>>>> int
>>>> nlm4svc_decode_reboot(struct svc_rqst *rqstp, __be32 *p, struct
>>>> nlm_reboot *argp)
>>>> {
>>>> +	struct sockaddr_in *sin = (struct sockaddr_in *)&argp->addr;
>>>> +	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&argp->addr;
>>>> +
>>>> 	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++;
>>>> +
>>>> +	/* Decode the 16-byte private field */
>>>> +	memset(&argp->addr, 0, sizeof(argp->addr));
>>>> +	switch (svc_addr(rqstp)->sa_family) {
>>>> +	case AF_INET:
>>>> +		/* address in recv buffer is already in network byte order */
>>>> +		sin->sin_family = AF_INET;
>>>> +		sin->sin_addr.s_addr = *p++;
>>>> +		argp->addrlen = sizeof(*sin);
>>>> +		break;
>>>> +	case AF_INET6:
>>>> +		sin6->sin6_family = AF_INET6;
>>>> +		sin6->sin6_addr.s6_addr32[0] = *p++;
>>>> +		sin6->sin6_addr.s6_addr32[1] = *p++;
>>>> +		sin6->sin6_addr.s6_addr32[2] = *p++;
>>>> +		sin6->sin6_addr.s6_addr32[3] = *p++;
>>>> +		argp->addrlen = sizeof(*sin6);
>>>> +		break;
>>>> +	default:
>>>> +		return -EIO;
>>>> +	}
>>>> +
>>>> 	return xdr_argsize_check(rqstp, p);
>>>> }
>>>>
>>>> diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
>>>> index d6b3a80..6057b7e 100644
>>>> --- a/include/linux/lockd/xdr.h
>>>> +++ b/include/linux/lockd/xdr.h
>>>> @@ -80,7 +80,8 @@ struct nlm_reboot {
>>>> 	char *		mon;
>>>> 	unsigned int	len;
>>>> 	u32		state;
>>>> -	__be32		addr;
>>>> +	struct sockaddr_storage	addr;
>>>> +	size_t		addrlen;
>>>> };
>>>>
>>>> /*
>>>>
>>> --
>>> 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
>>
>>
>>
>>

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

Re: [PATCH 08/10] lockd: struct nlm_reboot should contain a full socket address

[J. Bruce Fields]

On Wed, Oct 01, 2008 at 03:40:05PM -0400, Chuck Lever wrote:
> On Oct 1, 2008, at Oct 1, 2008, 2:18 PM, J. Bruce Fields wrote:
>> On Wed, Oct 01, 2008 at 12:17:02PM -0400, Chuck Lever wrote:
>>> On Sep 26, 2008, at Sep 26, 2008, 7:09 PM, J. Bruce Fields wrote:
>>>> On Wed, Sep 17, 2008 at 11:18:11AM -0500, Chuck Lever wrote:
>>>>> The XDR decoders for the NSM NOTIFY procedure should construct a  
>>>>> full
>>>>> socket address and store it in the nlm_reboot structure.  In  
>>>>> addition
>>>>> to being able to store larger addresses, this means upper layer
>>>>> routines get an address family tag so they can distinguish between
>>>>> AF_INET and AF_INET6 addresses.
>>>>>
>>>>> This also keeps potentially large socket addresses off the stack  
>>>>> and
>>>>> instead in dynamically allocated storage.
>>>>
>>>> So one way to think of this would be that you're extending the
>>>> kernel<->statd interface by using the address family of statd's  
>>>> notify
>>>> call to communicate the address family of the host that rebooted.
>>>>
>>>> Do I have that right?
>>>
>>> For statd, we're using the same technique that we used when  
>>> constructing
>>> the source address in nlmsvc_lookup_host().  There's no family tag
>>> associated with the address because the 16-byte opaque in the on- 
>>> the-wire
>>> format has room only for the sin6_addr part of the address.
>>
>> OK.  It seems a bit tricky, but I can't see why it doesn't work.
>
> Right.  I couldn't think of something simpler.
>
> rpc.statd has to continue to work with legacy kernels where the first 4 
> bytes of the opaque are a 32-bit sin_addr field and the following 12  
> bytes are zero.

Wait a minute, there's something not right there: rpc.statd shouldn't
interpret the contents of the opaque field at all, should it?

--b.

> Apparently you can have a valid IPv6 address that looks 
> like that, but I can't find anything that states this conclusively.

>
> The kernel needs to know the address family in order to interpret the  
> contents of the opaque and turn them back into a sockaddr so it can be  
> matched against an nlm_host.

Re: [PATCH 08/10] lockd: struct nlm_reboot should contain a full socket address

[J. Bruce Fields]

On Wed, Oct 01, 2008 at 04:08:39PM -0400, bfields wrote:
> On Wed, Oct 01, 2008 at 03:40:05PM -0400, Chuck Lever wrote:
> > On Oct 1, 2008, at Oct 1, 2008, 2:18 PM, J. Bruce Fields wrote:
> >> On Wed, Oct 01, 2008 at 12:17:02PM -0400, Chuck Lever wrote:
> >>> On Sep 26, 2008, at Sep 26, 2008, 7:09 PM, J. Bruce Fields wrote:
> >>>> On Wed, Sep 17, 2008 at 11:18:11AM -0500, Chuck Lever wrote:
> >>>>> The XDR decoders for the NSM NOTIFY procedure should construct a  
> >>>>> full
> >>>>> socket address and store it in the nlm_reboot structure.  In  
> >>>>> addition
> >>>>> to being able to store larger addresses, this means upper layer
> >>>>> routines get an address family tag so they can distinguish between
> >>>>> AF_INET and AF_INET6 addresses.
> >>>>>
> >>>>> This also keeps potentially large socket addresses off the stack  
> >>>>> and
> >>>>> instead in dynamically allocated storage.
> >>>>
> >>>> So one way to think of this would be that you're extending the
> >>>> kernel<->statd interface by using the address family of statd's  
> >>>> notify
> >>>> call to communicate the address family of the host that rebooted.
> >>>>
> >>>> Do I have that right?
> >>>
> >>> For statd, we're using the same technique that we used when  
> >>> constructing
> >>> the source address in nlmsvc_lookup_host().  There's no family tag
> >>> associated with the address because the 16-byte opaque in the on- 
> >>> the-wire
> >>> format has room only for the sin6_addr part of the address.
> >>
> >> OK.  It seems a bit tricky, but I can't see why it doesn't work.
> >
> > Right.  I couldn't think of something simpler.
> >
> > rpc.statd has to continue to work with legacy kernels where the first 4 
> > bytes of the opaque are a 32-bit sin_addr field and the following 12  
> > bytes are zero.
> 
> Wait a minute, there's something not right there: rpc.statd shouldn't
> interpret the contents of the opaque field at all, should it?

And from a quick look at nfs-utils/statd/ it certainly looks to me like
it's correctly treating the contents as a totally opaque object.

So I think we can put whatever we want in the priv field--an ipv6
address, an index into some kernel table, whatever.  (The priv field
shouldn't even have to make sense to a future boot instance--we don't
need to be notified of previously monitored hosts' reboots any more once
we've rebooted ourselves.)

--b.

Re: [PATCH 08/10] lockd: struct nlm_reboot should contain a full socket address

[Chuck Lever]

On Oct 1, 2008, at Oct 1, 2008, 4:33 PM, J. Bruce Fields wrote:
> On Wed, Oct 01, 2008 at 04:08:39PM -0400, bfields wrote:
>> On Wed, Oct 01, 2008 at 03:40:05PM -0400, Chuck Lever wrote:
>>> On Oct 1, 2008, at Oct 1, 2008, 2:18 PM, J. Bruce Fields wrote:
>>>> On Wed, Oct 01, 2008 at 12:17:02PM -0400, Chuck Lever wrote:
>>>>> On Sep 26, 2008, at Sep 26, 2008, 7:09 PM, J. Bruce Fields wrote:
>>>>>> On Wed, Sep 17, 2008 at 11:18:11AM -0500, Chuck Lever wrote:
>>>>>>> The XDR decoders for the NSM NOTIFY procedure should construct a
>>>>>>> full
>>>>>>> socket address and store it in the nlm_reboot structure.  In
>>>>>>> addition
>>>>>>> to being able to store larger addresses, this means upper layer
>>>>>>> routines get an address family tag so they can distinguish  
>>>>>>> between
>>>>>>> AF_INET and AF_INET6 addresses.
>>>>>>>
>>>>>>> This also keeps potentially large socket addresses off the stack
>>>>>>> and
>>>>>>> instead in dynamically allocated storage.
>>>>>>
>>>>>> So one way to think of this would be that you're extending the
>>>>>> kernel<->statd interface by using the address family of statd's
>>>>>> notify
>>>>>> call to communicate the address family of the host that rebooted.
>>>>>>
>>>>>> Do I have that right?
>>>>>
>>>>> For statd, we're using the same technique that we used when
>>>>> constructing
>>>>> the source address in nlmsvc_lookup_host().  There's no family tag
>>>>> associated with the address because the 16-byte opaque in the on-
>>>>> the-wire
>>>>> format has room only for the sin6_addr part of the address.
>>>>
>>>> OK.  It seems a bit tricky, but I can't see why it doesn't work.
>>>
>>> Right.  I couldn't think of something simpler.
>>>
>>> rpc.statd has to continue to work with legacy kernels where the  
>>> first 4
>>> bytes of the opaque are a 32-bit sin_addr field and the following 12
>>> bytes are zero.
>>
>> Wait a minute, there's something not right there: rpc.statd shouldn't
>> interpret the contents of the opaque field at all, should it?
>
> And from a quick look at nfs-utils/statd/ it certainly looks to me  
> like
> it's correctly treating the contents as a totally opaque object.

That's exactly what happens.  The cookie field is generated by the  
SM_MON upcall, and passed back by the SM_NOTIFY downcall.

> So I think we can put whatever we want in the priv field--an ipv6
> address, an index into some kernel table, whatever.  (The priv field
> shouldn't even have to make sense to a future boot instance--we don't
> need to be notified of previously monitored hosts' reboots any more  
> once
> we've rebooted ourselves.)

Apparently, then, rpc.statd doesn't have enough knowledge to make the  
downcall via IPv6 when needed.  Everything we need to look up the host  
must be encoded into the 16-byte opaque field.

I'll look into it.

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

Re: [PATCH 08/10] lockd: struct nlm_reboot should contain a full socket address

[J. Bruce Fields]

On Wed, Oct 01, 2008 at 04:48:41PM -0400, Chuck Lever wrote:
> On Oct 1, 2008, at Oct 1, 2008, 4:33 PM, J. Bruce Fields wrote:
>> And from a quick look at nfs-utils/statd/ it certainly looks to me  
>> like
>> it's correctly treating the contents as a totally opaque object.
>
> That's exactly what happens.  The cookie field is generated by the  
> SM_MON upcall, and passed back by the SM_NOTIFY downcall.

OK, that's great.  So we can just

	- Choose whatever contents for the "priv"/cookie field we want
	  to keep the lookup on receipt of SM_NOTIFY easy, and
	- remove the support for ipv6 on the loopback communication
	  with statd; we don't need it.

>> So I think we can put whatever we want in the priv field--an ipv6
>> address, an index into some kernel table, whatever.  (The priv field
>> shouldn't even have to make sense to a future boot instance--we don't
>> need to be notified of previously monitored hosts' reboots any more  
>> once
>> we've rebooted ourselves.)
>
> Apparently, then, rpc.statd doesn't have enough knowledge to make the  
> downcall via IPv6 when needed.  Everything we need to look up the host  
> must be encoded into the 16-byte opaque field.
>
> I'll look into it.

Thanks!

--b.

Re: [PATCH 08/10] lockd: struct nlm_reboot should contain a full socket address

[Chuck Lever]

On Oct 1, 2008, at Oct 1, 2008, 4:55 PM, J. Bruce Fields wrote:

> On Wed, Oct 01, 2008 at 04:48:41PM -0400, Chuck Lever wrote:
>> On Oct 1, 2008, at Oct 1, 2008, 4:33 PM, J. Bruce Fields wrote:
>>> And from a quick look at nfs-utils/statd/ it certainly looks to me
>>> like
>>> it's correctly treating the contents as a totally opaque object.
>>
>> That's exactly what happens.  The cookie field is generated by the
>> SM_MON upcall, and passed back by the SM_NOTIFY downcall.
>
> OK, that's great.  So we can just
>
> 	- Choose whatever contents for the "priv"/cookie field we want
> 	  to keep the lookup on receipt of SM_NOTIFY easy, and
> 	- remove the support for ipv6 on the loopback communication
> 	  with statd; we don't need it.

I agree that since lockd generates the cookies, we can be smarter  
about how lockd deals with cookies coming back from statd, and I  
strongly prefer an approach that treats the opaques as arbitrary  
rather than interpreted values.

I think we may need to keep some IPv6 support in lockd for loopback  
down calls.  On an AF_INET6 listener socket, loopback calls will come  
from the IPv6 loopback address, AFAICT, even if they are sent from  
user space via the IPv4 loopback address.  The kernel's network layer  
will map the IPv4 loopback address to the IPv6 loopback address  
automatically.

I am working on revising last week's patch set based on your  
comments.  I plan to repost an updated version of these soon, but I  
can leave out the last two or three patches that deal solely with the  
mechanics of these NSM-related cookies.

>>> So I think we can put whatever we want in the priv field--an ipv6
>>> address, an index into some kernel table, whatever.  (The priv field
>>> shouldn't even have to make sense to a future boot instance--we  
>>> don't
>>> need to be notified of previously monitored hosts' reboots any more
>>> once
>>> we've rebooted ourselves.)
>>
>> Apparently, then, rpc.statd doesn't have enough knowledge to make the
>> downcall via IPv6 when needed.  Everything we need to look up the  
>> host
>> must be encoded into the 16-byte opaque field.
>>
>> I'll look into it.
>
> Thanks!
>
> --b.

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

Re: [PATCH 08/10] lockd: struct nlm_reboot should contain a full socket address

[J. Bruce Fields]

On Wed, Oct 01, 2008 at 05:16:45PM -0400, Chuck Lever wrote:
>
> On Oct 1, 2008, at Oct 1, 2008, 4:55 PM, J. Bruce Fields wrote:
>
>> On Wed, Oct 01, 2008 at 04:48:41PM -0400, Chuck Lever wrote:
>>> On Oct 1, 2008, at Oct 1, 2008, 4:33 PM, J. Bruce Fields wrote:
>>>> And from a quick look at nfs-utils/statd/ it certainly looks to me
>>>> like
>>>> it's correctly treating the contents as a totally opaque object.
>>>
>>> That's exactly what happens.  The cookie field is generated by the
>>> SM_MON upcall, and passed back by the SM_NOTIFY downcall.
>>
>> OK, that's great.  So we can just
>>
>> 	- Choose whatever contents for the "priv"/cookie field we want
>> 	  to keep the lookup on receipt of SM_NOTIFY easy, and
>> 	- remove the support for ipv6 on the loopback communication
>> 	  with statd; we don't need it.
>
> I agree that since lockd generates the cookies, we can be smarter about 
> how lockd deals with cookies coming back from statd, and I strongly 
> prefer an approach that treats the opaques as arbitrary rather than 
> interpreted values.
>
> I think we may need to keep some IPv6 support in lockd for loopback down 
> calls.  On an AF_INET6 listener socket, loopback calls will come from the 
> IPv6 loopback address, AFAICT, even if they are sent from user space via 
> the IPv4 loopback address.  The kernel's network layer will map the IPv4 
> loopback address to the IPv6 loopback address automatically.

OK.  (Well, I guess we could do whatever we'd like here, including using
a separate socket and even program for the notification downcall, but
doing what you propose is probably simplest.)

> I am working on revising last week's patch set based on your comments.  I 
> plan to repost an updated version of these soon, but I can leave out the 
> last two or three patches that deal solely with the mechanics of these 
> NSM-related cookies.

Sounds fine, thanks!

--b.

Re: [PATCH 08/10] lockd: struct nlm_reboot should contain a full socket address

[Chuck Lever]

On Oct 1, 2008, at Oct 1, 2008, 4:08 PM, J. Bruce Fields wrote:
> On Wed, Oct 01, 2008 at 03:40:05PM -0400, Chuck Lever wrote:
>> On Oct 1, 2008, at Oct 1, 2008, 2:18 PM, J. Bruce Fields wrote:
>>> On Wed, Oct 01, 2008 at 12:17:02PM -0400, Chuck Lever wrote:
>>>> On Sep 26, 2008, at Sep 26, 2008, 7:09 PM, J. Bruce Fields wrote:
>>>>> On Wed, Sep 17, 2008 at 11:18:11AM -0500, Chuck Lever wrote:
>>>>>> The XDR decoders for the NSM NOTIFY procedure should construct a
>>>>>> full
>>>>>> socket address and store it in the nlm_reboot structure.  In
>>>>>> addition
>>>>>> to being able to store larger addresses, this means upper layer
>>>>>> routines get an address family tag so they can distinguish  
>>>>>> between
>>>>>> AF_INET and AF_INET6 addresses.
>>>>>>
>>>>>> This also keeps potentially large socket addresses off the stack
>>>>>> and
>>>>>> instead in dynamically allocated storage.
>>>>>
>>>>> So one way to think of this would be that you're extending the
>>>>> kernel<->statd interface by using the address family of statd's
>>>>> notify
>>>>> call to communicate the address family of the host that rebooted.
>>>>>
>>>>> Do I have that right?
>>>>
>>>> For statd, we're using the same technique that we used when
>>>> constructing
>>>> the source address in nlmsvc_lookup_host().  There's no family tag
>>>> associated with the address because the 16-byte opaque in the on-
>>>> the-wire
>>>> format has room only for the sin6_addr part of the address.
>>>
>>> OK.  It seems a bit tricky, but I can't see why it doesn't work.
>>
>> Right.  I couldn't think of something simpler.
>>
>> rpc.statd has to continue to work with legacy kernels where the  
>> first 4
>> bytes of the opaque are a 32-bit sin_addr field and the following 12
>> bytes are zero.
>
> Wait a minute, there's something not right there: rpc.statd shouldn't
> interpret the contents of the opaque field at all, should it?

Yes, our rpc.statd and lockd should agree on the contents and  
interpretation of the opaque field.  The field is opaque because it's  
internal content is not defined by a standard.  It's a cookie, just  
like a file handle.

I would certainly *prefer* the use of an entirely arbitrary cookie,  
but that's not the way Linux happens to work today.

>> Apparently you can have a valid IPv6 address that looks
>> like that, but I can't find anything that states this conclusively.
>>
>> The kernel needs to know the address family in order to interpret the
>> contents of the opaque and turn them back into a sockaddr so it can  
>> be
>> matched against an nlm_host.

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

Re: [PATCH 08/10] lockd: struct nlm_reboot should contain a full socket address

[J. Bruce Fields]

On Wed, Oct 01, 2008 at 04:42:51PM -0400, Chuck Lever wrote:
> On Oct 1, 2008, at Oct 1, 2008, 4:08 PM, J. Bruce Fields wrote:
>> On Wed, Oct 01, 2008 at 03:40:05PM -0400, Chuck Lever wrote:
>>> Right.  I couldn't think of something simpler.
>>>
>>> rpc.statd has to continue to work with legacy kernels where the  
>>> first 4
>>> bytes of the opaque are a 32-bit sin_addr field and the following 12
>>> bytes are zero.
>>
>> Wait a minute, there's something not right there: rpc.statd shouldn't
>> interpret the contents of the opaque field at all, should it?
>
> Yes, our rpc.statd and lockd should agree on the contents and  
> interpretation of the opaque field.  The field is opaque because it's  
> internal content is not defined by a standard.  It's a cookie, just like 
> a file handle.
>
> I would certainly *prefer* the use of an entirely arbitrary cookie, but 
> that's not the way Linux happens to work today.

I'm pretty certain it does--at least on the rpc.statd size.  (Any
evidence to the contrary.)  Sure, it means something to the kernel, but
that's just for the kernel's convenience.  We can do whatever we want in
the kernel.

--b.

Re: [PATCH 08/10] lockd: struct nlm_reboot should contain a full socket address

[J. Bruce Fields]

On Wed, Oct 01, 2008 at 04:51:56PM -0400, bfields wrote:
> On Wed, Oct 01, 2008 at 04:42:51PM -0400, Chuck Lever wrote:
> > On Oct 1, 2008, at Oct 1, 2008, 4:08 PM, J. Bruce Fields wrote:
> >> On Wed, Oct 01, 2008 at 03:40:05PM -0400, Chuck Lever wrote:
> >>> Right.  I couldn't think of something simpler.
> >>>
> >>> rpc.statd has to continue to work with legacy kernels where the  
> >>> first 4
> >>> bytes of the opaque are a 32-bit sin_addr field and the following 12
> >>> bytes are zero.
> >>
> >> Wait a minute, there's something not right there: rpc.statd shouldn't
> >> interpret the contents of the opaque field at all, should it?
> >
> > Yes, our rpc.statd and lockd should agree on the contents and  
> > interpretation of the opaque field.  The field is opaque because it's  
> > internal content is not defined by a standard.  It's a cookie, just like 
> > a file handle.
> >
> > I would certainly *prefer* the use of an entirely arbitrary cookie, but 
> > that's not the way Linux happens to work today.
> 
> I'm pretty certain it does--at least on the rpc.statd size.  (Any
> evidence to the contrary.)  Sure, it means something to the kernel, but

(Err, I think that parenthetical remark was meant to be a question.
Apologies.)

> that's just for the kernel's convenience.  We can do whatever we want in
> the kernel.
> 
> --b.

[PATCH 09/10] lockd: IPv6 support for SM_MON / SM_UNMON

[Chuck Lever]

Widen the field that stores addresses in the nsm_args structure to handle
non-AF_INET addresses, and adjust NSM XDR routines to handle AF_INET6
family addresses.

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

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

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 4e7e958..d49dfe6 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -18,7 +18,7 @@
 
 #define NLMDBG_FACILITY		NLMDBG_MONITOR
 
-#define XDR_ADDRBUF_LEN		(20)
+#define XDR_ADDRBUF_LEN		(48)
 
 static struct rpc_clnt *	nsm_create(void);
 
@@ -37,7 +37,12 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
 {
 	struct rpc_clnt	*clnt;
 	int		status;
-	struct nsm_args	args;
+	struct nsm_args	args = {
+		.prog		= NLM_PROGRAM,
+		.vers		= 3,
+		.proc		= NLMPROC_NSM_NOTIFY,
+		.mon_name	= nsm->sm_name,
+	};
 	struct rpc_message msg = {
 		.rpc_argp	= &args,
 		.rpc_resp	= res,
@@ -46,21 +51,18 @@ nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res)
 	clnt = nsm_create();
 	if (IS_ERR(clnt)) {
 		status = PTR_ERR(clnt);
+		printk(KERN_NOTICE "nsm_mon_unmon: failed to create transport, "
+			"status=%d\n", status);
 		goto out;
 	}
 
-	memset(&args, 0, sizeof(args));
-	args.mon_name = nsm->sm_name;
-	args.addr = nsm_addr_in(nsm)->sin_addr.s_addr;
-	args.prog = NLM_PROGRAM;
-	args.vers = 3;
-	args.proc = NLMPROC_NSM_NOTIFY;
+	memcpy(&args.addr, nsm_addr(nsm), nsm->sm_addrlen);
 	memset(res, 0, sizeof(*res));
 
 	msg.rpc_proc = &clnt->cl_procinfo[proc];
 	status = rpc_call_sync(clnt, &msg, 0);
 	if (status < 0)
-		printk(KERN_DEBUG "nsm_mon_unmon: rpc failed, status=%d\n",
+		printk(KERN_NOTICE "nsm_mon_unmon: rpc failed, status=%d\n",
 			status);
 	else
 		status = 0;
@@ -174,14 +176,30 @@ static __be32 *xdr_encode_nsm_string(__be32 *p, char *string)
  */
 static __be32 *xdr_encode_mon_name(__be32 *p, struct nsm_args *argp)
 {
-	char	buffer[XDR_ADDRBUF_LEN + 1];
-	char	*name = argp->mon_name;
+	char	*name, buffer[XDR_ADDRBUF_LEN + 1];
 
 	if (!nsm_use_hostnames) {
-		snprintf(buffer, XDR_ADDRBUF_LEN,
-			 NIPQUAD_FMT, NIPQUAD(argp->addr));
+		const struct sockaddr_storage *ssp = &argp->addr;
+		const struct sockaddr_in *sin = (struct sockaddr_in *)ssp;
+		const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)ssp;
+
+		switch (ssp->ss_family) {
+		case AF_INET:
+			snprintf(buffer, XDR_ADDRBUF_LEN, NIPQUAD_FMT,
+						NIPQUAD(sin->sin_addr.s_addr));
+			break;
+		case AF_INET6:
+			snprintf(buffer, XDR_ADDRBUF_LEN, NIP6_FMT,
+						NIP6(sin6->sin6_addr));
+			break;
+		default:
+			printk(KERN_NOTICE "%s: unsupported address family "
+				"(%u)\n", __func__, ssp->ss_family);
+			return ERR_PTR(-EIO);
+		}
 		name = buffer;
-	}
+	} else
+		name = argp->mon_name;
 
 	return xdr_encode_nsm_string(p, name);
 }
@@ -225,13 +243,35 @@ 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.
+ *
+ * We should send only AF_INET6 addresses in this field if the
+ * underlying transport is IPv6, and only AF_INET addresses if
+ * the underlying transport is IPv4.
  */
 static __be32 *xdr_encode_priv(__be32 *p, struct nsm_args *argp)
 {
-	*p++ = argp->addr;
-	*p++ = 0;
-	*p++ = 0;
-	*p++ = 0;
+	const struct sockaddr_storage *ssp = &argp->addr;
+	const struct sockaddr_in *sin = (struct sockaddr_in *)ssp;
+	const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)ssp;
+
+	switch (ssp->ss_family) {
+	case AF_INET:
+		*p++ = sin->sin_addr.s_addr;
+		*p++ = 0;
+		*p++ = 0;
+		*p++ = 0;
+		break;
+	case AF_INET6:
+		*p++ = sin6->sin6_addr.s6_addr32[0];
+		*p++ = sin6->sin6_addr.s6_addr32[1];
+		*p++ = sin6->sin6_addr.s6_addr32[2];
+		*p++ = sin6->sin6_addr.s6_addr32[3];
+		break;
+	default:
+		printk(KERN_NOTICE "%s: unsupported address family (%u)\n",
+			__func__, ssp->ss_family);
+		return ERR_PTR(-EIO);
+	}
 
 	return p;
 }
diff --git a/include/linux/lockd/sm_inter.h b/include/linux/lockd/sm_inter.h
index 5a5448b..0b0a0ae 100644
--- a/include/linux/lockd/sm_inter.h
+++ b/include/linux/lockd/sm_inter.h
@@ -25,7 +25,7 @@
  * Arguments for all calls to statd
  */
 struct nsm_args {
-	__be32		addr;		/* remote address */
+	struct sockaddr_storage addr;	/* remote address */
 	u32		prog;		/* RPC callback info */
 	u32		vers;
 	u32		proc;

[PATCH 10/10] lockd: Use "unsigned short" for lockd_up() "proto" argument

[Chuck Lever]

Clean up.  Use conventional data type for transport argument to the
lockd_up() function.

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

 fs/lockd/svc.c             |    5 ++---
 include/linux/lockd/bind.h |    2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 1553fec..311cf5e 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -207,7 +207,7 @@ lockd(void *vrqstp)
  * If nlm_udpport or nlm_tcpport were set as module
  * options, make those sockets unconditionally
  */
-static int make_socks(struct svc_serv *serv, int proto)
+static int make_socks(struct svc_serv *serv, const unsigned short proto)
 {
 	static int warned;
 	struct svc_xprt *xprt;
@@ -241,8 +241,7 @@ static int make_socks(struct svc_serv *serv, int proto)
 /*
  * Bring up the lockd process if it's not already up.
  */
-int
-lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
+int lockd_up(const unsigned short proto)
 {
 	struct svc_serv *serv;
 	int		error = 0;
diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
index 3d25bcd..8aeba6f 100644
--- a/include/linux/lockd/bind.h
+++ b/include/linux/lockd/bind.h
@@ -53,7 +53,7 @@ extern void	nlmclnt_done(struct nlm_host *host);
 
 extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
 					struct file_lock *fl);
-extern int	lockd_up(int proto);
+extern int	lockd_up(const unsigned short proto);
 extern void	lockd_down(void);
 
 unsigned long get_nfs_grace_period(void);

Re: [PATCH 00/10] Next series of IPv6 patches for lockd

[J. Bruce Fields]

On Wed, Sep 17, 2008 at 11:17:13AM -0500, Chuck Lever wrote:
> Hi Bruce-
> 
> Here are ten more IPv6 patches for lockd, as requested.  There are just
> a handful of remaining minor IPv6 patches to finish enabling IPv6 in
> lockd.  These can likely wait until we have a complete IPv6 user space
> implementation.

Thanks, I've read through them all.

Could you just split up those one or two initial patches?

My remaining point of confusion is over just exactly how the interface
to statd is going to work.  I don't think about statd enough and may
just be terribly confused....

--b.

> 
> Please consider these for 2.6.28.
> 
> ---
> 
> Chuck Lever (10):
>       lockd: Use "unsigned short" for lockd_up() "proto" argument
>       lockd: IPv6 support for SM_MON / SM_UNMON
>       lockd: struct nlm_reboot should contain a full socket address
>       lockd: Remove unused fields in the nlm_reboot structure
>       lockd: Add helper to sanity check incoming NOTIFY requests
>       lockd: Adjust signature of nlm_host_rebooted to handle non-AF_INET
>       lockd: change nlmclnt_grant() to take a "struct sockaddr *"
>       lockd: Adjust nlmsvc_lookup_host() to accomodate AF_INET6 addresses
>       lockd: Adjust nlmclnt_lookup_host() signature to accomodate non-AF_INET
>       lockd: Support non-AF_INET addresses in nlm_lookup_host()
> 
> 
>  fs/lockd/clntlock.c            |   10 +-
>  fs/lockd/host.c                |  189 ++++++++++++++++++++++++++++------------
>  fs/lockd/mon.c                 |   76 ++++++++++++----
>  fs/lockd/svc.c                 |    5 -
>  fs/lockd/svc4proc.c            |   18 +---
>  fs/lockd/svcproc.c             |   18 +---
>  fs/lockd/xdr.c                 |   30 ++++++
>  fs/lockd/xdr4.c                |   29 +++++-
>  include/linux/lockd/bind.h     |    2 
>  include/linux/lockd/lockd.h    |   63 ++++++++++++-
>  include/linux/lockd/sm_inter.h |    2 
>  include/linux/lockd/xdr.h      |    5 -
>  12 files changed, 315 insertions(+), 132 deletions(-)
> 
> -- 
> Chuck Lever