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

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

[Ronnie Sahlberg]

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.
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

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

[Tom Talpey]

> -----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

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

[ronnie sahlberg]

On Wed, Aug 7, 2019 at 1:19 AM Tom Talpey <ttalpey@microsoft.com> wrote:
>
> > -----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?
>

No but emperical testing, and xfstests, show it. It does not just
affect rename() but the same thing happens with "create and replace an
existing file".

There is definitely a conditional somewhere in the SMB server that
affects the behaviour or the delete-on-close flag.


This particular scenario is for the behaviour when we try to rename
onto an existing file that have open handles.
The process used in cifs.ko is :

1, Try to rename the file. This fails with ACCESS_DENIED if there are
open handles
if this fails, then fallback to
2, set delete-on-close
3, try the rename again

In SMB1, this works. In step 3 the file is renamed EVEN if there are
still open handles to it.
In SMB2+ this fails. If there are open handles then this fails with
DELETE_PENDING and the rename will not happen.
(then sometime later when the other process closes its handle then the
file is deleted)

This still happens in windows 2016. Delete-on-close and other similar
unlink() related operations
behave differently between SMB1 and SMB2.



> 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
> > 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

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

[Tom Talpey]

> -----Original Message-----
> From: ronnie sahlberg <ronniesahlberg@gmail.com>
> Sent: Tuesday, August 6, 2019 6:00 PM
> To: Tom Talpey <ttalpey@microsoft.com>
> Cc: Ronnie Sahlberg <lsahlber@redhat.com>; linux-cifs <linux-
> cifs@vger.kernel.org>; Steve French <smfrench@gmail.com>
> Subject: Re: [PATCH] cifs: do not attempt unlink-and-retry-rename for SMB2+
> mounts
> 
> On Wed, Aug 7, 2019 at 1:19 AM Tom Talpey <ttalpey@microsoft.com> wrote:
> >
> > > -----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?
> >
> 
> No but emperical testing, and xfstests, show it. It does not just
> affect rename() but the same thing happens with "create and replace an
> existing file".
> 
> There is definitely a conditional somewhere in the SMB server that
> affects the behaviour or the delete-on-close flag.
> 
> 
> This particular scenario is for the behaviour when we try to rename
> onto an existing file that have open handles.
> The process used in cifs.ko is :
> 
> 1, Try to rename the file. This fails with ACCESS_DENIED if there are
> open handles
> if this fails, then fallback to
> 2, set delete-on-close
> 3, try the rename again
> 
> In SMB1, this works. In step 3 the file is renamed EVEN if there are
> still open handles to it.
> In SMB2+ this fails. If there are open handles then this fails with
> DELETE_PENDING and the rename will not happen.
> (then sometime later when the other process closes its handle then the
> file is deleted)
> 
> This still happens in windows 2016. Delete-on-close and other similar
> unlink() related operations
> behave differently between SMB1 and SMB2.

Huh. I'm going to look into this on the Windows side. I wonder if something
in the Linux client is falling back to the ancient SMB_COM_RENAME or
SMB_COM_DELETE ops, which of course are path-based and exhibit different
behaviors. The mystery is that FILE_DELETE_ON_CLOSE is not relevant to those.

Anyway, thanks for the report. You wouldn't have a network trace of these
differences, would you?

Tom.

> > 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
> > > 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