Message-ID: <67b6d713-cd2e-252e-f702-7be722dcfee4@oracle.com>
Date:   Tue, 24 Jan 2023 12:10:01 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: [PATCH 1/1] NFSD: fix problems with cleanup on errors in
 nfsd4_copy
To:     Jeff Layton <jlayton@kernel.org>, chuck.lever@oracle.com
Cc:     aglo@umich.edu, linux-nfs@vger.kernel.org
References: <1674538340-12882-1-git-send-email-dai.ngo@oracle.com>
 <a6a220ae0645601eefc0d52ab852ebe37d6dff1c.camel@kernel.org>
Content-Language: en-US
From:   dai.ngo@oracle.com
In-Reply-To: <a6a220ae0645601eefc0d52ab852ebe37d6dff1c.camel@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ak1mUWIxZU8xM0NCSWJHZFFmRnVzTW92Qkp6anpBd083QjNlb0ZXbkozcHh5?=
 =?utf-8?B?M3c4SjBmclB2cU9NWXdlNm9FTWozeTBSb2V6OGtwMCt2bkZBNS9yVjBSWnhZ?=
 =?utf-8?B?OVJJY3NLeW9yQ2g2eno0dnkyWGFhUVdDV1BYN3FacDlOUUllWUdlV2VPMkRN?=
 =?utf-8?B?bHFvejRSRGRUSE1XRCtoZUN5ME1xMVBnSERuRnVRUDFSYUJCTXl2ZVBVOUZE?=
 =?utf-8?B?Skl3RU5kRWdLS1ZTUW94TkVyZ3kydEIxQkRIU0J0cTNtd1BNQUpmbDdEbGJx?=
 =?utf-8?B?WVlBV2V6RUlPQmVwT0E2djZQcjRoNERONDFqOCsyM1hjdEN5dEQrRVh4OE5H?=
 =?utf-8?B?V05VclZKR1FqbHgxV1hvT0pnM0ZFd2tRdjh0NHhFSDk1WkRYMTBNL0ZpUm83?=
 =?utf-8?B?TmQ0WWw4c1dIYWU1eTluWGFCS2VCNW54WmFoc0szd1BSWVJmUEpWOENXRitE?=
 =?utf-8?B?R0o2UDQ3WXdWVUJLMDI2TGw5K2tYc3dEL2luYjF0Q2JKQ1RFNmc3OVVqckxX?=
 =?utf-8?B?SVlGRnB1RXhVaFFld3BNQWw2ckJ0cjdvOXhJQ1ZmdEgvNHl1dlFoUk00eTFE?=
 =?utf-8?B?UTNIT05wSXltSENCN2ViK1IycnJnc3pHWUVJdVZSK3FkbG04ZXVuTjJOcVFw?=
 =?utf-8?B?ZlhXamg0NGptam51LzQ2a1RBQUQrb0FDdVVnbi9sME0zbis3Sm0xaDFpRVZl?=
 =?utf-8?B?ZUZuVnYwTW4wTnVFK2txdXg4c215ZEd4bzFWK1FsRWRmZzkvSzVsbjg0WW5h?=
 =?utf-8?B?VU5jcU1xSUJxU2lmYkU2alUrUW04Vjdad1JDNVEzaU1zK0s2QkZlUVJ6WXFS?=
 =?utf-8?B?OFM2dkwwQURXWldaQmxvclAzN0R3dEpnR21zcWdaS09CZzY1OEpKYmFrdVY3?=
 =?utf-8?B?eVlFVnpHbER4ODgwMkpCRm9ibTdKaVR0R3hhemFJSE5LS0hoa2pJYzZyN01H?=
 =?utf-8?B?L0M3SjI5dGRGWTZ4d1lrbWxUUXB5SkRWTEtkd1llK3JpeWtEY3Y1UVd3eHV4?=
 =?utf-8?B?Ykx2MEF5c0ZSQlBpMEsycHorSktudDRGT01MYitVMzVXdHR6ak1PcTZuSlRX?=
 =?utf-8?B?SFhFaTAycUJBS1JpWTdqRDBvdXJYMkVNelhrbGxTSkREcHNZN3NDY2o0VkpG?=
 =?utf-8?B?V0VYSXYyaEN2cTJJMHFXYzRJU28vbXhYVU52K0pJYVFlNVJpNVJMa3Foa3F1?=
 =?utf-8?B?NXQ3cnNQcDR3TzQrR2JmMkpGZHRIbytvRUxNRFR5NENYclo0RmxoODV3Q2VS?=
 =?utf-8?B?MEdhTTRvalM3dkVOSk55QVhNMitiTVlMR2JUcmQ3QWdRYk1XWVNvZGtJaTky?=
 =?utf-8?B?L3gxTUV4ZnJqOEpTR0xabFRUcmNTZ0QyRFlxaTZab1FnSXVJS0RBcXFLQUN3?=
 =?utf-8?B?eXUzVStOSjAwZlp3TEp5dlNjeG05TGJseVVjMGFrNUpGRHB1SS8rd1dvMENt?=
 =?utf-8?B?dmdjY1g3Wi9LWVMzYVFMUXVsZGFaNzRUR3hMNHBMSk84WVdCUDlmNE5lT3VB?=
 =?utf-8?B?eXBPMS9MYktCNE51SUNUNWoyWWhQMDg3dHB1UEdKTFlGbndoandEb01vVk52?=
 =?utf-8?B?a1FTQlc2aEVPZE9iM3B3VGNyL2QyZEppS3BpWk1oY28vbjhuN3lZdWJxa1JM?=
 =?utf-8?B?YXFDZTc5UXdtaW1GY1lqa2JoN01ESkJpLzJ6RjlOcVN4c0ZoVXgvbk5McEdH?=
 =?utf-8?B?OGY5dCsxWWIvSCtMakF6UldRUnJWajdGZDB1S3dQM2Vyc1Y5WkRiclVaL1Y2?=
 =?utf-8?B?VUNZZE5DVVh2U0JuT3V2aDNZZzBDMGR1blBPd3l5VDVHMUZVTjN3U2pNSmsy?=
 =?utf-8?B?ZGc3ejZNRkFVQXRuc3VuNFJmRHpzQXNSb0I5dDN5WWJmemcyN29STUdCMEdU?=
 =?utf-8?B?NG9QamZLNGl2a1BQbWVKcjh1NEVSWHd2a2U2dkplSHRneUttUTBQZG1GV3Jo?=
 =?utf-8?B?cExGVnhtdHpzbHU3ejVsUmw3ZWN3eFh2dHQvSTd3ODRNYXE1QkdGUUpXTUtq?=
 =?utf-8?B?bFgxRkhzeG9XTVltbUllOTJJYVgvY2x1aEZjdDYzUDFNbTV6TUFSNXBWRm5Z?=
 =?utf-8?B?WDhzNVlGeWZMWEdWMlgxWVNwL0d6WGFoUWRvU3FEYzhFSjlTbEFkaW95WGFL?=
 =?utf-8?Q?iQnGTFNwSSj/AVXb+A4/IcuNW?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 6Wo8dwWMw5a3mvEHyVMY7QhI99pSH62xGVFuHuIeNjNn40H35Jdx94w0jZlJ0TKgkNTuSnFzMUEGEVkxwVR+u9Tu9c+nNUfVf55jZ+KWdktvkuZSS00Alo06SsQJDXdmhXR9/7zI8qn4aNHuy9PI7Kmacm9/goK2J+Sxo6uZovzjn7ODVL+pKsMO+wxGpndnvghOADJwOt2+MLlHuBxw5kEHvH3C3KgpgGq78AhzWA+v/xh23cX2A1bKI0SDWyHNzpzu2OxpxradNjib58TjTVRbPOlt8rKBHaEUsgOBdNTgFF2WxV1qTNVnBn6KfEsTadzQFhNqegJTe145Mj0VWQKF8DjYg0Jl5vZOtrr1qTKJi9ROISv+Bz5pn8AecYOcAhMqfsN9GH2WcDPrKci7CoFvwc9GTsCjxTmD6MhM8ldMQ1qQtUHAZSS1c+RKeWyBbMJR6C/uf+QmqeFEv7h8O/ZF1engL9mpwOPDxbem3HvTme3y5y8wz3PXeKfBOx2Mz5hSmsymwh52fGlNr+3QQYqMZdlFVSk/GeTly6tOL9qzD6MZseb4zB9XdxhlEOMoS8Qer9fvqDwXLG0Fh7lc8iqkPoQTWqD2xoywdKbcLl6pcP1A1nBJ2m6x/ez6O+eTmvTqujHSEfdwUYbPJPyPLl7DBOL4jIMyAm1UXbZ2s09U4yBPMpi6e8PTNFT0h5rvzemK/rfw9m0Mik4C+1lzFVvvtt5YQaazJIn15gCiciEbw889xhh/ZlN+/SkkAILVoHWJu3dZCY0SonYxxcYqWJHqf+4yJ3rq+yAzdxrAy9xb7mnJFHgcc+Vl3VM8iipE
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d5ec3063-88f1-463f-a71b-08dafe46f9fa
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Jan 2023 20:10:02.3341
 (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: 8c0n2pzMmCTilvYa4953rDXcN3TXnw9+7yAFh2sB05rRVHfxWMngg0gdlXLbiFb33s8q5DAX9YYflu6Y7LxyVQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR10MB6158
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-24_14,2023-01-24_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0
 bulkscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 spamscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2212070000 definitions=main-2301240185
X-Proofpoint-GUID: bJOEqMaBpqOehgp2NehHCqD1kNdufrb2
X-Proofpoint-ORIG-GUID: bJOEqMaBpqOehgp2NehHCqD1kNdufrb2
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org

On 1/24/23 3:45 AM, Jeff Layton wrote:
> On Mon, 2023-01-23 at 21:32 -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.
>>
>> This patche sets async_copy to NULL to skip cleanup_async_copy
>> if async_copy is not yet initialized.
>>
>> 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 | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index 3b73e4d342bf..b4e7e18e1761 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -1688,7 +1688,8 @@ static void cleanup_async_copy(struct nfsd4_copy *copy)
>>   	if (!nfsd4_ssc_is_inter(copy))
>>   		nfsd_file_put(copy->nf_src);
>>   	spin_lock(&copy->cp_clp->async_lock);
>> -	list_del(&copy->copies);
>> +	if (!list_empty(&copy->copies))
>> +		list_del(&copy->copies);
>>   	spin_unlock(&copy->cp_clp->async_lock);
>>   	nfs4_put_copy(copy);
>>   }
>> @@ -1789,9 +1790,15 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>   			goto out_err;
>>   		async_copy->cp_src = kmalloc(sizeof(*async_copy->cp_src), GFP_KERNEL);
>>   		if (!async_copy->cp_src)
>> +			goto no_mem;
>> +		if (!nfs4_init_copy_state(nn, copy)) {
>> +			kfree(async_copy->cp_src);
>> +no_mem:
>> +			kfree(async_copy);
>> +			async_copy = NULL;
> This seems pretty fragile and the result begins to resemble spaghetti. I
> think it'd be cleaner to initialize the list_head and refcount before
> you do the allocation of cp_src. Then you can just call
> cleanup_async_copy no matter where it fails.

If we initialize the list_head and refcount before doing the allocation
of cp_src, we still can not call cleanup_async_copy if the allocation of
cp_src fails or nfs4_init_copy_state fails since:

. dst->cp_stateid is not initialized
. dst->nf_dst has not been incremented
. dst->ss_nsui is not set

The fields above are initialized by dup_copy_fields and we can not call
dup_copy_fields if allocation of cp_src fails or nfs4_init_copy_state
fails.

-Dai

>
> Bear in mind that these are failure codepaths, so we don't need to
> optimize for performance here.
>
>
>
>>   			goto out_err;
>> -		if (!nfs4_init_copy_state(nn, copy))
>> -			goto out_err;
>> +		}
>> +		INIT_LIST_HEAD(&async_copy->copies);
>>   		refcount_set(&async_copy->refcount, 1);
>>   		memcpy(&copy->cp_res.cb_stateid, &copy->cp_stateid.cs_stid,
>>   			sizeof(copy->cp_res.cb_stateid));