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;
Message-ID: <3ff5458c-88ab-18ab-ebfe-98ba8050fd84@oracle.com>
Date:   Thu, 19 Jan 2023 10:38:50 -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 2/2] nfsd: clean up potential nfsd_file refcount leaks in
 COPY codepath
Content-Language: en-US
To:     Jeff Layton <jlayton@kernel.org>, chuck.lever@oracle.com
Cc:     linux-nfs@vger.kernel.org, aglo@umich.edu
References: <20230117193831.75201-1-jlayton@kernel.org>
 <20230117193831.75201-3-jlayton@kernel.org>
 <9bff17d4-c305-1918-5079-d2e9cf291bc7@oracle.com>
 <eb5a9fa65a8c2bcc257101c96f7fbbe18a3b74ff.camel@kernel.org>
From:   dai.ngo@oracle.com
In-Reply-To: <eb5a9fa65a8c2bcc257101c96f7fbbe18a3b74ff.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?SXlnb0lyU1AwQXJOUk9GbFB4ZEJZKzJaZ2JDSDdyd3o1ZlFrVmlLbHpDTDdr?=
 =?utf-8?B?eGFTdzBnUlBoNjl6cnJ5aXAwVENmNzRHdkliaXgrMU10OWZzOGltOGdvVEZ6?=
 =?utf-8?B?RmxTamFwK0VFN1p4ZjZRQUxnNW5QT0hlZ2QxTDdKaWxLTUxrQ3BrcFF0aEo1?=
 =?utf-8?B?TjJ4ZUFidmtDaDYzYnVuZjMweE5yM3V1Qml3YUVlaDlROHppdEF2cjkxSWZO?=
 =?utf-8?B?emloWDlZaDdXdUl5ZmpmbXI3UGxzL1pweWloZS9FZzdPSnNkS1dzNGVPOWl5?=
 =?utf-8?B?T0JLeElsZDhGbTdjZW9vU3FKdERyOFNKWVYwY1FLcnl5bGZ3YW1iMFhKeHBy?=
 =?utf-8?B?RzhzVGtwMjBDczlKVk5pb2VZVStXVkxqbjdiTVFReFZaWDBDQW5XL09KSWFJ?=
 =?utf-8?B?eUY2WnZPa2JVRTA2b0xhU0VTUVpJMFFZelBpM2kyaU9wY3Z4dEN0Zi9jSHF4?=
 =?utf-8?B?aEZkM0c1VHJlSGl5K2F0THlTV3ltRE5UV1RvR09DTU4wSEV2NzBrZGNtUStI?=
 =?utf-8?B?b0JwM2d2UnQxR1MrdS9uK2hmY3hjQ1N2cEpJT3pwY0V5VjRHR3l3Nk9sV3lm?=
 =?utf-8?B?bTFGcWl0ZjFXR1h5NkF2UFU2TWJRMDdVRUU3KzBVTWRRK2xqRktQYlBvYnR4?=
 =?utf-8?B?MkxVczVTNGdHcE8zNTJIRlpyNStuZmRiTmJZczdFWVQwSzNWNFlJRHZhUTdZ?=
 =?utf-8?B?ZmQxQVAxSk9NOTdSOXBHNnJ2ZGRISFArcmEvZ3dFbC9hVGpEZ3BNMCtKOVJN?=
 =?utf-8?B?UngrYXRVM1c5cnV4dEVCZjhjdmdYbDN5UjZOK0hzRVVXeXJ3K0tQTlRPSDlq?=
 =?utf-8?B?RHVxQVpITU4zSmRUdkFDMThVTjVSVHBWVGJ4YUN5eXFNd0haUUpSUUR0Q1cz?=
 =?utf-8?B?d2w1ejhvcGFSdkVxaXNnbHlKRWRjYUFtek1qaFBjNU9acWQzbUZjWGF6RzBP?=
 =?utf-8?B?Tk5ka29sdXVVanIxOHMzMC9SVENtUGs2VnRENHJ2OVBaV01pQlF1dEZkQU9z?=
 =?utf-8?B?enZPc21Wc01KWmUrcFQ4T1hOL0sxZHdJMjhFSWNQUzhyK0o2Zyt4ZjJhT1ZO?=
 =?utf-8?B?UU8wQ0FRMkpndDk5bVVZMGE4MWsyclFxWGlNWVFaSkgxY1lxZmhiaVNXaVdk?=
 =?utf-8?B?VGNZQXVHV1M0Znhaby9Ea29ZdnUrLy9zNG9lSkliUXoxUEZxMWxubnZmQ0pl?=
 =?utf-8?B?dSt5ZE9mM0dpNzhMdDNZdjZaM0tWdnZBbTNENjExTHdNWjRUOWRDNFJPTTdw?=
 =?utf-8?B?VGlYSWk0akpXZDluUWIvemxRUC9WU0dEeWxadTVRVVVtNk9JbERyOFJUME0w?=
 =?utf-8?B?K0hnVUFFSHAyV0MzcU9IMWZoTlRNdmMvb0NJVU5VM2F4ZyswejZnL0pVekky?=
 =?utf-8?B?WXp4cFkwaUdCS1ZibzBvQ00rOUhvQmROcmw2RWRvTENRZXRlMS9GejZ2Y0NM?=
 =?utf-8?B?K2plajZjY0xCVWlmSzU0My9hSGpEeUpSWE5GVDhLVXhOOXlsV09ZY3ZOZHAw?=
 =?utf-8?B?QTRKdXk3UldIMk9DaDlsZUZDUnVvd2kzQTExUE1oSHpJcUtQVEJUdy90OUda?=
 =?utf-8?B?S1dWMERMTmMveE9kQmowUUZUT1g1YkdsNmN5QmVmOS9QZHB4WUlQTHV1UEx5?=
 =?utf-8?B?eTZTaWpyOFJHMmRkVnljYitiblcvdGtHTnNaYU02WUQ5RFBDbTdtcU5uYjU3?=
 =?utf-8?B?RmMzZmM2bzNOUVBST0xBd1gzUlA3L3dYMkd3ME44TkhHZXJURldmdUlRQ2pN?=
 =?utf-8?B?MGxON1JYNFVVaHErTS8vTTFleWhSc2tWY1B1WWdZdlVwajMybUFCd1BDNGph?=
 =?utf-8?B?TElaZWJqMG0zaGJJenRTUVJFM1dMQytIMGoyRDUrZUt5dUxydmFiMlpmcnpJ?=
 =?utf-8?B?WmRHNXhCbEdZUnpNemVFbTBGdmEySVkza1htUjZwaEtlWGtUNzBLRGRZTml1?=
 =?utf-8?B?bmFLZEQxTjhTMmQyQWRRV1AwakZjZC8wUkhZSWRZeG02d3BxMytmcjllYmln?=
 =?utf-8?B?WWpKWmpNTFUwMkVGZDlqRSt4WnNXNnpjd0FKUXRkY0c0OE1SNThJL1IvV3Q2?=
 =?utf-8?B?cVN2US96ZmRRZFE0UExrV1NRc2k1TS80MnFJdFFWVU52Qnl3bzhScVhWVnV5?=
 =?utf-8?Q?eJyTjCmJhxSItTxb1QR9aQgty?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: SX51DHJqhMUYIwOhTY2x5k7blNiEqUX8I8RjihlnioHFZw+yRiWDgiQ4UNL9aU/s0WlMl3DRydT7a30y/5OwXaEbQ+uf2UjBcFWzZugIKSDQv0cK23UCFws9dgNtzhFgXBKgMFqr9l5Z4pTVKz+xyuS30sUhUWCXhGrD0/hAlH0T2cat0UhTTucbAFowxc5NWrJ4+aSjJPorw6SawiIVvD7HZ8cZDnmd1xNz55iK7ofrQt9J1YBfd3U+SFJpKIn6Nf5BRGFzO6RlRcfxeuhSTWqNv4pIXTqUJ77pzzTOxQv7Rwk547tJe0Eu4cQbSxtW17yhNla4cuw7hpU6oM8C0pFnD5wJ4rJfZHS8YWHDF7jyC/NeM+E07sXNHUSi8wRkmZCsQiNdseMbKRSzOwzDOa0lFoZdCWIt75jBeii226adiowwXFkHgFQWn2IxJy/DN5OGAN2V7w/DckLUFbJHZsMGuslmTVDnqw3ks0DFw43ntE1EVXNuVpnmrIsMW4IxXcMqQieX3XvnnPUvF4uMoBULDOPYF89DLcK2UnPfpoYEO0w6q+a6EdqGnMHQQfcrNcffA4PYi/LIUSz18NKC25YUD5EnuKncG18MeF7SWvaWukiQG06YGH7dB4S4PGvcZv/KERO5df9ELtR2tHglxVSsjzycqbMVHU2eVPx9a4PPxez4hMd4qFdLlRIx4hh/wv/QJB7/EVmSfX8i2DfwXEEsrmNxqoNYoOD7ZVRgwIsx3jyVMpGQYctKpngdNlDAf2U6/KG+F7BFvNbuCBwBN6xMJaW4AhEGIFAdE+C56przPhQattthSYsgyliHSuIV
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d7a084f5-767a-4cc7-4b13-08dafa4c6a30
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jan 2023 18:38:53.4397
 (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: gPC777/u7ctPhNRhOQwfzgyRRE2dd93lrg7MaI45ERbTVVofctXxImARlUBkETQ9lvXdC8BB6ZBnKkVk39n49w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR10MB6185
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-19_12,2023-01-19_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxlogscore=999
 malwarescore=0 mlxscore=0 phishscore=0 suspectscore=0 spamscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2212070000 definitions=main-2301190155
X-Proofpoint-ORIG-GUID: l_7OluYd9C7gSehpwpzhFclqq-fXZQgz
X-Proofpoint-GUID: l_7OluYd9C7gSehpwpzhFclqq-fXZQgz
X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,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 1/19/23 2:56 AM, Jeff Layton wrote:
> On Wed, 2023-01-18 at 21:05 -0800, dai.ngo@oracle.com wrote:
>> On 1/17/23 11:38 AM, Jeff Layton wrote:
>>> There are two different flavors of the nfsd4_copy struct. One is
>>> embedded in the compound and is used directly in synchronous copies. The
>>> other is dynamically allocated, refcounted and tracked in the client
>>> struture. For the embedded one, the cleanup just involves releasing any
>>> nfsd_files held on its behalf. For the async one, the cleanup is a bit
>>> more involved, and we need to dequeue it from lists, unhash it, etc.
>>>
>>> There is at least one potential refcount leak in this code now. If the
>>> kthread_create call fails, then both the src and dst nfsd_files in the
>>> original nfsd4_copy object are leaked.
>>>
>>> The cleanup in this codepath is also sort of weird. In the async copy
>>> case, we'll have up to four nfsd_file references (src and dst for both
>>> flavors of copy structure). They are both put at the end of
>>> nfsd4_do_async_copy, even though the ones held on behalf of the embedded
>>> one outlive that structure.
>>>
>>> Change it so that we always clean up the nfsd_file refs held by the
>>> embedded copy structure before nfsd4_copy returns. Rework
>>> cleanup_async_copy to handle both inter and intra copies. Eliminate
>>> nfsd4_cleanup_intra_ssc since it now becomes a no-op.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>>    fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
>>>    1 file changed, 10 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index 37a9cc8ae7ae..62b9d6c1b18b 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
>>>    	long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>    
>>>    	nfs42_ssc_close(filp);
>>> -	nfsd_file_put(dst);
>> I think we still need this, in addition to release_copy_files called
>> from cleanup_async_copy. For async inter-copy, there are 2 reference
>> count added to the destination file, one from nfsd4_setup_inter_ssc
>> and the other one from dup_copy_fields. The above nfsd_file_put is for
>> the count added by dup_copy_fields.
>>
> With this patch, the references held by the original copy structure are
> put by the call to release_copy_files at the end of nfsd4_copy. That
> means that the kthread task is only responsible for putting the
> references held by the (kmalloc'ed) async_copy structure. So, I think
> this gets the nfsd_file refcounting right.

Yes, I see. One refcount is decremented by release_copy_files at end
of nfsd4_copy and another is decremented by release_copy_files in
cleanup_async_copy.

>
>
>>>    	fput(filp);
>>>    
>>>    	spin_lock(&nn->nfsd_ssc_lock);
>>> @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
>>>    				 &copy->nf_dst);
>>>    }
>>>    
>>> -static void
>>> -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
>>> -{
>>> -	nfsd_file_put(src);
>>> -	nfsd_file_put(dst);
>>> -}
>>> -
>>>    static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
>>>    {
>>>    	struct nfsd4_cb_offload *cbo =
>>> @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
>>>    	dst->ss_nsui = src->ss_nsui;
>>>    }
>>>    
>>> +static void release_copy_files(struct nfsd4_copy *copy)
>>> +{
>>> +	if (copy->nf_src)
>>> +		nfsd_file_put(copy->nf_src);
>>> +	if (copy->nf_dst)
>>> +		nfsd_file_put(copy->nf_dst);
>>> +}
>>> +
>>>    static void cleanup_async_copy(struct nfsd4_copy *copy)
>>>    {
>>>    	nfs4_free_copy_state(copy);
>>> -	nfsd_file_put(copy->nf_dst);
>>> -	if (!nfsd4_ssc_is_inter(copy))
>>> -		nfsd_file_put(copy->nf_src);
>>> +	release_copy_files(copy);
>>>    	spin_lock(&copy->cp_clp->async_lock);
>>>    	list_del(&copy->copies);
>>>    	spin_unlock(&copy->cp_clp->async_lock);
>>> @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
>>>    	} else {
>>>    		nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>    				       copy->nf_dst->nf_file, false);
>>> -		nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>    	}
>>>    
>>>    do_callback:
>>> @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>    	} else {
>>>    		status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>    				       copy->nf_dst->nf_file, true);
>>> -		nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>    	}
>>>    out:
>>> +	release_copy_files(copy);
>>>    	return status;
>>>    out_err:
>> This is unrelated to the reference count issue.
>>
>> Here if this is an inter-copy then we need to decrement the reference
>> count of the nfsd4_ssc_umount_item so that the vfsmount can be unmounted
>> later.
>>
>
> Oh, I think I see what you mean. Maybe something like the (untested)
> patch below on top of the original patch would fix that?
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index c9057462b973..7475c593553c 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
>          struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
>          long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>   
> -       nfs42_ssc_close(filp);
> -       fput(filp);
> +       if (filp) {
> +               nfs42_ssc_close(filp);
> +               fput(filp);
> +       }
>   
>          spin_lock(&nn->nfsd_ssc_lo
>          list_del(&nsui->nsui_list);
> @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>          release_copy_files(copy);
>          return status;
>   out_err:
> -       if (async_copy)
> +       if (async_copy) {
>                  cleanup_async_copy(async_copy);
> +               if (nfsd4_ssc_is_inter(async_copy))

We don't need to call nfsd4_cleanup_inter_ssc since the thread
nfsd4_do_async_copy has not started yet so the file is not opened.
We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
you want to change nfsd4_cleanup_inter_ssc to detect this error
condition and only decrement the reference count.

-Dai

> +                       nfsd4_cleanup_inter_ssc(copy->ss_nsui, NULL,
> +                                               copy->nf_dst);
> +
> +       }
>          status = nfserrno(-ENOMEM);
>          /*
>           * source's vfsmount of inter-copy will be unmounted
>