SUNRPC: PipeFS races fixes
Subsystems (3)
v3 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
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(-)
PATCH v3 1/4 SUNRPC: fix races on PipeFS MOUNT notifications
Below are races, when RPC client can be created without PiepFS dentries
CPU#0 CPU#1
----------------------------- -----------------------------rpc_new_client rpc_fill_superrpc_setup_pipedirmutex_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.
--- net/sunrpc/clnt.c | 26 +++++++++++++++----------- net/sunrpc/rpc_pipe.c | 3 +++ 2 files changed, 18 insertions(+), 11 deletions(-)
net/sunrpc/clnt.c +15 -11
@@ -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, strustruct 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, struatomic_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);
net/sunrpc/rpc_pipe.c +3 -0
@@ -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; }
PATCH v3 2/4 SUNRPC: fix races on PipeFS UMOUNT notifications
In reply to Stanislav KinsburskyCPU#0 CPU#1
----------------------------- -----------------------------rpc_kill_sbsn->pipefs_sb = NULL rpc_release_client
(UMOUNT_EVENT) rpc_free_authrpc_pipefs_eventrpc_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.
--- net/sunrpc/clnt.c | 5 +---- net/sunrpc/rpc_pipe.c | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-)
net/sunrpc/clnt.c +1 -4
@@ -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 ;
net/sunrpc/rpc_pipe.c +1 -1
@@ -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); }
Re: [PATCH v3 2/4] SUNRPC: fix races on PipeFS UMOUNT notifications
Quoted 27 lines · Stanislav Kinsbursky
CPU#0 CPU#1
----------------------------- -----------------------------rpc_kill_sbsn->pipefs_sb= NULLrpc_release_client
(UMOUNT_EVENT)rpc_free_authrpc_pipefs_eventrpc_get_client_for_event
!atomic_inc_not_zero(cl_count)
<skip the client>
atomic_inc(cl_count)rpc_free_clientrpc_clnt_remove_pipedir<skip client dir removing>To fix this, this patch does the following:
1) Calls
RPC_PIPEFS_UMOUNTnotification withsn->pipefs_sb_lockbeing 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 whilesn->pipefs_sb_lockin hold by notification caller.Signed-off-by Stanislav Kinsbursky <[email protected]>--- net/sunrpc/clnt.c | 5 +---- net/sunrpc/rpc_pipe.c | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-)
<snip>
Quoted 13 lines
net/sunrpc/rpc_pipe.c +0
-1@@ -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,
net/sunrpc/rpc_pipe.c static void rpc_kill_sb(struct super_block *sb) · line 1175
Quoted 4 lines
net/sunrpc/rpc_pipe.c +1
-0@@ -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()?
Quoted 4 lines
net/sunrpc/rpc_pipe.c +0
-0kill_litter_super (sb); }
Signature · 6 lines
Trond Myklebust Linux NFS client maintainer NetApp [email protected] www.netapp.com
Re: [PATCH v3 2/4] SUNRPC: fix races on PipeFS UMOUNT notifications
25.06.2013 20:13, Myklebust, Trond пишет:
Quoted 42 lines · Stanislav Kinsbursky
Quoted 27 lines
CPU#0 CPU#1
----------------------------- -----------------------------rpc_kill_sbsn->pipefs_sb= NULLrpc_release_client
(UMOUNT_EVENT)rpc_free_authrpc_pipefs_eventrpc_get_client_for_event
!atomic_inc_not_zero(cl_count)
<skip the client>
atomic_inc(cl_count)rpc_free_clientrpc_clnt_remove_pipedir<skip client dir removing>To fix this, this patch does the following:
1) Calls
RPC_PIPEFS_UMOUNTnotification withsn->pipefs_sb_lockbeing 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 whilesn->pipefs_sb_lockin hold by notification caller.Signed-off-by Stanislav Kinsbursky <[email protected]>--- net/sunrpc/clnt.c | 5 +---- net/sunrpc/rpc_pipe.c | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-)<snip>
Quoted 11 lines
net/sunrpc/rpc_pipe.c +0
-1@@ -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));
Quoted 9 lines
net/sunrpc/rpc_pipe.c static void rpc_kill_sb(struct super_block *sb) · line 1175
Quoted 6 lines
net/sunrpc/rpc_pipe.c +1
-0blocking_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.
Quoted 6 lines
Quoted 4 lines
net/sunrpc/rpc_pipe.c +0
-0kill_litter_super (sb); }
Signature · 2 lines
Best regards, Stanislav Kinsbursky
PATCH v4 1/1 SUNRPC: fix races on PipeFS UMOUNT notifications
In reply to Stanislav KinsburskyCPU#0 CPU#1
----------------------------- -----------------------------rpc_kill_sbsn->pipefs_sb = NULL rpc_release_client
(UMOUNT_EVENT) rpc_free_authrpc_pipefs_eventrpc_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.
--- net/sunrpc/clnt.c | 5 +---- net/sunrpc/rpc_pipe.c | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-)
net/sunrpc/clnt.c +1 -4
@@ -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 ;
net/sunrpc/rpc_pipe.c +1 -1
@@ -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);
Re: [PATCH v4] SUNRPC: fix races on PipeFS UMOUNT notifications
Quoted 39 lines · Stanislav Kinsbursky
net/sunrpc/clnt.c +1
-4@@ -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 ;net/sunrpc/rpc_pipe.c +0
-0@@ -1165,12 +1165,12 @@ static void rpc_kill_sb(struct super_block *sb) goto out; } sn->pipefs_sb =NULL ;
net/sunrpc/rpc_pipe.c static void rpc_kill_sb(struct super_block *sb) · line 1176
Quoted 10 lines
net/sunrpc/rpc_pipe.c +1
-1-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
PATCH v3 3/4 SUNRPC: split client creation routine into setup and registration
In reply to Stanislav KinsburskyThis 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.
--- net/sunrpc/clnt.c | 64 ++++++++++++++++++++++++++++++++--------------------- 1 files changed, 39 insertions(+), 25 deletions(-)
net/sunrpc/clnt.c +39 -25
@@ -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, struatomic_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);
PATCH v3 4/4 SUNRPC: PipeFS MOUNT notification optimization for dying clients
In reply to Stanislav KinsburskyNot 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.
--- net/sunrpc/clnt.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
net/sunrpc/clnt.c +2 -0
@@ -177,6 +177,8 @@ static inline int rpc_clnt_skip_event(struct rpc_clnt *clnt, unsigned long eventif (((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 ; }