Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
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 2/3] nfsd: fix potential race in nfsd_file_close
Thread-Topic: [PATCH 2/3] nfsd: fix potential race in nfsd_file_close
Thread-Index: AQHY1QEVVjBDwZrkT0mQUKwwKynMra34dMkAgAAAUIA=
Date:   Fri, 30 Sep 2022 20:59:40 +0000
Message-ID: <14F4F92E-427E-4D71-B57B-43C01422F1B1@oracle.com>
References: <20220930191550.172087-1-jlayton@kernel.org>
 <20220930191550.172087-3-jlayton@kernel.org>
 <225eb68c838607125ba82e605b7e02b7100d4cf6.camel@kernel.org>
In-Reply-To: <225eb68c838607125ba82e605b7e02b7100d4cf6.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?Ml4WaN6QVu4+QCwECk1k4/S4K1DM4LFYalNtxfs3cbCGdowyaSiudGp3fAIB?=
 =?us-ascii?Q?/wAStFV5Guc9qs9ajUc0BS3UhsIC9oDvW9rEDK3tiPomm7EgTXj1qImFxH46?=
 =?us-ascii?Q?xY77QNTqIWYckQ2BDpezTSkaUIcSCwUJb+FmmXj2VHtN94BEdhh32AU3Q+Lx?=
 =?us-ascii?Q?JHAAo6qeTb3fxC5FFGIWb6fdJEFlGLNDr4ByLRBH7EL6UaWnRVCnBtSNl3jn?=
 =?us-ascii?Q?8GfQkUB68y59rMRAvQIlpiukqTRbDhGoRS3BLx8SlXR8/Lh8SwMcGc4iAPuw?=
 =?us-ascii?Q?elk+BkenTHPsW1XuHJZbhi5oTJJOJCqnplZnR9NEY1f0TBFDygMupK2nDjpZ?=
 =?us-ascii?Q?kCpVMLb6zg5iVC4uqYWOL10cGxTZVDOcJ0CsSv+57G9cbL5bKsGEMT183ZX6?=
 =?us-ascii?Q?avgE+IbfDWjVNjjhIah7/j3YCRxGOo9RjdrwT5HeAoCgc8TDBO3YFD1slCvf?=
 =?us-ascii?Q?4c9LofaLOKxENZazDyUL78xhxgM0Erg5kKnP1l37sy608Or0deXPu/qRHyj3?=
 =?us-ascii?Q?wnfcZD9EXOQrz3L530kgviv7Di0XJNwGAXiCGObpYzYY0WbaEYev9loBH1wh?=
 =?us-ascii?Q?4hOu4sho+FzArE4IXGAypBsKhHOlZ5/5VILiw/8vT4Ypl6f921NWVo3YIEAW?=
 =?us-ascii?Q?KGSsmyKyORVjofWpvPRuDZszVjYUPPMsMT9537kqI+ifLOtUV9/51vvmLjBu?=
 =?us-ascii?Q?av2zS8USaBHgBL77Xak7W/8BeJdWMEBl50fQ+GeIpjnTKIgi7bypIBcGWUJN?=
 =?us-ascii?Q?w/bKJ2BAUkTsXy4+WFqyG0fbzYb80/nz2cRspK73ogGuoN5+2qpevFdNCF4G?=
 =?us-ascii?Q?YT9hwGaTNlDkNobGxduTMsK02u4g4W+qVNBm5n0RkMNMPHeiCXqIRvnqlLSe?=
 =?us-ascii?Q?YqYygDMYORD1bVuVV1UV8xJnEBkgv2cn1LCdrxRMqMTwZORK638PGe/HrfmJ?=
 =?us-ascii?Q?rIHdgrXGDYJ151lVzlwQSC0kzNJ6POwxdhJlAd2780YKc3u3dgplEo0gFSct?=
 =?us-ascii?Q?09BgqZYscQOsSHnAJ5eyf8P5Jcrw2y3lgxXcHAk65o1z12IRhDlHtqvyRp4C?=
 =?us-ascii?Q?5zHcdZOBx9eNOcYr8L46wb35SybNJoXvP9JgNNgHXQ9f9ttJdYOIeTIYOkw1?=
 =?us-ascii?Q?pIbfdaMvUD7WX4LXqHoxaiXBSBSMrfeyX9/+6Eyh164JCHltNyxXr1Gz7wxh?=
 =?us-ascii?Q?eTaPn7LhT8AaKSldpwgkYMZKCcykCcRQ9uWy603L2XHN19+M7P6CrrKr/LOy?=
 =?us-ascii?Q?9wEOrNeppg4Riz0pCmjsPQFz4XnVRazZtv4Flcu78JTfL88Dr/psUF8JUINw?=
 =?us-ascii?Q?qW254YWPO4EJ5MUwv9FGxJY9WR+iR7TiDXrZxJDho9tsbYVVlVnyX35L5VTm?=
 =?us-ascii?Q?Osq226D6XTTTM0gsVU34s8jkMgKoRB5164VLYqkCx1cuud12UvTQnPDsvFDh?=
 =?us-ascii?Q?cfZo196EYq438RwJYigMNopMULNmPJ8YQuo2X0RepIyKChA00boJMktjNbEJ?=
 =?us-ascii?Q?L1Xrb+jqXX9/p9QNfYr71TsblyLPKoViAxmY6o6awHc74N74L33wL1i1X6k0?=
 =?us-ascii?Q?sLPZuuGOvqykOe/oTrwx7EZfuXGiDdiFVIAG/kQufwL+dIdaffVBjDCqjdKg?=
 =?us-ascii?Q?mA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <619466B741F06640BAFC1A46E9911AD4@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: 4dcea253-69da-486e-cdd3-08daa326b157
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Sep 2022 20:59:40.5440
 (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: AutqmSek1QMyQ8MZfIXHWDoCnZxup6tXDAtaKtbnpbUa8irmwYbxSBw/SKpkOQJdpWlRXlQZKB3QoZOxIKJ1xA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB4817
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-30_04,2022-09-29_03,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 malwarescore=0 phishscore=0
 suspectscore=0 adultscore=0 bulkscore=0 mlxlogscore=999 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2209130000
 definitions=main-2209300131
X-Proofpoint-ORIG-GUID: TIcqMYTs_2RDVf08RiLSbx_MxX44P4ab
X-Proofpoint-GUID: TIcqMYTs_2RDVf08RiLSbx_MxX44P4ab
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        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 Sep 30, 2022, at 4:58 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Fri, 2022-09-30 at 15:15 -0400, Jeff Layton wrote:
>> Once we call nfsd_file_put, there is no guarantee that "nf" can still be
>> safely accessed. That may have been the last reference.
>>=20
>> Change the code to instead check for whether nf_ref is 2 and then unhash
>> it and put the reference if we're successful.
>>=20
>> We might occasionally race with another lookup and end up unhashing it
>> when it probably shouldn't have been, but that should hopefully be rare
>> and will just result in the competing lookup having to create a new
>> nfsd_file.
>>=20
>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> ---
>> fs/nfsd/filecache.c | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>=20
>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>> index 6237715bd23e..58f4d9267f4a 100644
>> --- a/fs/nfsd/filecache.c
>> +++ b/fs/nfsd/filecache.c
>> @@ -461,12 +461,14 @@ nfsd_file_put(struct nfsd_file *nf)
>>  */
>> void nfsd_file_close(struct nfsd_file *nf)
>> {
>> -	nfsd_file_put(nf);
>> -	if (refcount_dec_if_one(&nf->nf_ref)) {
>> -		nfsd_file_unhash(nf);
>> -		nfsd_file_lru_remove(nf);
>> -		nfsd_file_free(nf);
>> +	/* One for the reference being put, and one for the hash */
>> +	if (refcount_read(&nf->nf_ref) =3D=3D 2) {
>> +		if (nfsd_file_unhash(nf))
>> +			nfsd_file_put_noref(nf);
>> 	}
>> +	/* put the ref for the stateid */
>> +	nfsd_file_put(nf);
>> +
>=20
> Chuck if you're ok with this one, can you fix the stray newline above?

Sure.


>> }
>>=20
>> struct nfsd_file *
>=20
>=20
> Thanks,
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever