| Message ID | 20240620083729.28623-1-wangrong@uniontech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | smb: client: use actual path when queryfs | expand |
Paulo just found this potentially important fix in email (it had gotten missed). The one suspicious thing about this though ... we should have some code paths where we would use the cached root handle for statfs (which is preferable to doing an open of a new handle, since it is already open we don't risk an error coming back on open). Any ideas whether we also need additional changes to use the cached root handle better in statfs (since in most cases to Windows we would expect to have that)? On Thu, Jun 20, 2024 at 3:54 AM wangrong <wangrong@uniontech.com> wrote: > > Due to server permission control, the client does not have access to > the shared root directory, but can access subdirectories normally, so > users usually mount the shared subdirectories directly. In this case, > queryfs should use the actual path instead of the root directory to > avoid the call returning an error (EACCES). > > Signed-off-by: wangrong <wangrong@uniontech.com> > --- > fs/smb/client/cifsfs.c | 13 ++++++++++++- > fs/smb/client/cifsglob.h | 2 +- > fs/smb/client/smb1ops.c | 2 +- > fs/smb/client/smb2ops.c | 19 ++++++++++++------- > 4 files changed, 26 insertions(+), 10 deletions(-) > > diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c > index bb86fc064..a4d59f0f5 100644 > --- a/fs/smb/client/cifsfs.c > +++ b/fs/smb/client/cifsfs.c > @@ -313,8 +313,17 @@ cifs_statfs(struct dentry *dentry, struct kstatfs *buf) > struct TCP_Server_Info *server = tcon->ses->server; > unsigned int xid; > int rc = 0; > + const char *full_path; > + void *page; > > xid = get_xid(); > + page = alloc_dentry_path(); > + > + full_path = build_path_from_dentry(dentry, page); > + if (IS_ERR(full_path)) { > + rc = PTR_ERR(full_path); > + goto statfs_out; > + } > > if (le32_to_cpu(tcon->fsAttrInfo.MaxPathNameComponentLength) > 0) > buf->f_namelen = > @@ -330,8 +339,10 @@ cifs_statfs(struct dentry *dentry, struct kstatfs *buf) > buf->f_ffree = 0; /* unlimited */ > > if (server->ops->queryfs) > - rc = server->ops->queryfs(xid, tcon, cifs_sb, buf); > + rc = server->ops->queryfs(xid, tcon, full_path, cifs_sb, buf); > > +statfs_out: > + free_dentry_path(page); > free_xid(xid); > return rc; > } > diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h > index 73482734a..d3118d748 100644 > --- a/fs/smb/client/cifsglob.h > +++ b/fs/smb/client/cifsglob.h > @@ -483,7 +483,7 @@ struct smb_version_operations { > __u16 net_fid, struct cifsInodeInfo *cifs_inode); > /* query remote filesystem */ > int (*queryfs)(const unsigned int, struct cifs_tcon *, > - struct cifs_sb_info *, struct kstatfs *); > + const char *, struct cifs_sb_info *, struct kstatfs *); > /* send mandatory brlock to the server */ > int (*mand_lock)(const unsigned int, struct cifsFileInfo *, __u64, > __u64, __u32, int, int, bool); > diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c > index 212ec6f66..e3a195824 100644 > --- a/fs/smb/client/smb1ops.c > +++ b/fs/smb/client/smb1ops.c > @@ -909,7 +909,7 @@ cifs_oplock_response(struct cifs_tcon *tcon, __u64 persistent_fid, > > static int > cifs_queryfs(const unsigned int xid, struct cifs_tcon *tcon, > - struct cifs_sb_info *cifs_sb, struct kstatfs *buf) > + const char *path, struct cifs_sb_info *cifs_sb, struct kstatfs *buf) > { > int rc = -EOPNOTSUPP; > > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c > index c8e536540..bb7194386 100644 > --- a/fs/smb/client/smb2ops.c > +++ b/fs/smb/client/smb2ops.c > @@ -2784,7 +2784,7 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon, > > static int > smb2_queryfs(const unsigned int xid, struct cifs_tcon *tcon, > - struct cifs_sb_info *cifs_sb, struct kstatfs *buf) > + const char *path, struct cifs_sb_info *cifs_sb, struct kstatfs *buf) > { > struct smb2_query_info_rsp *rsp; > struct smb2_fs_full_size_info *info = NULL; > @@ -2793,7 +2793,7 @@ smb2_queryfs(const unsigned int xid, struct cifs_tcon *tcon, > int rc; > > > - rc = smb2_query_info_compound(xid, tcon, "", > + rc = smb2_query_info_compound(xid, tcon, path, > FILE_READ_ATTRIBUTES, > FS_FULL_SIZE_INFORMATION, > SMB2_O_INFO_FILESYSTEM, > @@ -2821,28 +2821,33 @@ smb2_queryfs(const unsigned int xid, struct cifs_tcon *tcon, > > static int > smb311_queryfs(const unsigned int xid, struct cifs_tcon *tcon, > - struct cifs_sb_info *cifs_sb, struct kstatfs *buf) > + const char *path, struct cifs_sb_info *cifs_sb, struct kstatfs *buf) > { > int rc; > - __le16 srch_path = 0; /* Null - open root of share */ > + __le16 *utf16_path = NULL; > u8 oplock = SMB2_OPLOCK_LEVEL_NONE; > struct cifs_open_parms oparms; > struct cifs_fid fid; > > if (!tcon->posix_extensions) > - return smb2_queryfs(xid, tcon, cifs_sb, buf); > + return smb2_queryfs(xid, tcon, path, cifs_sb, buf); > > oparms = (struct cifs_open_parms) { > .tcon = tcon, > - .path = "", > + .path = path, > .desired_access = FILE_READ_ATTRIBUTES, > .disposition = FILE_OPEN, > .create_options = cifs_create_options(cifs_sb, 0), > .fid = &fid, > }; > > - rc = SMB2_open(xid, &oparms, &srch_path, &oplock, NULL, NULL, > + utf16_path = cifs_convert_path_to_utf16(path, cifs_sb); > + if (utf16_path == NULL) > + return -ENOMEM; > + > + rc = SMB2_open(xid, &oparms, utf16_path, &oplock, NULL, NULL, > NULL, NULL); > + kfree(utf16_path); > if (rc) > return rc; > > -- > 2.20.1 > >
Merged into cifs-2.6.git for-next tentatively but want to do some more testing on this (and any additional review comments would be welcome) On Tue, Oct 1, 2024 at 9:33 PM Steve French <smfrench@gmail.com> wrote: > > Paulo just found this potentially important fix in email (it had gotten missed). > > The one suspicious thing about this though ... we should have some > code paths where we would use the cached root handle for statfs > (which is preferable to doing an open of a new handle, since it is > already open we don't risk an error coming back on open). > > Any ideas whether we also need additional changes to use the cached > root handle better in statfs (since in most cases to > Windows we would expect to have that)? > > > On Thu, Jun 20, 2024 at 3:54 AM wangrong <wangrong@uniontech.com> wrote: > > > > Due to server permission control, the client does not have access to > > the shared root directory, but can access subdirectories normally, so > > users usually mount the shared subdirectories directly. In this case, > > queryfs should use the actual path instead of the root directory to > > avoid the call returning an error (EACCES). > > > > Signed-off-by: wangrong <wangrong@uniontech.com> > > --- > > fs/smb/client/cifsfs.c | 13 ++++++++++++- > > fs/smb/client/cifsglob.h | 2 +- > > fs/smb/client/smb1ops.c | 2 +- > > fs/smb/client/smb2ops.c | 19 ++++++++++++------- > > 4 files changed, 26 insertions(+), 10 deletions(-) > > > > diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c > > index bb86fc064..a4d59f0f5 100644 > > --- a/fs/smb/client/cifsfs.c > > +++ b/fs/smb/client/cifsfs.c > > @@ -313,8 +313,17 @@ cifs_statfs(struct dentry *dentry, struct kstatfs *buf) > > struct TCP_Server_Info *server = tcon->ses->server; > > unsigned int xid; > > int rc = 0; > > + const char *full_path; > > + void *page; > > > > xid = get_xid(); > > + page = alloc_dentry_path(); > > + > > + full_path = build_path_from_dentry(dentry, page); > > + if (IS_ERR(full_path)) { > > + rc = PTR_ERR(full_path); > > + goto statfs_out; > > + } > > > > if (le32_to_cpu(tcon->fsAttrInfo.MaxPathNameComponentLength) > 0) > > buf->f_namelen = > > @@ -330,8 +339,10 @@ cifs_statfs(struct dentry *dentry, struct kstatfs *buf) > > buf->f_ffree = 0; /* unlimited */ > > > > if (server->ops->queryfs) > > - rc = server->ops->queryfs(xid, tcon, cifs_sb, buf); > > + rc = server->ops->queryfs(xid, tcon, full_path, cifs_sb, buf); > > > > +statfs_out: > > + free_dentry_path(page); > > free_xid(xid); > > return rc; > > } > > diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h > > index 73482734a..d3118d748 100644 > > --- a/fs/smb/client/cifsglob.h > > +++ b/fs/smb/client/cifsglob.h > > @@ -483,7 +483,7 @@ struct smb_version_operations { > > __u16 net_fid, struct cifsInodeInfo *cifs_inode); > > /* query remote filesystem */ > > int (*queryfs)(const unsigned int, struct cifs_tcon *, > > - struct cifs_sb_info *, struct kstatfs *); > > + const char *, struct cifs_sb_info *, struct kstatfs *); > > /* send mandatory brlock to the server */ > > int (*mand_lock)(const unsigned int, struct cifsFileInfo *, __u64, > > __u64, __u32, int, int, bool); > > diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c > > index 212ec6f66..e3a195824 100644 > > --- a/fs/smb/client/smb1ops.c > > +++ b/fs/smb/client/smb1ops.c > > @@ -909,7 +909,7 @@ cifs_oplock_response(struct cifs_tcon *tcon, __u64 persistent_fid, > > > > static int > > cifs_queryfs(const unsigned int xid, struct cifs_tcon *tcon, > > - struct cifs_sb_info *cifs_sb, struct kstatfs *buf) > > + const char *path, struct cifs_sb_info *cifs_sb, struct kstatfs *buf) > > { > > int rc = -EOPNOTSUPP; > > > > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c > > index c8e536540..bb7194386 100644 > > --- a/fs/smb/client/smb2ops.c > > +++ b/fs/smb/client/smb2ops.c > > @@ -2784,7 +2784,7 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon, > > > > static int > > smb2_queryfs(const unsigned int xid, struct cifs_tcon *tcon, > > - struct cifs_sb_info *cifs_sb, struct kstatfs *buf) > > + const char *path, struct cifs_sb_info *cifs_sb, struct kstatfs *buf) > > { > > struct smb2_query_info_rsp *rsp; > > struct smb2_fs_full_size_info *info = NULL; > > @@ -2793,7 +2793,7 @@ smb2_queryfs(const unsigned int xid, struct cifs_tcon *tcon, > > int rc; > > > > > > - rc = smb2_query_info_compound(xid, tcon, "", > > + rc = smb2_query_info_compound(xid, tcon, path, > > FILE_READ_ATTRIBUTES, > > FS_FULL_SIZE_INFORMATION, > > SMB2_O_INFO_FILESYSTEM, > > @@ -2821,28 +2821,33 @@ smb2_queryfs(const unsigned int xid, struct cifs_tcon *tcon, > > > > static int > > smb311_queryfs(const unsigned int xid, struct cifs_tcon *tcon, > > - struct cifs_sb_info *cifs_sb, struct kstatfs *buf) > > + const char *path, struct cifs_sb_info *cifs_sb, struct kstatfs *buf) > > { > > int rc; > > - __le16 srch_path = 0; /* Null - open root of share */ > > + __le16 *utf16_path = NULL; > > u8 oplock = SMB2_OPLOCK_LEVEL_NONE; > > struct cifs_open_parms oparms; > > struct cifs_fid fid; > > > > if (!tcon->posix_extensions) > > - return smb2_queryfs(xid, tcon, cifs_sb, buf); > > + return smb2_queryfs(xid, tcon, path, cifs_sb, buf); > > > > oparms = (struct cifs_open_parms) { > > .tcon = tcon, > > - .path = "", > > + .path = path, > > .desired_access = FILE_READ_ATTRIBUTES, > > .disposition = FILE_OPEN, > > .create_options = cifs_create_options(cifs_sb, 0), > > .fid = &fid, > > }; > > > > - rc = SMB2_open(xid, &oparms, &srch_path, &oplock, NULL, NULL, > > + utf16_path = cifs_convert_path_to_utf16(path, cifs_sb); > > + if (utf16_path == NULL) > > + return -ENOMEM; > > + > > + rc = SMB2_open(xid, &oparms, utf16_path, &oplock, NULL, NULL, > > NULL, NULL); > > + kfree(utf16_path); > > if (rc) > > return rc; > > > > -- > > 2.20.1 > > > > > > > -- > Thanks, > > Steve
diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c index bb86fc064..a4d59f0f5 100644 --- a/fs/smb/client/cifsfs.c +++ b/fs/smb/client/cifsfs.c @@ -313,8 +313,17 @@ cifs_statfs(struct dentry *dentry, struct kstatfs *buf) struct TCP_Server_Info *server = tcon->ses->server; unsigned int xid; int rc = 0; + const char *full_path; + void *page; xid = get_xid(); + page = alloc_dentry_path(); + + full_path = build_path_from_dentry(dentry, page); + if (IS_ERR(full_path)) { + rc = PTR_ERR(full_path); + goto statfs_out; + } if (le32_to_cpu(tcon->fsAttrInfo.MaxPathNameComponentLength) > 0) buf->f_namelen = @@ -330,8 +339,10 @@ cifs_statfs(struct dentry *dentry, struct kstatfs *buf) buf->f_ffree = 0; /* unlimited */ if (server->ops->queryfs) - rc = server->ops->queryfs(xid, tcon, cifs_sb, buf); + rc = server->ops->queryfs(xid, tcon, full_path, cifs_sb, buf); +statfs_out: + free_dentry_path(page); free_xid(xid); return rc; } diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h index 73482734a..d3118d748 100644 --- a/fs/smb/client/cifsglob.h +++ b/fs/smb/client/cifsglob.h @@ -483,7 +483,7 @@ struct smb_version_operations { __u16 net_fid, struct cifsInodeInfo *cifs_inode); /* query remote filesystem */ int (*queryfs)(const unsigned int, struct cifs_tcon *, - struct cifs_sb_info *, struct kstatfs *); + const char *, struct cifs_sb_info *, struct kstatfs *); /* send mandatory brlock to the server */ int (*mand_lock)(const unsigned int, struct cifsFileInfo *, __u64, __u64, __u32, int, int, bool); diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c index 212ec6f66..e3a195824 100644 --- a/fs/smb/client/smb1ops.c +++ b/fs/smb/client/smb1ops.c @@ -909,7 +909,7 @@ cifs_oplock_response(struct cifs_tcon *tcon, __u64 persistent_fid, static int cifs_queryfs(const unsigned int xid, struct cifs_tcon *tcon, - struct cifs_sb_info *cifs_sb, struct kstatfs *buf) + const char *path, struct cifs_sb_info *cifs_sb, struct kstatfs *buf) { int rc = -EOPNOTSUPP; diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c index c8e536540..bb7194386 100644 --- a/fs/smb/client/smb2ops.c +++ b/fs/smb/client/smb2ops.c @@ -2784,7 +2784,7 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon, static int smb2_queryfs(const unsigned int xid, struct cifs_tcon *tcon, - struct cifs_sb_info *cifs_sb, struct kstatfs *buf) + const char *path, struct cifs_sb_info *cifs_sb, struct kstatfs *buf) { struct smb2_query_info_rsp *rsp; struct smb2_fs_full_size_info *info = NULL; @@ -2793,7 +2793,7 @@ smb2_queryfs(const unsigned int xid, struct cifs_tcon *tcon, int rc; - rc = smb2_query_info_compound(xid, tcon, "", + rc = smb2_query_info_compound(xid, tcon, path, FILE_READ_ATTRIBUTES, FS_FULL_SIZE_INFORMATION, SMB2_O_INFO_FILESYSTEM, @@ -2821,28 +2821,33 @@ smb2_queryfs(const unsigned int xid, struct cifs_tcon *tcon, static int smb311_queryfs(const unsigned int xid, struct cifs_tcon *tcon, - struct cifs_sb_info *cifs_sb, struct kstatfs *buf) + const char *path, struct cifs_sb_info *cifs_sb, struct kstatfs *buf) { int rc; - __le16 srch_path = 0; /* Null - open root of share */ + __le16 *utf16_path = NULL; u8 oplock = SMB2_OPLOCK_LEVEL_NONE; struct cifs_open_parms oparms; struct cifs_fid fid; if (!tcon->posix_extensions) - return smb2_queryfs(xid, tcon, cifs_sb, buf); + return smb2_queryfs(xid, tcon, path, cifs_sb, buf); oparms = (struct cifs_open_parms) { .tcon = tcon, - .path = "", + .path = path, .desired_access = FILE_READ_ATTRIBUTES, .disposition = FILE_OPEN, .create_options = cifs_create_options(cifs_sb, 0), .fid = &fid, }; - rc = SMB2_open(xid, &oparms, &srch_path, &oplock, NULL, NULL, + utf16_path = cifs_convert_path_to_utf16(path, cifs_sb); + if (utf16_path == NULL) + return -ENOMEM; + + rc = SMB2_open(xid, &oparms, utf16_path, &oplock, NULL, NULL, NULL, NULL); + kfree(utf16_path); if (rc) return rc;
Due to server permission control, the client does not have access to the shared root directory, but can access subdirectories normally, so users usually mount the shared subdirectories directly. In this case, queryfs should use the actual path instead of the root directory to avoid the call returning an error (EACCES). Signed-off-by: wangrong <wangrong@uniontech.com> --- fs/smb/client/cifsfs.c | 13 ++++++++++++- fs/smb/client/cifsglob.h | 2 +- fs/smb/client/smb1ops.c | 2 +- fs/smb/client/smb2ops.c | 19 ++++++++++++------- 4 files changed, 26 insertions(+), 10 deletions(-)