linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] SUNRPC: PipeFS races fixes
@ 2013-06-24  7:52 Stanislav Kinsbursky
  2013-06-24  7:52 ` [PATCH v3 1/4] SUNRPC: fix races on PipeFS MOUNT notifications Stanislav Kinsbursky
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Stanislav Kinsbursky @ 2013-06-24  7:52 UTC (permalink / raw)
  To: Trond.Myklebust; +Cc: linux-nfs, devel, linux-kernel, jlayton

This series fixes races between PipeFS mount/umount notification calls and
SUNRPC clients creation and destruction.

https://bugzilla.redhat.com/show_bug.cgi?id=924649

v3: fixes first (for easy backport to stable)

v2: Fixed few silly locking bugs.

The following series implements...

---

Stanislav Kinsbursky (4):
      SUNRPC: fix races on PipeFS MOUNT notifications
      SUNRPC: fix races on PipeFS UMOUNT notifications
      SUNRPC: split client creation routine into setup and registration
      SUNRPC: PipeFS MOUNT notification optimization for dying clients


 net/sunrpc/clnt.c     |   71 ++++++++++++++++++++++++++++++-------------------
 net/sunrpc/rpc_pipe.c |    5 +++
 2 files changed, 48 insertions(+), 28 deletions(-)


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

* [PATCH v3 1/4] SUNRPC: fix races on PipeFS MOUNT notifications
  2013-06-24  7:52 [PATCH v3 0/4] SUNRPC: PipeFS races fixes Stanislav Kinsbursky
@ 2013-06-24  7:52 ` Stanislav Kinsbursky
  2013-06-24  7:52 ` [PATCH v3 2/4] SUNRPC: fix races on PipeFS UMOUNT notifications Stanislav Kinsbursky
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Stanislav Kinsbursky @ 2013-06-24  7:52 UTC (permalink / raw)
  To: Trond.Myklebust; +Cc: linux-nfs, devel, linux-kernel, jlayton

Below are races, when RPC client can be created without PiepFS dentries

CPU#0					CPU#1
-----------------------------		-----------------------------
rpc_new_client				rpc_fill_super
rpc_setup_pipedir
mutex_lock(&sn->pipefs_sb_lock)
rpc_get_sb_net == NULL
(no per-net PipeFS superblock)
					sn->pipefs_sb = sb;
					notifier_call_chain(MOUNT)
					(client is not in the list)
rpc_register_client
(client without pipes dentries)

To fix this patch:
1) makes PipeFS mount notification call with pipefs_sb_lock being held.
2) releases pipefs_sb_lock on new SUNRPC client creation only after
registration.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
Cc: stable@vger.kernel.org
---
 net/sunrpc/clnt.c     |   26 +++++++++++++++-----------
 net/sunrpc/rpc_pipe.c |    3 +++
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 5a750b9..b827a4b 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -157,20 +157,15 @@ static struct dentry *rpc_setup_pipedir_sb(struct super_block *sb,
 }
 
 static int
-rpc_setup_pipedir(struct rpc_clnt *clnt, const char *dir_name)
+rpc_setup_pipedir(struct rpc_clnt *clnt, const char *dir_name,
+		  struct super_block *pipefs_sb)
 {
-	struct net *net = rpc_net_ns(clnt);
-	struct super_block *pipefs_sb;
 	struct dentry *dentry;
 
 	clnt->cl_dentry = NULL;
 	if (dir_name == NULL)
 		return 0;
-	pipefs_sb = rpc_get_sb_net(net);
-	if (!pipefs_sb)
-		return 0;
 	dentry = rpc_setup_pipedir_sb(pipefs_sb, clnt, dir_name);
-	rpc_put_sb_net(net);
 	if (IS_ERR(dentry))
 		return PTR_ERR(dentry);
 	clnt->cl_dentry = dentry;
@@ -296,6 +291,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, stru
 	struct rpc_clnt		*clnt = NULL;
 	struct rpc_auth		*auth;
 	int err;
+	struct super_block *pipefs_sb;
 
 	/* sanity check the name before trying to print it */
 	dprintk("RPC:       creating %s client for %s (xprt %p)\n",
@@ -354,9 +350,12 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, stru
 
 	atomic_set(&clnt->cl_count, 1);
 
-	err = rpc_setup_pipedir(clnt, program->pipe_dir_name);
-	if (err < 0)
-		goto out_no_path;
+	pipefs_sb = rpc_get_sb_net(rpc_net_ns(clnt));
+	if (pipefs_sb) {
+		err = rpc_setup_pipedir(clnt, program->pipe_dir_name, pipefs_sb);
+		if (err)
+			goto out_no_path;
+	}
 
 	auth = rpcauth_create(args->authflavor, clnt);
 	if (IS_ERR(auth)) {
@@ -369,11 +368,16 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, stru
 	/* save the nodename */
 	rpc_clnt_set_nodename(clnt, utsname()->nodename);
 	rpc_register_client(clnt);
+	if (pipefs_sb)
+		rpc_put_sb_net(rpc_net_ns(clnt));
 	return clnt;
 
 out_no_auth:
-	rpc_clnt_remove_pipedir(clnt);
+	if (pipefs_sb)
+		__rpc_clnt_remove_pipedir(clnt);
 out_no_path:
+	if (pipefs_sb)
+		rpc_put_sb_net(rpc_net_ns(clnt));
 	kfree(clnt->cl_principal);
 out_no_principal:
 	rpc_free_iostats(clnt->cl_metrics);
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index e7ce4b3..c512448 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -1126,6 +1126,7 @@ rpc_fill_super(struct super_block *sb, void *data, int silent)
 		return -ENOMEM;
 	dprintk("RPC:       sending pipefs MOUNT notification for net %p%s\n",
 		net, NET_NAME(net));
+	mutex_lock(&sn->pipefs_sb_lock);
 	sn->pipefs_sb = sb;
 	err = blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
 					   RPC_PIPEFS_MOUNT,
@@ -1133,6 +1134,7 @@ rpc_fill_super(struct super_block *sb, void *data, int silent)
 	if (err)
 		goto err_depopulate;
 	sb->s_fs_info = get_net(net);
+	mutex_unlock(&sn->pipefs_sb_lock);
 	return 0;
 
 err_depopulate:
@@ -1141,6 +1143,7 @@ err_depopulate:
 					   sb);
 	sn->pipefs_sb = NULL;
 	__rpc_depopulate(root, files, RPCAUTH_lockd, RPCAUTH_RootEOF);
+	mutex_unlock(&sn->pipefs_sb_lock);
 	return err;
 }
 


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

* [PATCH v3 2/4] SUNRPC: fix races on PipeFS UMOUNT notifications
  2013-06-24  7:52 [PATCH v3 0/4] SUNRPC: PipeFS races fixes Stanislav Kinsbursky
  2013-06-24  7:52 ` [PATCH v3 1/4] SUNRPC: fix races on PipeFS MOUNT notifications Stanislav Kinsbursky
@ 2013-06-24  7:52 ` Stanislav Kinsbursky
  2013-06-25 16:13   ` Myklebust, Trond
  2013-06-26  6:15   ` [PATCH v4] " Stanislav Kinsbursky
  2013-06-24  7:52 ` [PATCH v3 3/4] SUNRPC: split client creation routine into setup and registration Stanislav Kinsbursky
  2013-06-24  7:52 ` [PATCH v3 4/4] SUNRPC: PipeFS MOUNT notification optimization for dying clients Stanislav Kinsbursky
  3 siblings, 2 replies; 9+ messages in thread
From: Stanislav Kinsbursky @ 2013-06-24  7:52 UTC (permalink / raw)
  To: Trond.Myklebust; +Cc: linux-nfs, devel, linux-kernel, jlayton

CPU#0                                   CPU#1
-----------------------------           -----------------------------
rpc_kill_sb
sn->pipefs_sb = NULL                    rpc_release_client
(UMOUNT_EVENT)                          rpc_free_auth
rpc_pipefs_event
rpc_get_client_for_event
!atomic_inc_not_zero(cl_count)
<skip the client>
                                        atomic_inc(cl_count)
                                        rpc_free_client
                                        rpc_clnt_remove_pipedir
                                        <skip client dir removing>

To fix this, this patch does the following:

1) Calls RPC_PIPEFS_UMOUNT notification with sn->pipefs_sb_lock being held.
2) Removes SUNRPC client from the list AFTER pipes destroying.
3) Doesn't hold RPC client on notification: if client in the list, then it
can't be destroyed while sn->pipefs_sb_lock in hold by notification caller.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
Cc: stable@vger.kernel.org
---
 net/sunrpc/clnt.c     |    5 +----
 net/sunrpc/rpc_pipe.c |    2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index b827a4b..41f180c 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -236,8 +236,6 @@ static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
 			continue;
 		if (rpc_clnt_skip_event(clnt, event))
 			continue;
-		if (atomic_inc_not_zero(&clnt->cl_count) == 0)
-			continue;
 		spin_unlock(&sn->rpc_client_lock);
 		return clnt;
 	}
@@ -254,7 +252,6 @@ static int rpc_pipefs_event(struct notifier_block *nb, unsigned long event,
 
 	while ((clnt = rpc_get_client_for_event(sb->s_fs_info, event))) {
 		error = __rpc_pipefs_event(clnt, event, sb);
-		rpc_release_client(clnt);
 		if (error)
 			break;
 	}
@@ -641,8 +638,8 @@ rpc_free_client(struct rpc_clnt *clnt)
 			rcu_dereference(clnt->cl_xprt)->servername);
 	if (clnt->cl_parent != clnt)
 		rpc_release_client(clnt->cl_parent);
-	rpc_unregister_client(clnt);
 	rpc_clnt_remove_pipedir(clnt);
+	rpc_unregister_client(clnt);
 	rpc_free_iostats(clnt->cl_metrics);
 	kfree(clnt->cl_principal);
 	clnt->cl_metrics = NULL;
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index c512448..efca2f7 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -1165,7 +1165,6 @@ static void rpc_kill_sb(struct super_block *sb)
 		goto out;
 	}
 	sn->pipefs_sb = NULL;
-	mutex_unlock(&sn->pipefs_sb_lock);
 	dprintk("RPC:       sending pipefs UMOUNT notification for net %p%s\n",
 		net, NET_NAME(net));
 	blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
@@ -1173,6 +1172,7 @@ static void rpc_kill_sb(struct super_block *sb)
 					   sb);
 	put_net(net);
 out:
+	mutex_unlock(&sn->pipefs_sb_lock);
 	kill_litter_super(sb);
 }
 


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

* [PATCH v3 3/4] SUNRPC: split client creation routine into setup and registration
  2013-06-24  7:52 [PATCH v3 0/4] SUNRPC: PipeFS races fixes Stanislav Kinsbursky
  2013-06-24  7:52 ` [PATCH v3 1/4] SUNRPC: fix races on PipeFS MOUNT notifications Stanislav Kinsbursky
  2013-06-24  7:52 ` [PATCH v3 2/4] SUNRPC: fix races on PipeFS UMOUNT notifications Stanislav Kinsbursky
@ 2013-06-24  7:52 ` Stanislav Kinsbursky
  2013-06-24  7:52 ` [PATCH v3 4/4] SUNRPC: PipeFS MOUNT notification optimization for dying clients Stanislav Kinsbursky
  3 siblings, 0 replies; 9+ messages in thread
From: Stanislav Kinsbursky @ 2013-06-24  7:52 UTC (permalink / raw)
  To: Trond.Myklebust; +Cc: linux-nfs, devel, linux-kernel, jlayton

This helper moves all "registration" code to the new rpc_client_register()
helper.
This helper will be used later in the series to synchronize against PipeFS
MOUNT/UMOUNT events.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
---
 net/sunrpc/clnt.c |   64 ++++++++++++++++++++++++++++++++---------------------
 1 files changed, 39 insertions(+), 25 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 41f180c..b4f1711 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -281,14 +281,47 @@ static void rpc_clnt_set_nodename(struct rpc_clnt *clnt, const char *nodename)
 	memcpy(clnt->cl_nodename, nodename, clnt->cl_nodelen);
 }
 
+static int rpc_client_register(const struct rpc_create_args *args,
+			       struct rpc_clnt *clnt)
+{
+	const struct rpc_program *program = args->program;
+	struct rpc_auth *auth;
+	struct net *net = rpc_net_ns(clnt);
+	struct super_block *pipefs_sb;
+	int err = 0;
+
+	pipefs_sb = rpc_get_sb_net(net);
+	if (pipefs_sb) {
+		err = rpc_setup_pipedir(clnt, program->pipe_dir_name, pipefs_sb);
+		if (err)
+			goto out;
+	}
+
+	auth = rpcauth_create(args->authflavor, clnt);
+	if (IS_ERR(auth)) {
+		dprintk("RPC:       Couldn't create auth handle (flavor %u)\n",
+				args->authflavor);
+		err = PTR_ERR(auth);
+		goto err_auth;
+	}
+
+	rpc_register_client(clnt);
+out:
+	if (pipefs_sb)
+		rpc_put_sb_net(net);
+	return err;
+
+err_auth:
+	__rpc_clnt_remove_pipedir(clnt);
+	goto out;
+}
+
 static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, struct rpc_xprt *xprt)
 {
 	const struct rpc_program *program = args->program;
 	const struct rpc_version *version;
 	struct rpc_clnt		*clnt = NULL;
-	struct rpc_auth		*auth;
 	int err;
-	struct super_block *pipefs_sb;
 
 	/* sanity check the name before trying to print it */
 	dprintk("RPC:       creating %s client for %s (xprt %p)\n",
@@ -347,34 +380,15 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, stru
 
 	atomic_set(&clnt->cl_count, 1);
 
-	pipefs_sb = rpc_get_sb_net(rpc_net_ns(clnt));
-	if (pipefs_sb) {
-		err = rpc_setup_pipedir(clnt, program->pipe_dir_name, pipefs_sb);
-		if (err)
-			goto out_no_path;
-	}
-
-	auth = rpcauth_create(args->authflavor, clnt);
-	if (IS_ERR(auth)) {
-		dprintk("RPC:       Couldn't create auth handle (flavor %u)\n",
-				args->authflavor);
-		err = PTR_ERR(auth);
-		goto out_no_auth;
-	}
-
 	/* save the nodename */
 	rpc_clnt_set_nodename(clnt, utsname()->nodename);
-	rpc_register_client(clnt);
-	if (pipefs_sb)
-		rpc_put_sb_net(rpc_net_ns(clnt));
+
+	err = rpc_client_register(args, clnt);
+	if (err)
+		goto out_no_path;
 	return clnt;
 
-out_no_auth:
-	if (pipefs_sb)
-		__rpc_clnt_remove_pipedir(clnt);
 out_no_path:
-	if (pipefs_sb)
-		rpc_put_sb_net(rpc_net_ns(clnt));
 	kfree(clnt->cl_principal);
 out_no_principal:
 	rpc_free_iostats(clnt->cl_metrics);


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

* [PATCH v3 4/4] SUNRPC: PipeFS MOUNT notification optimization for dying clients
  2013-06-24  7:52 [PATCH v3 0/4] SUNRPC: PipeFS races fixes Stanislav Kinsbursky
                   ` (2 preceding siblings ...)
  2013-06-24  7:52 ` [PATCH v3 3/4] SUNRPC: split client creation routine into setup and registration Stanislav Kinsbursky
@ 2013-06-24  7:52 ` Stanislav Kinsbursky
  3 siblings, 0 replies; 9+ messages in thread
