| Message ID | 20171109235249.8013-1-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2017-11-09 15:52 GMT-08:00 Ronnie Sahlberg <lsahlber@redhat.com>: > Cifs servers return ACCESS_DENIED when trying to rename onto a non-empty > directory. This is different from xfstest where we expect this to return > -EEXIST instead. > > As a workaround, if we failed to rename the file and we got -EACCES > then try to open the target file and check the attributes if it is a > directory or not and if so remap the error to -EEXIST. > > This makes us pass xfstest generic/245 > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/smb2ops.c | 40 ++++++++++++++++++++++++++++++++++++++++ > fs/cifs/smb2pdu.c | 12 +++++++++++- > fs/cifs/smb2proto.h | 2 ++ > 3 files changed, 53 insertions(+), 1 deletion(-) > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index bdb963d0ba32..5a620c71d91f 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -426,6 +426,46 @@ smb2_query_file_info(const unsigned int xid, struct cifs_tcon *tcon, > return rc; > } > > +int > +smb2_is_dir(const unsigned int xid, struct cifs_tcon *tcon, > + __le16 *target_file, int *is_dir) > +{ > + int rc; > + u8 oplock = SMB2_OPLOCK_LEVEL_NONE; > + struct cifs_open_parms oparms; > + struct cifs_fid fid; > + struct smb2_file_all_info *smb2_data; > + > + smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + PATH_MAX * 2, > + GFP_KERNEL); > + if (smb2_data == NULL) > + return -ENOMEM; > + > + oparms.tcon = tcon; > + oparms.desired_access = FILE_READ_ATTRIBUTES; > + oparms.disposition = FILE_OPEN; > + oparms.create_options = 0; > + oparms.fid = &fid; > + oparms.reconnect = false; > + > + rc = SMB2_open(xid, &oparms, target_file, &oplock, NULL, NULL); > + if (rc) > + goto out; > + > + rc = SMB2_query_info(xid, tcon, fid.persistent_fid, fid.volatile_fid, > + smb2_data); Can't we use FILE_DIRECTORY_FILE flag in CreateOptions of SMB2_CREATE command and avoid querying attributes from the server? This will save us 1 or 2 round trips. > + SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid); > + if (rc) > + goto out; > + > + *is_dir = !!(le32_to_cpu(smb2_data->Attributes) & > + FILE_ATTRIBUTE_DIRECTORY); > + > +out: > + kfree(smb2_data); > + return rc; > +} > + > #ifdef CONFIG_CIFS_XATTR > static ssize_t > move_smb2_ea_to_cifs(char *dst, size_t dst_size, > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 553d574940b9..ec5de0a23378 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -3144,7 +3144,7 @@ SMB2_rename(const unsigned int xid, struct cifs_tcon *tcon, > struct smb2_file_rename_info info; > void **data; > unsigned int size[2]; > - int rc; > + int rc, is_dir; > int len = (2 * UniStrnlen((wchar_t *)target_file, PATH_MAX)); > > data = kmalloc(sizeof(void *) * 2, GFP_KERNEL); > @@ -3165,6 +3165,16 @@ SMB2_rename(const unsigned int xid, struct cifs_tcon *tcon, > rc = send_set_info(xid, tcon, persistent_fid, volatile_fid, > current->tgid, FILE_RENAME_INFORMATION, SMB2_O_INFO_FILE, > 0, 2, data, size); > + > + /* SMB2 servers responds with ACCESS_DENIED when trying to rename > + * and replace onto a non-empty directory. Check for this and remap > + * to EEXIST. > + */ > + if (rc == -EACCES) { > + if (!smb2_is_dir(xid, tcon, target_file, &is_dir) && is_dir) > + rc = -EEXIST; > + } What if the target is directory but the permissions are read-only? > + > kfree(data); > return rc; > } > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h > index e9ab5227e7a8..a7528c0941c6 100644 > --- a/fs/cifs/smb2proto.h > +++ b/fs/cifs/smb2proto.h > @@ -203,4 +203,6 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *); > > extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *, > enum securityEnum); > +extern int smb2_is_dir(const unsigned int xid, struct cifs_tcon *tcon, > + __le16 *target_file, int *is_dir); > #endif /* _SMB2PROTO_H */ > -- > 2.13.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best regards, Pavel Shilovsky -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 16, 2017 at 5:31 PM, Pavel Shilovsky <piastryyy@gmail.com> wrote: > 2017-11-09 15:52 GMT-08:00 Ronnie Sahlberg <lsahlber@redhat.com>: >> Cifs servers return ACCESS_DENIED when trying to rename onto a non-empty >> directory. This is different from xfstest where we expect this to return >> -EEXIST instead. >> >> As a workaround, if we failed to rename the file and we got -EACCES >> then try to open the target file and check the attributes if it is a >> directory or not and if so remap the error to -EEXIST. >> >> This makes us pass xfstest generic/245 >> >> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> >> --- >> fs/cifs/smb2ops.c | 40 ++++++++++++++++++++++++++++++++++++++++ >> fs/cifs/smb2pdu.c | 12 +++++++++++- >> fs/cifs/smb2proto.h | 2 ++ >> 3 files changed, 53 insertions(+), 1 deletion(-) >> >> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c >> index bdb963d0ba32..5a620c71d91f 100644 >> --- a/fs/cifs/smb2ops.c >> +++ b/fs/cifs/smb2ops.c >> @@ -426,6 +426,46 @@ smb2_query_file_info(const unsigned int xid, struct cifs_tcon *tcon, >> return rc; >> } >> >> +int >> +smb2_is_dir(const unsigned int xid, struct cifs_tcon *tcon, >> + __le16 *target_file, int *is_dir) >> +{ >> + int rc; >> + u8 oplock = SMB2_OPLOCK_LEVEL_NONE; >> + struct cifs_open_parms oparms; >> + struct cifs_fid fid; >> + struct smb2_file_all_info *smb2_data; >> + >> + smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + PATH_MAX * 2, >> + GFP_KERNEL); >> + if (smb2_data == NULL) >> + return -ENOMEM; >> + >> + oparms.tcon = tcon; >> + oparms.desired_access = FILE_READ_ATTRIBUTES; >> + oparms.disposition = FILE_OPEN; >> + oparms.create_options = 0; >> + oparms.fid = &fid; >> + oparms.reconnect = false; >> + >> + rc = SMB2_open(xid, &oparms, target_file, &oplock, NULL, NULL); >> + if (rc) >> + goto out; >> + >> + rc = SMB2_query_info(xid, tcon, fid.persistent_fid, fid.volatile_fid, >> + smb2_data); > > Can't we use FILE_DIRECTORY_FILE flag in CreateOptions of SMB2_CREATE > command and avoid querying attributes from the server? This will save > us 1 or 2 round trips. > > >> + SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid); >> + if (rc) >> + goto out; >> + >> + *is_dir = !!(le32_to_cpu(smb2_data->Attributes) & >> + FILE_ATTRIBUTE_DIRECTORY); >> + >> +out: >> + kfree(smb2_data); >> + return rc; >> +} >> + >> #ifdef CONFIG_CIFS_XATTR >> static ssize_t >> move_smb2_ea_to_cifs(char *dst, size_t dst_size, >> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >> index 553d574940b9..ec5de0a23378 100644 >> --- a/fs/cifs/smb2pdu.c >> +++ b/fs/cifs/smb2pdu.c >> @@ -3144,7 +3144,7 @@ SMB2_rename(const unsigned int xid, struct cifs_tcon *tcon, >> struct smb2_file_rename_info info; >> void **data; >> unsigned int size[2]; >> - int rc; >> + int rc, is_dir; >> int len = (2 * UniStrnlen((wchar_t *)target_file, PATH_MAX)); >> >> data = kmalloc(sizeof(void *) * 2, GFP_KERNEL); >> @@ -3165,6 +3165,16 @@ SMB2_rename(const unsigned int xid, struct cifs_tcon *tcon, >> rc = send_set_info(xid, tcon, persistent_fid, volatile_fid, >> current->tgid, FILE_RENAME_INFORMATION, SMB2_O_INFO_FILE, >> 0, 2, data, size); >> + >> + /* SMB2 servers responds with ACCESS_DENIED when trying to rename >> + * and replace onto a non-empty directory. Check for this and remap >> + * to EEXIST. >> + */ >> + if (rc == -EACCES) { >> + if (!smb2_is_dir(xid, tcon, target_file, &is_dir) && is_dir) >> + rc = -EEXIST; >> + } > > What if the target is directory but the permissions are read-only? > >> + >> kfree(data); >> return rc; >> } >> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h >> index e9ab5227e7a8..a7528c0941c6 100644 >> --- a/fs/cifs/smb2proto.h >> +++ b/fs/cifs/smb2proto.h >> @@ -203,4 +203,6 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *); >> >> extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *, >> enum securityEnum); >> +extern int smb2_is_dir(const unsigned int xid, struct cifs_tcon *tcon, >> + __le16 *target_file, int *is_dir); >> #endif /* _SMB2PROTO_H */ >> -- >> 2.13.3 >> Pavel makes good points ...
Pavel Shilovsky <piastryyy@gmail.com> writes: > > What if the target is directory but the permissions are read-only? I get the same resulsts on a local filesystem (xfs) and on a mounted smb2 windows2016 share (both with and without the patch applied): overwrite dir b with file a (with access to b) ======================= rename("test/a", "test/b") = -1 EISDIR (Is a directory) overwrite dir b with file a (without access to b) ======================= rename("test/a", "test/b") = -1 EISDIR (Is a directory) overwrite file b with file a (with access to b) ======================= rename("test/a", "test/b") = 0 overwrite file b with file a (without access to b) ======================= rename("test/a", "test/b") = 0 So.. I'm not sure what the xfstest test is doing. The only difference between the local and remote fs is that for cifs you can't delete test if you can't access test/b. (I am deleting "test" between my tests). That is on cifs: mkdir -p test/b chmod a-rw test/b rm -rf test Will fail, but works on xfs. You you strace rm you will see: unlinkat(4, "b", AT_REMOVEDIR) = -1 EACCES (Permission denied) I think this is due to the fundamental difference in how file removal permission works in Windows vs POSIX. In POSIX you need +w on the parent dir ("test") to be able to delete a file. On Windows there is a special delete permission on the object itself. Not sure if this is "fixable". Cheers,
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index bdb963d0ba32..5a620c71d91f 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -426,6 +426,46 @@ smb2_query_file_info(const unsigned int xid, struct cifs_tcon *tcon, return rc; } +int +smb2_is_dir(const unsigned int xid, struct cifs_tcon *tcon, + __le16 *target_file, int *is_dir) +{ + int rc; + u8 oplock = SMB2_OPLOCK_LEVEL_NONE; + struct cifs_open_parms oparms; + struct cifs_fid fid; + struct smb2_file_all_info *smb2_data; + + smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + PATH_MAX * 2, + GFP_KERNEL); + if (smb2_data == NULL) + return -ENOMEM; + + oparms.tcon = tcon; + oparms.desired_access = FILE_READ_ATTRIBUTES; + oparms.disposition = FILE_OPEN; + oparms.create_options = 0; + oparms.fid = &fid; + oparms.reconnect = false; + + rc = SMB2_open(xid, &oparms, target_file, &oplock, NULL, NULL); + if (rc) + goto out; + + rc = SMB2_query_info(xid, tcon, fid.persistent_fid, fid.volatile_fid, + smb2_data); + SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid); + if (rc) + goto out; + + *is_dir = !!(le32_to_cpu(smb2_data->Attributes) & + FILE_ATTRIBUTE_DIRECTORY); + +out: + kfree(smb2_data); + return rc; +} + #ifdef CONFIG_CIFS_XATTR static ssize_t move_smb2_ea_to_cifs(char *dst, size_t dst_size, diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 553d574940b9..ec5de0a23378 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -3144,7 +3144,7 @@ SMB2_rename(const unsigned int xid, struct cifs_tcon *tcon, struct smb2_file_rename_info info; void **data; unsigned int size[2]; - int rc; + int rc, is_dir; int len = (2 * UniStrnlen((wchar_t *)target_file, PATH_MAX)); data = kmalloc(sizeof(void *) * 2, GFP_KERNEL); @@ -3165,6 +3165,16 @@ SMB2_rename(const unsigned int xid, struct cifs_tcon *tcon, rc = send_set_info(xid, tcon, persistent_fid, volatile_fid, current->tgid, FILE_RENAME_INFORMATION, SMB2_O_INFO_FILE, 0, 2, data, size); + + /* SMB2 servers responds with ACCESS_DENIED when trying to rename + * and replace onto a non-empty directory. Check for this and remap + * to EEXIST. + */ + if (rc == -EACCES) { + if (!smb2_is_dir(xid, tcon, target_file, &is_dir) && is_dir) + rc = -EEXIST; + } + kfree(data); return rc; } diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h index e9ab5227e7a8..a7528c0941c6 100644 --- a/fs/cifs/smb2proto.h +++ b/fs/cifs/smb2proto.h @@ -203,4 +203,6 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *); extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *, enum securityEnum); +extern int smb2_is_dir(const unsigned int xid, struct cifs_tcon *tcon, + __le16 *target_file, int *is_dir); #endif /* _SMB2PROTO_H */
Cifs servers return ACCESS_DENIED when trying to rename onto a non-empty directory. This is different from xfstest where we expect this to return -EEXIST instead. As a workaround, if we failed to rename the file and we got -EACCES then try to open the target file and check the attributes if it is a directory or not and if so remap the error to -EEXIST. This makes us pass xfstest generic/245 Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/smb2ops.c | 40 ++++++++++++++++++++++++++++++++++++++++ fs/cifs/smb2pdu.c | 12 +++++++++++- fs/cifs/smb2proto.h | 2 ++ 3 files changed, 53 insertions(+), 1 deletion(-)