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-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
