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 1/3] nfsd: nfsd_do_file_acquire should hold rcu_read_lock
 while getting refs
Thread-Topic: [PATCH 1/3] nfsd: nfsd_do_file_acquire should hold rcu_read_lock
 while getting refs
Thread-Index: AQHY1QEWqnYiB4uxs0SMdlnrLaX8o634WYIAgAADb4CAAAkqgA==
Date:   Fri, 30 Sep 2022 20:06:00 +0000
Message-ID: <A5B0F720-D982-4AA0-AE6C-AE0CA8CF1B3C@oracle.com>
References: <20220930191550.172087-1-jlayton@kernel.org>
 <20220930191550.172087-2-jlayton@kernel.org>
 <9D4FA4C1-2246-4CAE-BB8A-D152603E3A56@oracle.com>
 <74afc012abb06df6d0648222cb4c01d22c40942f.camel@kernel.org>
In-Reply-To: <74afc012abb06df6d0648222cb4c01d22c40942f.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?rF1XtSHdApYUU0X95IfeboSmDxK+tqXMJSaVYeXHEnHM13j65NfEH0HxmWaz?=
 =?us-ascii?Q?Q8TQnJGSzYB++KXS94X8PTAIdsh2Rv+TWhW0M7ddE397eDKOoCs6mf6UXE+b?=
 =?us-ascii?Q?fiNp4rpW6Ee5VGcljLf6tcdDQ5SpiVc0lFHxSuoD9i2+jQft/3Ezk5bh52pg?=
 =?us-ascii?Q?YlLz1HmFggVdIG/5SbgRJPm5zfgWVSiH9SuyT7xRfjjFS5JG7PbeJ8zM/jjY?=
 =?us-ascii?Q?2kw7wKEn3LkFZHDEENaN81CMSnzwW2dshMOamImjEFxgWCeAGHiW+JeX8ZjI?=
 =?us-ascii?Q?1Zv7ttQXFg0rVHVviJtQRyg2BFZCrRuse5cI06FsnRUUK/OyPbaXtzWq/H1X?=
 =?us-ascii?Q?ft1gcd7DT8NtXTNvlML44Z4wSn7UWsjlejn+AqoibayqiKOEN9zEPyNqCC/p?=
 =?us-ascii?Q?pLZuXLalu33Uw5yp7ppB9O70tJAdYVpRL3iCvRNtupjiW/UkHCJMB9NFkPbW?=
 =?us-ascii?Q?QdOXAB18x647X9V95kESrI4HcfqgNNpDDU3RPAzYpuiRADq3NcKvDiVCM+VB?=
 =?us-ascii?Q?yKzvxLy/CkpSkVVo5oPNn5whulYuqFX29hWatSk0ABaav8+dVbyf9lBhTKRn?=
 =?us-ascii?Q?HkkvDq9ekSmFn+1XDaDhwjEim1GY4WsW5/UBkpfiKeg5XXse1J+f/4qdsEtZ?=
 =?us-ascii?Q?+E+6FEYv+d2QtSA2Kbt9tnR5gaCEBkpX6gD6zMypXpY42JWX6vbnBi09p+Jt?=
 =?us-ascii?Q?qL8BPmDN9b2XdsjYlodoZb0XY1IAI0+PaWT9REzohPT6zE05OIVIqGIrbDyM?=
 =?us-ascii?Q?cfy4GDBHJDiYccR1RvR8tdqKz/zWDSLSsnfhvTqGUZMk0xXJm65f0tJS4YRi?=
 =?us-ascii?Q?O+Y7q+p+/ELvoiRe5rFBX7yYqBuTK7oFm6CxdyqlPGTcVpGCZh+bVPXA5eci?=
 =?us-ascii?Q?Il3CtabWMHwsHILUgd79ogZslFFZorvX0l+fQ8ybO2CgxS4t3RPDNsiq6oDE?=
 =?us-ascii?Q?MhtfXRWv1F4bExIZ73bPQmcVuJMpTYX79EQa5z3ZW9a2yn6Qprg8ETNjlu9q?=
 =?us-ascii?Q?Nk2RSBKC94WF+Ya9SaawhiZEM1SfXzSYl72Q8xpJjpbBKM4p24vqBZ/drsug?=
 =?us-ascii?Q?3Lnp3Y6H6W+bHi7BRFGnyg6WFQY81xTgB4RcNK5zu05BCshztKkTsvuR2vkn?=
 =?us-ascii?Q?IXPwg4LktmT3NHDVaLoMpSB3W/jYY+fC+utrlq4sDoDFpqZgZd9XtYpZhffl?=
 =?us-ascii?Q?eRPk/lBY/2BcwO1Itn5JQDmymXUpwW3fyiBIcYtjCIWYUz0oRPZCaJiLu42H?=
 =?us-ascii?Q?fShb0NOKbVcWbIFKzyI8CW+R1PEJnqjazEW2G9aL/b9yTkncX1NKSrzuSdOq?=
 =?us-ascii?Q?STnptglRC68Tn3zxQaR3TIgtOYmMt5pj2UbhUitx/ykW0+ZCwqFZmrdbDm7H?=
 =?us-ascii?Q?70Ad+1iA8xBxDH6FMBVFh9qOx6pQKJOwp77e3W5vDB10UUEnlbxYYwGthbz+?=
 =?us-ascii?Q?zRYpLShgYFdH/cNAw65qHkVpViMyjJrEOqmWneB1VXFxt3XlfBa7zXPBscrz?=
 =?us-ascii?Q?2dxsaQ2LW18y/gLOMqfbdkY8jI1C6ZRQWVa1PfaaQKO3CwTPBFExmhEX87uE?=
 =?us-ascii?Q?HLQZev4iR/nC+D3eyfyUkuVd87nMG4z/UE/9msYqkFEV2wzwa9tEfKE3g11P?=
 =?us-ascii?Q?KQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <5862335CE4E6744E9054FAEC07F514E3@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: 70116a9c-567d-491d-f288-08daa31f324c
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Sep 2022 20:06:00.9251
 (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: YosJUSAXXh+WouyP4rQVsACgmTLjZHqC25PgPzdOLZ5nXNZhECevbW1NZBWSnVR2Y+PEQqE8CBYScNpUDCKYAQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB5310
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 malwarescore=0 adultscore=0
 suspectscore=0 phishscore=0 mlxscore=0 mlxlogscore=999 spamscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2209300126
X-Proofpoint-ORIG-GUID: CKFpG_tu-uCRK3mO7LP2pvtRkc2kWQ2d
X-Proofpoint-GUID: CKFpG_tu-uCRK3mO7LP2pvtRkc2kWQ2d
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 3:33 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Fri, 2022-09-30 at 19:20 +0000, Chuck Lever III wrote:
>>=20
>>> On Sep 30, 2022, at 3:15 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>>=20
>>> nfsd_file is RCU-freed, so it's possible that one could be found that's
>>> in the process of being freed and the memory recycled. Ensure we hold
>>> the rcu_read_lock while attempting to get a reference on the object.
>>>=20
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>=20
>> IIUC, the rcu_read_lock() is held when nfsd_file_obj_cmpfn() is
>> invoked. So, couldn't we just call nfsd_file_get() on @nf in
>> there if it returns a match?
>>=20
>>=20
>=20
> Adding side effects to the comparison function seems kind of gross.
> Do you know for sure that it will only be called once when you do a searc=
h?

Yes, when a match occurs, the chain walk stops.


> Also, there are other places where we search the hashtable but don't
> take references. We could just make those places put the reference, but
> that gets messy.

nfsd_file_do_acquire() is the only place that calls the comparator
with NFSD_FILE_KEY_FULL.

 165                 if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) =3D=3D 0=
)
 166                         return 1;
