Re: [PATCH] lockd: create NSM handles per net namespace

[Andrey Ryabinin]

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@xxxxxxxxxxxxx>
>> Cc: <stable@xxxxxxxxxxxxxxx>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/