Linux-CIFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] cifs: do not attempt unlink-and-retry-rename for SMB2+ mounts
@ 2019-08-05 22:46 Ronnie Sahlberg
  2019-08-06 15:18 ` Tom Talpey
  0 siblings, 1 reply; 4+ messages in thread
From: Ronnie Sahlberg @ 2019-08-05 22:46 UTC (permalink / raw)
  To: linux-cifs; +Cc: Steve French, 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] cifs: do not attempt unlink-and-retry-rename for SMB2+ mounts
  2019-08-05 22:46 [PATCH] cifs: do not attempt unlink-and-retry-rename for SMB2+ mounts Ronnie Sahlberg
@ 2019-08-06 15:18 ` Tom Talpey
  2019-08-06 22:00   ` ronnie sahlberg
  0 siblings, 1 reply; 4+ messages in thread
From: Tom Talpey @ 2019-08-06 15:18 UTC (permalink / raw)
  To: Ronnie Sahlberg, linux-cifs; +Cc: Steve French

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] cifs: do not attempt unlink-and-retry-rename for SMB2+ mounts
  2019-08-06 15:18 ` Tom Talpey
@ 2019-08-06 22:00   ` ronnie sahlberg
  2019-08-07 16:22     ` Tom Talpey
  0 siblings, 1 reply; 4+ messages in thread
From: ronnie sahlberg @ 2019-08-06 22:00 UTC (permalink / raw)
  To: Tom Talpey; +Cc: Ronnie Sahlberg, linux-cifs, Steve French

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] cifs: do not attempt unlink-and-retry-rename for SMB2+ mounts
  2019-08-06 22:00   ` ronnie sahlberg
@ 2019-08-07 16:22     ` Tom Talpey
  0 siblings, 0 replies; 4+ messages in thread
From: Tom Talpey @ 2019-08-07 16:22 UTC (permalink / raw)
  To: ronnie sahlberg; +Cc: Ronnie Sahlberg, linux-cifs, Steve French

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-05 22:46 [PATCH] cifs: do not attempt unlink-and-retry-rename for SMB2+ mounts Ronnie Sahlberg
2019-08-06 15:18 ` Tom Talpey
2019-08-06 22:00   ` ronnie sahlberg
2019-08-07 16:22     ` Tom Talpey

Linux-CIFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cifs/0 linux-cifs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cifs linux-cifs/ https://lore.kernel.org/linux-cifs \
		linux-cifs@vger.kernel.org linux-cifs@archiver.kernel.org
	public-inbox-index linux-cifs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cifs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox