From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>, Dai Ngo <dai.ngo@oracle.com>
CC:     Olga Kornievskaia <aglo@umich.edu>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2 1/1] NFSD: fix problems with cleanup on errors in
 nfsd4_copy
Thread-Topic: [PATCH v2 1/1] NFSD: fix problems with cleanup on errors in
 nfsd4_copy
Thread-Index: AQHZM5xnPcNm2r6T0k6YHqx6MUvYXK64itCAgAAOMAA=
Date:   Tue, 31 Jan 2023 14:27:14 +0000
Message-ID: <E130FEB9-BAC4-42FF-8DC6-9769A0933832@oracle.com>
References: <1674967461-1366-1-git-send-email-dai.ngo@oracle.com>
 <3a2684bf63a044bf2a9f349c28b2b2ea5b9e1016.camel@kernel.org>
In-Reply-To: <3a2684bf63a044bf2a9f349c28b2b2ea5b9e1016.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?V2LvrJ6C7ACWvgtQaufsacxgIhEKu3Bt/ZDG3TCyf27uKU+J0kzgVRFaEgMQ?=
 =?us-ascii?Q?fGS9W6iRpy8oHwNz2q9m3X3EHCi/zkbYoKXOuwZKiVqV71cX9i68ThYKaSIX?=
 =?us-ascii?Q?neo6xH2gJdKO9VKjz27alEDjPxIj9MCqkvDIao8hZc9bsPa+zkkEkCERtUn/?=
 =?us-ascii?Q?FuFfs5daZ4ObqbPtnTI+7fDqsch3wLq1eIrjXBdgCLlXeheqR/SwF8d7XQUw?=
 =?us-ascii?Q?9aDNRxtwPzYakk7YiC/Pn7HCEML+xMEj03Tg0MZMH3oPgxEU0yYsQFMx1cIt?=
 =?us-ascii?Q?KBBHgdNxmFw52RgFdoOsbzwdPzuO/TLxTJbs8lgofLGvGgQtugxwGcoEebgF?=
 =?us-ascii?Q?CFDZcoe8EuMSXxI+vaFtxJIM43x0IngIc7shZxqk0ZdKuaaomVHw9GllUtzv?=
 =?us-ascii?Q?IvkXwY1WTvNgmL9ZRBs3510FYeXaFb4WWRjLgrjuNTYYm01brOrX/tnDbKNq?=
 =?us-ascii?Q?zIc3ES0PmSqzHoBoBQ+QhmN+/mfDGBjMMDbMJU5GdRKaZ3FuKiAa11i/DoSm?=
 =?us-ascii?Q?lLYhbINK8qRCbYw4vqrRwpqrFiHeHUyJVA9uQDn4QyT9zliWZFJU+/0FHmOb?=
 =?us-ascii?Q?XBIfsF5aEoKGNubzxvkmn3oNV8lZbAOO1ErnAg9Fdwy6SIM2uryauXe6OdPY?=
 =?us-ascii?Q?igphuUcMAB7GaxTXs+D67h2SWA2MDSKmA+Jbh1lkOXot/4kRSb8/YtgrrbOE?=
 =?us-ascii?Q?0eO8HVb+3MorqqshBL9f6sKrkTogtjFBXQz9Yc5FlRm0ONrSCkrcgH32eET/?=
 =?us-ascii?Q?ehZgQ5DErsjKD+OAfAEriFUr254/vDSIFfvUiLty6QKYnNe0D/d1JVYFiFDD?=
 =?us-ascii?Q?INvIWnHJZduflWFVjetregefgiWq5s/DEFfecSQW4zOCwhAi2lfokP9CmtmB?=
 =?us-ascii?Q?yZ2r3riC5NEBs2E2iMM9C8u3SGvczHa9zcajeGHeGl7x6uBJjxsIBGYIbo2Y?=
 =?us-ascii?Q?4t+gd0w92yzp8JAtT8S7/q+rxyiin/3/Ds6H7B986TJ+afYxYiMubzcCrphB?=
 =?us-ascii?Q?ofK09r1s5VvG5EbP3UOZbweePUz4JoY3xbFUR6Fad1uxjz5wqGoyrMIGtaZi?=
 =?us-ascii?Q?Q8z1H00lIKXIIxNZ4N+b8pnbU1x7bTQf8537NEuF0j2MavmrCAPGzfxcl28F?=
 =?us-ascii?Q?VNUxFsryEkgJlMbU0XoKaKI4S9uMkr44AyRBzHVp53LGEv/SPOgbu3NUVaQP?=
 =?us-ascii?Q?IvEZUjP6dXi2Li0/USlJkaAYmGiWcRatcitX3uG5apxW1D8HHGV6CzDGAh+4?=
 =?us-ascii?Q?JisQqXhdu4ql0D/v8gNQmTdccfZysGpJauPw0fDs9y57peMqGWvibCZQ5d/V?=
 =?us-ascii?Q?3obsSmQvmCM07sR6t2BE8nLPe+8RoN1Hrix871sqQabMm3jnJcw8rLGYQ7rT?=
 =?us-ascii?Q?sFKlqH83mQTZhkq6IwvqNQilYcIMBXWWTAn5oE7v7a5Ken/+3ce/YThGYUbo?=
 =?us-ascii?Q?8ICRoYuVdHHPrr0T/Lr9b5uns1OazdLSlEc2i4f3B2l1gxFg6v+zpC4sYgqz?=
 =?us-ascii?Q?PL2Cv1TXjiXqVjoEo6LFwhenPwKU+Jg5W+WoHXlqEEt6hyAn/nRRe0M1soNg?=
 =?us-ascii?Q?cGNeMUhnohgrIeYBWaU6BZwJbCndvyGms+chEKjTGwKhrUv9dWPoF+lvY863?=
 =?us-ascii?Q?jA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <0BF31E0FE4A14D41926FE2F8503A63DA@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: VOPK1thIjQXq1RJXwRv7euOgpWLa/ZfyWNX3Bk18ScUM0+F+ocLRuYSbNsmAK6BaPHkFqN3rO4NybuNT/8CNbpcfjUGVZCAuUgAlro8Pfnd/K0ZqHrrWPKvW/4Ah9MZULn0U8Ovi5bGiaPKtWFpb43GDvhjT+UiHBUlqxMDcU+2A1F5Adjco19EuwWi6HuHjq5Ry7eti00+s+BImR1+98AdrPQhX9rQwrpXHUFO/lAXlkqEz0vAyVne/dUiI44DwY4Lllu2YbTKXa8F8Yy7fC02IbkUBfTy2uXrusSbl5SEWr4WaYyT/Qmk2lMK05OQZhYuYkFEA7xUr9+MHMf4VcMFrK4yE/dXPUMi7GvHqedwRlB13BDFTiXf4rWqFl33bADaPQ9iA4Enbk7XxC0A0PlM6boqEm+uM7dZH12Ux6DH1TyCSBxivdyETD4MDNzi2SeW/EKmbgreBz8ClIAgDH+7fmVyCh+Nof4hrs3zd8olvELFnWHAyBUDaiU4RBVDDl7qE7mjrMoT5w6QEtI7BR0yHqTzRCndm73Ckw5Ulrki/dr+In5tUPZl4UQHICASXDV8Vpd2BQ4aVfFPi+pRVrsL6Q1DeWcKRj2KbUDnGDORLjSP3H97umplUQYpfVEdGNNjhUQMiL/0OEkYGrejh/B+j+nxF9kkQIR+2VTVyy025Qhs8611KPfh/IuG8EeKGEG/0fj6gGvvVtztpJc6w35rz5eZB7Dd/TBVFFMkCWhlRcWZzZX9TnnnnY/ZIaqtXCSvE4cM9xV9YACP7ZK+R7GKQSlnhA96WWN64tKx3/bxwBWufAFEaBEKHsKJQ9QMl
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: 745aa7eb-4f41-4bb2-72e3-08db03973fe9
X-MS-Exchange-CrossTenant-originalarrivaltime: 31 Jan 2023 14:27:14.9641
 (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: Vo3aGXN0rUy65DkXdtvT2Ul221IGPwJluSOtngbYhWwNR/RqDZLRLUZRWgEtVSNQOoO9VxkQyOBDf9/kXu58Rw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR10MB6290
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1
 definitions=2023-01-31_08,2023-01-31_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 malwarescore=0
 suspectscore=0 adultscore=0 mlxscore=0 phishscore=0 mlxlogscore=999
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2212070000 definitions=main-2301310129
X-Proofpoint-ORIG-GUID: wZHR7QjHGbLRajvIXuCwQZzDhFmB884R
X-Proofpoint-GUID: wZHR7QjHGbLRajvIXuCwQZzDhFmB884R
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jan 31, 2023, at 8:36 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Sat, 2023-01-28 at 20:44 -0800, Dai Ngo wrote:
>> When nfsd4_copy fails to allocate memory for async_copy->cp_src, or
>> nfs4_init_copy_state fails, it calls cleanup_async_copy to do the
>> cleanup for the async_copy which causes page fault since async_copy
>> is not yet initialized.
>>=20
>> This patch rearranges the order of initializing the fields in
>> async_copy and adds checks in cleanup_async_copy to skip un-initialized
>> fields.
>>=20
>> Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
>> Fixes: 87689df69491 ("NFSD: Shrink size of struct nfsd4_copy")
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>> fs/nfsd/nfs4proc.c  | 12 ++++++++----
>> fs/nfsd/nfs4state.c |  5 +++--
>> 2 files changed, 11 insertions(+), 6 deletions(-)
>>=20
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index 57f791899de3..0754b38d3a43 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -1687,9 +1687,12 @@ static void cleanup_async_copy(struct nfsd4_copy =
*copy)
>> {
>> 	nfs4_free_copy_state(copy);
>> 	release_copy_files(copy);
>> -	spin_lock(&copy->cp_clp->async_lock);
>> -	list_del(&copy->copies);
>> -	spin_unlock(&copy->cp_clp->async_lock);
>> +	if (copy->cp_clp) {
>> +		spin_lock(&copy->cp_clp->async_lock);
>> +		if (!list_empty(&copy->copies))
>> +			list_del(&copy->copies);
>=20
> Can we make this a list_del_init? If cleanup_async_copy were called on
> this twice, then the second time could end up corrupting the
> async_copies list. The cost difference is negligible here.

I noticed this yesterday and made the change in my tree.


>> +		spin_unlock(&copy->cp_clp->async_lock);
>> +	}
>> 	nfs4_put_copy(copy);
>> }
>>=20
>> @@ -1786,12 +1789,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_=
compound_state *cstate,
>> 		async_copy =3D kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
>> 		if (!async_copy)
>> 			goto out_err;
>> +		INIT_LIST_HEAD(&async_copy->copies);
>> +		refcount_set(&async_copy->refcount, 1);
>> 		async_copy->cp_src =3D kmalloc(sizeof(*async_copy->cp_src), GFP_KERNEL=
);
>> 		if (!async_copy->cp_src)
>> 			goto out_err;
>> 		if (!nfs4_init_copy_state(nn, copy))
>> 			goto out_err;
>> -		refcount_set(&async_copy->refcount, 1);
>> 		memcpy(&copy->cp_res.cb_stateid, &copy->cp_stateid.cs_stid,
>> 			sizeof(copy->cp_res.cb_stateid));
>> 		dup_copy_fields(copy, async_copy);
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index ace02fd0d590..c39e43742dd6 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -975,7 +975,6 @@ static int nfs4_init_cp_state(struct nfsd_net *nn, c=
opy_stateid_t *stid,
>>=20
>> 	stid->cs_stid.si_opaque.so_clid.cl_boot =3D (u32)nn->boot_time;
>> 	stid->cs_stid.si_opaque.so_clid.cl_id =3D nn->s2s_cp_cl_id;
>> -	stid->cs_type =3D cs_type;
>>=20
>> 	idr_preload(GFP_KERNEL);
>> 	spin_lock(&nn->s2s_cp_lock);
>> @@ -986,6 +985,7 @@ static int nfs4_init_cp_state(struct nfsd_net *nn, c=
opy_stateid_t *stid,
>> 	idr_preload_end();
>> 	if (new_id < 0)
>> 		return 0;
>> +	stid->cs_type =3D cs_type;
>> 	return 1;
>> }
>>=20
>> @@ -1019,7 +1019,8 @@ void nfs4_free_copy_state(struct nfsd4_copy *copy)
>> {
>> 	struct nfsd_net *nn;
>>=20
>> -	WARN_ON_ONCE(copy->cp_stateid.cs_type !=3D NFS4_COPY_STID);
>> +	if (copy->cp_stateid.cs_type !=3D NFS4_COPY_STID)
>> +		return;
>=20
> It's probably fine to keep the WARN_ON_ONCE here. You're testing the
> condition anyway so you can do:
>=20
>    if (WARN_ON_ONCE(copy->cp_stateid.cs_type !=3D NFS4_COPY_STID))

Six of one...

I'm OK leaving the WARN out, it doesn't seem high value to me.


>> 	nn =3D net_generic(copy->cp_clp->net, nfsd_net_id);
>> 	spin_lock(&nn->s2s_cp_lock);
>> 	idr_remove(&nn->s2s_cp_stateids,
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever