Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH RFC] NFSD: Fix possible sleep during
 nfsd4_release_lockowner()
Thread-Topic: [PATCH RFC] NFSD: Fix possible sleep during
 nfsd4_release_lockowner()
Thread-Index: AQHYbfIMzaj7dJY/skSZB97V0kxLh60seYcAgAAWLQCAAAdIAIAABFwA
Date:   Mon, 23 May 2022 15:41:51 +0000
Message-ID: <1A37E2B5-8113-48D6-AF7C-5381F364D99E@oracle.com>
References: <165323344948.2381.7808135229977810927.stgit@bazille.1015granger.net>
 <fe3f9ece807e1433631ee3e0bd6b78238305cb87.camel@kernel.org>
 <510282CB-38D3-438A-AF8A-9AC2519FCEF7@oracle.com>
 <c3d053dc36dd5e7dee1267f1c7107bbf911e4d53.camel@kernel.org>
In-Reply-To: <c3d053dc36dd5e7dee1267f1c7107bbf911e4d53.camel@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Z6H0aicMpbWWq2Z0UO0IFQ6RZzSYA3/0Cl5GDnmKwcDgcQhGCCodg9fCnfYh?=
 =?us-ascii?Q?J6nKjVfAQw/9tQSXJAhleG+ZrTMoxKDD146SeckSxGnlpvPSuQq1JGMf+xMv?=
 =?us-ascii?Q?nYSTEq8aQQhDMUZ1lr1wB8XjsZCLZDJRhXWUnbQWNcty9W7xDXYxFuO15SYL?=
 =?us-ascii?Q?4eFX74fu3Ic28wc73yJhBjZbOFnX5nwzMBWgd0PpcJ2h3KP2wF9j6NtI313Z?=
 =?us-ascii?Q?K1EMvVoHBb5tOq2m3abEBfwjpRBgyTK4tlTuN4xezW6R6CAnD6W4sTRQ/azh?=
 =?us-ascii?Q?7JrJJqVQ62tydEXTxNt5bLRHmJcSa63Vg8eZk9e1rt+vANjYrJe95xX0NZAW?=
 =?us-ascii?Q?esUz8XWQYJ1/cPH3whdvDga+XqlkDXUgXI5gLGVfVQNb57k31jH7BMPZv00n?=
 =?us-ascii?Q?u1dU8aEsE8y3kpVyD7BkecGXrmivmoNQ3psgf449gE78GgaAKZYNOtaXhdY4?=
 =?us-ascii?Q?lOR2ddiGh0ZFRQpgDK0mplWymJ3GifMLonANT3S7aULk+FvfpC3+ZDbqGTTm?=
 =?us-ascii?Q?4cIzmG1Fq60893oq7U9oOJvBEtk0Et+qKtUySWJLls0ZFu89NgU+iWqYEl8j?=
 =?us-ascii?Q?jgQj2CBTE7ca9pziQOAOZf467vnnGfy1jQgOv0rxSG8FTbuO2EzInLP33vv9?=
 =?us-ascii?Q?VQ3rIi+ut+cVvTNJw0rKiiKSWEksIvHQmCy0mDywJPSwmLMrgl3nH9fEoS2f?=
 =?us-ascii?Q?KILad5DPNKvrnNn/SET1s3jJzJPjMI9o/JVD3dozD3H5vH89cg9HRekknk/b?=
 =?us-ascii?Q?1GBgA6olG8I8GI6ekoUevliMlWrRiDqo1Xu01D5ZCnrGuAXfCgmP4dGdVgQR?=
 =?us-ascii?Q?Aj9jkp2Z9Qzgk74KtbQdXWpa0RdxmdEdcHVsZSAeXl+Fsh3mfe1VxY69MO/K?=
 =?us-ascii?Q?xbQhRtvzcuBKHOJbMWXfYaAsKvynBocrVteSrpJrHbsiTI8tBhcYdHMBnyoO?=
 =?us-ascii?Q?hETGX7fxN0AdxQ+6hEEaPJ3wXBciGN2T4ZausD3bRIons46R7c2W0q1x4O7y?=
 =?us-ascii?Q?iEQ0S0ZxI45wQFLMbXv8EMxCmUbZtghLR7QLawzad+cbL26bsyqjKiSIJiXr?=
 =?us-ascii?Q?bZS/ngGgDIei5ptUVKN2BMcuuJQH1T/qzbZf6CkeBse4J8Kemw8ieXN9UV47?=
 =?us-ascii?Q?UYm4Jcr7SK3A/K9l3ZtrN/u19KqG6clpZbLow8A7F7fuU8AsWlPyTjWmhQVh?=
 =?us-ascii?Q?H3qMRkJ+Mi7LYZK2NeyeYfwmjHpTOvIoHzzJmoQSXy88XHJq0glS3EYT978q?=
 =?us-ascii?Q?VelMmEcYK4CPoHcgcvGXO7UOOO2o7nywBYiLnvBT3mK/zsmqYm8jMIFT+pRm?=
 =?us-ascii?Q?MUsNmCgFYI4BkorNFeCf0k/zm1Sbv/q9M1S0kB/hvcHOJpxJnVWJoe+3Xm1K?=
 =?us-ascii?Q?TTWR2xtqXnjkpt8o4YBj2BzbipWErt79LMWRQ4XNdp2GZ2h4WsnUq72ftcbh?=
 =?us-ascii?Q?YH/CMplaowSCuhNIktG306fVBuQwg02vIbm6D614nuxsXGpn9ccdsB0AdwIw?=
 =?us-ascii?Q?lWCtFSVA0G69wNR84xrAkUBKyDw/PIuNITRQasl28imKRrqDFGh6UqsyBeWh?=
 =?us-ascii?Q?OT+p0xiRtlQgvPY1NPNisRh7962fMBYpiLQOW3YSHBiK15DHYzPN7KrmiqXt?=
 =?us-ascii?Q?p5IdsUYLSxEjrzJREB48FgyeiOfHKnM50egf4g/7rz+NvMkb9D0nc5MMaitO?=
 =?us-ascii?Q?u11DyLB8mKPtpHPd+vdBPM347RSVicX54WQTYXVYUTbxD9cj3taOiUs7L5xN?=
 =?us-ascii?Q?V061+hLlXRjSj1w4RvxLusxWKmxZxmk=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <485166EBBF8A544683F493546D28761F@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 64defedc-8eb3-4ac2-0850-08da3cd2c165
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 May 2022 15:41:51.1456
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 7lVvSjziXk18VrmygSU/OsUYwy9JbPcqQXgdODQerbbXmaGqaCjlgPMmvM5VMGy6qFC+ZpruuyJnjXu7O4k+8g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR1001MB2115
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.874
 definitions=2022-05-23_06:2022-05-23,2022-05-23 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxscore=0 mlxlogscore=999
 adultscore=0 bulkscore=0 malwarescore=0 phishscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2205230088
X-Proofpoint-ORIG-GUID: -_9ZgxPwaSXHkKMYy5Q6aQMI_24A1nEz
X-Proofpoint-GUID: -_9ZgxPwaSXHkKMYy5Q6aQMI_24A1nEz
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On May 23, 2022, at 11:26 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Mon, 2022-05-23 at 15:00 +0000, Chuck Lever III wrote:
>>=20
>>> On May 23, 2022, at 9:40 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>>=20
>>> On Sun, 2022-05-22 at 11:38 -0400, Chuck Lever wrote:
>>>> nfsd4_release_lockowner() holds clp->cl_lock when it calls
>>>> check_for_locks(). However, check_for_locks() calls nfsd_file_get()
>>>> / nfsd_file_put() to access the backing inode's flc_posix list, and
>>>> nfsd_file_put() can sleep if the inode was recently removed.
>>>>=20
>>>=20
>>> It might be good to add a might_sleep() to nfsd_file_put?
>>=20
>> I intend to include the patch you reviewed last week that
>> adds the might_sleep(), as part of this series.
>>=20
>>=20
>>>> Let's instead rely on the stateowner's reference count to gate
>>>> whether the release is permitted. This should be a reliable
>>>> indication of locks-in-use since file lock operations and
>>>> ->lm_get_owner take appropriate references, which are released
>>>> appropriately when file locks are removed.
>>>>=20
>>>> Reported-by: Dai Ngo <dai.ngo@oracle.com>
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>> fs/nfsd/nfs4state.c |    9 +++------
>>>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>>>=20
>>>> This might be a naive approach, but let's start with it.
>>>>=20
>>>> This passes light testing, but it's not clear how much our existing
>>>> fleet of tests exercises this area. I've locally built a couple of
>>>> pynfs tests (one is based on the one Dai posted last week) and they
>>>> pass too.
>>>>=20
>>>> I don't believe that FREE_STATEID needs the same simplification.
>>>>=20
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index a280256cbb03..b77894e668a4 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -7559,12 +7559,9 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>>>>=20
>>>> 		/* see if there are still any locks associated with it */
>>>> 		lo =3D lockowner(sop);
>>>> -		list_for_each_entry(stp, &sop->so_stateids, st_perstateowner) {
>>>> -			if (check_for_locks(stp->st_stid.sc_file, lo)) {
>>>> -				status =3D nfserr_locks_held;
>>>> -				spin_unlock(&clp->cl_lock);
>>>> -				return status;
>>>> -			}
>>>> +		if (atomic_read(&sop->so_count) > 1) {
>>>> +			spin_unlock(&clp->cl_lock);
>>>> +			return nfserr_locks_held;
>>>> 		}
>>>>=20
>>>> 		nfs4_get_stateowner(sop);
>>>>=20
>>>>=20
>>>=20
>>> lm_get_owner is called from locks_copy_conflock, so if someone else
>>> happens to be doing a LOCKT or F_GETLK call at the same time that
>>> RELEASE_LOCKOWNER gets called, then this may end up returning an error
>>> inappropriately.
>>=20
>> IMO releasing the lockowner while it's being used for _anything_
>> seems risky and surprising. If RELEASE_LOCKOWNER succeeds while
>> the client is still using the lockowner for any reason, a
>> subsequent error will occur if the client tries to use it again.
>> Heck, I can see the server failing in mid-COMPOUND with this kind
>> of race. Better I think to just leave the lockowner in place if
>> there's any ambiguity.
>>=20
>=20
> The problem here is not the client itself calling RELEASE_LOCKOWNER
> while it's still in use, but rather a different client altogether
> calling LOCKT (or a local process does a F_GETLK) on an inode where a
> lock is held by a client. The LOCKT gets a reference to it (for the
> conflock), while the client that has the lockowner releases the lock and
> then the lockowner while the refcount is still high.
>=20
> The race window for this is probably quite small, but I think it's
> theoretically possible. The point is that an elevated refcount on the
> lockowner doesn't necessarily mean that locks are actually being held by
> it.

Sure, I get that the lockowner's reference count is not 100%
reliable. The question is whether it's good enough.

We are looking for a mechanism that can simply count the number
of locks held by a lockowner. It sounds like you believe that
lm_get_owner / put_owner might not be a reliable way to do
that.


>> The spec language does not say RELEASE_LOCKOWNER must not return
>> LOCKS_HELD for other reasons, and it does say that there is no
>> choice of using another NFSERR value (RFC 7530 Section 13.2).
>>=20
>=20
> What recourse does the client have if this happens? It released all of
> its locks and tried to release the lockowner, but the server says "locks
> held". Should it just give up at that point? RELEASE_LOCKOWNER is a sort
> of a courtesy by the client, I suppose...

RELEASE_LOCKOWNER is a courtesy for the server. Most clients
ignore the return code IIUC.

So the hazard caused by this race would be a small resource
leak on the server that would go away once the client's lease
was purged.


>>> My guess is that that would be pretty hard to hit the
>>> timing right, but not impossible.
>>>=20
>>> What we may want to do is have the kernel do this check and only if it
>>> comes back >1 do the actual check for locks. That won't fix the origina=
l
>>> problem though.
>>>=20
>>> In other places in nfsd, we've plumbed in a dispose_list head and
>>> deferred the sleeping functions until the spinlock can be dropped. I
>>> haven't looked closely at whether that's possible here, but it may be a
>>> more reliable approach.
>>=20
>> That was proposed by Dai last week.
>>=20
>> https://lore.kernel.org/linux-nfs/1653079929-18283-1-git-send-email-dai.=
ngo@oracle.com/T/#u
>>=20
>> Trond pointed out that if two separate clients were releasing a
>> lockowner on the same inode, there is nothing that protects the
>> dispose_list, and it would get corrupted.
>>=20
>> https://lore.kernel.org/linux-nfs/31E87CEF-C83D-4FA8-A774-F2C389011FCE@o=
racle.com/T/#mf1fc1ae0503815c0a36ae75a95086c3eff892614
>>=20
>=20
> Yeah, that doesn't look like what's needed.
>=20
> What I was going to suggest is a nfsd_file_put variant that takes a
> list_head. If the refcount goes to zero and the thing ends up being
> unhashed, then you put it on the dispose list rather than doing the
> blocking operations, and then clean it up later.

Trond doesn't like that approach; see the e-mail thread.


> That said, nfsd_file_put has grown significantly in complexity over the
> years, so maybe that's not simple to do now.
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever