Re: [PATCH] CIFS: Fix symbolic links usage

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 1 Nov 2013 12:49:47 -0500
Steve French <smfrench@xxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > On Fri,  1 Nov 2013 17:57:39 +0400
> > Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> >
> >> From: Pavel Shilovsky <piastry@xxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxx>
> >> ---
> >>  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@xxxxxxxxxx>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 


-- 
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux