* [PATCH] SUNRPC: Convert from atomic_t to refcount_t on rpc_clnt->cl_count
@ 2021-07-17 10:18 Xiyu Yang
2021-07-17 14:43 ` Trond Myklebust
0 siblings, 1 reply; 4+ messages in thread
From: Xiyu Yang @ 2021-07-17 10:18 UTC (permalink / raw)
To: J. Bruce Fields, Chuck Lever, Trond Myklebust, Anna Schumaker,
David S. Miller, Jakub Kicinski, Olga Kornievskaia, Xiyu Yang,
Xin Tan, linux-nfs, linux-kernel, netdev
Cc: yuanxzhang
refcount_t type and corresponding API can protect refcounters from
accidental underflow and overflow and further use-after-free situations.
Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
---
include/linux/sunrpc/clnt.h | 3 ++-
net/sunrpc/auth_gss/gss_rpc_upcall.c | 2 +-
net/sunrpc/clnt.c | 14 +++++++-------
net/sunrpc/debugfs.c | 2 +-
net/sunrpc/rpc_pipe.c | 2 +-
5 files changed, 12 insertions(+), 11 deletions(-)
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 {
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] SUNRPC: Convert from atomic_t to refcount_t on rpc_clnt->cl_count
2021-07-17 10:18 [PATCH] SUNRPC: Convert from atomic_t to refcount_t on rpc_clnt->cl_count Xiyu Yang
@ 2021-07-17 14:43 ` Trond Myklebust
2021-07-17 16:32 ` Xiyu Yang
2021-07-18 10:47 ` Leon Romanovsky
0 siblings, 2 replies; 4+ messages in thread
From: Trond Myklebust @ 2021-07-17 14:43 UTC (permalink / raw)
To: tanxin.ctf, xiyuyang19, davem, chuck.lever, linux-kernel, kolga,
kuba, netdev, bfields, anna.schumaker, linux-nfs
Cc: yuanxzhang
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Re: [PATCH] SUNRPC: Convert from atomic_t to refcount_t on rpc_clnt->cl_count
2021-07-17 14:43 ` Trond Myklebust
@ 2021-07-17 16:32 ` Xiyu Yang
2021-07-18 10:47 ` Leon Romanovsky
1 sibling, 0 replies; 4+ messages in thread
From: Xiyu Yang @ 2021-07-17 16:32 UTC (permalink / raw)
To: Trond Myklebust
Cc: tanxin.ctf, davem, chuck.lever, linux-kernel, kolga, kuba,
netdev, bfields, anna.schumaker, linux-nfs, yuanxzhang
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] SUNRPC: Convert from atomic_t to refcount_t on rpc_clnt->cl_count
2021-07-17 14:43 ` Trond Myklebust
2021-07-17 16:32 ` Xiyu Yang
@ 2021-07-18 10:47 ` Leon Romanovsky
1 sibling, 0 replies; 4+ messages in thread
From: Leon Romanovsky @ 2021-07-18 10:47 UTC (permalink / raw)
To: Trond Myklebust
Cc: tanxin.ctf, xiyuyang19, davem, chuck.lever, linux-kernel, kolga,
kuba, netdev, bfields, anna.schumaker, linux-nfs, yuanxzhang
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-07-18 10:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-17 10:18 [PATCH] SUNRPC: Convert from atomic_t to refcount_t on rpc_clnt->cl_count Xiyu Yang
2021-07-17 14:43 ` Trond Myklebust
2021-07-17 16:32 ` Xiyu Yang
2021-07-18 10:47 ` Leon Romanovsky
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).