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

[Krzysztof Kozlowski]

On 12/03/2021 14:32, Tim Gardner wrote:
> 
> 
> 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 ?

I propose to fix the indentation, so it's not the Ack. This entire block
above is shown to me with spaces instead of tabs.

Best regards,
Krzysztof