Skip to contentSkip to conversation

ksmbd patches included vfs changes

PATCH Superseded Namjae Jeon 2 participants 5 messages Jul 7, 2022 – Jul 8, 2022

Subsystems (3)

v6 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Namjae Jeon

author maintainer
#0 lore raw

v6 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.

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

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    | 146 ++++-----------
 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(+), 400 deletions(-)
Signature · 1 line
2.34.1
author maintainer
#1 lore raw

PATCH v6 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
index 87e96b9024ce..c994f0408b29 100644
@@ -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
index 5d185564aef6..1770b30772da 100644
@@ -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
index caeb08a98536..40c693525f79 100644
@@ -63,6 +63,8 @@ extern struct dentry *kern_path_create(int, const char *, struct path *, unsigne
 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_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.34.1
author maintainer
#2 lore raw

PATCH v6 2/3 fs: introduce lock_rename_child() helper

In reply to Namjae Jeon

From: 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
index 1f28d3f463c3..c22bdef164ca 100644
@@ -2998,20 +2998,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);
@@ -3030,8 +3020,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
index 40c693525f79..7868732cce24 100644
@@ -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.34.1
author maintainer
#3 lore raw

PATCH v6 3/3 ksmbd: fix racy issue from using ->d_parent and ->d_name

In reply to Namjae Jeon

Al 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 and SMB2_CREATE request.

---
 fs/ksmbd/smb2pdu.c    | 146 ++++-----------
 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(+), 385 deletions(-)
fs/ksmbd/smb2pdu.c +34 -112
index 94ab1dcd80e7..7af4e91bb4a3 100644
@@ -2414,7 +2414,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);
@@ -2702,8 +2702,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
@@ -2712,7 +2714,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;
 			}
 
@@ -2720,27 +2721,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) {
+		user_ns = mnt_user_ns(path.mnt);
+		generic_fillattr(user_ns, d_inode(path.dentry), &stat);
+	} 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);
-		generic_fillattr(user_ns, d_inode(path.dentry), &stat);
 	}
