RE: [PATCH] cifs: do not attempt unlink-and-retry-rename for SMB2+ mounts

[Tom Talpey <ttalpey@microsoft.com>]

> -----Original Message-----
> From: linux-cifs-owner@vger.kernel.org <linux-cifs-owner@vger.kernel.org> On
> Behalf Of Ronnie Sahlberg
> Sent: Monday, August 5, 2019 6:47 PM
> To: linux-cifs <linux-cifs@vger.kernel.org>
> Cc: Steve French <smfrench@gmail.com>; Ronnie Sahlberg
> <lsahlber@redhat.com>
> Subject: [PATCH] cifs: do not attempt unlink-and-retry-rename for SMB2+
> mounts
> 
> Normally in smb you can not rename ontop of a destination that is held open
> except in SMB1 where this is allowed IFF the delete-on-close flag is also set.

The FILE_DELETE_ON_CLOSE flag doesn't really have any significance in the
protocol, it is passed to the underlying filesystem except in one special case
where the server validates that the DELETE or GENERIC is granted on the
share. Did you find some reference in the documents to the contrary?

In other words, I think the fix may be ok, but the server->vals->protocol_id != 0
conditional may need more thought.

Tom.

> This special case is not supported in SMB2 so should not attempt the unlink-
> and-try-again
> since the rename will still fail but we now also delete the destination file.
> 
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/inode.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 56ca4b8ccaba..fdea45267a39 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1777,6 +1777,7 @@ cifs_rename2(struct inode *source_dir, struct dentry
> *source_dentry,
>  	FILE_UNIX_BASIC_INFO *info_buf_target;
>  	unsigned int xid;
>  	int rc, tmprc;
> +	struct TCP_Server_Info *server;
> 
>  	if (flags & ~RENAME_NOREPLACE)
>  		return -EINVAL;
> @@ -1786,6 +1787,7 @@ cifs_rename2(struct inode *source_dir, struct dentry
> *source_dentry,
>  	if (IS_ERR(tlink))
>  		return PTR_ERR(tlink);
>  	tcon = tlink_tcon(tlink);
> +	server = tcon->ses->server;
> 
>  	xid = get_xid();
> 
> @@ -1809,6 +1811,14 @@ cifs_rename2(struct inode *source_dir, struct
> dentry *source_dentry,
>  			    to_name);
> 
>  	/*
> +	 * Do not attempt unlink-then-try-rename-again for SMB2+.
> +	 * Renaming ontop of an existing open file IF the delete-on-close
> +	 * flag is set is only supported for SMB1.
> +	 */
> +	if (rc == -EACCES && server->vals->protocol_id != 0)
> +		goto cifs_rename_exit;
> +
> +	/*
>  	 * No-replace is the natural behavior for CIFS, so skip unlink hacks.
>  	 */
>  	if (flags & RENAME_NOREPLACE)
> --
> 2.13.6