From: Stanislav Kinsbursky @ 2013-06-24  7:52 UTC (permalink / raw)
  To: Trond.Myklebust; +Cc: linux-nfs, devel, linux-kernel, jlayton

Not need to create pipes for dying client. So just skip them.

Note: we can safely dereference the client structure, because notification
caller is holding sn->pipefs_sb_lock.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
Cc: stable@vger.kernel.org
---
 net/sunrpc/clnt.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index b4f1711..f0339ae 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -177,6 +177,8 @@ static inline int rpc_clnt_skip_event(struct rpc_clnt *clnt, unsigned long event
 	if (((event == RPC_PIPEFS_MOUNT) && clnt->cl_dentry) ||
 	    ((event == RPC_PIPEFS_UMOUNT) && !clnt->cl_dentry))
 		return 1;
+	if ((event == RPC_PIPEFS_MOUNT) && atomic_read(&clnt->cl_count) == 0)
+		return 1;
 	return 0;
 }
 


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

* Re: [PATCH v3 2/4] SUNRPC: fix races on PipeFS UMOUNT notifications
  2013-06-24  7:52 ` [PATCH v3 2/4] SUNRPC: fix races on PipeFS UMOUNT notifications Stanislav Kinsbursky
@ 2013-06-25 16:13   ` Myklebust, Trond
  2013-06-26  5:32     ` Stanislav Kinsbursky
  2013-06-26  6:15   ` [PATCH v4] " Stanislav Kinsbursky
  1 sibling, 1 reply; 9+ messages in thread
