* [PATCH] CIFS: Fix symbolic links usage
@ 2013-11-01 13:57 Pavel Shilovsky
[not found] ` <1383314259-4694-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 12+ messages in thread
From: Pavel Shilovsky @ 2013-11-01 13:57 UTC (permalink / raw)
To: linux-cifs-u79uwXL29TY76Z2rM5mHXA
Cc: joaomiguelcorreia-Re5JQEeQqe8AvxtiuMwx3w
From: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
Now we treat any reparse point as a symbolic link and map it to a Unix
one that is not true in a common case due to many reparse point types
supported by SMB servers.
Distinguish reparse point types into two groups:
1) that can be accessed directly through a reparse point
(junctions, deduplicated files, NFS symlinks);
2) that need to be processed manually (Windows symbolic links, DFS);
and map only Windows symbolic links to Unix ones.
Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
---
fs/cifs/cifsglob.h | 2 +-
fs/cifs/inode.c | 23 +++++++++++++----------
fs/cifs/readdir.c | 40 ++++++++--------------------------------
fs/cifs/smb1ops.c | 21 ++++++++++++++++++++-
fs/cifs/smb2inode.c | 16 ++++++++++++----
fs/cifs/smb2proto.h | 2 +-
6 files changed, 55 insertions(+), 49 deletions(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index a67cf12..b688f2b 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -261,7 +261,7 @@ struct smb_version_operations {
/* query path data from the server */
int (*query_path_info)(const unsigned int, struct cifs_tcon *,
struct cifs_sb_info *, const char *,
- FILE_ALL_INFO *, bool *);
+ FILE_ALL_INFO *, bool *, bool *);
/* query file data from the server */
int (*query_file_info)(const unsigned int, struct cifs_tcon *,
struct cifs_fid *, FILE_ALL_INFO *);
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 867b7cd..36f9ebb 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path,
/* Fill a cifs_fattr struct with info from FILE_ALL_INFO */
static void
cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
- struct cifs_sb_info *cifs_sb, bool adjust_tz)
+ struct cifs_sb_info *cifs_sb, bool adjust_tz,
+ bool symlink)
{
struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
@@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
fattr->cf_createtime = le64_to_cpu(info->CreationTime);
fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
- if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
+
+ if (symlink) {
+ fattr->cf_mode = S_IFLNK;
+ fattr->cf_dtype = DT_LNK;
+ } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
fattr->cf_dtype = DT_DIR;
/*
@@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
*/
if (!tcon->unix_ext)
fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
- } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
- fattr->cf_mode = S_IFLNK;
- fattr->cf_dtype = DT_LNK;
- fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
} else {
fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
fattr->cf_dtype = DT_REG;
@@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp)
rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data);
switch (rc) {
case 0:
- cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false);
+ cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false,
+ false);
break;
case -EREMOTE:
cifs_create_dfs_fattr(&fattr, inode->i_sb);
@@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
bool adjust_tz = false;
struct cifs_fattr fattr;
struct cifs_search_info *srchinf = NULL;
+ bool symlink = false;
tlink = cifs_sb_tlink(cifs_sb);
if (IS_ERR(tlink))
@@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
}
data = (FILE_ALL_INFO *)buf;
rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path,
- data, &adjust_tz);
+ data, &adjust_tz, &symlink);
}
if (!rc) {
- cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb,
- adjust_tz);
+ cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz,
+ symlink);
} else if (rc == -EREMOTE) {
cifs_create_dfs_fattr(&fattr, sb);
rc = 0;
diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index 53a75f3..5940eca 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -134,22 +134,6 @@ out:
dput(dentry);
}
-/*
- * Is it possible that this directory might turn out to be a DFS referral
- * once we go to try and use it?
- */
-static bool
-cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb)
-{
-#ifdef CONFIG_CIFS_DFS_UPCALL
- struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
-
- if (tcon->Flags & SMB_SHARE_IS_IN_DFS)
- return true;
-#endif
- return false;
-}
-
static void
cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
{
@@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
fattr->cf_dtype = DT_DIR;
- /*
- * Windows CIFS servers generally make DFS referrals look
- * like directories in FIND_* responses with the reparse
- * attribute flag also set (since DFS junctions are
- * reparse points). We must revalidate at least these
- * directory inodes before trying to use them (if
- * they are DFS we will get PATH_NOT_COVERED back
- * when queried directly and can then try to connect
- * to the DFS target)
- */
- if (cifs_dfs_is_possible(cifs_sb) &&
- (fattr->cf_cifsattrs & ATTR_REPARSE))
- fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
- } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
- fattr->cf_mode = S_IFLNK;
- fattr->cf_dtype = DT_LNK;
} else {
fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
fattr->cf_dtype = DT_REG;
}
+ /*
+ * We need to revalidate it further to make a decision about whether it
+ * is a symbolic link, DFS referral or a reparse point with a direct
+ * access like junctions, deduplicated files, NFS symlinks.
+ */
+ if (fattr->cf_cifsattrs & ATTR_REPARSE)
+ fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
+
/* non-unix readdir doesn't provide nlink */
fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index ea99efe..47ab035 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
static int
cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
struct cifs_sb_info *cifs_sb, const char *full_path,
- FILE_ALL_INFO *data, bool *adjustTZ)
+ FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink)
{
int rc;
+ *symlink = false;
+
/* could do find first instead but this returns more info */
rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */,
cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
@@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
CIFS_MOUNT_MAP_SPECIAL_CHR);
*adjustTZ = true;
}
+
+ if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) {
+ int tmprc;
+ int oplock = 0;
+ __u16 netfid;
+
+ /* Need to check if this is a symbolic link or not */
+ tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
+ FILE_READ_ATTRIBUTES, 0, &netfid, &oplock,
+ NULL, cifs_sb->local_nls,
+ cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
+ if (tmprc == -EOPNOTSUPP)
+ *symlink = true;
+ else
+ CIFSSMBClose(xid, tcon, netfid);
+ }
+
return rc;
}
diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
index 78ff88c..84c012a 100644
--- a/fs/cifs/smb2inode.c
+++ b/fs/cifs/smb2inode.c
@@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src)
int
smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
struct cifs_sb_info *cifs_sb, const char *full_path,
- FILE_ALL_INFO *data, bool *adjust_tz)
+ FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink)
{
int rc;
struct smb2_file_all_info *smb2_data;
*adjust_tz = false;
+ *symlink = false;
smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2,
GFP_KERNEL);
@@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
return -ENOMEM;
rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
- FILE_READ_ATTRIBUTES, FILE_OPEN,
- OPEN_REPARSE_POINT, smb2_data,
- SMB2_OP_QUERY_INFO);
+ FILE_READ_ATTRIBUTES, FILE_OPEN, 0,
+ smb2_data, SMB2_OP_QUERY_INFO);
+ if (rc == -EOPNOTSUPP) {
+ *symlink = true;
+ /* Failed on a symbolic link - query a reparse point info */
+ rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
+ FILE_READ_ATTRIBUTES, FILE_OPEN,
+ OPEN_REPARSE_POINT, smb2_data,
+ SMB2_OP_QUERY_INFO);
+ }
if (rc)
goto out;
diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
index 313813e..b4eea10 100644
--- a/fs/cifs/smb2proto.h
+++ b/fs/cifs/smb2proto.h
@@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst,
extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
struct cifs_sb_info *cifs_sb,
const char *full_path, FILE_ALL_INFO *data,
- bool *adjust_tz);
+ bool *adjust_tz, bool *symlink);
extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
const char *full_path, __u64 size,
struct cifs_sb_info *cifs_sb, bool set_alloc);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] CIFS: Fix symbolic links usage
[not found] ` <1383314259-4694-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2013-11-01 15:08 ` Shirish Pargaonkar
[not found] ` <CADT32eKo3gQ06m1V3PZ64OdQp-iLw5VVnecZBO--T0pg9+CiBQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-11-01 17:42 ` Jeff Layton
2013-11-05 19:31 ` Jeff Layton
2 siblings, 1 reply; 12+ messages in thread
From: Shirish Pargaonkar @ 2013-11-01 15:08 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: linux-cifs, joaomiguelcorreia-Re5JQEeQqe8AvxtiuMwx3w
On Fri, Nov 1, 2013 at 8:57 AM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>
> Now we treat any reparse point as a symbolic link and map it to a Unix
> one that is not true in a common case due to many reparse point types
> supported by SMB servers.
>
> Distinguish reparse point types into two groups:
> 1) that can be accessed directly through a reparse point
> (junctions, deduplicated files, NFS symlinks);
> 2) that need to be processed manually (Windows symbolic links, DFS);
>
> and map only Windows symbolic links to Unix ones.
>
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
> fs/cifs/cifsglob.h | 2 +-
> fs/cifs/inode.c | 23 +++++++++++++----------
> fs/cifs/readdir.c | 40 ++++++++--------------------------------
> fs/cifs/smb1ops.c | 21 ++++++++++++++++++++-
> fs/cifs/smb2inode.c | 16 ++++++++++++----
> fs/cifs/smb2proto.h | 2 +-
> 6 files changed, 55 insertions(+), 49 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index a67cf12..b688f2b 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -261,7 +261,7 @@ struct smb_version_operations {
> /* query path data from the server */
> int (*query_path_info)(const unsigned int, struct cifs_tcon *,
> struct cifs_sb_info *, const char *,
> - FILE_ALL_INFO *, bool *);
> + FILE_ALL_INFO *, bool *, bool *);
> /* query file data from the server */
> int (*query_file_info)(const unsigned int, struct cifs_tcon *,
> struct cifs_fid *, FILE_ALL_INFO *);
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 867b7cd..36f9ebb 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path,
> /* Fill a cifs_fattr struct with info from FILE_ALL_INFO */
> static void
> cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> - struct cifs_sb_info *cifs_sb, bool adjust_tz)
> + struct cifs_sb_info *cifs_sb, bool adjust_tz,
> + bool symlink)
> {
> struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
>
> @@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> fattr->cf_createtime = le64_to_cpu(info->CreationTime);
>
> fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> - if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> +
> + if (symlink) {
> + fattr->cf_mode = S_IFLNK;
> + fattr->cf_dtype = DT_LNK;
> + } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> fattr->cf_dtype = DT_DIR;
> /*
> @@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> */
> if (!tcon->unix_ext)
> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
> - fattr->cf_mode = S_IFLNK;
> - fattr->cf_dtype = DT_LNK;
> - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> } else {
> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
> fattr->cf_dtype = DT_REG;
Pavel, does this handle a case where it could be symbolic link
to a directory?
> @@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp)
> rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data);
> switch (rc) {
> case 0:
> - cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false);
> + cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false,
> + false);
> break;
> case -EREMOTE:
> cifs_create_dfs_fattr(&fattr, inode->i_sb);
> @@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> bool adjust_tz = false;
> struct cifs_fattr fattr;
> struct cifs_search_info *srchinf = NULL;
> + bool symlink = false;
>
> tlink = cifs_sb_tlink(cifs_sb);
> if (IS_ERR(tlink))
> @@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> }
> data = (FILE_ALL_INFO *)buf;
> rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path,
> - data, &adjust_tz);
> + data, &adjust_tz, &symlink);
> }
>
> if (!rc) {
> - cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb,
> - adjust_tz);
> + cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz,
> + symlink);
> } else if (rc == -EREMOTE) {
> cifs_create_dfs_fattr(&fattr, sb);
> rc = 0;
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index 53a75f3..5940eca 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -134,22 +134,6 @@ out:
> dput(dentry);
> }
>
> -/*
> - * Is it possible that this directory might turn out to be a DFS referral
> - * once we go to try and use it?
> - */
> -static bool
> -cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb)
> -{
> -#ifdef CONFIG_CIFS_DFS_UPCALL
> - struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
> -
> - if (tcon->Flags & SMB_SHARE_IS_IN_DFS)
> - return true;
> -#endif
> - return false;
> -}
> -
> static void
> cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
> {
> @@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
> if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> fattr->cf_dtype = DT_DIR;
> - /*
> - * Windows CIFS servers generally make DFS referrals look
> - * like directories in FIND_* responses with the reparse
> - * attribute flag also set (since DFS junctions are
> - * reparse points). We must revalidate at least these
> - * directory inodes before trying to use them (if
> - * they are DFS we will get PATH_NOT_COVERED back
> - * when queried directly and can then try to connect
> - * to the DFS target)
> - */
> - if (cifs_dfs_is_possible(cifs_sb) &&
> - (fattr->cf_cifsattrs & ATTR_REPARSE))
> - fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
> - fattr->cf_mode = S_IFLNK;
> - fattr->cf_dtype = DT_LNK;
> } else {
> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
> fattr->cf_dtype = DT_REG;
> }
>
> + /*
> + * We need to revalidate it further to make a decision about whether it
> + * is a symbolic link, DFS referral or a reparse point with a direct
> + * access like junctions, deduplicated files, NFS symlinks.
> + */
> + if (fattr->cf_cifsattrs & ATTR_REPARSE)
> + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
> +
> /* non-unix readdir doesn't provide nlink */
> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
>
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index ea99efe..47ab035 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
> static int
> cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_sb_info *cifs_sb, const char *full_path,
> - FILE_ALL_INFO *data, bool *adjustTZ)
> + FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink)
> {
> int rc;
>
> + *symlink = false;
> +
> /* could do find first instead but this returns more info */
> rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */,
> cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
> @@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> CIFS_MOUNT_MAP_SPECIAL_CHR);
> *adjustTZ = true;
> }
> +
> + if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) {
> + int tmprc;
> + int oplock = 0;
> + __u16 netfid;
> +
> + /* Need to check if this is a symbolic link or not */
> + tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
> + FILE_READ_ATTRIBUTES, 0, &netfid, &oplock,
> + NULL, cifs_sb->local_nls,
> + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
> + if (tmprc == -EOPNOTSUPP)
> + *symlink = true;
> + else
> + CIFSSMBClose(xid, tcon, netfid);
> + }
> +
> return rc;
> }
>
> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> index 78ff88c..84c012a 100644
> --- a/fs/cifs/smb2inode.c
> +++ b/fs/cifs/smb2inode.c
> @@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src)
> int
> smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_sb_info *cifs_sb, const char *full_path,
> - FILE_ALL_INFO *data, bool *adjust_tz)
> + FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink)
> {
> int rc;
> struct smb2_file_all_info *smb2_data;
>
> *adjust_tz = false;
> + *symlink = false;
>
> smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2,
> GFP_KERNEL);
> @@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> return -ENOMEM;
>
> rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
> - FILE_READ_ATTRIBUTES, FILE_OPEN,
> - OPEN_REPARSE_POINT, smb2_data,
> - SMB2_OP_QUERY_INFO);
> + FILE_READ_ATTRIBUTES, FILE_OPEN, 0,
> + smb2_data, SMB2_OP_QUERY_INFO);
> + if (rc == -EOPNOTSUPP) {
> + *symlink = true;
> + /* Failed on a symbolic link - query a reparse point info */
> + rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
> + FILE_READ_ATTRIBUTES, FILE_OPEN,
> + OPEN_REPARSE_POINT, smb2_data,
> + SMB2_OP_QUERY_INFO);
> + }
> if (rc)
> goto out;
>
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index 313813e..b4eea10 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst,
> extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_sb_info *cifs_sb,
> const char *full_path, FILE_ALL_INFO *data,
> - bool *adjust_tz);
> + bool *adjust_tz, bool *symlink);
> extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> const char *full_path, __u64 size,
> struct cifs_sb_info *cifs_sb, bool set_alloc);
> --
> 1.7.10.4
>
> --
> 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] 12+ messages in thread
* Re: [PATCH] CIFS: Fix symbolic links usage
[not found] ` <1383314259-4694-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2013-11-01 15:08 ` Shirish Pargaonkar
@ 2013-11-01 17:42 ` Jeff Layton
[not found] ` <20131101134219.02d33b75-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2013-11-05 19:31 ` Jeff Layton
2 siblings, 1 reply; 12+ messages in thread
From: Jeff Layton @ 2013-11-01 17:42 UTC (permalink / raw)
To: Pavel Shilovsky
Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA,
joaomiguelcorreia-Re5JQEeQqe8AvxtiuMwx3w
On Fri, 1 Nov 2013 17:57:39 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>
> Now we treat any reparse point as a symbolic link and map it to a Unix
> one that is not true in a common case due to many reparse point types
> supported by SMB servers.
>
> Distinguish reparse point types into two groups:
> 1) that can be accessed directly through a reparse point
> (junctions, deduplicated files, NFS symlinks);
> 2) that need to be processed manually (Windows symbolic links, DFS);
>
> and map only Windows symbolic links to Unix ones.
>
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
> fs/cifs/cifsglob.h | 2 +-
> fs/cifs/inode.c | 23 +++++++++++++----------
> fs/cifs/readdir.c | 40 ++++++++--------------------------------
> fs/cifs/smb1ops.c | 21 ++++++++++++++++++++-
> fs/cifs/smb2inode.c | 16 ++++++++++++----
> fs/cifs/smb2proto.h | 2 +-
> 6 files changed, 55 insertions(+), 49 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index a67cf12..b688f2b 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -261,7 +261,7 @@ struct smb_version_operations {
> /* query path data from the server */
> int (*query_path_info)(const unsigned int, struct cifs_tcon *,
> struct cifs_sb_info *, const char *,
> - FILE_ALL_INFO *, bool *);
> + FILE_ALL_INFO *, bool *, bool *);
This looks much better for performance, but I think it really shines a
light on what an abortion the query_path_info and query_file_info
operations are. They work with a FILE_ALL_INFO struct which is SMB1
specific, so for SMB2/3 we have to convert data to something that
doesn't even match the protocol.
I think it would be best to base this fix on top of a patchset to fix
that properly. Those functions should be changed to pass data around in
a cifs_fattr. With that, you could just add a new cifs_fattr->flags
value and you wouldn't need to add this extra bool * argument.
> /* query file data from the server */
> int (*query_file_info)(const unsigned int, struct cifs_tcon *,
> struct cifs_fid *, FILE_ALL_INFO *);
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 867b7cd..36f9ebb 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path,
> /* Fill a cifs_fattr struct with info from FILE_ALL_INFO */
> static void
> cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> - struct cifs_sb_info *cifs_sb, bool adjust_tz)
> + struct cifs_sb_info *cifs_sb, bool adjust_tz,
> + bool symlink)
> {
> struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
>
> @@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> fattr->cf_createtime = le64_to_cpu(info->CreationTime);
>
> fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> - if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> +
> + if (symlink) {
> + fattr->cf_mode = S_IFLNK;
> + fattr->cf_dtype = DT_LNK;
> + } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> fattr->cf_dtype = DT_DIR;
> /*
> @@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> */
> if (!tcon->unix_ext)
> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
> - fattr->cf_mode = S_IFLNK;
> - fattr->cf_dtype = DT_LNK;
> - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> } else {
> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
> fattr->cf_dtype = DT_REG;
> @@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp)
> rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data);
> switch (rc) {
> case 0:
> - cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false);
> + cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false,
> + false);
> break;
> case -EREMOTE:
> cifs_create_dfs_fattr(&fattr, inode->i_sb);
> @@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> bool adjust_tz = false;
> struct cifs_fattr fattr;
> struct cifs_search_info *srchinf = NULL;
> + bool symlink = false;
>
> tlink = cifs_sb_tlink(cifs_sb);
> if (IS_ERR(tlink))
> @@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> }
> data = (FILE_ALL_INFO *)buf;
> rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path,
> - data, &adjust_tz);
> + data, &adjust_tz, &symlink);
> }
>
> if (!rc) {
> - cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb,
> - adjust_tz);
> + cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz,
> + symlink);
> } else if (rc == -EREMOTE) {
> cifs_create_dfs_fattr(&fattr, sb);
> rc = 0;
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index 53a75f3..5940eca 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -134,22 +134,6 @@ out:
> dput(dentry);
> }
>
> -/*
> - * Is it possible that this directory might turn out to be a DFS referral
> - * once we go to try and use it?
> - */
> -static bool
> -cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb)
> -{
> -#ifdef CONFIG_CIFS_DFS_UPCALL
> - struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
> -
> - if (tcon->Flags & SMB_SHARE_IS_IN_DFS)
> - return true;
> -#endif
> - return false;
> -}
> -
> static void
> cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
> {
> @@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
> if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> fattr->cf_dtype = DT_DIR;
> - /*
> - * Windows CIFS servers generally make DFS referrals look
> - * like directories in FIND_* responses with the reparse
> - * attribute flag also set (since DFS junctions are
> - * reparse points). We must revalidate at least these
> - * directory inodes before trying to use them (if
> - * they are DFS we will get PATH_NOT_COVERED back
> - * when queried directly and can then try to connect
> - * to the DFS target)
> - */
> - if (cifs_dfs_is_possible(cifs_sb) &&
> - (fattr->cf_cifsattrs & ATTR_REPARSE))
> - fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
> - fattr->cf_mode = S_IFLNK;
> - fattr->cf_dtype = DT_LNK;
> } else {
> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
> fattr->cf_dtype = DT_REG;
> }
>
> + /*
> + * We need to revalidate it further to make a decision about whether it
> + * is a symbolic link, DFS referral or a reparse point with a direct
> + * access like junctions, deduplicated files, NFS symlinks.
> + */
> + if (fattr->cf_cifsattrs & ATTR_REPARSE)
> + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
> +
> /* non-unix readdir doesn't provide nlink */
> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
>
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index ea99efe..47ab035 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
> static int
> cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_sb_info *cifs_sb, const char *full_path,
> - FILE_ALL_INFO *data, bool *adjustTZ)
> + FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink)
> {
> int rc;
>
> + *symlink = false;
> +
> /* could do find first instead but this returns more info */
> rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */,
> cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
> @@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> CIFS_MOUNT_MAP_SPECIAL_CHR);
> *adjustTZ = true;
> }
> +
> + if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) {
> + int tmprc;
> + int oplock = 0;
> + __u16 netfid;
> +
> + /* Need to check if this is a symbolic link or not */
> + tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
> + FILE_READ_ATTRIBUTES, 0, &netfid, &oplock,
> + NULL, cifs_sb->local_nls,
> + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
> + if (tmprc == -EOPNOTSUPP)
> + *symlink = true;
> + else
> + CIFSSMBClose(xid, tcon, netfid);
> + }
> +
> return rc;
> }
>
> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> index 78ff88c..84c012a 100644
> --- a/fs/cifs/smb2inode.c
> +++ b/fs/cifs/smb2inode.c
> @@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src)
> int
> smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_sb_info *cifs_sb, const char *full_path,
> - FILE_ALL_INFO *data, bool *adjust_tz)
> + FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink)
> {
> int rc;
> struct smb2_file_all_info *smb2_data;
>
> *adjust_tz = false;
> + *symlink = false;
>
> smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2,
> GFP_KERNEL);
> @@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> return -ENOMEM;
>
> rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
> - FILE_READ_ATTRIBUTES, FILE_OPEN,
> - OPEN_REPARSE_POINT, smb2_data,
> - SMB2_OP_QUERY_INFO);
> + FILE_READ_ATTRIBUTES, FILE_OPEN, 0,
> + smb2_data, SMB2_OP_QUERY_INFO);
> + if (rc == -EOPNOTSUPP) {
> + *symlink = true;
> + /* Failed on a symbolic link - query a reparse point info */
> + rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
> + FILE_READ_ATTRIBUTES, FILE_OPEN,
> + OPEN_REPARSE_POINT, smb2_data,
> + SMB2_OP_QUERY_INFO);
> + }
> if (rc)
> goto out;
>
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index 313813e..b4eea10 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst,
> extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_sb_info *cifs_sb,
> const char *full_path, FILE_ALL_INFO *data,
> - bool *adjust_tz);
> + bool *adjust_tz, bool *symlink);
> extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> const char *full_path, __u64 size,
> struct cifs_sb_info *cifs_sb, bool set_alloc);
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] CIFS: Fix symbolic links usage
[not found] ` <20131101134219.02d33b75-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2013-11-01 17:49 ` Steve French
[not found] ` <CAH2r5msFrvfWjYUO7pC84ggOZromA6_9jVM+KOqV_x61vNYSwg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-11-02 19:47 ` Pavel Shilovsky
1 sibling, 1 reply; 12+ messages in thread
From: Steve French @ 2013-11-01 17:49 UTC (permalink / raw)
To: Jeff Layton
Cc: Pavel Shilovsky, linux-cifs-u79uwXL29TY76Z2rM5mHXA, Joao Correia
FILE_ALL_INFO is not SMB1 specific, we request it in SMB2/SMB3 as well
and it is still a common level. See section 2.2.37 of MS-SMB2 or FSCC
description at:
http://msdn.microsoft.com/en-us/library/cc232117.aspx
Is there a different level that ou would like to call?
On Fri, Nov 1, 2013 at 12:42 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 1 Nov 2013 17:57:39 +0400
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> From: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>>
>> Now we treat any reparse point as a symbolic link and map it to a Unix
>> one that is not true in a common case due to many reparse point types
>> supported by SMB servers.
>>
>> Distinguish reparse point types into two groups:
>> 1) that can be accessed directly through a reparse point
>> (junctions, deduplicated files, NFS symlinks);
>> 2) that need to be processed manually (Windows symbolic links, DFS);
>>
>> and map only Windows symbolic links to Unix ones.
>>
>> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>> ---
>> fs/cifs/cifsglob.h | 2 +-
>> fs/cifs/inode.c | 23 +++++++++++++----------
>> fs/cifs/readdir.c | 40 ++++++++--------------------------------
>> fs/cifs/smb1ops.c | 21 ++++++++++++++++++++-
>> fs/cifs/smb2inode.c | 16 ++++++++++++----
>> fs/cifs/smb2proto.h | 2 +-
>> 6 files changed, 55 insertions(+), 49 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index a67cf12..b688f2b 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -261,7 +261,7 @@ struct smb_version_operations {
>> /* query path data from the server */
>> int (*query_path_info)(const unsigned int, struct cifs_tcon *,
>> struct cifs_sb_info *, const char *,
>> - FILE_ALL_INFO *, bool *);
>> + FILE_ALL_INFO *, bool *, bool *);
>
>
> This looks much better for performance, but I think it really shines a
> light on what an abortion the query_path_info and query_file_info
> operations are. They work with a FILE_ALL_INFO struct which is SMB1
> specific, so for SMB2/3 we have to convert data to something that
> doesn't even match the protocol.
>
> I think it would be best to base this fix on top of a patchset to fix
> that properly. Those functions should be changed to pass data around in
> a cifs_fattr. With that, you could just add a new cifs_fattr->flags
> value and you wouldn't need to add this extra bool * argument.
>
>
>> /* query file data from the server */
>> int (*query_file_info)(const unsigned int, struct cifs_tcon *,
>> struct cifs_fid *, FILE_ALL_INFO *);
>> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> index 867b7cd..36f9ebb 100644
>> --- a/fs/cifs/inode.c
>> +++ b/fs/cifs/inode.c
>> @@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path,
>> /* Fill a cifs_fattr struct with info from FILE_ALL_INFO */
>> static void
>> cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>> - struct cifs_sb_info *cifs_sb, bool adjust_tz)
>> + struct cifs_sb_info *cifs_sb, bool adjust_tz,
>> + bool symlink)
>> {
>> struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
>>
>> @@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>> fattr->cf_createtime = le64_to_cpu(info->CreationTime);
>>
>> fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
>> - if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
>> +
>> + if (symlink) {
>> + fattr->cf_mode = S_IFLNK;
>> + fattr->cf_dtype = DT_LNK;
>> + } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
>> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
>> fattr->cf_dtype = DT_DIR;
>> /*
>> @@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
>> */
>> if (!tcon->unix_ext)
>> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
>> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
>> - fattr->cf_mode = S_IFLNK;
>> - fattr->cf_dtype = DT_LNK;
>> - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
>> } else {
>> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
>> fattr->cf_dtype = DT_REG;
>> @@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp)
>> rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data);
>> switch (rc) {
>> case 0:
>> - cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false);
>> + cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false,
>> + false);
>> break;
>> case -EREMOTE:
>> cifs_create_dfs_fattr(&fattr, inode->i_sb);
>> @@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>> bool adjust_tz = false;
>> struct cifs_fattr fattr;
>> struct cifs_search_info *srchinf = NULL;
>> + bool symlink = false;
>>
>> tlink = cifs_sb_tlink(cifs_sb);
>> if (IS_ERR(tlink))
>> @@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>> }
>> data = (FILE_ALL_INFO *)buf;
>> rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path,
>> - data, &adjust_tz);
>> + data, &adjust_tz, &symlink);
>> }
>>
>> if (!rc) {
>> - cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb,
>> - adjust_tz);
>> + cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz,
>> + symlink);
>> } else if (rc == -EREMOTE) {
>> cifs_create_dfs_fattr(&fattr, sb);
>> rc = 0;
>> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
>> index 53a75f3..5940eca 100644
>> --- a/fs/cifs/readdir.c
>> +++ b/fs/cifs/readdir.c
>> @@ -134,22 +134,6 @@ out:
>> dput(dentry);
>> }
>>
>> -/*
>> - * Is it possible that this directory might turn out to be a DFS referral
>> - * once we go to try and use it?
>> - */
>> -static bool
>> -cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb)
>> -{
>> -#ifdef CONFIG_CIFS_DFS_UPCALL
>> - struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
>> -
>> - if (tcon->Flags & SMB_SHARE_IS_IN_DFS)
>> - return true;
>> -#endif
>> - return false;
>> -}
>> -
>> static void
>> cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
>> {
>> @@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
>> if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
>> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
>> fattr->cf_dtype = DT_DIR;
>> - /*
>> - * Windows CIFS servers generally make DFS referrals look
>> - * like directories in FIND_* responses with the reparse
>> - * attribute flag also set (since DFS junctions are
>> - * reparse points). We must revalidate at least these
>> - * directory inodes before trying to use them (if
>> - * they are DFS we will get PATH_NOT_COVERED back
>> - * when queried directly and can then try to connect
>> - * to the DFS target)
>> - */
>> - if (cifs_dfs_is_possible(cifs_sb) &&
>> - (fattr->cf_cifsattrs & ATTR_REPARSE))
>> - fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
>> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
>> - fattr->cf_mode = S_IFLNK;
>> - fattr->cf_dtype = DT_LNK;
>> } else {
>> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
>> fattr->cf_dtype = DT_REG;
>> }
>>
>> + /*
>> + * We need to revalidate it further to make a decision about whether it
>> + * is a symbolic link, DFS referral or a reparse point with a direct
>> + * access like junctions, deduplicated files, NFS symlinks.
>> + */
>> + if (fattr->cf_cifsattrs & ATTR_REPARSE)
>> + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
>> +
>> /* non-unix readdir doesn't provide nlink */
>> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
>>
>> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
>> index ea99efe..47ab035 100644
>> --- a/fs/cifs/smb1ops.c
>> +++ b/fs/cifs/smb1ops.c
>> @@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
>> static int
>> cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
>> struct cifs_sb_info *cifs_sb, const char *full_path,
>> - FILE_ALL_INFO *data, bool *adjustTZ)
>> + FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink)
>> {
>> int rc;
>>
>> + *symlink = false;
>> +
>> /* could do find first instead but this returns more info */
>> rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */,
>> cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
>> @@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
>> CIFS_MOUNT_MAP_SPECIAL_CHR);
>> *adjustTZ = true;
>> }
>> +
>> + if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) {
>> + int tmprc;
>> + int oplock = 0;
>> + __u16 netfid;
>> +
>> + /* Need to check if this is a symbolic link or not */
>> + tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
>> + FILE_READ_ATTRIBUTES, 0, &netfid, &oplock,
>> + NULL, cifs_sb->local_nls,
>> + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
>> + if (tmprc == -EOPNOTSUPP)
>> + *symlink = true;
>> + else
>> + CIFSSMBClose(xid, tcon, netfid);
>> + }
>> +
>> return rc;
>> }
>>
>> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
>> index 78ff88c..84c012a 100644
>> --- a/fs/cifs/smb2inode.c
>> +++ b/fs/cifs/smb2inode.c
>> @@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src)
>> int
>> smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
>> struct cifs_sb_info *cifs_sb, const char *full_path,
>> - FILE_ALL_INFO *data, bool *adjust_tz)
>> + FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink)
>> {
>> int rc;
>> struct smb2_file_all_info *smb2_data;
>>
>> *adjust_tz = false;
>> + *symlink = false;
>>
>> smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2,
>> GFP_KERNEL);
>> @@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
>> return -ENOMEM;
>>
>> rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
>> - FILE_READ_ATTRIBUTES, FILE_OPEN,
>> - OPEN_REPARSE_POINT, smb2_data,
>> - SMB2_OP_QUERY_INFO);
>> + FILE_READ_ATTRIBUTES, FILE_OPEN, 0,
>> + smb2_data, SMB2_OP_QUERY_INFO);
>> + if (rc == -EOPNOTSUPP) {
>> + *symlink = true;
>> + /* Failed on a symbolic link - query a reparse point info */
>> + rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
>> + FILE_READ_ATTRIBUTES, FILE_OPEN,
>> + OPEN_REPARSE_POINT, smb2_data,
>> + SMB2_OP_QUERY_INFO);
>> + }
>> if (rc)
>> goto out;
>>
>> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
>> index 313813e..b4eea10 100644
>> --- a/fs/cifs/smb2proto.h
>> +++ b/fs/cifs/smb2proto.h
>> @@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst,
>> extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
>> struct cifs_sb_info *cifs_sb,
>> const char *full_path, FILE_ALL_INFO *data,
>> - bool *adjust_tz);
>> + bool *adjust_tz, bool *symlink);
>> extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
>> const char *full_path, __u64 size,
>> struct cifs_sb_info *cifs_sb, bool set_alloc);
>
>
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> --
> 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] 12+ messages in thread
* Re: [PATCH] CIFS: Fix symbolic links usage
[not found] ` <CAH2r5msFrvfWjYUO7pC84ggOZromA6_9jVM+KOqV_x61vNYSwg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-11-01 18:00 ` Jeff Layton
2013-11-02 12:10 ` Jeff Layton
1 sibling, 0 replies; 12+ messages in thread
From: Jeff Layton @ 2013-11-01 18:00 UTC (permalink / raw)
To: Steve French
Cc: Pavel Shilovsky, linux-cifs-u79uwXL29TY76Z2rM5mHXA, Joao Correia
On Fri, 1 Nov 2013 12:49:47 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> FILE_ALL_INFO is not SMB1 specific, we request it in SMB2/SMB3 as well
> and it is still a common level. See section 2.2.37 of MS-SMB2 or FSCC
> description at:
>
>
> http://msdn.microsoft.com/en-us/library/cc232117.aspx
>
> Is there a different level that ou would like to call?
>
Ok, good point...
Still...I think we ought to not base this API on on-the-wire formats. It
ought to use the standard container for passing file attributes around
(struct cifs_fattr). In most cases where we need to call these
functions we don't actually have a FILE_ALL_INFO and have to construct
one. Blech...
> On Fri, Nov 1, 2013 at 12:42 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Fri, 1 Nov 2013 17:57:39 +0400
> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> From: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> >>
> >> Now we treat any reparse point as a symbolic link and map it to a Unix
> >> one that is not true in a common case due to many reparse point types
> >> supported by SMB servers.
> >>
> >> Distinguish reparse point types into two groups:
> >> 1) that can be accessed directly through a reparse point
> >> (junctions, deduplicated files, NFS symlinks);
> >> 2) that need to be processed manually (Windows symbolic links, DFS);
> >>
> >> and map only Windows symbolic links to Unix ones.
> >>
> >> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> >> ---
> >> fs/cifs/cifsglob.h | 2 +-
> >> fs/cifs/inode.c | 23 +++++++++++++----------
> >> fs/cifs/readdir.c | 40 ++++++++--------------------------------
> >> fs/cifs/smb1ops.c | 21 ++++++++++++++++++++-
> >> fs/cifs/smb2inode.c | 16 ++++++++++++----
> >> fs/cifs/smb2proto.h | 2 +-
> >> 6 files changed, 55 insertions(+), 49 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >> index a67cf12..b688f2b 100644
> >> --- a/fs/cifs/cifsglob.h
> >> +++ b/fs/cifs/cifsglob.h
> >> @@ -261,7 +261,7 @@ struct smb_version_operations {
> >> /* query path data from the server */
> >> int (*query_path_info)(const unsigned int, struct cifs_tcon *,
> >> struct cifs_sb_info *, const char *,
> >> - FILE_ALL_INFO *, bool *);
> >> + FILE_ALL_INFO *, bool *, bool *);
> >
> >
> > This looks much better for performance, but I think it really shines a
> > light on what an abortion the query_path_info and query_file_info
> > operations are. They work with a FILE_ALL_INFO struct which is SMB1
> > specific, so for SMB2/3 we have to convert data to something that
> > doesn't even match the protocol.
> >
> > I think it would be best to base this fix on top of a patchset to fix
> > that properly. Those functions should be changed to pass data around in
> > a cifs_fattr. With that, you could just add a new cifs_fattr->flags
> > value and you wouldn't need to add this extra bool * argument.
> >
> >
> >> /* query file data from the server */
> >> int (*query_file_info)(const unsigned int, struct cifs_tcon *,
> >> struct cifs_fid *, FILE_ALL_INFO *);
> >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> >> index 867b7cd..36f9ebb 100644
> >> --- a/fs/cifs/inode.c
> >> +++ b/fs/cifs/inode.c
> >> @@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path,
> >> /* Fill a cifs_fattr struct with info from FILE_ALL_INFO */
> >> static void
> >> cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> >> - struct cifs_sb_info *cifs_sb, bool adjust_tz)
> >> + struct cifs_sb_info *cifs_sb, bool adjust_tz,
> >> + bool symlink)
> >> {
> >> struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
> >>
> >> @@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> >> fattr->cf_createtime = le64_to_cpu(info->CreationTime);
> >>
> >> fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> >> - if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> >> +
> >> + if (symlink) {
> >> + fattr->cf_mode = S_IFLNK;
> >> + fattr->cf_dtype = DT_LNK;
> >> + } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> >> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> >> fattr->cf_dtype = DT_DIR;
> >> /*
> >> @@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> >> */
> >> if (!tcon->unix_ext)
> >> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
> >> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
> >> - fattr->cf_mode = S_IFLNK;
> >> - fattr->cf_dtype = DT_LNK;
> >> - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> >> } else {
> >> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
> >> fattr->cf_dtype = DT_REG;
> >> @@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp)
> >> rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data);
> >> switch (rc) {
> >> case 0:
> >> - cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false);
> >> + cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false,
> >> + false);
> >> break;
> >> case -EREMOTE:
> >> cifs_create_dfs_fattr(&fattr, inode->i_sb);
> >> @@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> >> bool adjust_tz = false;
> >> struct cifs_fattr fattr;
> >> struct cifs_search_info *srchinf = NULL;
> >> + bool symlink = false;
> >>
> >> tlink = cifs_sb_tlink(cifs_sb);
> >> if (IS_ERR(tlink))
> >> @@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> >> }
> >> data = (FILE_ALL_INFO *)buf;
> >> rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path,
> >> - data, &adjust_tz);
> >> + data, &adjust_tz, &symlink);
> >> }
> >>
> >> if (!rc) {
> >> - cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb,
> >> - adjust_tz);
> >> + cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz,
> >> + symlink);
> >> } else if (rc == -EREMOTE) {
> >> cifs_create_dfs_fattr(&fattr, sb);
> >> rc = 0;
> >> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> >> index 53a75f3..5940eca 100644
> >> --- a/fs/cifs/readdir.c
> >> +++ b/fs/cifs/readdir.c
> >> @@ -134,22 +134,6 @@ out:
> >> dput(dentry);
> >> }
> >>
> >> -/*
> >> - * Is it possible that this directory might turn out to be a DFS referral
> >> - * once we go to try and use it?
> >> - */
> >> -static bool
> >> -cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb)
> >> -{
> >> -#ifdef CONFIG_CIFS_DFS_UPCALL
> >> - struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
> >> -
> >> - if (tcon->Flags & SMB_SHARE_IS_IN_DFS)
> >> - return true;
> >> -#endif
> >> - return false;
> >> -}
> >> -
> >> static void
> >> cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
> >> {
> >> @@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
> >> if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> >> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> >> fattr->cf_dtype = DT_DIR;
> >> - /*
> >> - * Windows CIFS servers generally make DFS referrals look
> >> - * like directories in FIND_* responses with the reparse
> >> - * attribute flag also set (since DFS junctions are
> >> - * reparse points). We must revalidate at least these
> >> - * directory inodes before trying to use them (if
> >> - * they are DFS we will get PATH_NOT_COVERED back
> >> - * when queried directly and can then try to connect
> >> - * to the DFS target)
> >> - */
> >> - if (cifs_dfs_is_possible(cifs_sb) &&
> >> - (fattr->cf_cifsattrs & ATTR_REPARSE))
> >> - fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
> >> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
> >> - fattr->cf_mode = S_IFLNK;
> >> - fattr->cf_dtype = DT_LNK;
> >> } else {
> >> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
> >> fattr->cf_dtype = DT_REG;
> >> }
> >>
> >> + /*
> >> + * We need to revalidate it further to make a decision about whether it
> >> + * is a symbolic link, DFS referral or a reparse point with a direct
> >> + * access like junctions, deduplicated files, NFS symlinks.
> >> + */
> >> + if (fattr->cf_cifsattrs & ATTR_REPARSE)
> >> + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
> >> +
> >> /* non-unix readdir doesn't provide nlink */
> >> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
> >>
> >> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> >> index ea99efe..47ab035 100644
> >> --- a/fs/cifs/smb1ops.c
> >> +++ b/fs/cifs/smb1ops.c
> >> @@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
> >> static int
> >> cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> >> struct cifs_sb_info *cifs_sb, const char *full_path,
> >> - FILE_ALL_INFO *data, bool *adjustTZ)
> >> + FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink)
> >> {
> >> int rc;
> >>
> >> + *symlink = false;
> >> +
> >> /* could do find first instead but this returns more info */
> >> rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */,
> >> cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
> >> @@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> >> CIFS_MOUNT_MAP_SPECIAL_CHR);
> >> *adjustTZ = true;
> >> }
> >> +
> >> + if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) {
> >> + int tmprc;
> >> + int oplock = 0;
> >> + __u16 netfid;
> >> +
> >> + /* Need to check if this is a symbolic link or not */
> >> + tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
> >> + FILE_READ_ATTRIBUTES, 0, &netfid, &oplock,
> >> + NULL, cifs_sb->local_nls,
> >> + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
> >> + if (tmprc == -EOPNOTSUPP)
> >> + *symlink = true;
> >> + else
> >> + CIFSSMBClose(xid, tcon, netfid);
> >> + }
> >> +
> >> return rc;
> >> }
> >>
> >> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> >> index 78ff88c..84c012a 100644
> >> --- a/fs/cifs/smb2inode.c
> >> +++ b/fs/cifs/smb2inode.c
> >> @@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src)
> >> int
> >> smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> >> struct cifs_sb_info *cifs_sb, const char *full_path,
> >> - FILE_ALL_INFO *data, bool *adjust_tz)
> >> + FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink)
> >> {
> >> int rc;
> >> struct smb2_file_all_info *smb2_data;
> >>
> >> *adjust_tz = false;
> >> + *symlink = false;
> >>
> >> smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2,
> >> GFP_KERNEL);
> >> @@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> >> return -ENOMEM;
> >>
> >> rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
> >> - FILE_READ_ATTRIBUTES, FILE_OPEN,
> >> - OPEN_REPARSE_POINT, smb2_data,
> >> - SMB2_OP_QUERY_INFO);
> >> + FILE_READ_ATTRIBUTES, FILE_OPEN, 0,
> >> + smb2_data, SMB2_OP_QUERY_INFO);
> >> + if (rc == -EOPNOTSUPP) {
> >> + *symlink = true;
> >> + /* Failed on a symbolic link - query a reparse point info */
> >> + rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
> >> + FILE_READ_ATTRIBUTES, FILE_OPEN,
> >> + OPEN_REPARSE_POINT, smb2_data,
> >> + SMB2_OP_QUERY_INFO);
> >> + }
> >> if (rc)
> >> goto out;
> >>
> >> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> >> index 313813e..b4eea10 100644
> >> --- a/fs/cifs/smb2proto.h
> >> +++ b/fs/cifs/smb2proto.h
> >> @@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst,
> >> extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> >> struct cifs_sb_info *cifs_sb,
> >> const char *full_path, FILE_ALL_INFO *data,
> >> - bool *adjust_tz);
> >> + bool *adjust_tz, bool *symlink);
> >> extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> >> const char *full_path, __u64 size,
> >> struct cifs_sb_info *cifs_sb, bool set_alloc);
> >
> >
> > --
> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > --
> > 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
>
>
>
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] CIFS: Fix symbolic links usage
[not found] ` <CAH2r5msFrvfWjYUO7pC84ggOZromA6_9jVM+KOqV_x61vNYSwg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-11-01 18:00 ` Jeff Layton
@ 2013-11-02 12:10 ` Jeff Layton
1 sibling, 0 replies; 12+ messages in thread
From: Jeff Layton @ 2013-11-02 12:10 UTC (permalink / raw)
To: Steve French
Cc: Pavel Shilovsky, linux-cifs-u79uwXL29TY76Z2rM5mHXA, Joao Correia
On Fri, 1 Nov 2013 12:49:47 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> FILE_ALL_INFO is not SMB1 specific, we request it in SMB2/SMB3 as well
> and it is still a common level. See section 2.2.37 of MS-SMB2 or FSCC
> description at:
>
>
> http://msdn.microsoft.com/en-us/library/cc232117.aspx
>
> Is there a different level that ou would like to call?
>
Now that I look, I'm not sure that this is the case. If we get a
FILE_ALL_INFO from the server, what do we need move_smb2_info_to_cifs()
for?
Either way, this API would be *much* cleaner if we didn't try to use
on-the-wire formats to pass inode info around. That's what a cifs_fattr
is for, after all...
> On Fri, Nov 1, 2013 at 12:42 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Fri, 1 Nov 2013 17:57:39 +0400
> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> From: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> >>
> >> Now we treat any reparse point as a symbolic link and map it to a Unix
> >> one that is not true in a common case due to many reparse point types
> >> supported by SMB servers.
> >>
> >> Distinguish reparse point types into two groups:
> >> 1) that can be accessed directly through a reparse point
> >> (junctions, deduplicated files, NFS symlinks);
> >> 2) that need to be processed manually (Windows symbolic links, DFS);
> >>
> >> and map only Windows symbolic links to Unix ones.
> >>
> >> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> >> ---
> >> fs/cifs/cifsglob.h | 2 +-
> >> fs/cifs/inode.c | 23 +++++++++++++----------
> >> fs/cifs/readdir.c | 40 ++++++++--------------------------------
> >> fs/cifs/smb1ops.c | 21 ++++++++++++++++++++-
> >> fs/cifs/smb2inode.c | 16 ++++++++++++----
> >> fs/cifs/smb2proto.h | 2 +-
> >> 6 files changed, 55 insertions(+), 49 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >> index a67cf12..b688f2b 100644
> >> --- a/fs/cifs/cifsglob.h
> >> +++ b/fs/cifs/cifsglob.h
> >> @@ -261,7 +261,7 @@ struct smb_version_operations {
> >> /* query path data from the server */
> >> int (*query_path_info)(const unsigned int, struct cifs_tcon *,
> >> struct cifs_sb_info *, const char *,
> >> - FILE_ALL_INFO *, bool *);
> >> + FILE_ALL_INFO *, bool *, bool *);
> >
> >
> > This looks much better for performance, but I think it really shines a
> > light on what an abortion the query_path_info and query_file_info
> > operations are. They work with a FILE_ALL_INFO struct which is SMB1
> > specific, so for SMB2/3 we have to convert data to something that
> > doesn't even match the protocol.
> >
> > I think it would be best to base this fix on top of a patchset to fix
> > that properly. Those functions should be changed to pass data around in
> > a cifs_fattr. With that, you could just add a new cifs_fattr->flags
> > value and you wouldn't need to add this extra bool * argument.
> >
> >
> >> /* query file data from the server */
> >> int (*query_file_info)(const unsigned int, struct cifs_tcon *,
> >> struct cifs_fid *, FILE_ALL_INFO *);
> >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> >> index 867b7cd..36f9ebb 100644
> >> --- a/fs/cifs/inode.c
> >> +++ b/fs/cifs/inode.c
> >> @@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path,
> >> /* Fill a cifs_fattr struct with info from FILE_ALL_INFO */
> >> static void
> >> cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> >> - struct cifs_sb_info *cifs_sb, bool adjust_tz)
> >> + struct cifs_sb_info *cifs_sb, bool adjust_tz,
> >> + bool symlink)
> >> {
> >> struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
> >>
> >> @@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> >> fattr->cf_createtime = le64_to_cpu(info->CreationTime);
> >>
> >> fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> >> - if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> >> +
> >> + if (symlink) {
> >> + fattr->cf_mode = S_IFLNK;
> >> + fattr->cf_dtype = DT_LNK;
> >> + } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> >> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> >> fattr->cf_dtype = DT_DIR;
> >> /*
> >> @@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> >> */
> >> if (!tcon->unix_ext)
> >> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
> >> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
> >> - fattr->cf_mode = S_IFLNK;
> >> - fattr->cf_dtype = DT_LNK;
> >> - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> >> } else {
> >> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
> >> fattr->cf_dtype = DT_REG;
> >> @@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp)
> >> rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data);
> >> switch (rc) {
> >> case 0:
> >> - cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false);
> >> + cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false,
> >> + false);
> >> break;
> >> case -EREMOTE:
> >> cifs_create_dfs_fattr(&fattr, inode->i_sb);
> >> @@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> >> bool adjust_tz = false;
> >> struct cifs_fattr fattr;
> >> struct cifs_search_info *srchinf = NULL;
> >> + bool symlink = false;
> >>
> >> tlink = cifs_sb_tlink(cifs_sb);
> >> if (IS_ERR(tlink))
> >> @@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> >> }
> >> data = (FILE_ALL_INFO *)buf;
> >> rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path,
> >> - data, &adjust_tz);
> >> + data, &adjust_tz, &symlink);
> >> }
> >>
> >> if (!rc) {
> >> - cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb,
> >> - adjust_tz);
> >> + cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz,
> >> + symlink);
> >> } else if (rc == -EREMOTE) {
> >> cifs_create_dfs_fattr(&fattr, sb);
> >> rc = 0;
> >> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> >> index 53a75f3..5940eca 100644
> >> --- a/fs/cifs/readdir.c
> >> +++ b/fs/cifs/readdir.c
> >> @@ -134,22 +134,6 @@ out:
> >> dput(dentry);
> >> }
> >>
> >> -/*
> >> - * Is it possible that this directory might turn out to be a DFS referral
> >> - * once we go to try and use it?
> >> - */
> >> -static bool
> >> -cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb)
> >> -{
> >> -#ifdef CONFIG_CIFS_DFS_UPCALL
> >> - struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
> >> -
> >> - if (tcon->Flags & SMB_SHARE_IS_IN_DFS)
> >> - return true;
> >> -#endif
> >> - return false;
> >> -}
> >> -
> >> static void
> >> cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
> >> {
> >> @@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
> >> if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> >> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> >> fattr->cf_dtype = DT_DIR;
> >> - /*
> >> - * Windows CIFS servers generally make DFS referrals look
> >> - * like directories in FIND_* responses with the reparse
> >> - * attribute flag also set (since DFS junctions are
> >> - * reparse points). We must revalidate at least these
> >> - * directory inodes before trying to use them (if
> >> - * they are DFS we will get PATH_NOT_COVERED back
> >> - * when queried directly and can then try to connect
> >> - * to the DFS target)
> >> - */
> >> - if (cifs_dfs_is_possible(cifs_sb) &&
> >> - (fattr->cf_cifsattrs & ATTR_REPARSE))
> >> - fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
> >> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
> >> - fattr->cf_mode = S_IFLNK;
> >> - fattr->cf_dtype = DT_LNK;
> >> } else {
> >> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
> >> fattr->cf_dtype = DT_REG;
> >> }
> >>
> >> + /*
> >> + * We need to revalidate it further to make a decision about whether it
> >> + * is a symbolic link, DFS referral or a reparse point with a direct
> >> + * access like junctions, deduplicated files, NFS symlinks.
> >> + */
> >> + if (fattr->cf_cifsattrs & ATTR_REPARSE)
> >> + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
> >> +
> >> /* non-unix readdir doesn't provide nlink */
> >> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
> >>
> >> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> >> index ea99efe..47ab035 100644
> >> --- a/fs/cifs/smb1ops.c
> >> +++ b/fs/cifs/smb1ops.c
> >> @@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
> >> static int
> >> cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> >> struct cifs_sb_info *cifs_sb, const char *full_path,
> >> - FILE_ALL_INFO *data, bool *adjustTZ)
> >> + FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink)
> >> {
> >> int rc;
> >>
> >> + *symlink = false;
> >> +
> >> /* could do find first instead but this returns more info */
> >> rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */,
> >> cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
> >> @@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> >> CIFS_MOUNT_MAP_SPECIAL_CHR);
> >> *adjustTZ = true;
> >> }
> >> +
> >> + if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) {
> >> + int tmprc;
> >> + int oplock = 0;
> >> + __u16 netfid;
> >> +
> >> + /* Need to check if this is a symbolic link or not */
> >> + tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
> >> + FILE_READ_ATTRIBUTES, 0, &netfid, &oplock,
> >> + NULL, cifs_sb->local_nls,
> >> + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
> >> + if (tmprc == -EOPNOTSUPP)
> >> + *symlink = true;
> >> + else
> >> + CIFSSMBClose(xid, tcon, netfid);
> >> + }
> >> +
> >> return rc;
> >> }
> >>
> >> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> >> index 78ff88c..84c012a 100644
> >> --- a/fs/cifs/smb2inode.c
> >> +++ b/fs/cifs/smb2inode.c
> >> @@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src)
> >> int
> >> smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> >> struct cifs_sb_info *cifs_sb, const char *full_path,
> >> - FILE_ALL_INFO *data, bool *adjust_tz)
> >> + FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink)
> >> {
> >> int rc;
> >> struct smb2_file_all_info *smb2_data;
> >>
> >> *adjust_tz = false;
> >> + *symlink = false;
> >>
> >> smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2,
> >> GFP_KERNEL);
> >> @@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> >> return -ENOMEM;
> >>
> >> rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
> >> - FILE_READ_ATTRIBUTES, FILE_OPEN,
> >> - OPEN_REPARSE_POINT, smb2_data,
> >> - SMB2_OP_QUERY_INFO);
> >> + FILE_READ_ATTRIBUTES, FILE_OPEN, 0,
> >> + smb2_data, SMB2_OP_QUERY_INFO);
> >> + if (rc == -EOPNOTSUPP) {
> >> + *symlink = true;
> >> + /* Failed on a symbolic link - query a reparse point info */
> >> + rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
> >> + FILE_READ_ATTRIBUTES, FILE_OPEN,
> >> + OPEN_REPARSE_POINT, smb2_data,
> >> + SMB2_OP_QUERY_INFO);
> >> + }
> >> if (rc)
> >> goto out;
> >>
> >> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> >> index 313813e..b4eea10 100644
> >> --- a/fs/cifs/smb2proto.h
> >> +++ b/fs/cifs/smb2proto.h
> >> @@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst,
> >> extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> >> struct cifs_sb_info *cifs_sb,
> >> const char *full_path, FILE_ALL_INFO *data,
> >> - bool *adjust_tz);
> >> + bool *adjust_tz, bool *symlink);
> >> extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> >> const char *full_path, __u64 size,
> >> struct cifs_sb_info *cifs_sb, bool set_alloc);
> >
> >
> > --
> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > --
> > 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
>
>
>
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] CIFS: Fix symbolic links usage
[not found] ` <CADT32eKo3gQ06m1V3PZ64OdQp-iLw5VVnecZBO--T0pg9+CiBQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-11-02 19:42 ` Pavel Shilovsky
0 siblings, 0 replies; 12+ messages in thread
From: Pavel Shilovsky @ 2013-11-02 19:42 UTC (permalink / raw)
To: Shirish Pargaonkar; +Cc: linux-cifs, Joao Correia
2013/11/1 Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> Pavel, does this handle a case where it could be symbolic link
> to a directory?
Shirish, yes - it supports symbolic links to directories as well as to
files. It treats them both as Unix symbolic links without any
difference.
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] CIFS: Fix symbolic links usage
[not found] ` <20131101134219.02d33b75-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2013-11-01 17:49 ` Steve French
@ 2013-11-02 19:47 ` Pavel Shilovsky
[not found] ` <CAKywueQGBtpvJwLjPja_xN5OA54qRC=24zSC9pcwhCrRt5Z86A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
1 sibling, 1 reply; 12+ messages in thread
From: Pavel Shilovsky @ 2013-11-02 19:47 UTC (permalink / raw)
To: Jeff Layton; +Cc: linux-cifs, Joao Correia
2013/11/1 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> This looks much better for performance, but I think it really shines a
> light on what an abortion the query_path_info and query_file_info
> operations are. They work with a FILE_ALL_INFO struct which is SMB1
> specific, so for SMB2/3 we have to convert data to something that
> doesn't even match the protocol.
>
> I think it would be best to base this fix on top of a patchset to fix
> that properly. Those functions should be changed to pass data around in
> a cifs_fattr. With that, you could just add a new cifs_fattr->flags
> value and you wouldn't need to add this extra bool * argument.
I agree that using cifs_fattr structure rather than FILE_ALL_INFO
simplifies things and makes the code cleaner. But we have a problem in
the 3.11 stable branch and I think we should fix it without many
patches (code changes). We can apply this patch for stable and then
convert the code to use cifs_fattr structure in query path/file info
in mainline. Right?
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] CIFS: Fix symbolic links usage
[not found] ` <CAKywueQGBtpvJwLjPja_xN5OA54qRC=24zSC9pcwhCrRt5Z86A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-11-03 1:14 ` Jeff Layton
[not found] ` <20131102211419.1b86a02d-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Layton @ 2013-11-03 1:14 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: linux-cifs, Joao Correia
On Sat, 2 Nov 2013 23:47:16 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2013/11/1 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > This looks much better for performance, but I think it really shines a
> > light on what an abortion the query_path_info and query_file_info
> > operations are. They work with a FILE_ALL_INFO struct which is SMB1
> > specific, so for SMB2/3 we have to convert data to something that
> > doesn't even match the protocol.
> >
> > I think it would be best to base this fix on top of a patchset to fix
> > that properly. Those functions should be changed to pass data around in
> > a cifs_fattr. With that, you could just add a new cifs_fattr->flags
> > value and you wouldn't need to add this extra bool * argument.
>
> I agree that using cifs_fattr structure rather than FILE_ALL_INFO
> simplifies things and makes the code cleaner. But we have a problem in
> the 3.11 stable branch and I think we should fix it without many
> patches (code changes). We can apply this patch for stable and then
> convert the code to use cifs_fattr structure in query path/file info
> in mainline. Right?
>
If you intend to push a fix to stable for this, then that sounds like
the best approach. It will make cleaning up the API harder, but as long
as you're ok with that...
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] CIFS: Fix symbolic links usage
[not found] ` <20131102211419.1b86a02d-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2013-11-04 18:50 ` Pavel Shilovsky
[not found] ` <CAKywueTXaZtjPyT2jjU-PLj0ubzKF5Dn70WMrOXs0ZQPtQpmag-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 12+ messages in thread
From: Pavel Shilovsky @ 2013-11-04 18:50 UTC (permalink / raw)
To: Jeff Layton, Steve French; +Cc: linux-cifs, Joao Correia
2013/11/3 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> If you intend to push a fix to stable for this, then that sounds like
> the best approach. It will make cleaning up the API harder, but as long
> as you're ok with that...
Yes - it was targeted for stable as well. Any acks from your side?
Steve, do you have any objections to apply this patch (with Joao's
"Reported-and-tested-by: Joao Correia <joaomiguelcorreia-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>"
tag)?
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] CIFS: Fix symbolic links usage
[not found] ` <CAKywueTXaZtjPyT2jjU-PLj0ubzKF5Dn70WMrOXs0ZQPtQpmag-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-11-04 18:54 ` Steve French
0 siblings, 0 replies; 12+ messages in thread
From: Steve French @ 2013-11-04 18:54 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: Jeff Layton, linux-cifs, Joao Correia
a little large for stable but not sure yet what could be done to make it smaller
On Mon, Nov 4, 2013 at 12:50 PM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2013/11/3 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> If you intend to push a fix to stable for this, then that sounds like
>> the best approach. It will make cleaning up the API harder, but as long
>> as you're ok with that...
>
> Yes - it was targeted for stable as well. Any acks from your side?
>
> Steve, do you have any objections to apply this patch (with Joao's
> "Reported-and-tested-by: Joao Correia <joaomiguelcorreia-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>"
> tag)?
>
> --
> Best regards,
> Pavel Shilovsky.
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] CIFS: Fix symbolic links usage
[not found] ` <1383314259-4694-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2013-11-01 15:08 ` Shirish Pargaonkar
2013-11-01 17:42 ` Jeff Layton
@ 2013-11-05 19:31 ` Jeff Layton
2 siblings, 0 replies; 12+ messages in thread
From: Jeff Layton @ 2013-11-05 19:31 UTC (permalink / raw)
To: Pavel Shilovsky
Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA,
joaomiguelcorreia-Re5JQEeQqe8AvxtiuMwx3w
On Fri, 1 Nov 2013 17:57:39 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
>
> Now we treat any reparse point as a symbolic link and map it to a Unix
> one that is not true in a common case due to many reparse point types
> supported by SMB servers.
>
> Distinguish reparse point types into two groups:
> 1) that can be accessed directly through a reparse point
> (junctions, deduplicated files, NFS symlinks);
> 2) that need to be processed manually (Windows symbolic links, DFS);
>
> and map only Windows symbolic links to Unix ones.
>
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
> fs/cifs/cifsglob.h | 2 +-
> fs/cifs/inode.c | 23 +++++++++++++----------
> fs/cifs/readdir.c | 40 ++++++++--------------------------------
> fs/cifs/smb1ops.c | 21 ++++++++++++++++++++-
> fs/cifs/smb2inode.c | 16 ++++++++++++----
> fs/cifs/smb2proto.h | 2 +-
> 6 files changed, 55 insertions(+), 49 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index a67cf12..b688f2b 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -261,7 +261,7 @@ struct smb_version_operations {
> /* query path data from the server */
> int (*query_path_info)(const unsigned int, struct cifs_tcon *,
> struct cifs_sb_info *, const char *,
> - FILE_ALL_INFO *, bool *);
> + FILE_ALL_INFO *, bool *, bool *);
> /* query file data from the server */
> int (*query_file_info)(const unsigned int, struct cifs_tcon *,
> struct cifs_fid *, FILE_ALL_INFO *);
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 867b7cd..36f9ebb 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path,
> /* Fill a cifs_fattr struct with info from FILE_ALL_INFO */
> static void
> cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> - struct cifs_sb_info *cifs_sb, bool adjust_tz)
> + struct cifs_sb_info *cifs_sb, bool adjust_tz,
> + bool symlink)
> {
> struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
>
> @@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> fattr->cf_createtime = le64_to_cpu(info->CreationTime);
>
> fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> - if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> +
> + if (symlink) {
> + fattr->cf_mode = S_IFLNK;
> + fattr->cf_dtype = DT_LNK;
> + } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> fattr->cf_dtype = DT_DIR;
> /*
> @@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info,
> */
> if (!tcon->unix_ext)
> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
> - fattr->cf_mode = S_IFLNK;
> - fattr->cf_dtype = DT_LNK;
> - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks);
> } else {
> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
> fattr->cf_dtype = DT_REG;
> @@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp)
> rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data);
> switch (rc) {
> case 0:
> - cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false);
> + cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false,
> + false);
> break;
> case -EREMOTE:
> cifs_create_dfs_fattr(&fattr, inode->i_sb);
> @@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> bool adjust_tz = false;
> struct cifs_fattr fattr;
> struct cifs_search_info *srchinf = NULL;
> + bool symlink = false;
>
> tlink = cifs_sb_tlink(cifs_sb);
> if (IS_ERR(tlink))
> @@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
> }
> data = (FILE_ALL_INFO *)buf;
> rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path,
> - data, &adjust_tz);
> + data, &adjust_tz, &symlink);
> }
>
> if (!rc) {
> - cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb,
> - adjust_tz);
> + cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz,
> + symlink);
> } else if (rc == -EREMOTE) {
> cifs_create_dfs_fattr(&fattr, sb);
> rc = 0;
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index 53a75f3..5940eca 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -134,22 +134,6 @@ out:
> dput(dentry);
> }
>
> -/*
> - * Is it possible that this directory might turn out to be a DFS referral
> - * once we go to try and use it?
> - */
> -static bool
> -cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb)
> -{
> -#ifdef CONFIG_CIFS_DFS_UPCALL
> - struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
> -
> - if (tcon->Flags & SMB_SHARE_IS_IN_DFS)
> - return true;
> -#endif
> - return false;
> -}
> -
> static void
> cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
> {
> @@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
> if (fattr->cf_cifsattrs & ATTR_DIRECTORY) {
> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode;
> fattr->cf_dtype = DT_DIR;
> - /*
> - * Windows CIFS servers generally make DFS referrals look
> - * like directories in FIND_* responses with the reparse
> - * attribute flag also set (since DFS junctions are
> - * reparse points). We must revalidate at least these
> - * directory inodes before trying to use them (if
> - * they are DFS we will get PATH_NOT_COVERED back
> - * when queried directly and can then try to connect
> - * to the DFS target)
> - */
> - if (cifs_dfs_is_possible(cifs_sb) &&
> - (fattr->cf_cifsattrs & ATTR_REPARSE))
> - fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) {
> - fattr->cf_mode = S_IFLNK;
> - fattr->cf_dtype = DT_LNK;
> } else {
> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode;
> fattr->cf_dtype = DT_REG;
> }
>
> + /*
> + * We need to revalidate it further to make a decision about whether it
> + * is a symbolic link, DFS referral or a reparse point with a direct
> + * access like junctions, deduplicated files, NFS symlinks.
> + */
> + if (fattr->cf_cifsattrs & ATTR_REPARSE)
> + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;
> +
> /* non-unix readdir doesn't provide nlink */
> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;
>
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index ea99efe..47ab035 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
> static int
> cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_sb_info *cifs_sb, const char *full_path,
> - FILE_ALL_INFO *data, bool *adjustTZ)
> + FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink)
> {
> int rc;
>
> + *symlink = false;
> +
> /* could do find first instead but this returns more info */
> rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */,
> cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
> @@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> CIFS_MOUNT_MAP_SPECIAL_CHR);
> *adjustTZ = true;
> }
> +
> + if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) {
> + int tmprc;
> + int oplock = 0;
> + __u16 netfid;
> +
> + /* Need to check if this is a symbolic link or not */
> + tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
> + FILE_READ_ATTRIBUTES, 0, &netfid, &oplock,
> + NULL, cifs_sb->local_nls,
> + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
> + if (tmprc == -EOPNOTSUPP)
> + *symlink = true;
> + else
> + CIFSSMBClose(xid, tcon, netfid);
> + }
> +
> return rc;
> }
>
> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> index 78ff88c..84c012a 100644
> --- a/fs/cifs/smb2inode.c
> +++ b/fs/cifs/smb2inode.c
> @@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src)
> int
> smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_sb_info *cifs_sb, const char *full_path,
> - FILE_ALL_INFO *data, bool *adjust_tz)
> + FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink)
> {
> int rc;
> struct smb2_file_all_info *smb2_data;
>
> *adjust_tz = false;
> + *symlink = false;
>
> smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2,
> GFP_KERNEL);
> @@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> return -ENOMEM;
>
> rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
> - FILE_READ_ATTRIBUTES, FILE_OPEN,
> - OPEN_REPARSE_POINT, smb2_data,
> - SMB2_OP_QUERY_INFO);
> + FILE_READ_ATTRIBUTES, FILE_OPEN, 0,
> + smb2_data, SMB2_OP_QUERY_INFO);
> + if (rc == -EOPNOTSUPP) {
> + *symlink = true;
> + /* Failed on a symbolic link - query a reparse point info */
> + rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path,
> + FILE_READ_ATTRIBUTES, FILE_OPEN,
> + OPEN_REPARSE_POINT, smb2_data,
> + SMB2_OP_QUERY_INFO);
> + }
> if (rc)
> goto out;
>
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index 313813e..b4eea10 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst,
> extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> struct cifs_sb_info *cifs_sb,
> const char *full_path, FILE_ALL_INFO *data,
> - bool *adjust_tz);
> + bool *adjust_tz, bool *symlink);
> extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon,
> const char *full_path, __u64 size,
> struct cifs_sb_info *cifs_sb, bool set_alloc);
Looks reasonable for an interim fix I guess. I'm not thrilled with it
since it doesn't help the code overall, but I guess we can live with it
until we can clean up the API.
Acked-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-11-05 19:31 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-01 13:57 [PATCH] CIFS: Fix symbolic links usage Pavel Shilovsky
[not found] ` <1383314259-4694-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2013-11-01 15:08 ` Shirish Pargaonkar
[not found] ` <CADT32eKo3gQ06m1V3PZ64OdQp-iLw5VVnecZBO--T0pg9+CiBQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-11-02 19:42 ` Pavel Shilovsky
2013-11-01 17:42 ` Jeff Layton
[not found] ` <20131101134219.02d33b75-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2013-11-01 17:49 ` Steve French
[not found] ` <CAH2r5msFrvfWjYUO7pC84ggOZromA6_9jVM+KOqV_x61vNYSwg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-11-01 18:00 ` Jeff Layton
2013-11-02 12:10 ` Jeff Layton
2013-11-02 19:47 ` Pavel Shilovsky
[not found] ` <CAKywueQGBtpvJwLjPja_xN5OA54qRC=24zSC9pcwhCrRt5Z86A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-11-03 1:14 ` Jeff Layton
[not found] ` <20131102211419.1b86a02d-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2013-11-04 18:50 ` Pavel Shilovsky
[not found] ` <CAKywueTXaZtjPyT2jjU-PLj0ubzKF5Dn70WMrOXs0ZQPtQpmag-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-11-04 18:54 ` Steve French
2013-11-05 19:31 ` Jeff Layton
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.