[PATCH v2 0/3] lockd: use per-net refrence-counted NSM clients

[PATCH v2 0/3] lockd: use per-net refrence-counted NSM clients

[Stanislav Kinsbursky]

v2:
1) NSM transport is TCP based now. And all RPC tasks now called with
RPC_TASK_SOFTCONN. The advantage of this is that the kernel could discover
when statd is not running and fail the upcall immediately, rather than waiting
possibly many seconds for each upcall RPC to time out.
2) XDR layer violation (reference to upper RPC client cl_hostname) was
replaced by passing the string as a part of nlm_args structure.

This is a bug fix for https://bugzilla.redhat.com/show_bug.cgi?id=830862.

The problem is that with NFSv4 mount in container (with separated mount
namesapce) and active lock on it, dying child reaped of this container will
try to umount NFS and doing this will try to create RPC client to send
unmonitor request to statd.
But creation of RCP client requires valid current->nsproxy (for operation with
utsname()) and during umount on child reaper exit it's equal to zero.

Proposed solution is to introduce refrence-counter per-net NSM client, which
is created on fist monitor call and destroyed after the lst monitor call.

The following series implements...

---

Stanislav Kinsbursky (3):
      lockd: per-net NSM client creation and destruction helpers introduced
      lockd: use rpc client's cl_nodename for id encoding
      lockd: create and use per-net NSM RPC clients on MON/UNMON requests


 fs/lockd/mon.c   |   86 +++++++++++++++++++++++++++++++++++++++++++-----------
 fs/lockd/netns.h |    4 +++
 fs/lockd/svc.c   |    1 +
 3 files changed, 74 insertions(+), 17 deletions(-)

[PATCH v2 1/3] lockd: per-net NSM client creation and destruction helpers introduced

[Stanislav Kinsbursky]

NSM RPC client can be required on NFSv3 umount, when child reaper is dying (and
destroying it's mount namespace). It means, that current nsproxy is set to
NULL already, but creation of RPC client requires UTS namespace for gaining
hostname string.
This patch introduces reference counted NFS RPC clients creation and
destruction helpers (similar to RPCBIND RPC clients).

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
Cc: <stable@vger.kernel.org>
---
 fs/lockd/mon.c   |   51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/lockd/netns.h |    4 ++++
 fs/lockd/svc.c   |    1 +
 3 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 7ef14b3..38f240e 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -19,6 +19,8 @@
 
 #include <asm/unaligned.h>
 
+#include "netns.h"
+
 #define NLMDBG_FACILITY		NLMDBG_MONITOR
 #define NSM_PROGRAM		100024
 #define NSM_VERSION		1
@@ -70,7 +72,7 @@ static struct rpc_clnt *nsm_create(struct net *net)
 	};
 	struct rpc_create_args args = {
 		.net			= net,
-		.protocol		= XPRT_TRANSPORT_UDP,
+		.protocol		= XPRT_TRANSPORT_TCP,
 		.address		= (struct sockaddr *)&sin,
 		.addrsize		= sizeof(sin),
 		.servername		= "rpc.statd",
@@ -83,6 +85,51 @@ static struct rpc_clnt *nsm_create(struct net *net)
 	return rpc_create(&args);
 }
 
+__maybe_unused static struct rpc_clnt *nsm_client_get(struct net *net)
+{
+	static DEFINE_MUTEX(nsm_create_mutex);
+	struct rpc_clnt	*clnt;
+	struct lockd_net *ln = net_generic(net, lockd_net_id);
+
+	spin_lock(&ln->nsm_clnt_lock);
+	if (ln->nsm_users) {
+		ln->nsm_users++;
+		clnt = ln->nsm_clnt;
+		spin_unlock(&ln->nsm_clnt_lock);
+		goto out;
+	}
+	spin_unlock(&ln->nsm_clnt_lock);
+
+	mutex_lock(&nsm_create_mutex);
+	clnt = nsm_create(net);
+	if (!IS_ERR(clnt)) {
+		ln->nsm_clnt = clnt;
+		smp_wmb();
+		ln->nsm_users = 1;
+	}
+	mutex_unlock(&nsm_create_mutex);
+out:
+	return clnt;
+}
+
+__maybe_unused static void nsm_client_put(struct net *net)
+{
+	struct lockd_net *ln = net_generic(net, lockd_net_id);
+	struct rpc_clnt	*clnt = ln->nsm_clnt;
+	int shutdown = 0;
+
+	spin_lock(&ln->nsm_clnt_lock);
+	if (ln->nsm_users) {
+		if (--ln->nsm_users)
+			ln->nsm_clnt = NULL;
+		shutdown = !ln->nsm_users;
+	}
+	spin_unlock(&ln->nsm_clnt_lock);
+
+	if (shutdown)
+		rpc_shutdown_client(clnt);
+}
+
 static int nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res,
 			 struct net *net)
 {
@@ -111,7 +158,7 @@ static int nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res,
 	memset(res, 0, sizeof(*res));
 
 	msg.rpc_proc = &clnt->cl_procinfo[proc];
-	status = rpc_call_sync(clnt, &msg, 0);
+	status = rpc_call_sync(clnt, &msg, RPC_TASK_SOFTCONN);
 	if (status < 0)
 		dprintk("lockd: NSM upcall RPC failed, status=%d\n",
 				status);
diff --git a/fs/lockd/netns.h b/fs/lockd/netns.h
index 4eee248..5010b55 100644
--- a/fs/lockd/netns.h
+++ b/fs/lockd/netns.h
@@ -12,6 +12,10 @@ struct lockd_net {
 	struct delayed_work grace_period_end;
 	struct lock_manager lockd_manager;
 	struct list_head grace_list;
+
+	spinlock_t nsm_clnt_lock;
+	unsigned int nsm_users;
+	struct rpc_clnt *nsm_clnt;
 };
 
 extern int lockd_net_id;
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 31a63f8..7e35587 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -596,6 +596,7 @@ static int lockd_init_net(struct net *net)
 
 	INIT_DELAYED_WORK(&ln->grace_period_end, grace_ender);
 	INIT_LIST_HEAD(&ln->grace_list);
+	spin_lock_init(&ln->nsm_clnt_lock);
 	return 0;
 }
 

[PATCH v2 2/3] lockd: use rpc client's cl_nodename for id encoding

[Stanislav Kinsbursky]

Taking hostname from uts namespace if not safe, because this cuold be
performind during umount operation on child reaper death. And in this case
current->nsproxy is NULL already.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
Cc: <stable@vger.kernel.org>
---
 fs/lockd/mon.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 38f240e..e0bc36e 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -42,6 +42,7 @@ struct nsm_args {
 	u32			proc;
 
 	char			*mon_name;
+	char			*nodename;
 };
 
 struct nsm_res {
@@ -141,6 +142,7 @@ static int nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res,
 		.vers		= 3,
 		.proc		= NLMPROC_NSM_NOTIFY,
 		.mon_name	= nsm->sm_mon_name,
+		.nodename	= utsname()->nodename,
 	};
 	struct rpc_message msg = {
 		.rpc_argp	= &args,
@@ -477,7 +479,7 @@ static void encode_my_id(struct xdr_stream *xdr, const struct nsm_args *argp)
 {
 	__be32 *p;
 
-	encode_nsm_string(xdr, utsname()->nodename);
+	encode_nsm_string(xdr, argp->nodename);
 	p = xdr_reserve_space(xdr, 4 + 4 + 4);
 	*p++ = cpu_to_be32(argp->prog);
 	*p++ = cpu_to_be32(argp->vers);

[PATCH v2 3/3] lockd: create and use per-net NSM RPC clients on MON/UNMON requests

[Stanislav Kinsbursky]

NSM RPC client can be required on NFSv3 umount, when child reaper is dying
(and destroying it's mount namespace). It means, that current nsproxy is set
to NULL already, but creation of RPC client requires UTS namespace for gaining
hostname string.

This patch creates reference-counted per-net NSM client on first monitor
request and destroys it after last unmonitor request.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
Cc: <stable@vger.kernel.org>
---
 fs/lockd/mon.c |   37 ++++++++++++++++++++-----------------
 1 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index e0bc36e..e4fb3ba 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -7,7 +7,6 @@
  */
 
 #include <linux/types.h>
-#include <linux/utsname.h>
 #include <linux/kernel.h>
 #include <linux/ktime.h>
 #include <linux/slab.h>
@@ -86,7 +85,7 @@ static struct rpc_clnt *nsm_create(struct net *net)
 	return rpc_create(&args);
 }
 
-__maybe_unused static struct rpc_clnt *nsm_client_get(struct net *net)
+static struct rpc_clnt *nsm_client_get(struct net *net)
 {
 	static DEFINE_MUTEX(nsm_create_mutex);
 	struct rpc_clnt	*clnt;
@@ -113,7 +112,7 @@ out:
 	return clnt;
 }
 
-__maybe_unused static void nsm_client_put(struct net *net)
+static void nsm_client_put(struct net *net)
 {
 	struct lockd_net *ln = net_generic(net, lockd_net_id);
 	struct rpc_clnt	*clnt = ln->nsm_clnt;
@@ -132,9 +131,8 @@ __maybe_unused static void nsm_client_put(struct net *net)
 }
 
 static int nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res,
-			 struct net *net)
+			 struct rpc_clnt *clnt)
 {
-	struct rpc_clnt	*clnt;
 	int		status;
 	struct nsm_args args = {
 		.priv		= &nsm->sm_priv,
@@ -142,20 +140,14 @@ static int nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res,
 		.vers		= 3,
 		.proc		= NLMPROC_NSM_NOTIFY,
 		.mon_name	= nsm->sm_mon_name,
-		.nodename	= utsname()->nodename,
+		.nodename	= clnt->cl_nodename,
 	};
 	struct rpc_message msg = {
 		.rpc_argp	= &args,
 		.rpc_resp	= res,
 	};
 
-	clnt = nsm_create(net);
-	if (IS_ERR(clnt)) {
-		status = PTR_ERR(clnt);
-		dprintk("lockd: failed to create NSM upcall transport, "
-				"status=%d\n", status);
-		goto out;
-	}
+	BUG_ON(clnt == NULL);
 
 	memset(res, 0, sizeof(*res));
 
@@ -166,8 +158,6 @@ static int nsm_mon_unmon(struct nsm_handle *nsm, u32 proc, struct nsm_res *res,
 				status);
 	else
 		status = 0;
-	rpc_shutdown_client(clnt);
- out:
 	return status;
 }
 
@@ -187,6 +177,7 @@ int nsm_monitor(const struct nlm_host *host)
 	struct nsm_handle *nsm = host->h_nsmhandle;
 	struct nsm_res	res;
 	int		status;
+	struct rpc_clnt *clnt;
 
 	dprintk("lockd: nsm_monitor(%s)\n", nsm->sm_name);
 
@@ -199,7 +190,15 @@ int nsm_monitor(const struct nlm_host *host)
 	 */
 	nsm->sm_mon_name = nsm_use_hostnames ? nsm->sm_name : nsm->sm_addrbuf;
 
-	status = nsm_mon_unmon(nsm, NSMPROC_MON, &res, host->net);
+	clnt = nsm_client_get(host->net);
+	if (IS_ERR(clnt)) {
+		status = PTR_ERR(clnt);
+		dprintk("lockd: failed to create NSM upcall transport, "
+				"status=%d, net=%p\n", status, host->net);
+		return status;
+	}
+
+	status = nsm_mon_unmon(nsm, NSMPROC_MON, &res, clnt);
 	if (unlikely(res.status != 0))
 		status = -EIO;
 	if (unlikely(status < 0)) {
@@ -231,9 +230,11 @@ void nsm_unmonitor(const struct nlm_host *host)
 
 	if (atomic_read(&nsm->sm_count) == 1
 	 && nsm->sm_monitored && !nsm->sm_sticky) {
+		struct lockd_net *ln = net_generic(host->net, lockd_net_id);
+
 		dprintk("lockd: nsm_unmonitor(%s)\n", nsm->sm_name);
 
-		status = nsm_mon_unmon(nsm, NSMPROC_UNMON, &res, host->net);
+		status = nsm_mon_unmon(nsm, NSMPROC_UNMON, &res, ln->nsm_clnt);
 		if (res.status != 0)
 			status = -EIO;
 		if (status < 0)
@@ -241,6 +242,8 @@ void nsm_unmonitor(const struct nlm_host *host)
 					nsm->sm_name);
 		else
 			nsm->sm_monitored = 0;
+
+		nsm_client_put(host->net);
 	}
 }
 

kernel BUG at fs/lockd/mon.c:150, was Re: [PATCH v2 3/3] lockd: create and ...

[Kees Bakker]

On 18-09-12 11:37, Stanislav Kinsbursky wrote:
> NSM RPC client can be required on NFSv3 umount, when child reaper is dying
> (and destroying it's mount namespace). It means, that current nsproxy is set
> to NULL already, but creation of RPC client requires UTS namespace for gaining
> hostname string.
>
> This patch creates reference-counted per-net NSM client on first monitor
> request and destroys it after last unmonitor request.
>

We're hitting the BUG_ON that was added in this patch. I've tried it with
3.6.3 and 3.6.4

It happens almost immediate when I do apt-get update (the archive is on NFS).
-- 
Kees

Re: kernel BUG at fs/lockd/mon.c:150, was Re: [PATCH v2 3/3] lockd: create and ...

[Kees Bakker]

On 30-10-12 14:04, Kees Bakker wrote:
> On 18-09-12 11:37, Stanislav Kinsbursky wrote:
>> NSM RPC client can be required on NFSv3 umount, when child reaper is dying
>> (and destroying it's mount namespace). It means, that current nsproxy is set
>> to NULL already, but creation of RPC client requires UTS namespace for gaining
>> hostname string.
>>
>> This patch creates reference-counted per-net NSM client on first monitor
>> request and destroys it after last unmonitor request.
>>
>
> We're hitting the BUG_ON that was added in this patch. I've tried it with
> 3.6.3 and 3.6.4
>
> It happens almost immediate when I do apt-get update (the archive is on NFS).

And here is the oops message in case you need it.

------------[ cut here ]------------
kernel BUG at fs/lockd/mon.c:150!
ACPI: Invalid Power Resource to register!
invalid opcode: 0000 [#1] PREEMPT SMP
Modules linked in: vmthrottle
Pid: 18025, comm: apt-get Not tainted 3.6.4-x274+ #1 Dell Inc.                 OptiPlex 745                 /0RF703
EIP: 0060:[<c1232c63>] EFLAGS: 00010246 CPU: 1
EIP is at nsm_mon_unmon+0xa3/0xb0
EAX: e228dde8 EBX: 00000000 ECX: e228de2c EDX: 00000002
ESI: e446b989 EDI: e9073250 EBP: e228de18 ESP: e228dde0
  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 8005003b CR2: b762c000 CR3: 342da000 CR4: 00000770
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff0ff0 DR7: 00000400
Process apt-get (pid: 18025, ti=e228c000 task=da1e9e30 task.ti=e228c000)
Stack:
  da218500 e228ddfc e446b989 000186b5 00000003 00000010 e446b999 00000058
  c18470c2 e228dde8 e228de2c 00000000 e446b8f0 00000000 e228de40 c12330a5
  00000000 e51782e2 c1a88fb6 ff0a0004 ffffffff 00000001 e5178000 e9073250
Call Trace:
  [<c18470c2>] ? _raw_spin_unlock+0x12/0x30
  [<c12330a5>] nsm_monitor+0x65/0x120
  [<c122d1fc>] nlmclnt_lock+0x3c/0x260
  [<c122c778>] ? nlmclnt_setlockargs+0xd8/0x170
  [<c122d62c>] nlmclnt_proc+0x8c/0x130
  [<c11e1dfa>] nfs3_proc_lock+0x1a/0x20
  [<c11d0499>] do_setlk+0xc9/0xf0
  [<c11d0655>] nfs_lock+0xa5/0x1b0
  [<c113e123>] ? locks_alloc_lock+0x13/0x50
  [<c182e426>] ? __slab_free+0x26c/0x2d4
  [<c1100e5b>] ? do_sys_open+0x16b/0x1d0
  [<c11d05b0>] ? nfs_flock+0xf0/0xf0
  [<c113f120>] vfs_lock_file+0x20/0x50
  [<c113f210>] do_lock_file_wait+0x30/0xb0
  [<c12c923a>] ? __copy_from_user_ll+0x1a/0x30
  [<c113fe69>] fcntl_setlk64+0xd9/0x1c0
  [<c18470c2>] ? _raw_spin_unlock+0x12/0x30
  [<c1110516>] sys_fcntl64+0x76/0xb0
  [<c1847b9c>] sysenter_do_call+0x12/0x2c
Code: db 83 c4 30 89 d8 5b 5e 5d c3 8d b6 00 00 00 00 f6 05 00 84 cb c1 10 74 e8 89 44 24 04 c7 04 24 20 a2 a9 c1 e8 31 8c 5f 00 eb d6 <0f> 0b 8d 74 26 00 8d bc 27 00 00 00 00 55 89 e5 83 ec 0c 89 5d
EIP: [<c1232c63>] nsm_mon_unmon+0xa3/0xb0 SS:ESP 0068:e228dde0
---[ end trace 3316e005c1fbd275 ]---

-- 
Kees

Re: kernel BUG at fs/lockd/mon.c:150, was Re: [PATCH v2 3/3] lockd: create and ...

[Kees Bakker]

On 30-10-12 14:04, Kees Bakker wrote:
> On 18-09-12 11:37, Stanislav Kinsbursky wrote:
>> NSM RPC client can be required on NFSv3 umount, when child reaper is dying
>> (and destroying it's mount namespace). It means, that current nsproxy is set
>> to NULL already, but creation of RPC client requires UTS namespace for gaining
>> hostname string.
>>
>> This patch creates reference-counted per-net NSM client on first monitor
>> request and destroys it after last unmonitor request.
>>
>
> We're hitting the BUG_ON that was added in this patch. I've tried it with
> 3.6.3 and 3.6.4
>
> It happens almost immediate when I do apt-get update (the archive is on NFS).

To follow up (just talking to myself I guess), stable v3.6.5 solves the problem,
thanks. I did try v3.6.4 first but the patch inthere was not sufficient.
-- 
Kees