Message ID | 1626517112-42831-1-git-send-email-xiyuyang19@fudan.edu.cn (mailing list archive) |
---|---|
State | Awaiting Upstream |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | SUNRPC: Convert from atomic_t to refcount_t on rpc_clnt->cl_count | expand |
Context | Check | Description |
---|---|---|
netdev/cover_letter | success | Link |
netdev/fixes_present | success | Link |
netdev/patch_count | success | Link |
netdev/tree_selection | success | Guessed tree name to be net-next |
netdev/subject_prefix | warning | Target tree name not specified in the subject |
netdev/cc_maintainers | success | CCed 11 of 11 maintainers |
netdev/source_inline | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Link |
netdev/module_param | success | Was 0 now: 0 |
netdev/build_32bit | success | Errors and warnings before: 32 this patch: 32 |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/verify_fixes | success | Link |
netdev/checkpatch | warning | CHECK: Comparison to NULL could be written "clnt" |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 31 this patch: 31 |
netdev/header_inline | success | Link |
On Sat, 2021-07-17 at 18:18 +0800, Xiyu Yang wrote: > refcount_t type and corresponding API can protect refcounters from > accidental underflow and overflow and further use-after-free > situations. > Have you tested this patch? As far as I remember, the reason why we never converted is that refcount_inc() gets upset and WARNs when you bump a zero refcount, like we do very much on purpose in rpc_free_auth(). Is that no longer the case?
Sorry, I'm not sure why you need to bump a zero refcount in a normal situation. But maybe we can use refcount_inc_not_zero() API in rpc_free_auth() instead? > -----Original Messages----- > From: "Trond Myklebust" <trondmy@hammerspace.com> > Sent Time: 2021-07-17 22:43:26 (Saturday) > To: "tanxin.ctf@gmail.com" <tanxin.ctf@gmail.com>, "xiyuyang19@fudan.edu.cn" <xiyuyang19@fudan.edu.cn>, "davem@davemloft.net" <davem@davemloft.net>, "chuck.lever@oracle.com" <chuck.lever@oracle.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "kolga@netapp.com" <kolga@netapp.com>, "kuba@kernel.org" <kuba@kernel.org>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "bfields@fieldses.org" <bfields@fieldses.org>, "anna.schumaker@netapp.com" <anna.schumaker@netapp.com>, "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org> > Cc: "yuanxzhang@fudan.edu.cn" <yuanxzhang@fudan.edu.cn> > Subject: Re: [PATCH] SUNRPC: Convert from atomic_t to refcount_t on rpc_clnt->cl_count > > On Sat, 2021-07-17 at 18:18 +0800, Xiyu Yang wrote: > > refcount_t type and corresponding API can protect refcounters from > > accidental underflow and overflow and further use-after-free > > situations. > > > > Have you tested this patch? As far as I remember, the reason why we > never converted is that refcount_inc() gets upset and WARNs when you > bump a zero refcount, like we do very much on purpose in > rpc_free_auth(). Is that no longer the case? > > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
On Sat, Jul 17, 2021 at 02:43:26PM +0000, Trond Myklebust wrote: > On Sat, 2021-07-17 at 18:18 +0800, Xiyu Yang wrote: > > refcount_t type and corresponding API can protect refcounters from > > accidental underflow and overflow and further use-after-free > > situations. > > > > Have you tested this patch? As far as I remember, the reason why we > never converted is that refcount_inc() gets upset and WARNs when you > bump a zero refcount, like we do very much on purpose in > rpc_free_auth(). Is that no longer the case? It is still the case, they sent gazillion conversion patches with same mistake. Thanks > > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h index 8b5d5c97553e..61f725b9f865 100644 --- a/include/linux/sunrpc/clnt.h +++ b/include/linux/sunrpc/clnt.h @@ -10,6 +10,7 @@ #ifndef _LINUX_SUNRPC_CLNT_H #define _LINUX_SUNRPC_CLNT_H +#include <linux/refcount.h> #include <linux/types.h> #include <linux/socket.h> #include <linux/in.h> @@ -35,7 +36,7 @@ struct rpc_sysfs_client; * The high-level client handle */ struct rpc_clnt { - atomic_t cl_count; /* Number of references */ + refcount_t cl_count; /* Number of references */ unsigned int cl_clid; /* client id */ struct list_head cl_clients; /* Global list of clients */ struct list_head cl_tasks; /* List of tasks */ diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.c b/net/sunrpc/auth_gss/gss_rpc_upcall.c index d1c003a25b0f..61c276bddaf2 100644 --- a/net/sunrpc/auth_gss/gss_rpc_upcall.c +++ b/net/sunrpc/auth_gss/gss_rpc_upcall.c @@ -160,7 +160,7 @@ static struct rpc_clnt *get_gssp_clnt(struct sunrpc_net *sn) mutex_lock(&sn->gssp_lock); clnt = sn->gssp_clnt; if (clnt) - atomic_inc(&clnt->cl_count); + refcount_inc(&clnt->cl_count); mutex_unlock(&sn->gssp_lock); return clnt; } diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c index 8b4de70e8ead..d6b64622bd04 100644 --- a/net/sunrpc/clnt.c +++ b/net/sunrpc/clnt.c @@ -167,7 +167,7 @@ static int rpc_clnt_skip_event(struct rpc_clnt *clnt, unsigned long event) case RPC_PIPEFS_MOUNT: if (clnt->cl_pipedir_objects.pdh_dentry != NULL) return 1; - if (atomic_read(&clnt->cl_count) == 0) + if (refcount_read(&clnt->cl_count) == 0) return 1; break; case RPC_PIPEFS_UMOUNT: @@ -419,7 +419,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, clnt->cl_rtt = &clnt->cl_rtt_default; rpc_init_rtt(&clnt->cl_rtt_default, clnt->cl_timeout->to_initval); - atomic_set(&clnt->cl_count, 1); + refcount_set(&clnt->cl_count, 1); if (nodename == NULL) nodename = utsname()->nodename; @@ -431,7 +431,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args, if (err) goto out_no_path; if (parent) - atomic_inc(&parent->cl_count); + refcount_inc(&parent->cl_count); trace_rpc_clnt_new(clnt, xprt, program->name, args->servername); return clnt; @@ -926,10 +926,10 @@ rpc_free_auth(struct rpc_clnt *clnt) * release remaining GSS contexts. This mechanism ensures * that it can do so safely. */ - atomic_inc(&clnt->cl_count); + refcount_inc(&clnt->cl_count); rpcauth_release(clnt->cl_auth); clnt->cl_auth = NULL; - if (atomic_dec_and_test(&clnt->cl_count)) + if (refcount_dec_and_test(&clnt->cl_count)) return rpc_free_client(clnt); return NULL; } @@ -943,7 +943,7 @@ rpc_release_client(struct rpc_clnt *clnt) do { if (list_empty(&clnt->cl_tasks)) wake_up(&destroy_wait); - if (!atomic_dec_and_test(&clnt->cl_count)) + if (!refcount_dec_and_test(&clnt->cl_count)) break; clnt = rpc_free_auth(clnt); } while (clnt != NULL); @@ -1082,7 +1082,7 @@ void rpc_task_set_client(struct rpc_task *task, struct rpc_clnt *clnt) if (clnt != NULL) { rpc_task_set_transport(task, clnt); task->tk_client = clnt; - atomic_inc(&clnt->cl_count); + refcount_inc(&clnt->cl_count); if (clnt->cl_softrtry) task->tk_flags |= RPC_TASK_SOFT; if (clnt->cl_softerr) diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c index 56029e3af6ff..79995eb95927 100644 --- a/net/sunrpc/debugfs.c +++ b/net/sunrpc/debugfs.c @@ -90,7 +90,7 @@ static int tasks_open(struct inode *inode, struct file *filp) struct seq_file *seq = filp->private_data; struct rpc_clnt *clnt = seq->private = inode->i_private; - if (!atomic_inc_not_zero(&clnt->cl_count)) { + if (!refcount_inc_not_zero(&clnt->cl_count)) { seq_release(inode, filp); ret = -EINVAL; } diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c index 09c000d490a1..ee5336d73fdd 100644 --- a/net/sunrpc/rpc_pipe.c +++ b/net/sunrpc/rpc_pipe.c @@ -423,7 +423,7 @@ rpc_info_open(struct inode *inode, struct file *file) spin_lock(&file->f_path.dentry->d_lock); if (!d_unhashed(file->f_path.dentry)) clnt = RPC_I(inode)->private; - if (clnt != NULL && atomic_inc_not_zero(&clnt->cl_count)) { + if (clnt != NULL && refcount_inc_not_zero(&clnt->cl_count)) { spin_unlock(&file->f_path.dentry->d_lock); m->private = clnt; } else {