* [PATCH] cifs: fix return code when failing to rename a file onto a directory
@ 2017-11-09 5:11 Ronnie Sahlberg
[not found] ` <20171109051157.30814-1-lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 9+ messages in thread
From: Ronnie Sahlberg @ 2017-11-09 5:11 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
fs/cifs/smb2ops.c | 39 +++++++++++++++++++++++++++++++++++++++
fs/cifs/smb2pdu.c | 13 ++++++++++++-
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..1c46aab0d015 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -426,6 +426,45 @@ 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 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;
+
+ rc = !!(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..91f0b4a5e749 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,17 @@ 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) {
+ is_dir = smb2_is_dir(xid, tcon, target_file);
+ if (is_dir == 1)
+ rc = -EEXIST;
+ }
+
kfree(data);
return rc;
}
diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
index e9ab5227e7a8..35c075364f7e 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);
#endif /* _SMB2PROTO_H */
--
2.13.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] cifs: fix return code when failing to rename a file onto a directory
[not found] ` <20171109051157.30814-1-lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2017-11-09 10:54 ` Aurélien Aptel
0 siblings, 0 replies; 9+ messages in thread
From: Aurélien Aptel @ 2017-11-09 10:54 UTC (permalink / raw)
To: Ronnie Sahlberg, linux-cifs; +Cc: Steve French
Hi Ronnie,
Ronnie Sahlberg <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> +int
> +smb2_is_dir(const unsigned int xid, struct cifs_tcon *tcon,
> + __le16 *target_file)
I think I'd feel better if the actual result was an out parameter rather
than mixed with errors in the return value.
> + if (rc)
> + goto out;
> +
> + rc = !!(le32_to_cpu(smb2_data->Attributes) & FILE_ATTRIBUTE_DIRECTORY);
> +
> +out:
> + kfree(smb2_data);
> + return rc;
I think this can hide EPERM (errno 1) errors.
Also it's too bad we have functions taking in utf16 params and some not
but that's a whole another story. I had this idea of introducing a path
struct and only serialize it at the very last moment which I'll try to
implement ..one day :)
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cifs: fix return code when failing to rename a file onto a directory
[not found] ` <CAN05THT53nWN7w161L6CVc0ZjwFbBC+1zcO++z4DdXzQoh=CqQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-11-20 21:17 ` Steve French
0 siblings, 0 replies; 9+ messages in thread
From: Steve French @ 2017-11-20 21:17 UTC (permalink / raw)
To: ronnie sahlberg; +Cc: Ronnie Sahlberg, linux-cifs
ok - makes sense - sounds like a great argument to focus on
compounding this one special case as an experiment to see how we will
do it more generally.
On Mon, Nov 20, 2017 at 3:09 PM, ronnie sahlberg
<ronniesahlberg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hmm,
>
> Steve, disregard and drop this patch.
>
>
> We can do this much better once we have compounding support.
>
>
> On Mon, Nov 20, 2017 at 3:25 PM, Ronnie Sahlberg <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> 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.
>>
>> This makes us pass xfstest generic/245
>>
>> Signed-off-by: Ronnie Sahlberg <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>> fs/cifs/smb2inode.c | 7 +++++--
>> fs/cifs/smb2pdu.c | 10 +++++++++-
>> fs/cifs/smb2proto.h | 2 +-
>> 3 files changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
>> index 1238cd3552f9..6ada981f1f83 100644
>> --- a/fs/cifs/smb2inode.c
>> +++ b/fs/cifs/smb2inode.c
>> @@ -48,6 +48,7 @@ smb2_open_op_close(const unsigned int xid, struct cifs_tcon *tcon,
>> __u8 oplock = SMB2_OPLOCK_LEVEL_NONE;
>> struct cifs_open_parms oparms;
>> struct cifs_fid fid;
>> + struct smb2_file_all_info all_info;
>>
>> utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb);
>> if (!utf16_path)
>> @@ -60,7 +61,7 @@ smb2_open_op_close(const unsigned int xid, struct cifs_tcon *tcon,
>> oparms.fid = &fid;
>> oparms.reconnect = false;
>>
>> - rc = SMB2_open(xid, &oparms, utf16_path, &oplock, NULL, NULL);
>> + rc = SMB2_open(xid, &oparms, utf16_path, &oplock, &all_info, NULL);
>> if (rc) {
>> kfree(utf16_path);
>> return rc;
>> @@ -86,7 +87,9 @@ smb2_open_op_close(const unsigned int xid, struct cifs_tcon *tcon,
>> break;
>> case SMB2_OP_RENAME:
>> tmprc = SMB2_rename(xid, tcon, fid.persistent_fid,
>> - fid.volatile_fid, (__le16 *)data);
>> + fid.volatile_fid, (__le16 *)data,
>> + le32_to_cpu(all_info.Attributes) &
>> + FILE_ATTRIBUTE_DIRECTORY);
>> break;
>> case SMB2_OP_HARDLINK:
>> tmprc = SMB2_set_hardlink(xid, tcon, fid.persistent_fid,
>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>> index 553d574940b9..56c71a58d971 100644
>> --- a/fs/cifs/smb2pdu.c
>> +++ b/fs/cifs/smb2pdu.c
>> @@ -3139,7 +3139,8 @@ send_set_info(const unsigned int xid, struct cifs_tcon *tcon,
>>
>> int
>> SMB2_rename(const unsigned int xid, struct cifs_tcon *tcon,
>> - u64 persistent_fid, u64 volatile_fid, __le16 *target_file)
>> + u64 persistent_fid, u64 volatile_fid, __le16 *target_file,
>> + bool is_dir)
>> {
>> struct smb2_file_rename_info info;
>> void **data;
>> @@ -3165,6 +3166,13 @@ 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 && is_dir)
>> + rc = -EEXIST;
>> +
>> kfree(data);
>> return rc;
>> }
>> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
>> index e9ab5227e7a8..69fa9a39c7fa 100644
>> --- a/fs/cifs/smb2proto.h
>> +++ b/fs/cifs/smb2proto.h
>> @@ -158,7 +158,7 @@ extern int SMB2_query_directory(const unsigned int xid, struct cifs_tcon *tcon,
>> struct cifs_search_info *srch_inf);
>> extern int SMB2_rename(const unsigned int xid, struct cifs_tcon *tcon,
>> u64 persistent_fid, u64 volatile_fid,
>> - __le16 *target_file);
>> + __le16 *target_file, bool is_dir);
>> extern int SMB2_rmdir(const unsigned int xid, struct cifs_tcon *tcon,
>> u64 persistent_fid, u64 volatile_fid);
>> extern int SMB2_set_hardlink(const unsigned int xid, struct cifs_tcon *tcon,
>> --
>> 2.13.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cifs: fix return code when failing to rename a file onto a directory
[not found] ` <20171120052549.17909-1-lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2017-11-20 21:09 ` ronnie sahlberg
[not found] ` <CAN05THT53nWN7w161L6CVc0ZjwFbBC+1zcO++z4DdXzQoh=CqQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 9+ messages in thread
From: ronnie sahlberg @ 2017-11-20 21:09 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French
Hmm,
Steve, disregard and drop this patch.
We can do this much better once we have compounding support.
On Mon, Nov 20, 2017 at 3:25 PM, Ronnie Sahlberg <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 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.
>
> This makes us pass xfstest generic/245
>
> Signed-off-by: Ronnie Sahlberg <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> fs/cifs/smb2inode.c | 7 +++++--
> fs/cifs/smb2pdu.c | 10 +++++++++-
> fs/cifs/smb2proto.h | 2 +-
> 3 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> index 1238cd3552f9..6ada981f1f83 100644
> --- a/fs/cifs/smb2inode.c
> +++ b/fs/cifs/smb2inode.c
> @@ -48,6 +48,7 @@ smb2_open_op_close(const unsigned int xid, struct cifs_tcon *tcon,
> __u8 oplock = SMB2_OPLOCK_LEVEL_NONE;
> struct cifs_open_parms oparms;
> struct cifs_fid fid;
> + struct smb2_file_all_info all_info;
>
> utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb);
> if (!utf16_path)
> @@ -60,7 +61,7 @@ smb2_open_op_close(const unsigned int xid, struct cifs_tcon *tcon,
> oparms.fid = &fid;
> oparms.reconnect = false;
>
> - rc = SMB2_open(xid, &oparms, utf16_path, &oplock, NULL, NULL);
> + rc = SMB2_open(xid, &oparms, utf16_path, &oplock, &all_info, NULL);
> if (rc) {
> kfree(utf16_path);
> return rc;
> @@ -86,7 +87,9 @@ smb2_open_op_close(const unsigned int xid, struct cifs_tcon *tcon,
> break;
> case SMB2_OP_RENAME:
> tmprc = SMB2_rename(xid, tcon, fid.persistent_fid,
> - fid.volatile_fid, (__le16 *)data);
> + fid.volatile_fid, (__le16 *)data,
> + le32_to_cpu(all_info.Attributes) &
> + FILE_ATTRIBUTE_DIRECTORY);
> break;
> case SMB2_OP_HARDLINK:
> tmprc = SMB2_set_hardlink(xid, tcon, fid.persistent_fid,
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 553d574940b9..56c71a58d971 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -3139,7 +3139,8 @@ send_set_info(const unsigned int xid, struct cifs_tcon *tcon,
>
> int
> SMB2_rename(const unsigned int xid, struct cifs_tcon *tcon,
> - u64 persistent_fid, u64 volatile_fid, __le16 *target_file)
> + u64 persistent_fid, u64 volatile_fid, __le16 *target_file,
> + bool is_dir)
> {
> struct smb2_file_rename_info info;
> void **data;
> @@ -3165,6 +3166,13 @@ 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 && is_dir)
> + rc = -EEXIST;
> +
> kfree(data);
> return rc;
> }
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index e9ab5227e7a8..69fa9a39c7fa 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -158,7 +158,7 @@ extern int SMB2_query_directory(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_search_info *srch_inf);
> extern int SMB2_rename(const unsigned int xid, struct cifs_tcon *tcon,
> u64 persistent_fid, u64 volatile_fid,
> - __le16 *target_file);
> + __le16 *target_file, bool is_dir);
> extern int SMB2_rmdir(const unsigned int xid, struct cifs_tcon *tcon,
> u64 persistent_fid, u64 volatile_fid);
> extern int SMB2_set_hardlink(const unsigned int xid, struct cifs_tcon *tcon,
> --
> 2.13.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] cifs: fix return code when failing to rename a file onto a directory
@ 2017-11-20 5:25 Ronnie Sahlberg
[not found] ` <20171120052549.17909-1-lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 9+ messages in thread
From: Ronnie Sahlberg @ 2017-11-20 5:25 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
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.
This makes us pass xfstest generic/245
Signed-off-by: Ronnie Sahlberg <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
fs/cifs/smb2inode.c | 7 +++++--
fs/cifs/smb2pdu.c | 10 +++++++++-
fs/cifs/smb2proto.h | 2 +-
3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
index 1238cd3552f9..6ada981f1f83 100644
--- a/fs/cifs/smb2inode.c
+++ b/fs/cifs/smb2inode.c
@@ -48,6 +48,7 @@ smb2_open_op_close(const unsigned int xid, struct cifs_tcon *tcon,
__u8 oplock = SMB2_OPLOCK_LEVEL_NONE;
struct cifs_open_parms oparms;
struct cifs_fid fid;
+ struct smb2_file_all_info all_info;
utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb);
if (!utf16_path)
@@ -60,7 +61,7 @@ smb2_open_op_close(const unsigned int xid, struct cifs_tcon *tcon,
oparms.fid = &fid;
oparms.reconnect = false;
- rc = SMB2_open(xid, &oparms, utf16_path, &oplock, NULL, NULL);
+ rc = SMB2_open(xid, &oparms, utf16_path, &oplock, &all_info, NULL);
if (rc) {
kfree(utf16_path);
return rc;
@@ -86,7 +87,9 @@ smb2_open_op_close(const unsigned int xid, struct cifs_tcon *tcon,
break;
case SMB2_OP_RENAME:
tmprc = SMB2_rename(xid, tcon, fid.persistent_fid,
- fid.volatile_fid, (__le16 *)data);
+ fid.volatile_fid, (__le16 *)data,
+ le32_to_cpu(all_info.Attributes) &
+ FILE_ATTRIBUTE_DIRECTORY);
break;
case SMB2_OP_HARDLINK:
tmprc = SMB2_set_hardlink(xid, tcon, fid.persistent_fid,
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 553d574940b9..56c71a58d971 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -3139,7 +3139,8 @@ send_set_info(const unsigned int xid, struct cifs_tcon *tcon,
int
SMB2_rename(const unsigned int xid, struct cifs_tcon *tcon,
- u64 persistent_fid, u64 volatile_fid, __le16 *target_file)
+ u64 persistent_fid, u64 volatile_fid, __le16 *target_file,
+ bool is_dir)
{
struct smb2_file_rename_info info;
void **data;
@@ -3165,6 +3166,13 @@ 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 && is_dir)
+ rc = -EEXIST;
+
kfree(data);
return rc;
}
diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
index e9ab5227e7a8..69fa9a39c7fa 100644
--- a/fs/cifs/smb2proto.h
+++ b/fs/cifs/smb2proto.h
@@ -158,7 +158,7 @@ extern int SMB2_query_directory(const unsigned int xid, struct cifs_tcon *tcon,
struct cifs_search_info *srch_inf);
extern int SMB2_rename(const unsigned int xid, struct cifs_tcon *tcon,
u64 persistent_fid, u64 volatile_fid,
- __le16 *target_file);
+ __le16 *target_file, bool is_dir);
extern int SMB2_rmdir(const unsigned int xid, struct cifs_tcon *tcon,
u64 persistent_fid, u64 volatile_fid);
extern int SMB2_set_hardlink(const unsigned int xid, struct cifs_tcon *tcon,
--
2.13.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] cifs: fix return code when failing to rename a file onto a directory
[not found] ` <CAKywueStf29fgZ-52ONqL+WLSYotaVwMpsqnu2gQdppuw5xtoA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-11-16 23:38 ` Steve French
@ 2017-11-17 15:10 ` Aurélien Aptel
1 sibling, 0 replies; 9+ messages in thread
From: Aurélien Aptel @ 2017-11-17 15:10 UTC (permalink / raw)
To: Pavel Shilovsky, Ronnie Sahlberg; +Cc: linux-cifs, Steve French
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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,
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cifs: fix return code when failing to rename a file onto a directory
[not found] ` <CAKywueStf29fgZ-52ONqL+WLSYotaVwMpsqnu2gQdppuw5xtoA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-11-16 23:38 ` Steve French
2017-11-17 15:10 ` Aurélien Aptel
1 sibling, 0 replies; 9+ messages in thread
From: Steve French @ 2017-11-16 23:38 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: Ronnie Sahlberg, linux-cifs
On Thu, Nov 16, 2017 at 5:31 PM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2017-11-09 15:52 GMT-08:00 Ronnie Sahlberg <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>> 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 ...
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cifs: fix return code when failing to rename a file onto a directory
[not found] ` <20171109235249.8013-1-lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2017-11-16 23:31 ` Pavel Shilovsky
[not found] ` <CAKywueStf29fgZ-52ONqL+WLSYotaVwMpsqnu2gQdppuw5xtoA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 9+ messages in thread
From: Pavel Shilovsky @ 2017-11-16 23:31 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French
2017-11-09 15:52 GMT-08:00 Ronnie Sahlberg <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] cifs: fix return code when failing to rename a file onto a directory
@ 2017-11-09 23:52 Ronnie Sahlberg
[not found] ` <20171109235249.8013-1-lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 9+ messages in thread
From: Ronnie Sahlberg @ 2017-11-09 23:52 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
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);
+ 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 */
--
2.13.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-11-20 21:17 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-09 5:11 [PATCH] cifs: fix return code when failing to rename a file onto a directory Ronnie Sahlberg
[not found] ` <20171109051157.30814-1-lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2017-11-09 10:54 ` Aurélien Aptel
2017-11-09 23:52 Ronnie Sahlberg
[not found] ` <20171109235249.8013-1-lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2017-11-16 23:31 ` Pavel Shilovsky
[not found] ` <CAKywueStf29fgZ-52ONqL+WLSYotaVwMpsqnu2gQdppuw5xtoA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-11-16 23:38 ` Steve French
2017-11-17 15:10 ` Aurélien Aptel
2017-11-20 5:25 Ronnie Sahlberg
[not found] ` <20171120052549.17909-1-lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2017-11-20 21:09 ` ronnie sahlberg
[not found] ` <CAN05THT53nWN7w161L6CVc0ZjwFbBC+1zcO++z4DdXzQoh=CqQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-11-20 21:17 ` Steve French
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.