From: Myklebust, Trond @ 2013-06-25 16:13 UTC (permalink / raw)
  To: Stanislav Kinsbursky; +Cc: linux-nfs, devel, linux-kernel, jlayton

On Mon, 2013-06-24 at 11:52 +0400, Stanislav Kinsbursky wrote:
> CPU#0                                   CPU#1
> -----------------------------           -----------------------------
> rpc_kill_sb
> sn->pipefs_sb = NULL                    rpc_release_client
> (UMOUNT_EVENT)                          rpc_free_auth
> rpc_pipefs_event
> rpc_get_client_for_event
> !atomic_inc_not_zero(cl_count)
> <skip the client>
>                                         atomic_inc(cl_count)
>                                         rpc_free_client
>                                         rpc_clnt_remove_pipedir
>                                         <skip client dir removing>
> 
> To fix this, this patch does the following:
> 
> 1) Calls RPC_PIPEFS_UMOUNT notification with sn->pipefs_sb_lock being held.
> 2) Removes SUNRPC client from the list AFTER pipes destroying.
> 3) Doesn't hold RPC client on notification: if client in the list, then it
> can't be destroyed while sn->pipefs_sb_lock in hold by notification caller.
> 
> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
> Cc: stable@vger.kernel.org
> ---
>  net/sunrpc/clnt.c     |    5 +----
>  net/sunrpc/rpc_pipe.c |    2 +-
>  2 files changed, 2 insertions(+), 5 deletions(-)

