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

[Krzysztof Kozlowski]

On 12/03/2021 15:26, 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>
> ---
> 
> v2 - ran scripts/checkpatch.pl and corrected formatting errors.
> 
>  fs/cifs/smb2misc.c  | 59 ++++++++++++++++++++++++++++++++++-----------
>  fs/cifs/smb2pdu.c   | 11 +++++++--
>  fs/cifs/smb2proto.h |  3 +++
>  3 files changed, 57 insertions(+), 16 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..04e1cb66baf8 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -2133,9 +2133,17 @@ 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);
> +		}
>  		cifs_stats_fail_inc(tcon, SMB2_CLOSE_HE);
>  		goto close_exit;
> -	}
> +		}
>
Still not good here. The closing bracket should stay as it was.

Best regards,
Krzysztof