In the future if we want to make this more visible to the client
(although presumably query fs info/statfs can still return the correct
information depending on the path given by the user) we could always
make this a fake DFS referral after we cross a mount point on the
server
On Fri, Jul 14, 2023 at 8:25 AM Namjae Jeon <linkinjeon@xxxxxxxxxx> wrote:
>
> 2023-07-14 21:44 GMT+09:00, Tom Talpey <tom@xxxxxxxxxx>:
> > On 7/12/2023 7:40 PM, Namjae Jeon wrote:
> >> 2023-07-13 2:23 GMT+09:00, Tom Talpey <tom@xxxxxxxxxx>:
> >>> On 7/11/2023 8:30 AM, Namjae Jeon wrote:
> >>>> Since commit 74d7970febf7 ("ksmbd: fix racy issue from using ->d_parent
> >>>> and ->d_name"),
> >>>> ksmbd can not lookup cross mount points. If last component is a cross
> >>>> mount point during path lookup, check if it is crossed to follow it
> >>>> down.
> >>>
> >>> I actually thought not crossing mount points was intended, since
> >>> semantics can shift if the crossing changes filesystems or fs types.
> >>>
> >>> Does this change somehow prevent walking out of a mount via embedded
> >>> "/../" in the path? It's not obvious to me whether
> >>> vfs_path_parent_lookup
> >>> denies this.
> >> Yes, LOOKUP_BENEATH flags protect to lookup out of share.
> >>>
> >>> Tom.
> >>>
> >>> I'm not familiar enough with the VFS protection, but does this
> >> I have checked it before and now again. There is no problem.
> >
> > I'm still concerned about this. Crossing mount points in the server is
> > completely invisible to the client, yet the semantics change radically.
> Have you checked it with samba server ? This is exactly the same behavior
> as samba server.
>
> And users are also wanting exactly the same behavior as before.
> https://github.com/namjaejeon/ksmbd/issues/436
> >
> > What if someone shares "/mnt", and proceeds to mount an ext4, a USB
> > FAT32, a CD-Rom, and an NFS filesystem in subdirectories? The client
> > has no clue, and will walk into them all, finding a very different
> > world in each.
> >
> > How will Posix extensions work across this? Case sensitivity/preserving?
> > Caching and locks/leases?
> I didn't understand exactly your question.
> And NFS also supports crossmnt, and we will be able to add 'crossmnt' parameter
> in smb.conf and work crossmnt by default.
>
> Thanks.
> >
> > Tom.
> >> Thanks!
> >>>>
> >>>> Fixes: 74d7970febf7 ("ksmbd: fix racy issue from using ->d_parent and
> >>>> ->d_name")
> >>>> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
> >>>> ---
> >>>> fs/smb/server/smb2pdu.c | 27 ++++++++++++--------
> >>>> fs/smb/server/vfs.c | 56
> >>>> +++++++++++++++++++++++------------------
> >>>> fs/smb/server/vfs.h | 4 +--
> >>>> 3 files changed, 49 insertions(+), 38 deletions(-)
> >>>>
> >>>> diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
> >>>> index cf8822103f50..ca276634fd58 100644
> >>>> --- a/fs/smb/server/smb2pdu.c
> >>>> +++ b/fs/smb/server/smb2pdu.c
> >>>> @@ -2467,8 +2467,9 @@ static void smb2_update_xattrs(struct
> >>>> ksmbd_tree_connect *tcon,
> >>>> }
> >>>> }
> >>>>
> >>>> -static int smb2_creat(struct ksmbd_work *work, struct path *path, char
> >>>> *name,
> >>>> - int open_flags, umode_t posix_mode, bool is_dir)
> >>>> +static int smb2_creat(struct ksmbd_work *work, struct path
> >>>> *parent_path,
> >>>> + struct path *path, char *name, int open_flags,
> >>>> + umode_t posix_mode, bool is_dir)
> >>>> {
> >>>> struct ksmbd_tree_connect *tcon = work->tcon;
> >>>> struct ksmbd_share_config *share = tcon->share_conf;
> >>>> @@ -2495,7 +2496,7 @@ static int smb2_creat(struct ksmbd_work *work,
> >>>> struct path *path, char *name,
> >>>> return rc;
> >>>> }
> >>>>
> >>>> - rc = ksmbd_vfs_kern_path_locked(work, name, 0, path, 0);
> >>>> + rc = ksmbd_vfs_kern_path_locked(work, name, 0, parent_path, path, 0);
> >>>> if (rc) {
> >>>> pr_err("cannot get linux path (%s), err = %d\n",
> >>>> name, rc);
> >>>> @@ -2565,7 +2566,7 @@ int smb2_open(struct ksmbd_work *work)
> >>>> struct ksmbd_tree_connect *tcon = work->tcon;
> >>>> struct smb2_create_req *req;
> >>>> struct smb2_create_rsp *rsp;
> >>>> - struct path path;
> >>>> + struct path path, parent_path;
> >>>> struct ksmbd_share_config *share = tcon->share_conf;
> >>>> struct ksmbd_file *fp = NULL;
> >>>> struct file *filp = NULL;
> >>>> @@ -2786,7 +2787,8 @@ int smb2_open(struct ksmbd_work *work)
> >>>> goto err_out1;
> >>>> }
> >>>>
> >>>> - rc = ksmbd_vfs_kern_path_locked(work, name, LOOKUP_NO_SYMLINKS,
> >>>> &path,
> >>>> 1);
> >>>> + rc = ksmbd_vfs_kern_path_locked(work, name, LOOKUP_NO_SYMLINKS,
> >>>> + &parent_path, &path, 1);
> >>>> if (!rc) {
> >>>> file_present = true;
> >>>>
> >>>> @@ -2906,7 +2908,8 @@ int smb2_open(struct ksmbd_work *work)
> >>>>
> >>>> /*create file if not present */
> >>>> if (!file_present) {
> >>>> - rc = smb2_creat(work, &path, name, open_flags, posix_mode,
> >>>> + rc = smb2_creat(work, &parent_path, &path, name, open_flags,
> >>>> + posix_mode,
> >>>> req->CreateOptions & FILE_DIRECTORY_FILE_LE);
> >>>> if (rc) {
> >>>> if (rc == -ENOENT) {
> >>>> @@ -3321,8 +3324,9 @@ int smb2_open(struct ksmbd_work *work)
> >>>>
> >>>> err_out:
> >>>> if (file_present || created) {
> >>>> - inode_unlock(d_inode(path.dentry->d_parent));
> >>>> - dput(path.dentry);
> >>>> + inode_unlock(d_inode(parent_path.dentry));
> >>>> + path_put(&path);
> >>>> + path_put(&parent_path);
> >>>> }
> >>>> ksmbd_revert_fsids(work);
> >>>> err_out1:
> >>>> @@ -5545,7 +5549,7 @@ static int smb2_create_link(struct ksmbd_work
> >>>> *work,
> >>>> struct nls_table *local_nls)
> >>>> {
> >>>> char *link_name = NULL, *target_name = NULL, *pathname = NULL;
> >>>> - struct path path;
> >>>> + struct path path, parent_path;
> >>>> bool file_present = false;
> >>>> int rc;
> >>>>
> >>>> @@ -5575,7 +5579,7 @@ static int smb2_create_link(struct ksmbd_work
> >>>> *work,
> >>>>
> >>>> ksmbd_debug(SMB, "target name is %s\n", target_name);
> >>>> rc = ksmbd_vfs_kern_path_locked(work, link_name,
> >>>> LOOKUP_NO_SYMLINKS,
> >>>> - &path, 0);
> >>>> + &parent_path, &path, 0);
> >>>> if (rc) {
> >>>> if (rc != -ENOENT)
> >>>> goto out;
> >>>> @@ -5605,8 +5609,9 @@ static int smb2_create_link(struct ksmbd_work
> >>>> *work,
> >>>> rc = -EINVAL;
> >>>> out:
> >>>> if (file_present) {
> >>>> - inode_unlock(d_inode(path.dentry->d_parent));
> >>>> + inode_unlock(d_inode(parent_path.dentry));
> >>>> path_put(&path);
> >>>> + path_put(&parent_path);
> >>>> }
> >>>> if (!IS_ERR(link_name))
> >>>> kfree(link_name);
> >>>> diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
> >>>> index e35914457350..fd06d267de61 100644
> >>>> --- a/fs/smb/server/vfs.c
> >>>> +++ b/fs/smb/server/vfs.c
> >>>> @@ -63,13 +63,13 @@ int ksmbd_vfs_lock_parent(struct dentry *parent,
> >>>> struct dentry *child)
> >>>>
> >>>> static int ksmbd_vfs_path_lookup_locked(struct ksmbd_share_config
> >>>> *share_conf,
> >>>> char *pathname, unsigned int flags,
> >>>> + struct path *parent_path,
> >>>> struct path *path)
> >>>> {
> >>>> struct qstr last;
> >>>> struct filename *filename;
> >>>> struct path *root_share_path = &share_conf->vfs_path;
> >>>> int err, type;
> >>>> - struct path parent_path;
> >>>> struct dentry *d;
> >>>>
> >>>> if (pathname[0] == '\0') {
> >>>> @@ -84,7 +84,7 @@ static int ksmbd_vfs_path_lookup_locked(struct
> >>>> ksmbd_share_config *share_conf,
> >>>> return PTR_ERR(filename);
> >>>>
> >>>> err = vfs_path_parent_lookup(filename, flags,
> >>>> - &parent_path, &last, &type,
> >>>> + parent_path, &last, &type,
> >>>> root_share_path);
> >>>> if (err) {
> >>>> putname(filename);
> >>>> @@ -92,13 +92,13 @@ static int ksmbd_vfs_path_lookup_locked(struct
> >>>> ksmbd_share_config *share_conf,
> >>>> }
> >>>>
> >>>> if (unlikely(type != LAST_NORM)) {
> >>>> - path_put(&parent_path);
> >>>> + path_put(parent_path);
> >>>> putname(filename);
> >>>> return -ENOENT;
> >>>> }
> >>>>
> >>>> - inode_lock_nested(parent_path.dentry->d_inode, I_MUTEX_PARENT);
> >>>> - d = lookup_one_qstr_excl(&last, parent_path.dentry, 0);
> >>>> + inode_lock_nested(parent_path->dentry->d_inode, I_MUTEX_PARENT);
> >>>> + d = lookup_one_qstr_excl(&last, parent_path->dentry, 0);
> >>>> if (IS_ERR(d))
> >>>> goto err_out;
> >>>>
> >>>> @@ -108,15 +108,20 @@ static int ksmbd_vfs_path_lookup_locked(struct
> >>>> ksmbd_share_config *share_conf,
> >>>> }
> >>>>
> >>>> path->dentry = d;
> >>>> - path->mnt = share_conf->vfs_path.mnt;
> >>>> - path_put(&parent_path);
> >>>> - putname(filename);
> >>>> + path->mnt = mntget(parent_path->mnt);
> >>>> +
> >>>> + err = follow_down(path, 0);
> >>>> + if (err < 0) {
> >>>> + path_put(path);
> >>>> + goto err_out;
> >>>> + }
> >>>>
> >>>> + putname(filename);
> >>>> return 0;
> >>>>
> >>>> err_out:
> >>>> - inode_unlock(parent_path.dentry->d_inode);
> >>>> - path_put(&parent_path);
> >>>> + inode_unlock(d_inode(parent_path->dentry));
> >>>> + path_put(parent_path);
> >>>> putname(filename);
> >>>> return -ENOENT;
> >>>> }
> >>>> @@ -1194,14 +1199,14 @@ static int ksmbd_vfs_lookup_in_dir(const struct
> >>>> path *dir, char *name,
> >>>> * Return: 0 on success, otherwise error
> >>>> */
> >>>> int ksmbd_vfs_kern_path_locked(struct ksmbd_work *work, char *name,
> >>>> - unsigned int flags, struct path *path,
> >>>> - bool caseless)
> >>>> + unsigned int flags, struct path *parent_path,
> >>>> + struct path *path, bool caseless)
> >>>> {
> >>>> struct ksmbd_share_config *share_conf = work->tcon->share_conf;
> >>>> int err;
> >>>> - struct path parent_path;
> >>>>
> >>>> - err = ksmbd_vfs_path_lookup_locked(share_conf, name, flags, path);
> >>>> + err = ksmbd_vfs_path_lookup_locked(share_conf, name, flags,
> >>>> parent_path,
> >>>> + path);
> >>>> if (!err)
> >>>> return 0;
> >>>>
> >>>> @@ -1216,10 +1221,10 @@ int ksmbd_vfs_kern_path_locked(struct
> >>>> ksmbd_work
> >>>> *work, char *name,
> >>>> path_len = strlen(filepath);
> >>>> remain_len = path_len;
> >>>>
> >>>> - parent_path = share_conf->vfs_path;
> >>>> - path_get(&parent_path);
> >>>> + *parent_path = share_conf->vfs_path;
> >>>> + path_get(parent_path);
> >>>>
> >>>> - while (d_can_lookup(parent_path.dentry)) {
> >>>> + while (d_can_lookup(parent_path->dentry)) {
> >>>> char *filename = filepath + path_len - remain_len;
> >>>> char *next = strchrnul(filename, '/');
> >>>> size_t filename_len = next - filename;
> >>>> @@ -1228,7 +1233,7 @@ int ksmbd_vfs_kern_path_locked(struct ksmbd_work
> >>>> *work, char *name,
> >>>> if (filename_len == 0)
> >>>> break;
> >>>>
> >>>> - err = ksmbd_vfs_lookup_in_dir(&parent_path, filename,
> >>>> + err = ksmbd_vfs_lookup_in_dir(parent_path, filename,
> >>>> filename_len,
> >>>> work->conn->um);
> >>>> if (err)
> >>>> @@ -1245,8 +1250,8 @@ int ksmbd_vfs_kern_path_locked(struct ksmbd_work
> >>>> *work, char *name,
> >>>> goto out2;
> >>>> else if (is_last)
> >>>> goto out1;
> >>>> - path_put(&parent_path);
> >>>> - parent_path = *path;
> >>>> + path_put(parent_path);
> >>>> + *parent_path = *path;
> >>>>
> >>>> next[0] = '/';
> >>>> remain_len -= filename_len + 1;
> >>>> @@ -1254,16 +1259,17 @@ int ksmbd_vfs_kern_path_locked(struct
> >>>> ksmbd_work
> >>>> *work, char *name,
> >>>>
> >>>> err = -EINVAL;
> >>>> out2:
> >>>> - path_put(&parent_path);
> >>>> + path_put(parent_path);
> >>>> out1:
> >>>> kfree(filepath);
> >>>> }
> >>>>
> >>>> if (!err) {
> >>>> - err = ksmbd_vfs_lock_parent(parent_path.dentry, path->dentry);
> >>>> - if (err)
> >>>> - dput(path->dentry);
> >>>> - path_put(&parent_path);
> >>>> + err = ksmbd_vfs_lock_parent(parent_path->dentry, path->dentry);
> >>>> + if (err) {
> >>>> + path_put(path);
> >>>> + path_put(parent_path);
> >>>> + }
> >>>> }
> >>>> return err;
> >>>> }
> >>>> diff --git a/fs/smb/server/vfs.h b/fs/smb/server/vfs.h
> >>>> index 80039312c255..72f9fb4b48d1 100644
> >>>> --- a/fs/smb/server/vfs.h
> >>>> +++ b/fs/smb/server/vfs.h
> >>>> @@ -115,8 +115,8 @@ int ksmbd_vfs_xattr_stream_name(char *stream_name,
> >>>> char **xattr_stream_name,
> >>>> int ksmbd_vfs_remove_xattr(struct mnt_idmap *idmap,
> >>>> const struct path *path, char *attr_name);
> >>>> int ksmbd_vfs_kern_path_locked(struct ksmbd_work *work, char *name,
> >>>> - unsigned int flags, struct path *path,
> >>>> - bool caseless);
> >>>> + unsigned int flags, struct path *parent_path,
> >>>> + struct path *path, bool caseless);
> >>>> struct dentry *ksmbd_vfs_kern_path_create(struct ksmbd_work *work,
> >>>> const char *name,
> >>>> unsigned int flags,
> >>>
> >>
> >
--
Thanks,
Steve