<snip>

> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index c512448..efca2f7 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -1165,7 +1165,6 @@ static void rpc_kill_sb(struct super_block *sb)
>  		goto out;
>  	}
>  	sn->pipefs_sb = NULL;
> -	mutex_unlock(&sn->pipefs_sb_lock);
>  	dprintk("RPC:       sending pipefs UMOUNT notification for net %p%s\n",
>  		net, NET_NAME(net));
>  	blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
> @@ -1173,6 +1172,7 @@ static void rpc_kill_sb(struct super_block *sb)
>  					   sb);
>  	put_net(net);
>  out:
> +	mutex_unlock(&sn->pipefs_sb_lock);

Is this safe to do after the put_net()?

>  	kill_litter_super(sb);
>  }
>  
> 


-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

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

* Re: [PATCH v3 2/4] SUNRPC: fix races on PipeFS UMOUNT notifications
  2013-06-25 16:13   ` Myklebust, Trond
@ 2013-06-26  5:32     ` Stanislav Kinsbursky
  0 siblings, 0 replies; 9+ messages in thread
From: Stanislav Kinsbursky @ 2013-06-26  5:32 UTC (permalink / raw)
  To: Myklebust, Trond; +Cc: linux-nfs, devel, linux-kernel, jlayton

25.06.2013 20:13, Myklebust, Trond пишет:
> On Mon, 2013-06-24 at 11:52 +0400, Stanislav Kinsbursky wrote:
>> CPU#0                                   CPU#1
>> -----------------------------           -----------------------------
>> rpc_kill_sb
>> sn->pipefs_sb = NULL                    rpc_release_client
>> (UMOUNT_EVENT)                          rpc_free_auth
>> rpc_pipefs_event
>> rpc_get_client_for_event
>> !atomic_inc_not_zero(cl_count)
>> <skip the client>
>>                                          atomic_inc(cl_count)
>>                                          rpc_free_client
>>                                          rpc_clnt_remove_pipedir
>>                                          <skip client dir removing>
>>
>> To fix this, this patch does the following:
>>
>> 1) Calls RPC_PIPEFS_UMOUNT notification with sn->pipefs_sb_lock being held.
>> 2) Removes SUNRPC client from the list AFTER pipes destroying.
>> 3) Doesn't hold RPC client on notification: if client in the list, then it
>> can't be destroyed while sn->pipefs_sb_lock in hold by notification caller.
>>
>> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
>> Cc: stable@vger.kernel.org
>> ---
>>   net/sunrpc/clnt.c     |    5 +----
>>   net/sunrpc/rpc_pipe.c |    2 +-
>>   2 files changed, 2 insertions(+), 5 deletions(-)
>
> <snip>
>
>> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
>> index c512448..efca2f7 100644
>> --- a/net/sunrpc/rpc_pipe.c
>> +++ b/net/sunrpc/rpc_pipe.c
>> @@ -1165,7 +1165,6 @@ static void rpc_kill_sb(struct super_block *sb)
>>   		goto out;
>>   	}
>>   	sn->pipefs_sb = NULL;
>> -	mutex_unlock(&sn->pipefs_sb_lock);
>>   	dprintk("RPC:       sending pipefs UMOUNT notification for net %p%s\n",
>>   		net, NET_NAME(net));
>>   	blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
>> @@ -1173,6 +1172,7 @@ static void rpc_kill_sb(struct super_block *sb)
>>   					   sb);
>>   	put_net(net);
>>   out:
>> +	mutex_unlock(&sn->pipefs_sb_lock);
>
> Is this safe to do after the put_net()?
>

