Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Benjamin Coddington" <bcodding@redhat.com>
To:     "Trond Myklebust" <trondmy@hammerspace.com>
Cc:     xiyuyang19@fudan.edu.cn, linux-nfs@vger.kernel.org
Subject: Re: [PATCH] SUNRPC: Convert rpc_client refcount to use refcount_t
Date:   Mon, 19 Jul 2021 08:27:09 -0400
Message-ID: <2A9784CC-8774-4633-A0E3-B90CDFC5CF7E@redhat.com>
In-Reply-To: <190d0dec631a2219c4c16a41f7c17e914f625082.camel@hammerspace.com>
References: <20210717172052.232420-1-trondmy@kernel.org>
 <6AF75462-495E-4B63-9A3E-C9639C45C1F2@redhat.com>
 <190d0dec631a2219c4c16a41f7c17e914f625082.camel@hammerspace.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk

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, trondmy@kernel.org 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 <bcodding@redhat.com>

Ben