ksmbd patches included vfs changes
Subsystems (3)
Namjae Jeon
maintainerv7 cover ksmbd patches included vfs changes
This patchset contains vfs changes that are needed to fix racy issue in ksmbd. Added the 1/3 patch by Al's additional review comment.
v7:
- constify struct path.
- recreate patch-set base on recent Al's patches.
v6:
- rename __lookup_hash() to lookup_one_qstr_excl and export.
- change dget() to dget_parent() in unlink.
- lock parent of open file in smb2_open() to make file_present worthable.
v5:
- add lock_rename_child() helper.
- remove d_is_symlink() check for new_path.dentry.
- use lock_rename_child() helper instead of lock_rename().
- use dget() instead of dget_parent().
- check that old_child is still hashed.
- directly check child->parent instead of using take_dentry_name_snapshot().
v4:
- switch the order of 3/4 and 4/4 patch.
- fix vfs_path_parent_lookup() parameter description mismatch.
v3:
- use dget_parent + take_dentry_name_snapshot() to check stability of source rename in smb2_vfs_rename().
v2:
- add filename_lock to avoid racy issue from fp->filename. (Sergey Senozhatsky)
- fix warning: variable 'old_dentry' is used uninitialized (kernel test robot)
Al Viro (1):
fs: introduce lock_rename_child() helper
Namjae Jeon (2):
ksmbd: remove internal.h include
ksmbd: fix racy issue from using ->d_parent and ->d_name
fs/internal.h | 2 - fs/ksmbd/smb2pdu.c | 145 ++++----------- fs/ksmbd/vfs.c | 415 +++++++++++++++++------------------------- fs/ksmbd/vfs.h | 19 +- fs/ksmbd/vfs_cache.c | 5 +- fs/namei.c | 103 ++++++++--- include/linux/namei.h | 9 + 7 files changed, 299 insertions(+), 399 deletions(-)
Signature · 1 line
2.25.1
Namjae Jeon
maintainerPATCH v7 1/3 ksmbd: remove internal.h include
Since vfs_path_lookup is exported, It should not be internal. Move vfs_path_lookup prototype in internal.h to linux/namei.h.
--- fs/internal.h | 2 -- fs/ksmbd/vfs.c | 2 -- include/linux/namei.h | 2 ++ 3 files changed, 2 insertions(+), 4 deletions(-)
fs/internal.h +0 -2
@@ -58,8 +58,6 @@ extern int finish_clean_context(struct fs_context *fc);*/ extern int filename_lookup (int dfd,struct filename *name,unsigned flags,struct path *path,struct path *root); -extern int vfs_path_lookup (struct dentry *,struct vfsmount *, -const char *,unsigned int ,struct path *);int do_rmdir (int dfd,struct filename *name);int do_unlinkat (int dfd,struct filename *name);int may_linkat (struct user_namespace *mnt_userns,struct path *link);
fs/ksmbd/vfs.c +0 -2
@@ -18,8 +18,6 @@#include <linux/sched/xacct.h> #include <linux/crc32c.h> -#include "../internal.h" /* for vfs_path_lookup */ -#include "glob.h" #include "oplock.h" #include "connection.h"
include/linux/namei.h +2 -0
@@ -63,6 +63,8 @@ extern struct dentry *kern_path_create(int, const char *, struct path *, unsigneextern struct dentry *user_path_create (int ,const char __user *,struct path *,unsigned int );extern void done_path_create (struct path *,struct dentry *);extern struct dentry *kern_path_locked (const char *,struct path *); +int vfs_path_lookup (struct dentry *,struct vfsmount *,const char *, +unsigned int ,struct path *);extern struct dentry *try_lookup_one_len (const char *,struct dentry *,int );extern struct dentry *lookup_one_len (const char *,struct dentry *,int );
Signature · 1 line
2.25.1
Namjae Jeon
maintainerPATCH v7 2/3 fs: introduce lock_rename_child() helper
In reply to Namjae JeonFrom: Al Viro <[email protected]>
Pass the dentry of a source file and the dentry of a destination directory to lock parent inodes for rename. As soon as this function returns, ->d_parent of the source file dentry is stable and inodes are properly locked for calling vfs-rename. This helper is needed for ksmbd server. rename request of SMB protocol has to rename an opened file, no matter which directory it's in.
--- fs/namei.c | 46 ++++++++++++++++++++++++++++++++----------- include/linux/namei.h | 1 + 2 files changed, 36 insertions(+), 11 deletions(-)
fs/namei.c +35 -11
@@ -2979,20 +2979,10 @@ static inline int may_create(struct user_namespace *mnt_userns,return inode_permission (mnt_userns, dir,MAY_WRITE |MAY_EXEC ); } -/* -* p1 and p2 should be directories on the same fs. -*/ -struct dentry *lock_rename (struct dentry *p1,struct dentry *p2) +static struct dentry *lock_two_directories (struct dentry *p1,struct dentry *p2) {struct dentry *p; -if (p1 == p2) { -inode_lock_nested (p1->d_inode,I_MUTEX_PARENT ); -return NULL ; - } - -mutex_lock (&p1->d_sb->s_vfs_rename_mutex); - p =d_ancestor (p2, p1);if (p) {inode_lock_nested (p2->d_inode,I_MUTEX_PARENT );
@@ -3011,8 +3001,42 @@ struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)inode_lock_nested (p2->d_inode,I_MUTEX_PARENT2 );return NULL ; } + +/* +* p1 and p2 should be directories on the same fs. +*/ +struct dentry *lock_rename (struct dentry *p1,struct dentry *p2) +{ +if (p1 == p2) { +inode_lock_nested (p1->d_inode,I_MUTEX_PARENT ); +return NULL ; + } + +mutex_lock (&p1->d_sb->s_vfs_rename_mutex); +return lock_two_directories (p1, p2); +}EXPORT_SYMBOL (lock_rename); +struct dentry *lock_rename_child (struct dentry *c1,struct dentry *p2) +{ +if (READ_ONCE (c1->d_parent) == p2) { +inode_lock_nested (p2->d_inode,I_MUTEX_PARENT ); +if (likely (c1->d_parent == p2)) +return NULL ; + +inode_unlock (p2->d_inode); + } + +mutex_lock (&c1->d_sb->s_vfs_rename_mutex); +if (likely (c1->d_parent != p2)) +return lock_two_directories (c1->d_parent, p2); + +inode_lock_nested (p2->d_inode,I_MUTEX_PARENT ); +mutex_unlock (&c1->d_sb->s_vfs_rename_mutex); +return NULL ; +} +EXPORT_SYMBOL (lock_rename_child); +void unlock_rename (struct dentry *p1,struct dentry *p2) {inode_unlock (p1->d_inode);
include/linux/namei.h +1 -0
@@ -83,6 +83,7 @@ extern int follow_down(struct path *);extern int follow_up (struct path *);extern struct dentry *lock_rename (struct dentry *,struct dentry *); +extern struct dentry *lock_rename_child (struct dentry *,struct dentry *);extern void unlock_rename (struct dentry *,struct dentry *);extern int __must_check nd_jump_link (struct path *path);
Signature · 1 line
2.25.1
Al Viro
maintainerRe: [PATCH v7 2/3] fs: introduce lock_rename_child() helper
On Wed, Sep 21, 2022 at 07:43:37AM +0900, Namjae Jeon wrote:
FWIW, it probably needs a few comments:
// c1 and p2 should be on the same fs
fs/namei.c struct dentry *lock_rename(struct dentry *p1, struct dentry *p2) · line 3022
Quoted 3 lines
fs/namei.c +3
-0+struct dentry *lock_rename_child (struct dentry *c1,struct dentry *p2) +{ +if (READ_ONCE (c1->d_parent) == p2) {
// hopefully won't need to touch ->s_vfs_rename_mutex at all.
Review
Quoted 1 line
(quoted diff) +1
-0+ inode_lock_nested(p2->d_inode, I_MUTEX_PARENT);
// now that p2 is locked, nobody can move in or out of it, // so the test below is safe
fs/namei.c struct dentry *lock_rename(struct dentry *p1, struct dentry *p2) · line 3026
Quoted 3 lines
fs/namei.c +3
-0+if (likely (c1->d_parent == p2)) +return NULL ; +
// c1 got moved out of p2 while we'd been taking locks; // unlock and fall back to slow case
fs/namei.c struct dentry *lock_rename(struct dentry *p1, struct dentry *p2) · line 3030
Quoted 4 lines
fs/namei.c +4
-0+inode_unlock (p2->d_inode); + } + +mutex_lock (&c1->d_sb->s_vfs_rename_mutex);
// nobody can move out of any directories on this fs
fs/namei.c struct dentry *lock_rename(struct dentry *p1, struct dentry *p2) · line 3033
Quoted 3 lines
fs/namei.c +3
-0+if (likely (c1->d_parent != p2)) +return lock_two_directories (c1->d_parent, p2); +
// c1 got moved into p2 while we were taking locks; // we need p2 locked and ->s_vfs_rename_mutex unlocked, // for consistency with lock_rename().
Quoted 5 lines
fs/namei.c +5
-0+inode_lock_nested (p2->d_inode,I_MUTEX_PARENT ); +mutex_unlock (&c1->d_sb->s_vfs_rename_mutex); +return NULL ; +} +EXPORT_SYMBOL (lock_rename_child);
Namjae Jeon
maintainerRe: [PATCH v7 2/3] fs: introduce lock_rename_child() helper
2022-10-02 9:21 GMT+09:00, Al Viro <[email protected]>:
Quoted 4 lines · Namjae Jeon
FWIW, it probably needs a few comments:
I will add it on next spin.
Thanks for your review!
Quoted 31 lines
// c1 and p2 should be on the same fs
fs/namei.c struct dentry *lock_rename(struct dentry *p1, struct dentry *p2) · line 3022
Quoted 3 lines
fs/namei.c +3
-0+struct dentry *lock_rename_child (struct dentry *c1,struct dentry *p2) +{ +if (READ_ONCE (c1->d_parent) == p2) {// hopefully won't need to touch ->s_vfs_rename_mutex at all.Review
Quoted 1 line
(quoted diff) +1
-0+ inode_lock_nested(p2->d_inode, I_MUTEX_PARENT);// now that p2 is locked, nobody can move in or out of it, // so the test below is safefs/namei.c struct dentry *lock_rename(struct dentry *p1, struct dentry *p2) · line 3026
Quoted 3 lines
fs/namei.c +3
-0+if (likely (c1->d_parent == p2)) +return NULL ; +// c1 got moved out of p2 while we'd been taking locks; // unlock and fall back to slow casefs/namei.c struct dentry *lock_rename(struct dentry *p1, struct dentry *p2) · line 3030
Quoted 4 lines
fs/namei.c +4
-0+inode_unlock (p2->d_inode); + } + +mutex_lock (&c1->d_sb->s_vfs_rename_mutex);// nobody can move out of any directories on this fsfs/namei.c struct dentry *lock_rename(struct dentry *p1, struct dentry *p2) · line 3033
Quoted 3 lines
fs/namei.c +3
-0+if (likely (c1->d_parent != p2)) +return lock_two_directories (c1->d_parent, p2); +// c1 got moved into p2 while we were taking locks; // we need p2 locked and ->s_vfs_rename_mutex unlocked, // for consistency with lock_rename().Quoted 5 lines
fs/namei.c +5
-0+inode_lock_nested (p2->d_inode,I_MUTEX_PARENT ); +mutex_unlock (&c1->d_sb->s_vfs_rename_mutex); +return NULL ; +} +EXPORT_SYMBOL (lock_rename_child);
Namjae Jeon
maintainerPATCH v7 3/3 ksmbd: fix racy issue from using ->d_parent and ->d_name
In reply to Namjae JeonAl pointed out that ksmbd has racy issue from using ->d_parent and ->d_name in ksmbd_vfs_unlink and smb2_vfs_rename(). and use new lock_rename_child() to lock stable parent while underlying rename racy.
Introduce vfs_path_parent_lookup helper to avoid out of share access and export vfs functions like the following ones to use vfs_path_parent_lookup().
- rename __lookup_hash() to lookup_one_qstr_excl().
- export lookup_one_qstr_excl().
- export getname_kernel() and putname().
vfs_path_parent_lookup() is used for parent lookup of destination file using absolute pathname given from FILE_RENAME_INFORMATION request.
--- fs/ksmbd/smb2pdu.c | 145 ++++----------- fs/ksmbd/vfs.c | 413 +++++++++++++++++------------------------- fs/ksmbd/vfs.h | 19 +- fs/ksmbd/vfs_cache.c | 5 +- fs/namei.c | 57 ++++-- include/linux/namei.h | 6 + 6 files changed, 261 insertions(+), 384 deletions(-)
fs/ksmbd/smb2pdu.c +34 -111
@@ -2436,7 +2436,7 @@ static int smb2_creat(struct ksmbd_work *work, struct path *path, char *name,return rc; } - rc =ksmbd_vfs_kern_path (work, name,0 , path,0 ); + rc =ksmbd_vfs_kern_path_locked (work, name,0 , path,0 );if (rc) {pr_err ("cannot get linux path (%s), err = %d\n" , name, rc);
@@ -2727,8 +2727,10 @@ int smb2_open(struct ksmbd_work *work) goto err_out1; } - rc =ksmbd_vfs_kern_path (work, name,LOOKUP_NO_SYMLINKS , &path,1 ); + rc =ksmbd_vfs_kern_path_locked (work, name,LOOKUP_NO_SYMLINKS , &path,1 );if (!rc) { + file_present = true; +if (req->CreateOptions &FILE_DELETE_ON_CLOSE_LE ) {/* * If file exists with under flags, return access
@@ -2737,7 +2739,6 @@ int smb2_open(struct ksmbd_work *work)if (req->CreateDisposition ==FILE_OVERWRITE_IF_LE || req->CreateDisposition ==FILE_OPEN_IF_LE ) { rc = -EACCES ; -path_put (&path); goto err_out; }
@@ -2745,26 +2746,23 @@ int smb2_open(struct ksmbd_work *work)ksmbd_debug (SMB ,"User does not have write permission\n" ); rc = -EACCES ; -path_put (&path); goto err_out; } }else if (d_is_symlink (path.dentry)) { rc = -EACCES ; -path_put (&path); goto err_out; } - } -if (rc) { + file_present = true; + user_ns =mnt_user_ns (path.mnt); + }else {if (rc != -ENOENT ) goto err_out;ksmbd_debug (SMB ,"can not get linux path for %s, rc = %d\n" , name, rc); rc =0 ; - }else { - file_present = true; - user_ns =mnt_user_ns (path.mnt); } +if (stream_name) {if (req->CreateOptions &FILE_DIRECTORY_FILE_LE ) {if (s_type ==DATA_STREAM ) {
@@ -2892,8 +2890,9 @@ int smb2_open(struct ksmbd_work *work)if ((daccess &FILE_DELETE_LE ) || (req->CreateOptions &FILE_DELETE_ON_CLOSE_LE )) { - rc =ksmbd_vfs_may_delete (user_ns, - path.dentry); + rc =inode_permission (user_ns, +d_inode (path.dentry->d_parent), +MAY_EXEC |MAY_WRITE );if (rc) goto err_out; }
@@ -3261,10 +3260,13 @@ int smb2_open(struct ksmbd_work *work) } err_out: -if (file_present || created) +if (file_present || created) { +inode_unlock (d_inode (path.dentry->d_parent));path_put (&path); + }ksmbd_revert_fsids (work); err_out1: +if (rc) {if (rc == -EINVAL ) rsp->hdr.Status =STATUS_INVALID_PARAMETER ;
@@ -5403,44 +5405,19 @@ int smb2_echo(struct ksmbd_work *work)static int smb2_rename (struct ksmbd_work *work,struct ksmbd_file *fp, -struct user_namespace *user_ns,struct smb2_file_rename_info *file_info,struct nls_table *local_nls) {struct ksmbd_share_config *share = fp->tcon->share_conf; -char *new_name =NULL , *abs_oldname =NULL , *old_name =NULL ; -char *pathname =NULL ; -struct path path; -bool file_present = true; -int rc; +char *new_name =NULL ; +int rc, flags =0 ;ksmbd_debug (SMB ,"setting FILE_RENAME_INFO\n" ); - pathname =kmalloc (PATH_MAX ,GFP_KERNEL ); -if (!pathname) -return -ENOMEM ; - - abs_oldname =file_path (fp->filp, pathname,PATH_MAX ); -if (IS_ERR (abs_oldname)) { - rc = -EINVAL ; - goto out; - } - old_name =strrchr (abs_oldname,'/' ); -if (old_name && old_name[1 ] !='\0' ) { - old_name++; - }else { -ksmbd_debug (SMB ,"can't get last component in path %s\n" , - abs_oldname); - rc = -ENOENT ; - goto out; - } - new_name =smb2_get_name (file_info->FileName,le32_to_cpu (file_info->FileNameLength), local_nls); -if (IS_ERR (new_name)) { - rc =PTR_ERR (new_name); - goto out; - } +if (IS_ERR (new_name)) +return PTR_ERR (new_name);if (strchr (new_name,':' )) {int s_type;
@@ -5466,7 +5443,7 @@ static int smb2_rename(struct ksmbd_work *work,if (rc) goto out; - rc =ksmbd_vfs_setxattr (user_ns, + rc =ksmbd_vfs_setxattr (file_mnt_user_ns (fp->filp), fp->filp->f_path.dentry, xattr_stream_name,NULL ,0 ,0 );
@@ -5481,47 +5458,18 @@ static int smb2_rename(struct ksmbd_work *work, }ksmbd_debug (SMB ,"new name %s\n" , new_name); - rc =ksmbd_vfs_kern_path (work, new_name,LOOKUP_NO_SYMLINKS , &path,1 ); -if (rc) { -if (rc != -ENOENT ) - goto out; - file_present = false; - }else { -path_put (&path); - } -if (ksmbd_share_veto_filename (share, new_name)) { rc = -ENOENT ;ksmbd_debug (SMB ,"Can't rename vetoed file: %s\n" , new_name); goto out; } -if (file_info->ReplaceIfExists) { -if (file_present) { - rc =ksmbd_vfs_remove_file (work, new_name); -if (rc) { -if (rc != -ENOTEMPTY ) - rc = -EINVAL ; -ksmbd_debug (SMB ,"cannot delete %s, rc %d\n" , - new_name, rc); - goto out; - } - } - }else { -if (file_present && -strncmp (old_name, path.dentry->d_name.name,strlen (old_name))) { - rc = -EEXIST ; -ksmbd_debug (SMB , -"cannot rename already existing file\n" ); - goto out; - } - } +if (!file_info->ReplaceIfExists) + flags =RENAME_NOREPLACE ; - rc =ksmbd_vfs_fp_rename (work, fp, new_name); + rc =ksmbd_vfs_rename (work, &fp->filp->f_path, new_name, flags); out: -kfree (pathname); -if (!IS_ERR (new_name)) -kfree (new_name); +kfree (new_name);return rc; }
@@ -5561,18 +5509,17 @@ static int smb2_create_link(struct ksmbd_work *work, }ksmbd_debug (SMB ,"target name is %s\n" , target_name); - rc =ksmbd_vfs_kern_path (work, link_name,LOOKUP_NO_SYMLINKS , &path,0 ); + rc =ksmbd_vfs_kern_path_locked (work, link_name,LOOKUP_NO_SYMLINKS , + &path,0 );if (rc) {if (rc != -ENOENT ) goto out; file_present = false; - }else { -path_put (&path); }if (file_info->ReplaceIfExists) {if (file_present) { - rc =ksmbd_vfs_remove_file (work, link_name); + rc =ksmbd_vfs_remove_file (work, &path);if (rc) { rc = -EINVAL ; ksmbd_debug(SMB, "cannot delete %s\n",
@@ -5592,6 +5539,10 @@ static int smb2_create_link(struct ksmbd_work *work,if (rc) rc = -EINVAL ; out: +if (file_present) { +inode_unlock (d_inode (path.dentry->d_parent)); +path_put (&path); + }if (!IS_ERR (link_name))kfree (link_name);kfree (pathname);
@@ -5769,12 +5720,6 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp, struct smb2_file_rename_info *rename_info, unsigned int buf_len) { -struct user_namespace *user_ns; -struct ksmbd_file *parent_fp; -struct dentry *parent; -struct dentry *dentry = fp->filp->f_path.dentry; -int ret; -if (!(fp->daccess &FILE_DELETE_LE )) {pr_err ("no right to delete : 0x%x\n" , fp->daccess);return -EACCES ;
@@ -5784,32 +5729,10 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp, le32_to_cpu(rename_info->FileNameLength)) return -EINVAL; - user_ns =file_mnt_user_ns (fp->filp); -if (ksmbd_stream_fd (fp)) - goto next; - - parent =dget_parent (dentry); - ret =ksmbd_vfs_lock_parent (user_ns, parent, dentry); -if (ret) { -dput (parent); -return ret; - } - - parent_fp =ksmbd_lookup_fd_inode (d_inode (parent)); -inode_unlock (d_inode (parent)); -dput (parent); +if (!le32_to_cpu (rename_info->FileNameLength)) +return -EINVAL ; -if (parent_fp) { -if (parent_fp->daccess &FILE_DELETE_LE ) { -pr_err ("parent dir is opened with delete access\n" ); -ksmbd_fd_put (work, parent_fp); -return -ESHARE ; - } -ksmbd_fd_put (work, parent_fp); - } -next: -return smb2_rename (work, fp, user_ns, rename_info, - work->conn->local_nls); +return smb2_rename (work, fp, rename_info, work->conn->local_nls); } static int set_file_disposition_info(struct ksmbd_file *fp,
fs/ksmbd/vfs.c +167 -246
566 lines not on this page · raw message
fs/ksmbd/vfs.h +8 -11
@@ -69,9 +69,7 @@ struct ksmbd_kstat {__le32 file_attributes; }; -int ksmbd_vfs_lock_parent (struct user_namespace *user_ns,struct dentry *parent, -struct dentry *child); -int ksmbd_vfs_may_delete (struct user_namespace *user_ns,struct dentry *dentry); +int ksmbd_vfs_lock_parent (struct dentry *parent,struct dentry *child);int ksmbd_vfs_query_maximal_access (struct user_namespace *user_ns,struct dentry *dentry,__le32 *daccess);int ksmbd_vfs_create (struct ksmbd_work *work,const char *name,umode_t mode);
@@ -82,12 +80,12 @@ int ksmbd_vfs_write(struct ksmbd_work *work, struct ksmbd_file *fp, char *buf, size_t count, loff_t *pos, bool sync, ssize_t *written);int ksmbd_vfs_fsync (struct ksmbd_work *work,u64 fid,u64 p_id); -int ksmbd_vfs_remove_file (struct ksmbd_work *work,char *name); +int ksmbd_vfs_remove_file (struct ksmbd_work *work,const struct path *path);int ksmbd_vfs_link (struct ksmbd_work *work,const char *oldname,const char *newname);int ksmbd_vfs_getattr (const struct path *path,struct kstat *stat); -int ksmbd_vfs_fp_rename (struct ksmbd_work *work,struct ksmbd_file *fp, -char *newname); +int ksmbd_vfs_rename (struct ksmbd_work *work,const struct path *old_path, +char *newname,int flags);int ksmbd_vfs_truncate (struct ksmbd_work *work,struct ksmbd_file *fp,loff_t size);struct srv_copychunk ;
@@ -114,9 +112,9 @@ int ksmbd_vfs_xattr_stream_name(char *stream_name, char **xattr_stream_name, size_t *xattr_stream_name_size, int s_type);int ksmbd_vfs_remove_xattr (struct user_namespace *user_ns,struct dentry *dentry,char *attr_name); -int ksmbd_vfs_kern_path (struct ksmbd_work *work, -char *name,unsigned int flags,struct path *path, -bool caseless); +int ksmbd_vfs_kern_path_locked (struct ksmbd_work *work,char *name, +unsigned int flags,struct path *path, +bool caseless); struct dentry *ksmbd_vfs_kern_path_create(struct ksmbd_work *work, const char *name, unsigned int flags,
@@ -129,8 +127,7 @@ struct file_allocated_range_buffer;int ksmbd_vfs_fqar_lseek (struct ksmbd_file *fp,loff_t start,loff_t length,struct file_allocated_range_buffer *ranges,unsigned int in_count,unsigned int *out_count); -int ksmbd_vfs_unlink (struct user_namespace *user_ns, -struct dentry *dir,struct dentry *dentry); +int ksmbd_vfs_unlink (struct file *filp);void *ksmbd_vfs_init_kstat (char **p,struct ksmbd_kstat *ksmbd_kstat); int ksmbd_vfs_fill_dentry_attrs(struct ksmbd_work *work, struct user_namespace *user_ns,
fs/ksmbd/vfs_cache.c +1 -4
@@ -243,7 +243,6 @@ void ksmbd_release_inode_hash(void)static void __ksmbd_inode_close (struct ksmbd_file *fp) { -struct dentry *dir, *dentry;struct ksmbd_inode *ci = fp->f_ci;int err;struct file *filp;
@@ -262,11 +261,9 @@ static void __ksmbd_inode_close(struct ksmbd_file *fp)if (atomic_dec_and_test (&ci->m_count)) {write_lock (&ci->m_lock);if (ci->m_flags & (S_DEL_ON_CLS |S_DEL_PENDING )) { - dentry = filp->f_path.dentry; - dir = dentry->d_parent; ci->m_flags &= ~(S_DEL_ON_CLS |S_DEL_PENDING );write_unlock (&ci->m_lock); -ksmbd_vfs_unlink (file_mnt_user_ns (filp), dir, dentry); +ksmbd_vfs_unlink (filp);write_lock (&ci->m_lock); }write_unlock (&ci->m_lock);
fs/namei.c +45 -12
141 lines not on this page · raw message
include/linux/namei.h +6 -0
@@ -57,12 +57,18 @@ static inline int user_path_at(int dfd, const char __user *name, unsigned flags,return user_path_at_empty (dfd, name, flags, path,NULL ); } +struct dentry *lookup_one_qstr_excl (const struct qstr *name, +struct dentry *base, +unsigned int flags);extern int kern_path (const char *,unsigned ,struct path *);extern struct dentry *kern_path_create (int ,const char *,struct path *,unsigned int );extern struct dentry *user_path_create (int ,const char __user *,struct path *,unsigned int );extern void done_path_create (struct path *,struct dentry *);extern struct dentry *kern_path_locked (const char *,struct path *); +int vfs_path_parent_lookup (struct filename *filename,unsigned int flags, +struct path *parent,struct qstr *last,int *type, +const struct path *root);int vfs_path_lookup (struct dentry *,struct vfsmount *,const char *,unsigned int ,struct path *);
Signature · 1 line
2.25.1
Al Viro
maintainerRe: [PATCH v7 3/3] ksmbd: fix racy issue from using ->d_parent and ->d_name
Quoted 19 lines · Namjae Jeon
fs/ksmbd/vfs.c +6
-8-int ksmbd_vfs_kern_path (struct ksmbd_work *work,char *name, -unsigned int flags,struct path *path,bool caseless) +int ksmbd_vfs_kern_path_locked (struct ksmbd_work *work,char *name, +unsigned int flags,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_parent_lookup (share_conf, name, flags, + &parent_path); flags |=LOOKUP_BENEATH ; - err =vfs_path_lookup (share_conf->vfs_path.dentry, - share_conf->vfs_path.mnt, - name, - flags, - path); -if (!err)
fs/ksmbd/vfs.c static int ksmbd_vfs_lookup_in_dir(const struct path *dir, char *name, size_t na · line 1125
Quoted 10 lines
fs/ksmbd/vfs.c +9
-1-return 0 ; +if (!err) { + err =vfs_path_lookup (share_conf->vfs_path.dentry, + share_conf->vfs_path.mnt, + name, + flags, + path); +if (!err) + goto lock_parent; +path_put (&parent_path);
This is wrong. You have already resolved the sucker to parent
+ last component. Now you ask vfs_path_lookup() to
* redo the same thing, hopefully arriving to the same spot.
* look the last component up in wherever it has arrived.
then you
* lock the place you'd originally arrived at
* check if the result of last lookup is its child (i.e.
it hadn't moved since we looked it up and lookup hopefully arrived to the same spot for parent.
That's far too convoluted...
Namjae Jeon
maintainerRe: [PATCH v7 3/3] ksmbd: fix racy issue from using ->d_parent and ->d_name
2022-10-02 9:39 GMT+09:00, Al Viro <[email protected]>:
Quoted 44 lines · Namjae Jeon
Quoted 21 lines
fs/ksmbd/vfs.c +6
-8-int ksmbd_vfs_kern_path (struct ksmbd_work *work,char *name, -unsigned int flags,struct path *path,bool caseless) +int ksmbd_vfs_kern_path_locked (struct ksmbd_work *work,char *name, +unsigned int flags,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_parent_lookup (share_conf, name, flags, + &parent_path); flags |=LOOKUP_BENEATH ; - err =vfs_path_lookup (share_conf->vfs_path.dentry, - share_conf->vfs_path.mnt, - name, - flags, - path); -if (!err)fs/ksmbd/vfs.c static int ksmbd_vfs_lookup_in_dir(const struct path *dir, char *name, size_t na · line 1125
Quoted 10 lines
fs/ksmbd/vfs.c +9
-1-return 0 ; +if (!err) { + err =vfs_path_lookup (share_conf->vfs_path.dentry, + share_conf->vfs_path.mnt, + name, + flags, + path); +if (!err) + goto lock_parent; +path_put (&parent_path);This is wrong. You have already resolved the sucker to parent
+ last component. Now you askvfs_path_lookup()to
* redo the same thing, hopefully arriving to the same spot.
* look the last component up in wherever it has arrived.
then you
* lock the place you'd originally arrived at
* check if the result of last lookup is its child (i.e.
it hadn't moved since we looked it up and lookup hopefully arrived to the same spot for parent.
Quoted 1 line
That's far too convoluted...
Right. Need to avoid repeat lookup.
I have called vfs_path_lookup() again to avoid accessing out of share and get struct path of child. I may try to change vfs_path_lookup()(or create new vfs function) to return struct path of parent as well struct path of child... ?
Thanks for your review!