Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [PATCH] nfsd: fix race between cache_clean and cache_purge
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <5372f88d-efb7-25a3-789f-53bfa7bb6f26@linux.alibaba.com>
Date:   Wed, 25 Mar 2020 10:24:54 -0400
Cc:     Trond Myklebust <trondmy@hammerspace.com>,
        Bruce Fields <bfields@fieldses.org>,
        "neilb@suse.com" <neilb@suse.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <6AA6A103-6A9C-40CF-A625-B7A29B0D6BA0@oracle.com>
References: <5eed50660eb13326b0fbf537fb58481ea53c1acb.1585043174.git.wuyihao@linux.alibaba.com>
 <8B2BC124-6911-46C9-9B01-A237AC149F0A@oracle.com>
 <13c45bdcb67d689bfcb4f4b720b631e56c662f2b.camel@hammerspace.com>
 <CCFA2CA8-150C-432C-B939-9085B791FE74@oracle.com>
 <5372f88d-efb7-25a3-789f-53bfa7bb6f26@linux.alibaba.com>
To:     Yihao Wu <wuyihao@linux.alibaba.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Mar 25, 2020, at 2:37 AM, Yihao Wu <wuyihao@linux.alibaba.com> =
wrote:
>=20
> On 2020/3/25 1:46 AM, Chuck Lever wrote:
>>>>> ---
>>>>> net/sunrpc/cache.c | 3 +++
>>>>> 1 file changed, 3 insertions(+)
>>>>>=20
>>>>> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
>>>>> index bd843a81afa0..3e523eefc47f 100644
>>>>> --- a/net/sunrpc/cache.c
>>>>> +++ b/net/sunrpc/cache.c
>>>>> @@ -524,9 +524,11 @@ void cache_purge(struct cache_detail *detail)
>>>>> 	struct hlist_node *tmp =3D NULL;
>>>>> 	int i =3D 0;
>>>>>=20
>>>>> +	spin_lock(&cache_list_lock);
>>>>> 	spin_lock(&detail->hash_lock);
>>>>> 	if (!detail->entries) {
>>>>> 		spin_unlock(&detail->hash_lock);
>>>>> +		spin_unlock(&cache_list_lock);
>>>>> 		return;
>>>>> 	}
>>>>>=20
>>>>> @@ -541,6 +543,7 @@ void cache_purge(struct cache_detail *detail)
>>>>> 		}
>>>>> 	}
>>>>> 	spin_unlock(&detail->hash_lock);
>>>>> +	spin_unlock(&cache_list_lock);
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(cache_purge);
>>>=20
>>> Hmm... Shouldn't this patch be dropping cache_list_lock() when we =
call
>>> sunrpc_end_cache_remove_entry()? The latter does call both
>>> cache_revisit_request() and cache_put(), and while they do not
>>> explicitly call anything that holds cache_list_lock, some of those =
cd-
>>>> cache_put callbacks do look as if there is potential for deadlock.
>> I see svc_export_put calling dput, eventually, which might_sleep().
>=20
> Wow that's a little strange. If svc_export_put->dput might_sleep, why =
can we
> spin_lock(&detail->hash_lock); in cache_purge in the first place?
>=20
> And I agree with Trond those cd->cache_put callbacks are dangerous. I =
will look
> into them today.
>=20
> But if we dropping cache_list_lock when we call =
sunrpc_end_cache_remove_entry,
> cache_put is not protected, and this patch won't work anymore, right?

IMHO Neil's proposed solution seems pretty safe, and follows a =
well-understood
pattern.

It would be nice (but not 100% necessary) if the race you found was =
spelled out
in the patch description.

Thanks!


--
Chuck Lever