+                    nfsd_file_get(nf);
 167                 break;


> I think this is cleaner.

Unfortunately, the comparator's second parameter is a const
void *, so we can't modify the matching nf. Ho-hum.


>>> ---
>>> fs/nfsd/filecache.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>=20
>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>> index d5c57360b418..6237715bd23e 100644
>>> --- a/fs/nfsd/filecache.c
>>> +++ b/fs/nfsd/filecache.c
>>> @@ -1077,10 +1077,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, st=
ruct svc_fh *fhp,
>>>=20
>>> retry:
>>> 	/* Avoid allocation if the item is already in cache */
>>> +	rcu_read_lock();
>>> 	nf =3D rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
>>> 				    nfsd_file_rhash_params);

This probably should be changed to rhashtable_lookup() so that
we're not double-wrapping the call with rcu_read_lock/unlock.


>>> 	if (nf)
>>> 		nf =3D nfsd_file_get(nf);
>>> +	rcu_read_unlock();
>>> 	if (nf)
>>> 		goto wait_for_construction;
>>>=20
>>> @@ -1090,16 +1092,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, st=
ruct svc_fh *fhp,
>>> 		goto out_status;
>>> 	}
>>>=20
>>> +	rcu_read_lock();
>>> 	nf =3D rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
>>> 					      &key, &new->nf_rhash,
>>> 					      nfsd_file_rhash_params);
>>> 	if (!nf) {
>>> +		rcu_read_unlock();
>>> 		nf =3D new;
>>> 		goto open_file;
>>> 	}
>>> -	if (IS_ERR(nf))
>>> +	if (IS_ERR(nf)) {
>>> +		rcu_read_unlock();
>>> 		goto insert_err;
>>> +	}
>>> 	nf =3D nfsd_file_get(nf);
>>> +	rcu_read_unlock();

What might be nicer is if this used rhashtable_lookup_insert_key()
instead, and if it returns EEXIST, it simply frees @new and retries
the first lookup.


>>> 	if (nf =3D=3D NULL) {
>>> 		nf =3D new;
>>> 		goto open_file;
>>> --=20
>>> 2.37.3
>>>=20
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>>=20
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever