linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] lockd: create NSM handles per net namespace
@ 2015-09-23 12:49 Andrey Ryabinin
  2015-09-29 18:47 ` J. Bruce Fields
  0 siblings, 1 reply; 5+ messages in thread
From: Andrey Ryabinin @ 2015-09-23 12:49 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker, J. Bruce Fields, Jeff Layton
  Cc: linux-nfs, linux-kernel, skinsbursky, Andrey Ryabinin, stable

Commit cb7323fffa85 ("lockd: create and use per-net NSM
 RPC clients on MON/UNMON requests") introduced per-net
NSM RPC clients. Unfortunately this doesn't make any sense
without per-net nsm_handle.

E.g. the following scenario could happen
Two hosts (X and Y) in different namespaces (A and B) share
the same nsm struct.

1. nsm_monitor(host_X) called => NSM rpc client created,
	nsm->sm_monitored bit set.
2. nsm_mointor(host-Y) called => nsm->sm_monitored already set,
	we just exit. Thus in namespace B ln->nsm_clnt == NULL.
3. host X destroyed => nsm->sm_count decremented to 1
4. host Y destroyed => nsm_unmonitor() => nsm_mon_unmon() => NULL-ptr
	dereference of *ln->nsm_clnt

So this could be fixed by making per-net nsm_handles list,
instead of global. Thus different net namespaces will not be able
share the same nsm_handle.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: <stable@vger.kernel.org>
---
 fs/lockd/host.c             |  7 ++++---
 fs/lockd/mon.c              | 36 ++++++++++++++++++++++--------------
 fs/lockd/netns.h            |  1 +
 fs/lockd/svc.c              |  1 +
 fs/lockd/svc4proc.c         |  2 +-
 fs/lockd/svcproc.c          |  2 +-
 include/linux/lockd/lockd.h |  9 ++++++---
 7 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index 969d589..b5f3c3a 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -116,7 +116,7 @@ static struct nlm_host *nlm_alloc_host(struct nlm_lookup_host_info *ni,
 		atomic_inc(&nsm->sm_count);
 	else {
 		host = NULL;
-		nsm = nsm_get_handle(ni->sap, ni->salen,
+		nsm = nsm_get_handle(ni->net, ni->sap, ni->salen,
 					ni->hostname, ni->hostname_len);
 		if (unlikely(nsm == NULL)) {
 			dprintk("lockd: %s failed; no nsm handle\n",
@@ -534,17 +534,18 @@ static struct nlm_host *next_host_state(struct hlist_head *cache,
 
 /**
  * nlm_host_rebooted - Release all resources held by rebooted host
+ * @net:  network namespace
  * @info: pointer to decoded results of NLM_SM_NOTIFY call
  *
  * We were notified that the specified host has rebooted.  Release
  * all resources held by that peer.
  */
-void nlm_host_rebooted(const struct nlm_reboot *info)
+void nlm_host_rebooted(const struct net *net, const struct nlm_reboot *info)
 {
 	struct nsm_handle *nsm;
 	struct nlm_host	*host;
 
-	nsm = nsm_reboot_lookup(info);
+	nsm = nsm_reboot_lookup(net, info);
 	if (unlikely(nsm == NULL))
 		return;
 
diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 47a32b6..6c05cd1 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -51,7 +51,6 @@ struct nsm_res {
 };
 
 static const struct rpc_program	nsm_program;
-static				LIST_HEAD(nsm_handles);
 static				DEFINE_SPINLOCK(nsm_lock);
 
 /*
@@ -264,33 +263,35 @@ void nsm_unmonitor(const struct nlm_host *host)
 	}
 }
 
-static struct nsm_handle *nsm_lookup_hostname(const char *hostname,
-					      const size_t len)
+static struct nsm_handle *nsm_lookup_hostname(const struct list_head *nsm_handles,
+					const char *hostname, const size_t len)
 {
 	struct nsm_handle *nsm;
 
-	list_for_each_entry(nsm, &nsm_handles, sm_link)
+	list_for_each_entry(nsm, nsm_handles, sm_link)
 		if (strlen(nsm->sm_name) == len &&
 		    memcmp(nsm->sm_name, hostname, len) == 0)
 			return nsm;
 	return NULL;
 }
 
-static struct nsm_handle *nsm_lookup_addr(const struct sockaddr *sap)
+static struct nsm_handle *nsm_lookup_addr(const struct list_head *nsm_handles,
+					const struct sockaddr *sap)
 {
 	struct nsm_handle *nsm;
 
-	list_for_each_entry(nsm, &nsm_handles, sm_link)
+	list_for_each_entry(nsm, nsm_handles, sm_link)
 		if (rpc_cmp_addr(nsm_addr(nsm), sap))
 			return nsm;
 	return NULL;
 }
 
-static struct nsm_handle *nsm_lookup_priv(const struct nsm_private *priv)
+static struct nsm_handle *nsm_lookup_priv(const struct list_head *nsm_handles,
+					const struct nsm_private *priv)
 {
 	struct nsm_handle *nsm;
 
-	list_for_each_entry(nsm, &nsm_handles, sm_link)
+	list_for_each_entry(nsm, nsm_handles, sm_link)
 		if (memcmp(nsm->sm_priv.data, priv->data,
 					sizeof(priv->data)) == 0)
 			return nsm;
@@ -353,6 +354,7 @@ static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
 
 /**
  * nsm_get_handle - Find or create a cached nsm_handle
+ * @net: network namespace
  * @sap: pointer to socket address of handle to find
  * @salen: length of socket address
  * @hostname: pointer to C string containing hostname to find
@@ -365,11 +367,13 @@ static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
  * @hostname cannot be found in the handle cache.  Returns NULL if
  * an error occurs.
  */
-struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
+struct nsm_handle *nsm_get_handle(const struct net *net,
+				  const struct sockaddr *sap,
 				  const size_t salen, const char *hostname,
 				  const size_t hostname_len)
 {
 	struct nsm_handle *cached, *new = NULL;
+	struct lockd_net *ln = net_generic(net, lockd_net_id);
 
 	if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
 		if (printk_ratelimit()) {
@@ -384,9 +388,10 @@ retry:
 	spin_lock(&nsm_lock);
 
 	if (nsm_use_hostnames && hostname != NULL)
-		cached = nsm_lookup_hostname(hostname, hostname_len);
+		cached = nsm_lookup_hostname(&ln->nsm_handles,
+					hostname, hostname_len);
 	else
-		cached = nsm_lookup_addr(sap);
+		cached = nsm_lookup_addr(&ln->nsm_handles, sap);
 
 	if (cached != NULL) {
 		atomic_inc(&cached->sm_count);
@@ -400,7 +405,7 @@ retry:
 	}
 
 	if (new != NULL) {
-		list_add(&new->sm_link, &nsm_handles);
+		list_add(&new->sm_link, &ln->nsm_handles);
 		spin_unlock(&nsm_lock);
 		dprintk("lockd: created nsm_handle for %s (%s)\n",
 				new->sm_name, new->sm_addrbuf);
@@ -417,19 +422,22 @@ retry:
 
 /**
  * nsm_reboot_lookup - match NLMPROC_SM_NOTIFY arguments to an nsm_handle
+ * @net:  network namespace
  * @info: pointer to NLMPROC_SM_NOTIFY arguments
  *
  * Returns a matching nsm_handle if found in the nsm cache. The returned
  * nsm_handle's reference count is bumped. Otherwise returns NULL if some
  * error occurred.
  */
-struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info)
+struct nsm_handle *nsm_reboot_lookup(const struct net *net,
+				const struct nlm_reboot *info)
 {
 	struct nsm_handle *cached;
+	struct lockd_net *ln = net_generic(net, lockd_net_id);
 
 	spin_lock(&nsm_lock);
 
-	cached = nsm_lookup_priv(&info->priv);
+	cached = nsm_lookup_priv(&ln->nsm_handles, &info->priv);
 	if (unlikely(cached == NULL)) {
 		spin_unlock(&nsm_lock);
 		dprintk("lockd: never saw rebooted peer '%.*s' before\n",
diff --git a/fs/lockd/netns.h b/fs/lockd/netns.h
index 097bfa3..89fe011 100644
--- a/fs/lockd/netns.h
+++ b/fs/lockd/netns.h
@@ -15,6 +15,7 @@ struct lockd_net {
 	spinlock_t nsm_clnt_lock;
 	unsigned int nsm_users;
 	struct rpc_clnt *nsm_clnt;
+	struct list_head nsm_handles;
 };
 
 extern int lockd_net_id;
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index d678bcc..0dff13f 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -593,6 +593,7 @@ static int lockd_init_net(struct net *net)
 	INIT_LIST_HEAD(&ln->lockd_manager.list);
 	ln->lockd_manager.block_opens = false;
 	spin_lock_init(&ln->nsm_clnt_lock);
+	INIT_LIST_HEAD(&ln->nsm_handles);
 	return 0;
 }
 
diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index b147d1a..09c576f 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -421,7 +421,7 @@ nlm4svc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
 		return rpc_system_err;
 	}
 
-	nlm_host_rebooted(argp);
+	nlm_host_rebooted(SVC_NET(rqstp), argp);
 	return rpc_success;
 }
 
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index 21171f0..fb26b9f 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -464,7 +464,7 @@ nlmsvc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
 		return rpc_system_err;
 	}
 
-	nlm_host_rebooted(argp);
+	nlm_host_rebooted(SVC_NET(rqstp), argp);
 	return rpc_success;
 }
 
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index ff82a32..fd3b65b 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -235,7 +235,8 @@ void		  nlm_rebind_host(struct nlm_host *);
 struct nlm_host * nlm_get_host(struct nlm_host *);
 void		  nlm_shutdown_hosts(void);
 void		  nlm_shutdown_hosts_net(struct net *net);
-void		  nlm_host_rebooted(const struct nlm_reboot *);
+void		  nlm_host_rebooted(const struct net *net,
+					const struct nlm_reboot *);
 
 /*
  * Host monitoring
@@ -243,11 +244,13 @@ void		  nlm_host_rebooted(const struct nlm_reboot *);
 int		  nsm_monitor(const struct nlm_host *host);
 void		  nsm_unmonitor(const struct nlm_host *host);
 
-struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
+struct nsm_handle *nsm_get_handle(const struct net *net,
+					const struct sockaddr *sap,
 					const size_t salen,
 					const char *hostname,
 					const size_t hostname_len);
-struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info);
+struct nsm_handle *nsm_reboot_lookup(const struct net *net,
+					const struct nlm_reboot *info);
 void		  nsm_release(struct nsm_handle *nsm);
 
 /*
-- 
2.4.9


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] lockd: create NSM handles per net namespace
  2015-09-23 12:49 [PATCH] lockd: create NSM handles per net namespace Andrey Ryabinin
@ 2015-09-29 18:47 ` J. Bruce Fields
  2015-10-01 16:36   ` Andrey Ryabinin
  0 siblings, 1 reply; 5+ messages in thread
From: J. Bruce Fields @ 2015-09-29 18:47 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Trond Myklebust, Anna Schumaker, Jeff Layton, linux-nfs,
	linux-kernel, skinsbursky, stable

On Wed, Sep 23, 2015 at 03:49:29PM +0300, Andrey Ryabinin wrote:
> Commit cb7323fffa85 ("lockd: create and use per-net NSM
>  RPC clients on MON/UNMON requests") introduced per-net
> NSM RPC clients. Unfortunately this doesn't make any sense
> without per-net nsm_handle.

Makes sense to me.  Is anyone doing testing to make sure we've got this
right now?

(For example, have you reproduced the below problem and verified that
it's fixed after this patch?)

--b.

> 
> E.g. the following scenario could happen
> Two hosts (X and Y) in different namespaces (A and B) share
> the same nsm struct.
> 
> 1. nsm_monitor(host_X) called => NSM rpc client created,
> 	nsm->sm_monitored bit set.
> 2. nsm_mointor(host-Y) called => nsm->sm_monitored already set,
> 	we just exit. Thus in namespace B ln->nsm_clnt == NULL.
> 3. host X destroyed => nsm->sm_count decremented to 1
> 4. host Y destroyed => nsm_unmonitor() => nsm_mon_unmon() => NULL-ptr
> 	dereference of *ln->nsm_clnt
> 
> So this could be fixed by making per-net nsm_handles list,
> instead of global. Thus different net namespaces will not be able
> share the same nsm_handle.
> 
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: <stable@vger.kernel.org>
> ---
>  fs/lockd/host.c             |  7 ++++---
>  fs/lockd/mon.c              | 36 ++++++++++++++++++++++--------------
>  fs/lockd/netns.h            |  1 +
>  fs/lockd/svc.c              |  1 +
>  fs/lockd/svc4proc.c         |  2 +-
>  fs/lockd/svcproc.c          |  2 +-
>  include/linux/lockd/lockd.h |  9 ++++++---
>  7 files changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index 969d589..b5f3c3a 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -116,7 +116,7 @@ static struct nlm_host *nlm_alloc_host(struct nlm_lookup_host_info *ni,
>  		atomic_inc(&nsm->sm_count);
>  	else {
>  		host = NULL;
> -		nsm = nsm_get_handle(ni->sap, ni->salen,
> +		nsm = nsm_get_handle(ni->net, ni->sap, ni->salen,
>  					ni->hostname, ni->hostname_len);
>  		if (unlikely(nsm == NULL)) {
>  			dprintk("lockd: %s failed; no nsm handle\n",
> @@ -534,17 +534,18 @@ static struct nlm_host *next_host_state(struct hlist_head *cache,
>  
>  /**
>   * nlm_host_rebooted - Release all resources held by rebooted host
> + * @net:  network namespace
>   * @info: pointer to decoded results of NLM_SM_NOTIFY call
>   *
>   * We were notified that the specified host has rebooted.  Release
>   * all resources held by that peer.
>   */
> -void nlm_host_rebooted(const struct nlm_reboot *info)
> +void nlm_host_rebooted(const struct net *net, const struct nlm_reboot *info)
>  {
>  	struct nsm_handle *nsm;
>  	struct nlm_host	*host;
>  
> -	nsm = nsm_reboot_lookup(info);
> +	nsm = nsm_reboot_lookup(net, info);
>  	if (unlikely(nsm == NULL))
>  		return;
>  
> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> index 47a32b6..6c05cd1 100644
> --- a/fs/lockd/mon.c
> +++ b/fs/lockd/mon.c
> @@ -51,7 +51,6 @@ struct nsm_res {
>  };
>  
>  static const struct rpc_program	nsm_program;
> -static				LIST_HEAD(nsm_handles);
>  static				DEFINE_SPINLOCK(nsm_lock);
>  
>  /*
> @@ -264,33 +263,35 @@ void nsm_unmonitor(const struct nlm_host *host)
>  	}
>  }
>  
> -static struct nsm_handle *nsm_lookup_hostname(const char *hostname,
> -					      const size_t len)
> +static struct nsm_handle *nsm_lookup_hostname(const struct list_head *nsm_handles,
> +					const char *hostname, const size_t len)
>  {
>  	struct nsm_handle *nsm;
>  
> -	list_for_each_entry(nsm, &nsm_handles, sm_link)
> +	list_for_each_entry(nsm, nsm_handles, sm_link)
>  		if (strlen(nsm->sm_name) == len &&
>  		    memcmp(nsm->sm_name, hostname, len) == 0)
>  			return nsm;
>  	return NULL;
>  }
>  
> -static struct nsm_handle *nsm_lookup_addr(const struct sockaddr *sap)
> +static struct nsm_handle *nsm_lookup_addr(const struct list_head *nsm_handles,
> +					const struct sockaddr *sap)
>  {
>  	struct nsm_handle *nsm;
>  
> -	list_for_each_entry(nsm, &nsm_handles, sm_link)
> +	list_for_each_entry(nsm, nsm_handles, sm_link)
>  		if (rpc_cmp_addr(nsm_addr(nsm), sap))
>  			return nsm;
>  	return NULL;
>  }
>  
> -static struct nsm_handle *nsm_lookup_priv(const struct nsm_private *priv)
> +static struct nsm_handle *nsm_lookup_priv(const struct list_head *nsm_handles,
> +					const struct nsm_private *priv)
>  {
>  	struct nsm_handle *nsm;
>  
> -	list_for_each_entry(nsm, &nsm_handles, sm_link)
> +	list_for_each_entry(nsm, nsm_handles, sm_link)
>  		if (memcmp(nsm->sm_priv.data, priv->data,
>  					sizeof(priv->data)) == 0)
>  			return nsm;
> @@ -353,6 +354,7 @@ static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
>  
>  /**
>   * nsm_get_handle - Find or create a cached nsm_handle
> + * @net: network namespace
>   * @sap: pointer to socket address of handle to find
>   * @salen: length of socket address
>   * @hostname: pointer to C string containing hostname to find
> @@ -365,11 +367,13 @@ static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
>   * @hostname cannot be found in the handle cache.  Returns NULL if
>   * an error occurs.
>   */
> -struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
> +struct nsm_handle *nsm_get_handle(const struct net *net,
> +				  const struct sockaddr *sap,
>  				  const size_t salen, const char *hostname,
>  				  const size_t hostname_len)
>  {
>  	struct nsm_handle *cached, *new = NULL;
> +	struct lockd_net *ln = net_generic(net, lockd_net_id);
>  
>  	if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
>  		if (printk_ratelimit()) {
> @@ -384,9 +388,10 @@ retry:
>  	spin_lock(&nsm_lock);
>  
>  	if (nsm_use_hostnames && hostname != NULL)
> -		cached = nsm_lookup_hostname(hostname, hostname_len);
> +		cached = nsm_lookup_hostname(&ln->nsm_handles,
> +					hostname, hostname_len);
>  	else
> -		cached = nsm_lookup_addr(sap);
> +		cached = nsm_lookup_addr(&ln->nsm_handles, sap);
>  
>  	if (cached != NULL) {
>  		atomic_inc(&cached->sm_count);
> @@ -400,7 +405,7 @@ retry:
>  	}
>  
>  	if (new != NULL) {
> -		list_add(&new->sm_link, &nsm_handles);
> +		list_add(&new->sm_link, &ln->nsm_handles);
>  		spin_unlock(&nsm_lock);
>  		dprintk("lockd: created nsm_handle for %s (%s)\n",
>  				new->sm_name, new->sm_addrbuf);
> @@ -417,19 +422,22 @@ retry:
>  
>  /**
>   * nsm_reboot_lookup - match NLMPROC_SM_NOTIFY arguments to an nsm_handle
> + * @net:  network namespace
>   * @info: pointer to NLMPROC_SM_NOTIFY arguments
>   *
>   * Returns a matching nsm_handle if found in the nsm cache. The returned
>   * nsm_handle's reference count is bumped. Otherwise returns NULL if some
>   * error occurred.
>   */
> -struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info)
> +struct nsm_handle *nsm_reboot_lookup(const struct net *net,
> +				const struct nlm_reboot *info)
>  {
>  	struct nsm_handle *cached;
> +	struct lockd_net *ln = net_generic(net, lockd_net_id);
>  
>  	spin_lock(&nsm_lock);
>  
> -	cached = nsm_lookup_priv(&info->priv);
> +	cached = nsm_lookup_priv(&ln->nsm_handles, &info->priv);
>  	if (unlikely(cached == NULL)) {
>  		spin_unlock(&nsm_lock);
>  		dprintk("lockd: never saw rebooted peer '%.*s' before\n",
> diff --git a/fs/lockd/netns.h b/fs/lockd/netns.h
> index 097bfa3..89fe011 100644
> --- a/fs/lockd/netns.h
> +++ b/fs/lockd/netns.h
> @@ -15,6 +15,7 @@ struct lockd_net {
>  	spinlock_t nsm_clnt_lock;
>  	unsigned int nsm_users;
>  	struct rpc_clnt *nsm_clnt;
> +	struct list_head nsm_handles;
>  };
>  
>  extern int lockd_net_id;
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index d678bcc..0dff13f 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -593,6 +593,7 @@ static int lockd_init_net(struct net *net)
>  	INIT_LIST_HEAD(&ln->lockd_manager.list);
>  	ln->lockd_manager.block_opens = false;
>  	spin_lock_init(&ln->nsm_clnt_lock);
> +	INIT_LIST_HEAD(&ln->nsm_handles);
>  	return 0;
>  }
>  
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index b147d1a..09c576f 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -421,7 +421,7 @@ nlm4svc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
>  		return rpc_system_err;
>  	}
>  
> -	nlm_host_rebooted(argp);
> +	nlm_host_rebooted(SVC_NET(rqstp), argp);
>  	return rpc_success;
>  }
>  
> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> index 21171f0..fb26b9f 100644
> --- a/fs/lockd/svcproc.c
> +++ b/fs/lockd/svcproc.c
> @@ -464,7 +464,7 @@ nlmsvc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
>  		return rpc_system_err;
>  	}
>  
> -	nlm_host_rebooted(argp);
> +	nlm_host_rebooted(SVC_NET(rqstp), argp);
>  	return rpc_success;
>  }
>  
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index ff82a32..fd3b65b 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -235,7 +235,8 @@ void		  nlm_rebind_host(struct nlm_host *);
>  struct nlm_host * nlm_get_host(struct nlm_host *);
>  void		  nlm_shutdown_hosts(void);
>  void		  nlm_shutdown_hosts_net(struct net *net);
> -void		  nlm_host_rebooted(const struct nlm_reboot *);
> +void		  nlm_host_rebooted(const struct net *net,
> +					const struct nlm_reboot *);
>  
>  /*
>   * Host monitoring
> @@ -243,11 +244,13 @@ void		  nlm_host_rebooted(const struct nlm_reboot *);
>  int		  nsm_monitor(const struct nlm_host *host);
>  void		  nsm_unmonitor(const struct nlm_host *host);
>  
> -struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
> +struct nsm_handle *nsm_get_handle(const struct net *net,
> +					const struct sockaddr *sap,
>  					const size_t salen,
>  					const char *hostname,
>  					const size_t hostname_len);
> -struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info);
> +struct nsm_handle *nsm_reboot_lookup(const struct net *net,
> +					const struct nlm_reboot *info);
>  void		  nsm_release(struct nsm_handle *nsm);
>  
>  /*
> -- 
> 2.4.9

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] lockd: create NSM handles per net namespace
  2015-09-29 18:47 ` J. Bruce Fields
@ 2015-10-01 16:36   ` Andrey Ryabinin
  2015-10-01 18:26     ` J. Bruce Fields
  0 siblings, 1 reply; 5+ messages in thread
From: Andrey Ryabinin @ 2015-10-01 16:36 UTC (permalink / raw)
  To: J. Bruce Fields
  Cc: Trond Myklebust, Anna Schumaker, Jeff Layton, linux-nfs,
	linux-kernel, skinsbursky, stable

On 09/29/2015 09:47 PM, J. Bruce Fields wrote:
> On Wed, Sep 23, 2015 at 03:49:29PM +0300, Andrey Ryabinin wrote:
>> Commit cb7323fffa85 ("lockd: create and use per-net NSM
>>  RPC clients on MON/UNMON requests") introduced per-net
>> NSM RPC clients. Unfortunately this doesn't make any sense
>> without per-net nsm_handle.
> 
> Makes sense to me.  Is anyone doing testing to make sure we've got this
> right now?
> 
> (For example, have you reproduced the below problem and verified that
> it's fixed after this patch?)
> 

Yes, that NULL-ptr was actually hit, so I've fixed it with this patch.

BTW, after this patch we could get rid of that per-net nsm-rpc-clients stuff.
With per-net nsm_handles, rpc clients are already per-net.

AFAIK the only reason for them was introduced is because RPC client need to know 'utsname()->nodename',
but utsname() might be NULL when nsm_unmonitor() called.
So we could just save nodename e.g. in nlm_host struct and pass it to rpc_create(). Thus we don't need
to keep rpc client untill last unmonitor request. We could create/destroy separate RPC client for each
monitor/unmonitor request. IOW, like it was before cb7323fffa85 ("lockd: create and use per-net NSM RPC clients on MON/UNMON requests")

Anyway, this is a subject for a separate patch.



> --b.
> 
>>
>> E.g. the following scenario could happen
>> Two hosts (X and Y) in different namespaces (A and B) share
>> the same nsm struct.
>>
>> 1. nsm_monitor(host_X) called => NSM rpc client created,
>> 	nsm->sm_monitored bit set.
>> 2. nsm_mointor(host-Y) called => nsm->sm_monitored already set,
>> 	we just exit. Thus in namespace B ln->nsm_clnt == NULL.
>> 3. host X destroyed => nsm->sm_count decremented to 1
>> 4. host Y destroyed => nsm_unmonitor() => nsm_mon_unmon() => NULL-ptr
>> 	dereference of *ln->nsm_clnt
>>
>> So this could be fixed by making per-net nsm_handles list,
>> instead of global. Thus different net namespaces will not be able
>> share the same nsm_handle.
>>
>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>>  fs/lockd/host.c             |  7 ++++---
>>  fs/lockd/mon.c              | 36 ++++++++++++++++++++++--------------
>>  fs/lockd/netns.h            |  1 +
>>  fs/lockd/svc.c              |  1 +
>>  fs/lockd/svc4proc.c         |  2 +-
>>  fs/lockd/svcproc.c          |  2 +-
>>  include/linux/lockd/lockd.h |  9 ++++++---
>>  7 files changed, 36 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>> index 969d589..b5f3c3a 100644
>> --- a/fs/lockd/host.c
>> +++ b/fs/lockd/host.c
>> @@ -116,7 +116,7 @@ static struct nlm_host *nlm_alloc_host(struct nlm_lookup_host_info *ni,
>>  		atomic_inc(&nsm->sm_count);
>>  	else {
>>  		host = NULL;
>> -		nsm = nsm_get_handle(ni->sap, ni->salen,
>> +		nsm = nsm_get_handle(ni->net, ni->sap, ni->salen,
>>  					ni->hostname, ni->hostname_len);
>>  		if (unlikely(nsm == NULL)) {
>>  			dprintk("lockd: %s failed; no nsm handle\n",
>> @@ -534,17 +534,18 @@ static struct nlm_host *next_host_state(struct hlist_head *cache,
>>  
>>  /**
>>   * nlm_host_rebooted - Release all resources held by rebooted host
>> + * @net:  network namespace
>>   * @info: pointer to decoded results of NLM_SM_NOTIFY call
>>   *
>>   * We were notified that the specified host has rebooted.  Release
>>   * all resources held by that peer.
>>   */
>> -void nlm_host_rebooted(const struct nlm_reboot *info)
>> +void nlm_host_rebooted(const struct net *net, const struct nlm_reboot *info)
>>  {
>>  	struct nsm_handle *nsm;
>>  	struct nlm_host	*host;
>>  
>> -	nsm = nsm_reboot_lookup(info);
>> +	nsm = nsm_reboot_lookup(net, info);
>>  	if (unlikely(nsm == NULL))
>>  		return;
>>  
>> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
>> index 47a32b6..6c05cd1 100644
>> --- a/fs/lockd/mon.c
>> +++ b/fs/lockd/mon.c
>> @@ -51,7 +51,6 @@ struct nsm_res {
>>  };
>>  
>>  static const struct rpc_program	nsm_program;
>> -static				LIST_HEAD(nsm_handles);
>>  static				DEFINE_SPINLOCK(nsm_lock);
>>  
>>  /*
>> @@ -264,33 +263,35 @@ void nsm_unmonitor(const struct nlm_host *host)
>>  	}
>>  }
>>  
>> -static struct nsm_handle *nsm_lookup_hostname(const char *hostname,
>> -					      const size_t len)
>> +static struct nsm_handle *nsm_lookup_hostname(const struct list_head *nsm_handles,
>> +					const char *hostname, const size_t len)
>>  {
>>  	struct nsm_handle *nsm;
>>  
>> -	list_for_each_entry(nsm, &nsm_handles, sm_link)
>> +	list_for_each_entry(nsm, nsm_handles, sm_link)
>>  		if (strlen(nsm->sm_name) == len &&
>>  		    memcmp(nsm->sm_name, hostname, len) == 0)
>>  			return nsm;
>>  	return NULL;
>>  }
>>  
>> -static struct nsm_handle *nsm_lookup_addr(const struct sockaddr *sap)
>> +static struct nsm_handle *nsm_lookup_addr(const struct list_head *nsm_handles,
>> +					const struct sockaddr *sap)
>>  {
>>  	struct nsm_handle *nsm;
>>  
>> -	list_for_each_entry(nsm, &nsm_handles, sm_link)
>> +	list_for_each_entry(nsm, nsm_handles, sm_link)
>>  		if (rpc_cmp_addr(nsm_addr(nsm), sap))
>>  			return nsm;
>>  	return NULL;
>>  }
>>  
>> -static struct nsm_handle *nsm_lookup_priv(const struct nsm_private *priv)
>> +static struct nsm_handle *nsm_lookup_priv(const struct list_head *nsm_handles,
>> +					const struct nsm_private *priv)
>>  {
>>  	struct nsm_handle *nsm;
>>  
>> -	list_for_each_entry(nsm, &nsm_handles, sm_link)
>> +	list_for_each_entry(nsm, nsm_handles, sm_link)
>>  		if (memcmp(nsm->sm_priv.data, priv->data,
>>  					sizeof(priv->data)) == 0)
>>  			return nsm;
>> @@ -353,6 +354,7 @@ static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
>>  
>>  /**
>>   * nsm_get_handle - Find or create a cached nsm_handle
>> + * @net: network namespace
>>   * @sap: pointer to socket address of handle to find
>>   * @salen: length of socket address
>>   * @hostname: pointer to C string containing hostname to find
>> @@ -365,11 +367,13 @@ static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
>>   * @hostname cannot be found in the handle cache.  Returns NULL if
>>   * an error occurs.
>>   */
>> -struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
>> +struct nsm_handle *nsm_get_handle(const struct net *net,
>> +				  const struct sockaddr *sap,
>>  				  const size_t salen, const char *hostname,
>>  				  const size_t hostname_len)
>>  {
>>  	struct nsm_handle *cached, *new = NULL;
>> +	struct lockd_net *ln = net_generic(net, lockd_net_id);
>>  
>>  	if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
>>  		if (printk_ratelimit()) {
>> @@ -384,9 +388,10 @@ retry:
>>  	spin_lock(&nsm_lock);
>>  
>>  	if (nsm_use_hostnames && hostname != NULL)
>> -		cached = nsm_lookup_hostname(hostname, hostname_len);
>> +		cached = nsm_lookup_hostname(&ln->nsm_handles,
>> +					hostname, hostname_len);
>>  	else
>> -		cached = nsm_lookup_addr(sap);
>> +		cached = nsm_lookup_addr(&ln->nsm_handles, sap);
>>  
>>  	if (cached != NULL) {
>>  		atomic_inc(&cached->sm_count);
>> @@ -400,7 +405,7 @@ retry:
>>  	}
>>  
>>  	if (new != NULL) {
>> -		list_add(&new->sm_link, &nsm_handles);
>> +		list_add(&new->sm_link, &ln->nsm_handles);
>>  		spin_unlock(&nsm_lock);
>>  		dprintk("lockd: created nsm_handle for %s (%s)\n",
>>  				new->sm_name, new->sm_addrbuf);
>> @@ -417,19 +422,22 @@ retry:
>>  
>>  /**
>>   * nsm_reboot_lookup - match NLMPROC_SM_NOTIFY arguments to an nsm_handle
>> + * @net:  network namespace
>>   * @info: pointer to NLMPROC_SM_NOTIFY arguments
>>   *
>>   * Returns a matching nsm_handle if found in the nsm cache. The returned
>>   * nsm_handle's reference count is bumped. Otherwise returns NULL if some
>>   * error occurred.
>>   */
>> -struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info)
>> +struct nsm_handle *nsm_reboot_lookup(const struct net *net,
>> +				const struct nlm_reboot *info)
>>  {
>>  	struct nsm_handle *cached;
>> +	struct lockd_net *ln = net_generic(net, lockd_net_id);
>>  
>>  	spin_lock(&nsm_lock);
>>  
>> -	cached = nsm_lookup_priv(&info->priv);
>> +	cached = nsm_lookup_priv(&ln->nsm_handles, &info->priv);
>>  	if (unlikely(cached == NULL)) {
>>  		spin_unlock(&nsm_lock);
>>  		dprintk("lockd: never saw rebooted peer '%.*s' before\n",
>> diff --git a/fs/lockd/netns.h b/fs/lockd/netns.h
>> index 097bfa3..89fe011 100644
>> --- a/fs/lockd/netns.h
>> +++ b/fs/lockd/netns.h
>> @@ -15,6 +15,7 @@ struct lockd_net {
>>  	spinlock_t nsm_clnt_lock;
>>  	unsigned int nsm_users;
>>  	struct rpc_clnt *nsm_clnt;
>> +	struct list_head nsm_handles;
>>  };
>>  
>>  extern int lockd_net_id;
>> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
>> index d678bcc..0dff13f 100644
>> --- a/fs/lockd/svc.c
>> +++ b/fs/lockd/svc.c
>> @@ -593,6 +593,7 @@ static int lockd_init_net(struct net *net)
>>  	INIT_LIST_HEAD(&ln->lockd_manager.list);
>>  	ln->lockd_manager.block_opens = false;
>>  	spin_lock_init(&ln->nsm_clnt_lock);
>> +	INIT_LIST_HEAD(&ln->nsm_handles);
>>  	return 0;
>>  }
>>  
>> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
>> index b147d1a..09c576f 100644
>> --- a/fs/lockd/svc4proc.c
>> +++ b/fs/lockd/svc4proc.c
>> @@ -421,7 +421,7 @@ nlm4svc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
>>  		return rpc_system_err;
>>  	}
>>  
>> -	nlm_host_rebooted(argp);
>> +	nlm_host_rebooted(SVC_NET(rqstp), argp);
>>  	return rpc_success;
>>  }
>>  
>> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
>> index 21171f0..fb26b9f 100644
>> --- a/fs/lockd/svcproc.c
>> +++ b/fs/lockd/svcproc.c
>> @@ -464,7 +464,7 @@ nlmsvc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
>>  		return rpc_system_err;
>>  	}
>>  
>> -	nlm_host_rebooted(argp);
>> +	nlm_host_rebooted(SVC_NET(rqstp), argp);
>>  	return rpc_success;
>>  }
>>  
>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
>> index ff82a32..fd3b65b 100644
>> --- a/include/linux/lockd/lockd.h
>> +++ b/include/linux/lockd/lockd.h
>> @@ -235,7 +235,8 @@ void		  nlm_rebind_host(struct nlm_host *);
>>  struct nlm_host * nlm_get_host(struct nlm_host *);
>>  void		  nlm_shutdown_hosts(void);
>>  void		  nlm_shutdown_hosts_net(struct net *net);
>> -void		  nlm_host_rebooted(const struct nlm_reboot *);
>> +void		  nlm_host_rebooted(const struct net *net,
>> +					const struct nlm_reboot *);
>>  
>>  /*
>>   * Host monitoring
>> @@ -243,11 +244,13 @@ void		  nlm_host_rebooted(const struct nlm_reboot *);
>>  int		  nsm_monitor(const struct nlm_host *host);
>>  void		  nsm_unmonitor(const struct nlm_host *host);
>>  
>> -struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
>> +struct nsm_handle *nsm_get_handle(const struct net *net,
>> +					const struct sockaddr *sap,
>>  					const size_t salen,
>>  					const char *hostname,
>>  					const size_t hostname_len);
>> -struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info);
>> +struct nsm_handle *nsm_reboot_lookup(const struct net *net,
>> +					const struct nlm_reboot *info);
>>  void		  nsm_release(struct nsm_handle *nsm);
>>  
>>  /*
>> -- 
>> 2.4.9

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] lockd: create NSM handles per net namespace
  2015-10-01 16:36   ` Andrey Ryabinin
@ 2015-10-01 18:26     ` J. Bruce Fields
  2015-10-02  9:16       ` Andrey Ryabinin
  0 siblings, 1 reply; 5+ messages in thread
From: J. Bruce Fields @ 2015-10-01 18:26 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Trond Myklebust, Anna Schumaker, Jeff Layton, linux-nfs,
	linux-kernel, skinsbursky, stable

On Thu, Oct 01, 2015 at 07:36:19PM +0300, Andrey Ryabinin wrote:
> On 09/29/2015 09:47 PM, J. Bruce Fields wrote:
> > On Wed, Sep 23, 2015 at 03:49:29PM +0300, Andrey Ryabinin wrote:
> >> Commit cb7323fffa85 ("lockd: create and use per-net NSM
> >>  RPC clients on MON/UNMON requests") introduced per-net
> >> NSM RPC clients. Unfortunately this doesn't make any sense
> >> without per-net nsm_handle.
> > 
> > Makes sense to me.  Is anyone doing testing to make sure we've got this
> > right now?
> > 
> > (For example, have you reproduced the below problem and verified that
> > it's fixed after this patch?)
> > 
> 
> Yes, that NULL-ptr was actually hit, so I've fixed it with this patch.

Great, thanks.  I'm queuing this up for 4.4 in the nfsd tree (assuming
that's OK with Trond).

How extensively have you tested containerized nfs (client or server
side) at this point?  Have you seen any other issues?

> BTW, after this patch we could get rid of that per-net nsm-rpc-clients stuff.
> With per-net nsm_handles, rpc clients are already per-net.
> 
> AFAIK the only reason for them was introduced is because RPC client need to know 'utsname()->nodename',
> but utsname() might be NULL when nsm_unmonitor() called.
> So we could just save nodename e.g. in nlm_host struct and pass it to rpc_create(). Thus we don't need
> to keep rpc client untill last unmonitor request. We could create/destroy separate RPC client for each
> monitor/unmonitor request. IOW, like it was before cb7323fffa85 ("lockd: create and use per-net NSM RPC clients on MON/UNMON requests")
> 
> Anyway, this is a subject for a separate patch.

OK.  Sounds reasonable.

--b.

> 
> 
> 
> > --b.
> > 
> >>
> >> E.g. the following scenario could happen
> >> Two hosts (X and Y) in different namespaces (A and B) share
> >> the same nsm struct.
> >>
> >> 1. nsm_monitor(host_X) called => NSM rpc client created,
> >> 	nsm->sm_monitored bit set.
> >> 2. nsm_mointor(host-Y) called => nsm->sm_monitored already set,
> >> 	we just exit. Thus in namespace B ln->nsm_clnt == NULL.
> >> 3. host X destroyed => nsm->sm_count decremented to 1
> >> 4. host Y destroyed => nsm_unmonitor() => nsm_mon_unmon() => NULL-ptr
> >> 	dereference of *ln->nsm_clnt
> >>
> >> So this could be fixed by making per-net nsm_handles list,
> >> instead of global. Thus different net namespaces will not be able
> >> share the same nsm_handle.
> >>
> >> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> >> Cc: <stable@vger.kernel.org>
> >> ---
> >>  fs/lockd/host.c             |  7 ++++---
> >>  fs/lockd/mon.c              | 36 ++++++++++++++++++++++--------------
> >>  fs/lockd/netns.h            |  1 +
> >>  fs/lockd/svc.c              |  1 +
> >>  fs/lockd/svc4proc.c         |  2 +-
> >>  fs/lockd/svcproc.c          |  2 +-
> >>  include/linux/lockd/lockd.h |  9 ++++++---
> >>  7 files changed, 36 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> >> index 969d589..b5f3c3a 100644
> >> --- a/fs/lockd/host.c
> >> +++ b/fs/lockd/host.c
> >> @@ -116,7 +116,7 @@ static struct nlm_host *nlm_alloc_host(struct nlm_lookup_host_info *ni,
> >>  		atomic_inc(&nsm->sm_count);
> >>  	else {
> >>  		host = NULL;
> >> -		nsm = nsm_get_handle(ni->sap, ni->salen,
> >> +		nsm = nsm_get_handle(ni->net, ni->sap, ni->salen,
> >>  					ni->hostname, ni->hostname_len);
> >>  		if (unlikely(nsm == NULL)) {
> >>  			dprintk("lockd: %s failed; no nsm handle\n",
> >> @@ -534,17 +534,18 @@ static struct nlm_host *next_host_state(struct hlist_head *cache,
> >>  
> >>  /**
> >>   * nlm_host_rebooted - Release all resources held by rebooted host
> >> + * @net:  network namespace
> >>   * @info: pointer to decoded results of NLM_SM_NOTIFY call
> >>   *
> >>   * We were notified that the specified host has rebooted.  Release
> >>   * all resources held by that peer.
> >>   */
> >> -void nlm_host_rebooted(const struct nlm_reboot *info)
> >> +void nlm_host_rebooted(const struct net *net, const struct nlm_reboot *info)
> >>  {
> >>  	struct nsm_handle *nsm;
> >>  	struct nlm_host	*host;
> >>  
> >> -	nsm = nsm_reboot_lookup(info);
> >> +	nsm = nsm_reboot_lookup(net, info);
> >>  	if (unlikely(nsm == NULL))
> >>  		return;
> >>  
> >> diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
> >> index 47a32b6..6c05cd1 100644
> >> --- a/fs/lockd/mon.c
> >> +++ b/fs/lockd/mon.c
> >> @@ -51,7 +51,6 @@ struct nsm_res {
> >>  };
> >>  
> >>  static const struct rpc_program	nsm_program;
> >> -static				LIST_HEAD(nsm_handles);
> >>  static				DEFINE_SPINLOCK(nsm_lock);
> >>  
> >>  /*
> >> @@ -264,33 +263,35 @@ void nsm_unmonitor(const struct nlm_host *host)
> >>  	}
> >>  }
> >>  
> >> -static struct nsm_handle *nsm_lookup_hostname(const char *hostname,
> >> -					      const size_t len)
> >> +static struct nsm_handle *nsm_lookup_hostname(const struct list_head *nsm_handles,
> >> +					const char *hostname, const size_t len)
> >>  {
> >>  	struct nsm_handle *nsm;
> >>  
> >> -	list_for_each_entry(nsm, &nsm_handles, sm_link)
> >> +	list_for_each_entry(nsm, nsm_handles, sm_link)
> >>  		if (strlen(nsm->sm_name) == len &&
> >>  		    memcmp(nsm->sm_name, hostname, len) == 0)
> >>  			return nsm;
> >>  	return NULL;
> >>  }
> >>  
> >> -static struct nsm_handle *nsm_lookup_addr(const struct sockaddr *sap)
> >> +static struct nsm_handle *nsm_lookup_addr(const struct list_head *nsm_handles,
> >> +					const struct sockaddr *sap)
> >>  {
> >>  	struct nsm_handle *nsm;
> >>  
> >> -	list_for_each_entry(nsm, &nsm_handles, sm_link)
> >> +	list_for_each_entry(nsm, nsm_handles, sm_link)
> >>  		if (rpc_cmp_addr(nsm_addr(nsm), sap))
> >>  			return nsm;
> >>  	return NULL;
> >>  }
> >>  
> >> -static struct nsm_handle *nsm_lookup_priv(const struct nsm_private *priv)
> >> +static struct nsm_handle *nsm_lookup_priv(const struct list_head *nsm_handles,
> >> +					const struct nsm_private *priv)
> >>  {
> >>  	struct nsm_handle *nsm;
> >>  
> >> -	list_for_each_entry(nsm, &nsm_handles, sm_link)
> >> +	list_for_each_entry(nsm, nsm_handles, sm_link)
> >>  		if (memcmp(nsm->sm_priv.data, priv->data,
> >>  					sizeof(priv->data)) == 0)
> >>  			return nsm;
> >> @@ -353,6 +354,7 @@ static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
> >>  
> >>  /**
> >>   * nsm_get_handle - Find or create a cached nsm_handle
> >> + * @net: network namespace
> >>   * @sap: pointer to socket address of handle to find
> >>   * @salen: length of socket address
> >>   * @hostname: pointer to C string containing hostname to find
> >> @@ -365,11 +367,13 @@ static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
> >>   * @hostname cannot be found in the handle cache.  Returns NULL if
> >>   * an error occurs.
> >>   */
> >> -struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
> >> +struct nsm_handle *nsm_get_handle(const struct net *net,
> >> +				  const struct sockaddr *sap,
> >>  				  const size_t salen, const char *hostname,
> >>  				  const size_t hostname_len)
> >>  {
> >>  	struct nsm_handle *cached, *new = NULL;
> >> +	struct lockd_net *ln = net_generic(net, lockd_net_id);
> >>  
> >>  	if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
> >>  		if (printk_ratelimit()) {
> >> @@ -384,9 +388,10 @@ retry:
> >>  	spin_lock(&nsm_lock);
> >>  
> >>  	if (nsm_use_hostnames && hostname != NULL)
> >> -		cached = nsm_lookup_hostname(hostname, hostname_len);
> >> +		cached = nsm_lookup_hostname(&ln->nsm_handles,
> >> +					hostname, hostname_len);
> >>  	else
> >> -		cached = nsm_lookup_addr(sap);
> >> +		cached = nsm_lookup_addr(&ln->nsm_handles, sap);
> >>  
> >>  	if (cached != NULL) {
> >>  		atomic_inc(&cached->sm_count);
> >> @@ -400,7 +405,7 @@ retry:
> >>  	}
> >>  
> >>  	if (new != NULL) {
> >> -		list_add(&new->sm_link, &nsm_handles);
> >> +		list_add(&new->sm_link, &ln->nsm_handles);
> >>  		spin_unlock(&nsm_lock);
> >>  		dprintk("lockd: created nsm_handle for %s (%s)\n",
> >>  				new->sm_name, new->sm_addrbuf);
> >> @@ -417,19 +422,22 @@ retry:
> >>  
> >>  /**
> >>   * nsm_reboot_lookup - match NLMPROC_SM_NOTIFY arguments to an nsm_handle
> >> + * @net:  network namespace
> >>   * @info: pointer to NLMPROC_SM_NOTIFY arguments
> >>   *
> >>   * Returns a matching nsm_handle if found in the nsm cache. The returned
> >>   * nsm_handle's reference count is bumped. Otherwise returns NULL if some
> >>   * error occurred.
> >>   */
> >> -struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info)
> >> +struct nsm_handle *nsm_reboot_lookup(const struct net *net,
> >> +				const struct nlm_reboot *info)
> >>  {
> >>  	struct nsm_handle *cached;
> >> +	struct lockd_net *ln = net_generic(net, lockd_net_id);
> >>  
> >>  	spin_lock(&nsm_lock);
> >>  
> >> -	cached = nsm_lookup_priv(&info->priv);
> >> +	cached = nsm_lookup_priv(&ln->nsm_handles, &info->priv);
> >>  	if (unlikely(cached == NULL)) {
> >>  		spin_unlock(&nsm_lock);
> >>  		dprintk("lockd: never saw rebooted peer '%.*s' before\n",
> >> diff --git a/fs/lockd/netns.h b/fs/lockd/netns.h
> >> index 097bfa3..89fe011 100644
> >> --- a/fs/lockd/netns.h
> >> +++ b/fs/lockd/netns.h
> >> @@ -15,6 +15,7 @@ struct lockd_net {
> >>  	spinlock_t nsm_clnt_lock;
> >>  	unsigned int nsm_users;
> >>  	struct rpc_clnt *nsm_clnt;
> >> +	struct list_head nsm_handles;
> >>  };
> >>  
> >>  extern int lockd_net_id;
> >> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> >> index d678bcc..0dff13f 100644
> >> --- a/fs/lockd/svc.c
> >> +++ b/fs/lockd/svc.c
> >> @@ -593,6 +593,7 @@ static int lockd_init_net(struct net *net)
> >>  	INIT_LIST_HEAD(&ln->lockd_manager.list);
> >>  	ln->lockd_manager.block_opens = false;
> >>  	spin_lock_init(&ln->nsm_clnt_lock);
> >> +	INIT_LIST_HEAD(&ln->nsm_handles);
> >>  	return 0;
> >>  }
> >>  
> >> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> >> index b147d1a..09c576f 100644
> >> --- a/fs/lockd/svc4proc.c
> >> +++ b/fs/lockd/svc4proc.c
> >> @@ -421,7 +421,7 @@ nlm4svc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
> >>  		return rpc_system_err;
> >>  	}
> >>  
> >> -	nlm_host_rebooted(argp);
> >> +	nlm_host_rebooted(SVC_NET(rqstp), argp);
> >>  	return rpc_success;
> >>  }
> >>  
> >> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> >> index 21171f0..fb26b9f 100644
> >> --- a/fs/lockd/svcproc.c
> >> +++ b/fs/lockd/svcproc.c
> >> @@ -464,7 +464,7 @@ nlmsvc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
> >>  		return rpc_system_err;
> >>  	}
> >>  
> >> -	nlm_host_rebooted(argp);
> >> +	nlm_host_rebooted(SVC_NET(rqstp), argp);
> >>  	return rpc_success;
> >>  }
> >>  
> >> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> >> index ff82a32..fd3b65b 100644
> >> --- a/include/linux/lockd/lockd.h
> >> +++ b/include/linux/lockd/lockd.h
> >> @@ -235,7 +235,8 @@ void		  nlm_rebind_host(struct nlm_host *);
> >>  struct nlm_host * nlm_get_host(struct nlm_host *);
> >>  void		  nlm_shutdown_hosts(void);
> >>  void		  nlm_shutdown_hosts_net(struct net *net);
> >> -void		  nlm_host_rebooted(const struct nlm_reboot *);
> >> +void		  nlm_host_rebooted(const struct net *net,
> >> +					const struct nlm_reboot *);
> >>  
> >>  /*
> >>   * Host monitoring
> >> @@ -243,11 +244,13 @@ void		  nlm_host_rebooted(const struct nlm_reboot *);
> >>  int		  nsm_monitor(const struct nlm_host *host);
> >>  void		  nsm_unmonitor(const struct nlm_host *host);
> >>  
> >> -struct nsm_handle *nsm_get_handle(const struct sockaddr *sap,
> >> +struct nsm_handle *nsm_get_handle(const struct net *net,
> >> +					const struct sockaddr *sap,
> >>  					const size_t salen,
> >>  					const char *hostname,
> >>  					const size_t hostname_len);
> >> -struct nsm_handle *nsm_reboot_lookup(const struct nlm_reboot *info);
> >> +struct nsm_handle *nsm_reboot_lookup(const struct net *net,
> >> +					const struct nlm_reboot *info);
> >>  void		  nsm_release(struct nsm_handle *nsm);
> >>  
> >>  /*
> >> -- 
> >> 2.4.9

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] lockd: create NSM handles per net namespace
  2015-10-01 18:26     ` J. Bruce Fields
@ 2015-10-02  9:16       ` Andrey Ryabinin
  0 siblings, 0 replies; 5+ messages in thread
From: Andrey Ryabinin @ 2015-10-02  9:16 UTC (permalink / raw)
  To: J. Bruce Fields
  Cc: Trond Myklebust, Anna Schumaker, Jeff Layton, linux-nfs,
	linux-kernel, skinsbursky, stable

On 10/01/2015 09:26 PM, J. Bruce Fields wrote:
> On Thu, Oct 01, 2015 at 07:36:19PM +0300, Andrey Ryabinin wrote:
>> On 09/29/2015 09:47 PM, J. Bruce Fields wrote:
>>> On Wed, Sep 23, 2015 at 03:49:29PM +0300, Andrey Ryabinin wrote:
>>>> Commit cb7323fffa85 ("lockd: create and use per-net NSM
>>>>  RPC clients on MON/UNMON requests") introduced per-net
>>>> NSM RPC clients. Unfortunately this doesn't make any sense
>>>> without per-net nsm_handle.
>>>
>>> Makes sense to me.  Is anyone doing testing to make sure we've got this
>>> right now?
>>>
>>> (For example, have you reproduced the below problem and verified that
>>> it's fixed after this patch?)
>>>
>>
>> Yes, that NULL-ptr was actually hit, so I've fixed it with this patch.
> 
> Great, thanks.  I'm queuing this up for 4.4 in the nfsd tree (assuming
> that's OK with Trond).
> 
> How extensively have you tested containerized nfs (client or server
> side) at this point? 

We have some tests for that, but we don't run them on upstream kernels,
only on OpenVZ.

> Have you seen any other issues?
> 

None, so far.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-10-02  9:16 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-23 12:49 [PATCH] lockd: create NSM handles per net namespace Andrey Ryabinin
2015-09-29 18:47 ` J. Bruce Fields
2015-10-01 16:36   ` Andrey Ryabinin
2015-10-01 18:26     ` J. Bruce Fields
2015-10-02  9:16       ` Andrey Ryabinin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).