| Message ID | 20190805224639.2322-1-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: do not attempt unlink-and-retry-rename for SMB2+ mounts | expand |
> -----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
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
> -----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
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)
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(+)