[PATCH 1/3] CIFS: Close open handle after interrupted close

[Tim Gardner]

On 3/12/21 3:32 AM, Krzysztof Kozlowski wrote:
> On 11/03/2021 19:18, Tim Gardner wrote:
>> From: Pavel Shilovsky <pshilov at microsoft.com>
>>
>> BugLink: https://bugs.launchpad.net/bugs/1918714
>>
>> If Close command is interrupted before sending a request
>> to the server the client ends up leaking an open file
>> handle. This wastes server resources and can potentially
>> block applications that try to remove the file or any
>> directory containing this file.
>>
>> Fix this by putting the close command into a worker queue,
>> so another thread retries it later.
>>
>> Cc: Stable <stable at vger.kernel.org>
>> Tested-by: Frank Sorenson <sorenson at redhat.com>
>> Reviewed-by: Ronnie Sahlberg <lsahlber at redhat.com>
>> Signed-off-by: Pavel Shilovsky <pshilov at microsoft.com>
>> Signed-off-by: Steve French <stfrench at microsoft.com>
>> (backported from commit 9150c3adbf24d77cfba37f03639d4a908ca4ac25)
>> [ fs/cifs/smb2pdu.c has had significant changes, so the part of this
>> patch affecting fs/cifs/smb2pdu.c was pieced in where the logic looked
>> correct ]
>> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
>> ---
>>
>>   fs/cifs/smb2misc.c  | 59 ++++++++++++++++++++++++++++++++++-----------
>>   fs/cifs/smb2pdu.c   |  9 ++++++-
>>   fs/cifs/smb2proto.h |  3 +++
>>   3 files changed, 56 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
>> index 0c6e5450ff76..d15ace6151d9 100644
>> --- a/fs/cifs/smb2misc.c
>> +++ b/fs/cifs/smb2misc.c
>> @@ -674,34 +674,65 @@ smb2_cancelled_close_fid(struct work_struct *work)
>>   	kfree(cancelled);
>>   }
>>   
>> +/* Caller should already has an extra reference to @tcon */
>> +static int
>> +__smb2_handle_cancelled_close(struct cifs_tcon *tcon, __u64 persistent_fid,
>> +			      __u64 volatile_fid)
>> +{
>> +	struct close_cancelled_open *cancelled;
>> +
>> +	cancelled = kzalloc(sizeof(*cancelled), GFP_KERNEL);
>> +	if (!cancelled)
>> +		return -ENOMEM;
>> +
>> +	cancelled->fid.persistent_fid = persistent_fid;
>> +	cancelled->fid.volatile_fid = volatile_fid;
>> +	cancelled->tcon = tcon;
>> +	INIT_WORK(&cancelled->work, smb2_cancelled_close_fid);
>> +	WARN_ON(queue_work(cifsiod_wq, &cancelled->work) == false);
>> +
>> +	return 0;
>> +}
>> +
>> +int
>> +smb2_handle_cancelled_close(struct cifs_tcon *tcon, __u64 persistent_fid,
>> +			    __u64 volatile_fid)
>> +{
>> +	int rc;
>> +
>> +	cifs_dbg(FYI, "%s: tc_count=%d\n", __func__, tcon->tc_count);
>> +	spin_lock(&cifs_tcp_ses_lock);
>> +	tcon->tc_count++;
>> +	spin_unlock(&cifs_tcp_ses_lock);
>> +
>> +	rc = __smb2_handle_cancelled_close(tcon, persistent_fid, volatile_fid);
>> +	if (rc)
>> +		cifs_put_tcon(tcon);
>> +
>> +	return rc;
>> +}
>> +
>>   int
>>   smb2_handle_cancelled_mid(char *buffer, struct TCP_Server_Info *server)
>>   {
>>   	struct smb2_sync_hdr *sync_hdr = get_sync_hdr(buffer);
>>   	struct smb2_create_rsp *rsp = (struct smb2_create_rsp *)buffer;
>>   	struct cifs_tcon *tcon;
>> -	struct close_cancelled_open *cancelled;
>> +	int rc;
>>   
>>   	if (sync_hdr->Command != SMB2_CREATE ||
>>   	    sync_hdr->Status != STATUS_SUCCESS)
>>   		return 0;
>>   
>> -	cancelled = kzalloc(sizeof(*cancelled), GFP_KERNEL);
>> -	if (!cancelled)
>> -		return -ENOMEM;
>> -
>>   	tcon = smb2_find_smb_tcon(server, sync_hdr->SessionId,
>>   				  sync_hdr->TreeId);
>> -	if (!tcon) {
>> -		kfree(cancelled);
>> +	if (!tcon)
>>   		return -ENOENT;
>> -	}
>>   
>> -	cancelled->fid.persistent_fid = rsp->PersistentFileId;
>> -	cancelled->fid.volatile_fid = rsp->VolatileFileId;
>> -	cancelled->tcon = tcon;
>> -	INIT_WORK(&cancelled->work, smb2_cancelled_close_fid);
>> -	queue_work(cifsiod_wq, &cancelled->work);
>> +	rc = __smb2_handle_cancelled_close(tcon, rsp->PersistentFileId,
>> +					   rsp->VolatileFileId);
>> +	if (rc)
>> +		cifs_put_tcon(tcon);
>>   
>> -	return 0;
>> +	return rc;
>>   }
>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>> index ab1df4311a6d..03e4725390d4 100644
>> --- a/fs/cifs/smb2pdu.c
>> +++ b/fs/cifs/smb2pdu.c
>> @@ -2133,6 +2133,14 @@ SMB2_close(const unsigned int xid, struct cifs_tcon *tcon,
>>   	rsp = (struct smb2_close_rsp *)rsp_iov.iov_base;
>>   
>>   	if (rc != 0) {
>> +        /* retry close in a worker thread if this one is interrupted */
>> +        if (rc == -EINTR) {
>> +            int tmp_rc = smb2_handle_cancelled_close(tcon, persistent_fid,
>> +                                 volatile_fid);
>> +            if (tmp_rc)
>> +                cifs_dbg(VFS, "handle cancelled close fid 0x%llx returned error %d\n",
>> +                     persistent_fid, tmp_rc);
>> +        }
> 
> Hi Tim,
> 
> The context looks good but you have here mismatched indentation.
> 

Is that an ACK, or do you want me to fix the indentation ? Which line in 
particular do you think is wrong ?

rtg
-----------
Tim Gardner
Canonical, Inc