Sure it's not. Sorry.
Will send the patch once more.

>>   	kill_litter_super(sb);
>>   }
>>
>>
>
>


-- 
Best regards,
Stanislav Kinsbursky

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

* [PATCH v4] SUNRPC: fix races on PipeFS UMOUNT notifications
  2013-06-24  7:52 ` [PATCH v3 2/4] SUNRPC: fix races on PipeFS UMOUNT notifications Stanislav Kinsbursky
  2013-06-25 16:13   ` Myklebust, Trond
@ 2013-06-26  6:15   ` Stanislav Kinsbursky
  2013-06-28 11:51     ` William Dauchy
  1 sibling, 1 reply; 9+ messages in thread
From: Stanislav Kinsbursky @ 2013-06-26  6:15 UTC (permalink / raw)
  To: Trond.Myklebust; +Cc: linux-nfs, devel, linux-kernel, jlayton

CPU#0                                   CPU#1
-----------------------------           -----------------------------
rpc_kill_sb
sn->pipefs_sb = NULL                    rpc_release_client
(UMOUNT_EVENT)                          rpc_free_auth
rpc_pipefs_event
rpc_get_client_for_event
!atomic_inc_not_zero(cl_count)
<skip the client>
                                        atomic_inc(cl_count)
                                        rpc_free_client
                                        rpc_clnt_remove_pipedir
                                        <skip client dir removing>

To fix this, this patch does the following:

1) Calls RPC_PIPEFS_UMOUNT notification with sn->pipefs_sb_lock being held.
2) Removes SUNRPC client from the list AFTER pipes destroying.
3) Doesn't hold RPC client on notification: if client in the list, then it
can't be destroyed while sn->pipefs_sb_lock in hold by notification caller.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
Cc: stable@vger.kernel.org
---
 net/sunrpc/clnt.c     |    5 +----
 net/sunrpc/rpc_pipe.c |    2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index b827a4b..41f180c 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -236,8 +236,6 @@ static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
 			continue;
 		if (rpc_clnt_skip_event(clnt, event))
 			continue;
-		if (atomic_inc_not_zero(&clnt->cl_count) == 0)
-			continue;
 		spin_unlock(&sn->rpc_client_lock);
 		return clnt;
 	}
@@ -254,7 +252,6 @@ static int rpc_pipefs_event(struct notifier_block *nb, unsigned long event,
 
 	while ((clnt = rpc_get_client_for_event(sb->s_fs_info, event))) {
 		error = __rpc_pipefs_event(clnt, event, sb);
-		rpc_release_client(clnt);
 		if (error)
 			break;
 	}
@@ -641,8 +638,8 @@ rpc_free_client(struct rpc_clnt *clnt)
 			rcu_dereference(clnt->cl_xprt)->servername);
 	if (clnt->cl_parent != clnt)
 		rpc_release_client(clnt->cl_parent);
-	rpc_unregister_client(clnt);
 	rpc_clnt_remove_pipedir(clnt);
+	rpc_unregister_client(clnt);
 	rpc_free_iostats(clnt->cl_metrics);
 	kfree(clnt->cl_principal);
 	clnt->cl_metrics = NULL;
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index c512448..9b4ec5b 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -1165,12 +1165,12 @@ static void rpc_kill_sb(struct super_block *sb)
 		goto out;
 	}
 	sn->pipefs_sb = NULL;
-	mutex_unlock(&sn->pipefs_sb_lock);
 	dprintk("RPC:       sending pipefs UMOUNT notification for net %p%s\n",
 		net, NET_NAME(net));
 	blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
 					   RPC_PIPEFS_UMOUNT,
 					   sb);
+	mutex_unlock(&sn->pipefs_sb_lock);
 	put_net(net);
 out:
 	kill_litter_super(sb);


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

* Re: [PATCH v4] SUNRPC: fix races on PipeFS UMOUNT notifications
  2013-06-26  6:15   ` [PATCH v4] " Stanislav Kinsbursky
@ 2013-06-28 11:51     ` William Dauchy
  0 siblings, 0 replies; 9+ messages in thread
From: William Dauchy @ 2013-06-28 11:51 UTC (permalink / raw)
  To: Stanislav Kinsbursky
  Cc: Trond Myklebust, Linux NFS mailing list, devel, linux-kernel, jlayton

On Wed, Jun 26, 2013 at 8:15 AM, Stanislav Kinsbursky
<skinsbursky@parallels.com> wrote:
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index b827a4b..41f180c 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -236,8 +236,6 @@ static struct rpc_clnt *rpc_get_client_for_event(struct net *net, int event)
>                         continue;
>                 if (rpc_clnt_skip_event(clnt, event))
>                         continue;
> -               if (atomic_inc_not_zero(&clnt->cl_count) == 0)
> -                       continue;
>                 spin_unlock(&sn->rpc_client_lock);
>                 return clnt;
>         }
> @@ -254,7 +252,6 @@ static int rpc_pipefs_event(struct notifier_block *nb, unsigned long event,
>
>         while ((clnt = rpc_get_client_for_event(sb->s_fs_info, event))) {
>                 error = __rpc_pipefs_event(clnt, event, sb);
> -               rpc_release_client(clnt);
>                 if (error)
>                         break;
>         }
> @@ -641,8 +638,8 @@ rpc_free_client(struct rpc_clnt *clnt)
>                         rcu_dereference(clnt->cl_xprt)->servername);
>         if (clnt->cl_parent != clnt)
>                 rpc_release_client(clnt->cl_parent);
> -       rpc_unregister_client(clnt);
>         rpc_clnt_remove_pipedir(clnt);
> +       rpc_unregister_client(clnt);
>         rpc_free_iostats(clnt->cl_metrics);
>         kfree(clnt->cl_principal);
>         clnt->cl_metrics = NULL;
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index c512448..9b4ec5b 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -1165,12 +1165,12 @@ static void rpc_kill_sb(struct super_block *sb)
>                 goto out;
>         }
>         sn->pipefs_sb = NULL;
> -       mutex_unlock(&sn->pipefs_sb_lock);
>         dprintk("RPC:       sending pipefs UMOUNT notification for net %p%s\n",
>                 net, NET_NAME(net));
>         blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
>                                            RPC_PIPEFS_UMOUNT,
>                                            sb);
> +       mutex_unlock(&sn->pipefs_sb_lock);
>         put_net(net);
>  out:
>         kill_litter_super(sb);

are these patches validated?
any chance to see them in tree soon? I wasn't able to find them on
http://git.linux-nfs.org/?p=trondmy/linux-nfs.git

Thanks,
--
William

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

end of thread, other threads:[~2013-06-28 11:51 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-24  7:52 [PATCH v3 0/4] SUNRPC: PipeFS races fixes Stanislav Kinsbursky
2013-06-24  7:52 ` [PATCH v3 1/4] SUNRPC: fix races on PipeFS MOUNT notifications Stanislav Kinsbursky
2013-06-24  7:52 ` [PATCH v3 2/4] SUNRPC: fix races on PipeFS UMOUNT notifications Stanislav Kinsbursky
2013-06-25 16:13   ` Myklebust, Trond
2013-06-26  5:32     ` Stanislav Kinsbursky
2013-06-26  6:15   ` [PATCH v4] " Stanislav Kinsbursky
2013-06-28 11:51     ` William Dauchy
2013-06-24  7:52 ` [PATCH v3 3/4] SUNRPC: split client creation routine into setup and registration Stanislav Kinsbursky
2013-06-24  7:52 ` [PATCH v3 4/4] SUNRPC: PipeFS MOUNT notification optimization for dying clients Stanislav Kinsbursky

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).