[PATCH] SUNRPC: Convert from atomic_t to refcount_t on rpc_clnt->cl_count

Subsystems: kernel nfsd, sunrpc, and lockd servers, networking [general], nfs, sunrpc, and lockd clients, the rest

STALE1878d

4 messages, 3 authors, 2021-07-18 · open the first message on its own page

[PATCH] SUNRPC: Convert from atomic_t to refcount_t on rpc_clnt->cl_count

From: Xiyu Yang <hidden>
Date: 2021-07-17 10:20:17

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 <redacted>
Signed-off-by: Xin Tan <redacted>
---
 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

Re: [PATCH] SUNRPC: Convert from atomic_t to refcount_t on rpc_clnt->cl_count

From: Trond Myklebust <hidden>
Date: 2021-07-17 14:43:37

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

Re: Re: [PATCH] SUNRPC: Convert from atomic_t to refcount_t on rpc_clnt->cl_count

From: Xiyu Yang <hidden>
Date: 2021-07-17 16:33:32

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" <redacted>
Sent Time: 2021-07-17 22:43:26 (Saturday)
To: "tanxin.ctf@gmail.com" <redacted>, "xiyuyang19@fudan.edu.cn" <redacted>, "davem@davemloft.net" <davem@davemloft.net>, "chuck.lever@oracle.com" <redacted>, "linux-kernel@vger.kernel.org" <redacted>, "kolga@netapp.com" <redacted>, "kuba@kernel.org" <kuba@kernel.org>, "netdev@vger.kernel.org" <redacted>, "bfields@fieldses.org" <redacted>, "anna.schumaker@netapp.com" <redacted>, "linux-nfs@vger.kernel.org" <redacted>
Cc: "yuanxzhang@fudan.edu.cn" <redacted>
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:
quoted
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




Re: [PATCH] SUNRPC: Convert from atomic_t to refcount_t on rpc_clnt->cl_count

From: Leon Romanovsky <leon@kernel.org>
Date: 2021-07-18 10:47:15

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:
quoted
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help