From: Trond Myklebust <[email protected]>
There are now tools in the refcount library that allow us to convert the
client shutdown code.
Reported-by: Xiyu Yang <[email protected]>
Signed-off-by: Trond Myklebust <[email protected]>
---
include/linux/sunrpc/clnt.h | 3 ++-
net/sunrpc/auth_gss/gss_rpc_upcall.c | 2 +-
net/sunrpc/clnt.c | 22 ++++++++++------------
net/sunrpc/debugfs.c | 2 +-
net/sunrpc/rpc_pipe.c | 2 +-
5 files changed, 15 insertions(+), 16 deletions(-)
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 8b5d5c97553e..b2edd5fc2f0c 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -14,6 +14,7 @@
#include <linux/socket.h>
#include <linux/in.h>
#include <linux/in6.h>
+#include <linux/refcount.h>
#include <linux/sunrpc/msg_prot.h>
#include <linux/sunrpc/sched.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..9952fc0b1de6 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;
@@ -918,18 +918,16 @@ rpc_free_client(struct rpc_clnt *clnt)
static struct rpc_clnt *
rpc_free_auth(struct rpc_clnt *clnt)
{
- if (clnt->cl_auth == NULL)
- return rpc_free_client(clnt);
-
/*
* Note: RPCSEC_GSS may need to send NULL RPC calls in order to
* release remaining GSS contexts. This mechanism ensures
* that it can do so safely.
*/
- atomic_inc(&clnt->cl_count);
- rpcauth_release(clnt->cl_auth);
- clnt->cl_auth = NULL;
- if (atomic_dec_and_test(&clnt->cl_count))
+ if (clnt->cl_auth != NULL) {
+ rpcauth_release(clnt->cl_auth);
+ clnt->cl_auth = NULL;
+ }
+ if (refcount_dec_and_test(&clnt->cl_count))
return rpc_free_client(clnt);
return NULL;
}
@@ -943,7 +941,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_not_one(&clnt->cl_count))
break;
clnt = rpc_free_auth(clnt);
} while (clnt != NULL);
@@ -1082,7 +1080,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.31.1
Hi Trond,
On 17 Jul 2021, at 13:20, [email protected] wrote:
> @@ -943,7 +941,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_not_one(&clnt->cl_count))
I guess we're not worried about extra calls racing into rpc_free_auth?
.. hmm, it looks like current code can do that already since we're bumping the
ref up again. Seems like we could end up in rpcauth_release twice with
an underflow on au_count.
Ben
On Mon, 2021-07-19 at 08:01 -0400, Benjamin Coddington wrote:
> Hi Trond,
>
> On 17 Jul 2021, at 13:20, [email protected] wrote:
>
> > @@ -943,7 +941,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_not_one(&clnt->cl_count))
>
> I guess we're not worried about extra calls racing into
> rpc_free_auth?
The refcount would normally be going to zero in the above case. If
anything outside the RPC code itself tries to bump the counter then
that is a very clear cut case of use-after-free.
>
> .. hmm, it looks like current code can do that already since we're
> bumping the
> ref up again. Seems like we could end up in rpcauth_release twice
> with
> an underflow on au_count.
>
As I said, only if there is a use-after-free bug somewhere else.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On 19 Jul 2021, at 8:07, Trond Myklebust wrote:
> On Mon, 2021-07-19 at 08:01 -0400, Benjamin Coddington wrote:
>> Hi Trond,
>>
>> On 17 Jul 2021, at 13:20, [email protected] wrote:
>>
>>> @@ -943,7 +941,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_not_one(&clnt->cl_count))
>>
>> I guess we're not worried about extra calls racing into
>> rpc_free_auth?
>
> The refcount would normally be going to zero in the above case. If
> anything outside the RPC code itself tries to bump the counter then
> that is a very clear cut case of use-after-free.
I am thinking about users of rpc_release_client() calling it multiple times,
but perhaps that's not something that happens. This is a different issue
that's not added by your patch, I was noticing it.
Reviewed-by: Benjamin Coddington <[email protected]>
Ben