+
 	if (stream_name) {
 		if (req->CreateOptions & FILE_DIRECTORY_FILE_LE) {
 			if (s_type == DATA_STREAM) {
@@ -2866,8 +2863,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;
 			}
@@ -3240,10 +3238,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;
@@ -5370,44 +5371,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 = d_path(&fp->filp->f_path, 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;
@@ -5433,7 +5409,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);
@@ -5448,47 +5424,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;
 }
 
@@ -5528,18 +5475,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",
@@ -5559,6 +5505,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);
@@ -5736,12 +5686,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;
@@ -5751,32 +5695,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->sess->conn->local_nls);
+	return smb2_rename(work, fp, rename_info, work->sess->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
index 8c37aaf936ab..4abfe7f41914 100644
@@ -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, struct path *path);
 int ksmbd_vfs_link(struct ksmbd_work *work,
 		   const char *oldname, const char *newname);
 int ksmbd_vfs_getattr(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, 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
index c4d59d2735f0..df600eb04552 100644
@@ -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
index 7868732cce24..53c03d9ed34c 100644
@@ -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.34.1
#4 lore raw

Re: [PATCH v6 3/3] ksmbd: fix racy issue from using ->d_parent and ->d_name

I love your patch! Perhaps something to improve:

Hi Namjae,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.19-rc5 next-20220708]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Namjae-Jeon/ksmbd-patches-included-vfs-changes/20220707-203352
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9f09069cde34dcd86f5ecf3a3139fd752020812f
config: mips-randconfig-r035-20220708 (https://download.01.org/0day-ci/archive/20220709/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 562c3467a6738aa89203f72fc1d1343e5baadf3c)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mips-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/5f33c60a6259952c632f92a27a1f71b04b484dcc
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Namjae-Jeon/ksmbd-patches-included-vfs-changes/20220707-203352 git checkout 5f33c60a6259952c632f92a27a1f71b04b484dcc
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash fs/ksmbd/

If you fix the issue, kindly add following tag where applicable

All warnings (new ones prefixed by >>):

fs/ksmbd/vfs.c:53: warning: Function parameter or member 'parent' not described in 'ksmbd_vfs_lock_parent' fs/ksmbd/vfs.c:53: warning: Function parameter or member 'child' not described in 'ksmbd_vfs_lock_parent' fs/ksmbd/vfs.c:324: warning: Function parameter or member 'fp' not described in 'ksmbd_vfs_read' fs/ksmbd/vfs.c:324: warning: Excess function parameter 'fid' description in 'ksmbd_vfs_read' fs/ksmbd/vfs.c:441: warning: Function parameter or member 'fp' not described in 'ksmbd_vfs_write' fs/ksmbd/vfs.c:441: warning: Excess function parameter 'fid' description in 'ksmbd_vfs_write' fs/ksmbd/vfs.c:506: warning: Function parameter or member 'path' not described in 'ksmbd_vfs_getattr' fs/ksmbd/vfs.c:506: warning: Function parameter or member 'stat' not described in 'ksmbd_vfs_getattr' fs/ksmbd/vfs.c:506: warning: Excess function parameter 'work' description in 'ksmbd_vfs_getattr' fs/ksmbd/vfs.c:506: warning: Excess function parameter 'fid' description in 'ksmbd_vfs_getattr' fs/ksmbd/vfs.c:506: warning: Excess function parameter 'attrs' description in 'ksmbd_vfs_getattr' fs/ksmbd/vfs.c:523: warning: Function parameter or member 'p_id' not described in 'ksmbd_vfs_fsync' fs/ksmbd/vfs.c:546: warning: Function parameter or member 'work' not described in 'ksmbd_vfs_remove_file' fs/ksmbd/vfs.c:546: warning: Function parameter or member 'path' not described in 'ksmbd_vfs_remove_file' fs/ksmbd/vfs.c:546: warning: Excess function parameter 'name' description in 'ksmbd_vfs_remove_file' fs/ksmbd/vfs.c:584: warning: Function parameter or member 'work' not described in 'ksmbd_vfs_link' fs/ksmbd/vfs.c:750: warning: Function parameter or member 'fp' not described in 'ksmbd_vfs_truncate' fs/ksmbd/vfs.c:750: warning: Excess function parameter 'fid' description in 'ksmbd_vfs_truncate' fs/ksmbd/vfs.c:791: warning: Excess function parameter 'size' description in 'ksmbd_vfs_listxattr' fs/ksmbd/vfs.c:868: warning: Function parameter or member 'attr_name' not described in 'ksmbd_vfs_setxattr' fs/ksmbd/vfs.c:868: warning: Function parameter or member 'attr_value' not described in 'ksmbd_vfs_setxattr' fs/ksmbd/vfs.c:868: warning: Function parameter or member 'attr_size' not described in 'ksmbd_vfs_setxattr' fs/ksmbd/vfs.c:868: warning: Excess function parameter 'name' description in 'ksmbd_vfs_setxattr' fs/ksmbd/vfs.c:868: warning: Excess function parameter 'value' description in 'ksmbd_vfs_setxattr' fs/ksmbd/vfs.c:868: warning: Excess function parameter 'size' description in 'ksmbd_vfs_setxattr' fs/ksmbd/vfs.c:888: warning: Function parameter or member 'option' not described in 'ksmbd_vfs_set_fadvise' fs/ksmbd/vfs.c:888: warning: Excess function parameter 'options' description in 'ksmbd_vfs_set_fadvise' fs/ksmbd/vfs.c:1112: warning: Function parameter or member 'work' not described in 'ksmbd_vfs_kern_path_locked'

>> fs/ksmbd/vfs.c:1112: warning: expecting prototype for ksmbd_vfs_kern_path(). Prototype was for ksmbd_vfs_kern_path_locked() instead

vim +1112 fs/ksmbd/vfs.c

f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1099  
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1100  /**
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1101   * ksmbd_vfs_kern_path() - lookup a file and get path info
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1102   * @name:	file path that is relative to share
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1103   * @flags:	lookup flags
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1104   * @path:	if lookup succeed, return path info
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1105   * @caseless:	caseless filename lookup
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1106   *
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1107   * Return:	0 on success, otherwise error
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1108   */
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1109  int ksmbd_vfs_kern_path_locked(struct ksmbd_work *work, char *name,
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1110  			       unsigned int flags, struct path *path,
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1111  			       bool caseless)
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16 @1112  {
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1113  	struct ksmbd_share_config *share_conf = work->tcon->share_conf;
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1114  	int err;
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1115  	struct path parent_path;
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1116  
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1117  	err = ksmbd_vfs_path_parent_lookup(share_conf, name, flags,
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1118  					   &parent_path);
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1119  	flags |= LOOKUP_BENEATH;
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1120  	if (!err) {
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1121  		err = vfs_path_lookup(share_conf->vfs_path.dentry,
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1122  				      share_conf->vfs_path.mnt,
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1123  				      name,
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1124  				      flags,
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1125  				      path);
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1126  		if (!err)
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1127  			goto lock_parent;
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1128  		path_put(&parent_path);
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1129  	}
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1130  
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1131  	if (caseless) {
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1132  		char *filepath;
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1133  		size_t path_len, remain_len;
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1134  
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1135  		filepath = kstrdup(name, GFP_KERNEL);
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1136  		if (!filepath)
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1137  			return -ENOMEM;
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1138  
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1139  		path_len = strlen(filepath);
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1140  		remain_len = path_len;
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1141  
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1142  		parent_path = share_conf->vfs_path;
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1143  		path_get(&parent_path);
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1144  
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1145  		while (d_can_lookup(parent_path.dentry)) {
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1146  			char *filename = filepath + path_len - remain_len;
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1147  			char *next = strchrnul(filename, '/');
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1148  			size_t filename_len = next - filename;
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1149  			bool is_last = !next[0];
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1150  
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1151  			if (filename_len == 0)
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1152  				break;
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1153  
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1154  			err = ksmbd_vfs_lookup_in_dir(&parent_path, filename,
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1155  						      filename_len);
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1156  			if (err)
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1157  				goto out2;
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1158  
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1159  			next[0] = '\0';
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1160  
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1161  			err = vfs_path_lookup(share_conf->vfs_path.dentry,
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1162  					      share_conf->vfs_path.mnt,
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1163  					      filepath,
265fd1991c1db8 fs/ksmbd/vfs.c Hyunchul Lee 2021-09-25  1164  					      flags,
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1165  					      path);
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1166  			if (err)
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1167  				goto out2;
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1168  			else if (is_last)
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1169  				goto out1;
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1170  			path_put(&parent_path);
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1171  			parent_path = *path;
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1172  
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1173  			next[0] = '/';
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1174  			remain_len -= filename_len + 1;
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1175  		}
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1176  
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1177  		err = -EINVAL;
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1178  out2:
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1179  		path_put(&parent_path);
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1180  out1:
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1181  		kfree(filepath);
3c20378325c710 fs/cifsd/vfs.c Hyunchul Lee 2021-04-13  1182  	}
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1183  
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1184  lock_parent:
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1185  	if (!err) {
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1186  		err = ksmbd_vfs_lock_parent(parent_path.dentry, path->dentry);
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1187  		if (err)
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1188  			path_put(path);
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1189  		path_put(&parent_path);
5f33c60a625995 fs/ksmbd/vfs.c Namjae Jeon  2022-07-07  1190  	}
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1191  	return err;
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1192  }
f44158485826c0 fs/cifsd/vfs.c Namjae Jeon  2021-03-16  1193  
Signature · 2 lines
0-DAY CI Kernel Test Service
https://01.org/lkp