Skip to contentSkip to conversation

SUNRPC: PipeFS races fixes

PATCH Superseded Stanislav Kinsbursky 2 participants 7 messages Jun 11, 2013 – Jun 18, 2013

Subsystems (3)
Networking NFS client NFS server

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Stanislav Kinsbursky

author
#0 lore raw

v2 cover SUNRPC: PipeFS races fixes

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

v2: Fixed few silly locking bugs.

The following series implements...

---

Stanislav Kinsbursky (4):
SUNRPC: split client creation routine into setup and registration
SUNRPC: fix races on PipeFS MOUNT notifications
SUNRPC: fix races on PipeFS UMOUNT notifications
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(-)
author
#1 lore raw

PATCH v2 1/4 SUNRPC: split client creation routine into setup and registration

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 <[email protected]>
---
 net/sunrpc/clnt.c |   48 ++++++++++++++++++++++++++++++++----------------
 1 files changed, 32 insertions(+), 16 deletions(-)
net/sunrpc/clnt.c +32 -16
index 5a750b9..4bf2409 100644
@@ -289,12 +289,39 @@ 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;
+	int err = 0;
+
+	err = rpc_setup_pipedir(clnt, program->pipe_dir_name);
+	if (err < 0)
+		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:
+	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;
 
 	/* sanity check the name before trying to print it */

@@ -354,25 +381,14 @@ 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;
-
-	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);
+
+	err = rpc_client_register(args, clnt);
+	if (err)
+		goto out_no_path;
 	return clnt;
 
-out_no_auth:
-	rpc_clnt_remove_pipedir(clnt);
 out_no_path:
 	kfree(clnt->cl_principal);
 out_no_principal:
author
#2 lore raw

PATCH v2 2/4 SUNRPC: fix races on PipeFS MOUNT notifications

In reply to Stanislav Kinsbursky

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 <[email protected]>
Cc [email protected]
---
 net/sunrpc/clnt.c     |   24 +++++++++++++-----------
 net/sunrpc/rpc_pipe.c |    3 +++
 2 files changed, 16 insertions(+), 11 deletions(-)
net/sunrpc/clnt.c +13 -11
index 4bf2409..48888ee 100644
@@ -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;

@@ -294,11 +289,16 @@ static int rpc_client_register(const struct rpc_create_args *args,
 {
 	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;
 
-	err = rpc_setup_pipedir(clnt, program->pipe_dir_name);
-	if (err < 0)
-		goto out;
+	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)) {

@@ -310,10 +310,12 @@ static int rpc_client_register(const struct rpc_create_args *args,
 
 	rpc_register_client(clnt);
 out:
+	if (pipefs_sb)
+		rpc_put_sb_net(net);
 	return err;
 
 err_auth:
-	rpc_clnt_remove_pipedir(clnt);
+	__rpc_clnt_remove_pipedir(clnt);
 	goto out;
 }
net/sunrpc/rpc_pipe.c +3 -0
index e7ce4b3..c512448 100644
@@ -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;
 }
#3 lore raw

Re: [PATCH v2 2/4] SUNRPC: fix races on PipeFS MOUNT notifications

Quoted 15 lines · Stanislav Kinsbursky

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)

Quoted 7 lines

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 <[email protected]>
Cc [email protected]

Hi Stanislav,

This isn't going to apply to the stable kernels without the cleanup patch. Could you please reorganise this patch series so that the cleanup comes last.

Thanks,
Trond

Signature · 6 lines
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com
author
#4 lore raw

Re: [PATCH v2 2/4] SUNRPC: fix races on PipeFS MOUNT notifications

17.06.2013 22:20, Myklebust, Trond пишет:

Quoted 24 lines · Stanislav Kinsbursky
Quoted 15 lines

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)

Quoted 7 lines

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 <[email protected]>
Cc [email protected]
Quoted 10 lines

Hi Stanislav,

This isn't going to apply to the stable kernels without the cleanup patch. Could you please reorganise this patch series so that the cleanup comes last.

Thanks,
Trond

Hello, Trond.
Sure, will do.

Signature · 2 lines
Best regards,
Stanislav Kinsbursky
author
#5 lore raw

PATCH v2 3/4 SUNRPC: fix races on PipeFS UMOUNT notifications

In reply to Stanislav Kinsbursky

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 <[email protected]>
Cc [email protected]
---
 net/sunrpc/clnt.c     |    5 +----
 net/sunrpc/rpc_pipe.c |    2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)
net/sunrpc/clnt.c +1 -4
index 48888ee..b4f1711 100644
@@ -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;
 	}

@@ -655,8 +652,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;
net/sunrpc/rpc_pipe.c +1 -1
index c512448..efca2f7 100644
@@ -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);
 }
author
#6 lore raw

PATCH v2 4/4 SUNRPC: PipeFS MOUNT notification optimization for dying clients

In reply to Stanislav Kinsbursky

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 <[email protected]>
Cc [email protected]
---
 net/sunrpc/clnt.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
net/sunrpc/clnt.c +2 -0
index b4f1711..f0339ae 100644
@@ -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;
 }