[PATCH 0/4] smb/server: various clean-ups

[PATCH 0/4] smb/server: various clean-ups

[NeilBrown]

I am working towards making some changes to how locking is managed for
directory operations.  Prior to attempting to land these changes I am
reviewing code that requests directory operations and cleaning up things
that might cause me problems later.

These 4 patches are the result of my review of smb/server.  Note that
patch 3 fixes what appears to be a real deadlock that should be trivial
to hit if the client can actually set the flag which, as mentioned in
the patch, can trigger the deadlock.

Patch 1 is trivial but the others deserve careful review by someone who
knows the code.  I think they are correct, but I've been wrong before.

Thanks,
NeilBrown

 [PATCH 1/4] smb/server: use lookup_one_unlocked()
 [PATCH 2/4] smb/server: simplify ksmbd_vfs_kern_path_locked()
 [PATCH 3/4] smb/server: avoid deadlock when linking with
 [PATCH 4/4] smb/server: add ksmbd_vfs_kern_path()

[PATCH 1/4] smb/server: use lookup_one_unlocked()

[NeilBrown]

In process_query_dir_entries(), instead of locking the directory,
performing a lookup, then unlocking, we can simply call
lookup_one_unlocked().  That takes locks the directory only when needed.

This removes the only users of lock_dir() and unlock_dir() so they can
be removed.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/smb/server/smb2pdu.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 1a308171b599..0013052f5d98 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -4107,20 +4107,6 @@ struct smb2_query_dir_private {
 	int			info_level;
 };
 
-static void lock_dir(struct ksmbd_file *dir_fp)
-{
-	struct dentry *dir = dir_fp->filp->f_path.dentry;
-
-	inode_lock_nested(d_inode(dir), I_MUTEX_PARENT);
-}
-
-static void unlock_dir(struct ksmbd_file *dir_fp)
-{
-	struct dentry *dir = dir_fp->filp->f_path.dentry;
-
-	inode_unlock(d_inode(dir));
-}
-
 static int process_query_dir_entries(struct smb2_query_dir_private *priv)
 {
 	struct mnt_idmap	*idmap = file_mnt_idmap(priv->dir_fp->filp);
@@ -4135,12 +4121,10 @@ static int process_query_dir_entries(struct smb2_query_dir_private *priv)
 		if (dentry_name(priv->d_info, priv->info_level))
 			return -EINVAL;
 
-		lock_dir(priv->dir_fp);
-		dent = lookup_one(idmap,
-				  &QSTR_LEN(priv->d_info->name,
-					    priv->d_info->name_len),
-				  priv->dir_fp->filp->f_path.dentry);
-		unlock_dir(priv->dir_fp);
+		dent = lookup_one_unlocked(idmap,
+					   &QSTR_LEN(priv->d_info->name,
+						     priv->d_info->name_len),
+					   priv->dir_fp->filp->f_path.dentry);
 
 		if (IS_ERR(dent)) {
 			ksmbd_debug(SMB, "Cannot lookup `%s' [%ld]\n",
-- 
2.49.0

[PATCH 2/4] smb/server: simplify ksmbd_vfs_kern_path_locked()

[NeilBrown]

ksmbd_vfs_kern_path_locked() first tries to look up the path with the
given case.  When this fails, if caseless is set, it loops over the
components in the path, opening the relevant directory and searching
for a name which matches.  This name is copied over the original and the
the process repeats.  Each time a lookup with the newly updated path is
repeated from the top (vfs_path_lookup()).

When the last component has been case-corrected the simplest next step
is to repeat the original lookup with ksmbd_vfs_path_lookup_locked().
If this works it gives exactly what we want, if it fails it gives the
correct failure.

This observation allows the code to be simplified, in particular
removing the ksmbd_vfs_lock_parent() call.

This patch also removes the duplication of "name" and "filepath" (two
names for the one thing) and calls path_put(parent_path) sooner so
parent_path can be passed directly to vfs_path_lookup avoiding the need
to store it temporarily in "path" and then copying into parent_path.

This patch removes one user of ksmbd_vfs_lock_parent() which will
simplify a future patch.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/smb/server/vfs.c | 101 ++++++++++++++++++--------------------------
 1 file changed, 40 insertions(+), 61 deletions(-)

diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
index ba45e809555a..654c2f14a9e6 100644
--- a/fs/smb/server/vfs.c
+++ b/fs/smb/server/vfs.c
@@ -1208,83 +1208,62 @@ 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,
+int ksmbd_vfs_kern_path_locked(struct ksmbd_work *work, char *filepath,
 			       unsigned int flags, struct path *parent_path,
 			       struct path *path, bool caseless)
 {
 	struct ksmbd_share_config *share_conf = work->tcon->share_conf;
+	size_t path_len, remain_len;
 	int err;
 
-	err = ksmbd_vfs_path_lookup_locked(share_conf, name, flags, parent_path,
+retry:
+	err = ksmbd_vfs_path_lookup_locked(share_conf, filepath, flags, parent_path,
 					   path);
-	if (!err)
-		return 0;
-
-	if (caseless) {
-		char *filepath;
-		size_t path_len, remain_len;
-
-		filepath = name;
-		path_len = strlen(filepath);
-		remain_len = path_len;
-
-		*parent_path = share_conf->vfs_path;
-		path_get(parent_path);
-
-		while (d_can_lookup(parent_path->dentry)) {
-			char *filename = filepath + path_len - remain_len;
-			char *next = strchrnul(filename, '/');
-			size_t filename_len = next - filename;
-			bool is_last = !next[0];
-
-			if (filename_len == 0)
-				break;
+	if (!err || !caseless)
+		return err;
 
-			err = ksmbd_vfs_lookup_in_dir(parent_path, filename,
-						      filename_len,
-						      work->conn->um);
-			if (err)
-				goto out2;
+	path_len = strlen(filepath);
+	remain_len = path_len;
 
-			next[0] = '\0';
+	*parent_path = share_conf->vfs_path;
+	path_get(parent_path);
 
-			err = vfs_path_lookup(share_conf->vfs_path.dentry,
-					      share_conf->vfs_path.mnt,
-					      filepath,
-					      flags,
-					      path);
-			if (!is_last)
-				next[0] = '/';
-			if (err)
-				goto out2;
-			else if (is_last)
-				goto out1;
-			path_put(parent_path);
-			*parent_path = *path;
+	while (d_can_lookup(parent_path->dentry)) {
+		char *filename = filepath + path_len - remain_len;
+		char *next = strchrnul(filename, '/');
+		size_t filename_len = next - filename;
+		bool is_last = !next[0];
 
-			remain_len -= filename_len + 1;
-		}
+		if (filename_len == 0)
+			break;
 
-		err = -EINVAL;
-out2:
+		err = ksmbd_vfs_lookup_in_dir(parent_path, filename,
+					      filename_len,
+					      work->conn->um);
 		path_put(parent_path);
-	}
-
-out1:
-	if (!err) {
-		err = mnt_want_write(parent_path->mnt);
-		if (err) {
-			path_put(path);
-			path_put(parent_path);
-			return err;
+		if (err)
+			goto out;
+		if (is_last) {
+			caseless = false;
+			goto retry;
 		}
+		next[0] = '\0';
+
+		err = vfs_path_lookup(share_conf->vfs_path.dentry,
+				      share_conf->vfs_path.mnt,
+				      filepath,
+				      flags,
+				      parent_path);
+		next[0] = '/';
+		if (err)
+			goto out;
 
-		err = ksmbd_vfs_lock_parent(parent_path->dentry, path->dentry);
-		if (err) {
-			path_put(path);
-			path_put(parent_path);
-		}
+		remain_len -= filename_len + 1;
 	}
+
+	err = -EINVAL;
+	path_put(parent_path);
+out:
 	return err;
 }
 
-- 
2.49.0

[PATCH 3/4] smb/server: avoid deadlock when linking with ReplaceIfExists

[NeilBrown]

If smb2_create_link() is called with ReplaceIfExists set and the name
does exist then a deadlock will happen.

ksmbd_vfs_kern_path_locked() will return with success and the parent
directory will be locked.  ksmbd_vfs_remove_file() will then remove the
file.  ksmbd_vfs_link() will then be called while the parent is still
locked.  It will try to lock the same parent and will deadlock.

This patch moves the ksmbd_vfs_kern_path_unlock() call to *before*
ksmbd_vfs_link() and then simplifies the code, removing the file_present
flag variable.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/smb/server/smb2pdu.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 0013052f5d98..2e98717a0268 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -6009,7 +6009,6 @@ static int smb2_create_link(struct ksmbd_work *work,
 {
 	char *link_name = NULL, *target_name = NULL, *pathname = NULL;
 	struct path path, parent_path;
-	bool file_present = false;
 	int rc;
 
 	if (buf_len < (u64)sizeof(struct smb2_file_link_info) +
@@ -6042,11 +6041,8 @@ static int smb2_create_link(struct ksmbd_work *work,
 	if (rc) {
 		if (rc != -ENOENT)
 			goto out;
-	} else
-		file_present = true;
-
-	if (file_info->ReplaceIfExists) {
-		if (file_present) {
+	} else {
+		if (file_info->ReplaceIfExists) {
 			rc = ksmbd_vfs_remove_file(work, &path);
 			if (rc) {
 				rc = -EINVAL;
@@ -6054,21 +6050,17 @@ static int smb2_create_link(struct ksmbd_work *work,
 					    link_name);
 				goto out;
 			}
-		}
-	} else {
-		if (file_present) {
+		} else {
 			rc = -EEXIST;
 			ksmbd_debug(SMB, "link already exists\n");
 			goto out;
 		}
+		ksmbd_vfs_kern_path_unlock(&parent_path, &path);
 	}
-
 	rc = ksmbd_vfs_link(work, target_name, link_name);
 	if (rc)
 		rc = -EINVAL;
 out:
-	if (file_present)
-		ksmbd_vfs_kern_path_unlock(&parent_path, &path);
 
 	if (!IS_ERR(link_name))
 		kfree(link_name);
-- 
2.49.0

[PATCH 4/4] smb/server: add ksmbd_vfs_kern_path()

[NeilBrown]

The function ksmbd_vfs_kern_path_locked() seems to serve two functions
and as a result has an odd interface.

On success it returns with the parent directory locked and with write
access on that filesystem requested, but it may have crossed over a
mount point to return the "path", which makes the lock and the write
access irrelevant.

This patches separates the functionality into two functions:
- ksmbd_vfs_kern_path() does not lock the parent, does not request
  write access, but does cross mount points
- ksmbd_vfs_kern_path_locked() does not cross mount points but
  does lock the parent and request write access.

The parent_path parameter is no longer needed.  For the _locked case
the final path is sufficient to drop write access and to unlock the
parent (using path->dentry->d_parent which is safe while the lock is
held).

There were 3 caller of ksmbd_vfs_kern_path_locked().

- smb2_create_link() needs to remove the target if it existed and
  needs the lock and the write-access, so it continues to use
  ksmbd_vfs_kern_path_locked().  It would not make sense to
  cross mount points in this case.
- smb2_open() is the only user that needs to cross mount points
  and it has no need for the lock or write access, so it now uses
  ksmbd_vfs_kern_path()
- smb2_creat() does not need to cross mountpoints as it is accessing
  a file that it has just created on *this* filesystem.  But also it
  does not need the lock or write access because by the time
  ksmbd_vfs_kern_path_locked() was called it has already created the
  file.  So it could use either interface.  It is simplest to use
  ksmbd_vfs_kern_path().

ksmbd_vfs_kern_path_unlock() is still needed after
ksmbd_vfs_kern_path_locked() but it doesn't require the parent_path any
more.  After a successful call to ksmbd_vfs_kern_path(), only path_put()
is needed to release the path.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/smb/server/smb2pdu.c |  22 +++---
 fs/smb/server/vfs.c     | 156 ++++++++++++++++++++++++++--------------
 fs/smb/server/vfs.h     |   7 +-
 3 files changed, 118 insertions(+), 67 deletions(-)

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 2e98717a0268..6d2faf19ab96 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -2580,7 +2580,7 @@ static void smb2_update_xattrs(struct ksmbd_tree_connect *tcon,
 	}
 }
 
-static int smb2_creat(struct ksmbd_work *work, struct path *parent_path,
+static int smb2_creat(struct ksmbd_work *work,
 		      struct path *path, char *name, int open_flags,
 		      umode_t posix_mode, bool is_dir)
 {
@@ -2609,7 +2609,7 @@ static int smb2_creat(struct ksmbd_work *work, struct path *parent_path,
 			return rc;
 	}
 
-	rc = ksmbd_vfs_kern_path_locked(work, name, 0, parent_path, path, 0);
+	rc = ksmbd_vfs_kern_path(work, name, 0, path, 0);
 	if (rc) {
 		pr_err("cannot get linux path (%s), err = %d\n",
 		       name, rc);
@@ -2859,7 +2859,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, parent_path;
+	struct path path;
 	struct ksmbd_share_config *share = tcon->share_conf;
 	struct ksmbd_file *fp = NULL;
 	struct file *filp = NULL;
@@ -3115,8 +3115,8 @@ int smb2_open(struct ksmbd_work *work)
 		goto err_out2;
 	}
 
-	rc = ksmbd_vfs_kern_path_locked(work, name, LOOKUP_NO_SYMLINKS,
-					&parent_path, &path, 1);
+	rc = ksmbd_vfs_kern_path(work, name, LOOKUP_NO_SYMLINKS,
+				 &path, 1);
 	if (!rc) {
 		file_present = true;
 
@@ -3237,7 +3237,7 @@ int smb2_open(struct ksmbd_work *work)
 
 	/*create file if not present */
 	if (!file_present) {
-		rc = smb2_creat(work, &parent_path, &path, name, open_flags,
+		rc = smb2_creat(work, &path, name, open_flags,
 				posix_mode,
 				req->CreateOptions & FILE_DIRECTORY_FILE_LE);
 		if (rc) {
@@ -3442,7 +3442,7 @@ int smb2_open(struct ksmbd_work *work)
 	}
 
 	if (file_present || created)
-		ksmbd_vfs_kern_path_unlock(&parent_path, &path);
+		path_put(&path);
 
 	if (!S_ISDIR(file_inode(filp)->i_mode) && open_flags & O_TRUNC &&
 	    !fp->attrib_only && !stream_name) {
@@ -3723,7 +3723,7 @@ int smb2_open(struct ksmbd_work *work)
 
 err_out:
 	if (rc && (file_present || created))
-		ksmbd_vfs_kern_path_unlock(&parent_path, &path);
+		path_put(&path);
 
 err_out1:
 	ksmbd_revert_fsids(work);
@@ -6008,7 +6008,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, parent_path;
+	struct path path;
 	int rc;
 
 	if (buf_len < (u64)sizeof(struct smb2_file_link_info) +
@@ -6037,7 +6037,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,
-					&parent_path, &path, 0);
+					&path, 0);
 	if (rc) {
 		if (rc != -ENOENT)
 			goto out;
@@ -6055,7 +6055,7 @@ static int smb2_create_link(struct ksmbd_work *work,
 			ksmbd_debug(SMB, "link already exists\n");
 			goto out;
 		}
-		ksmbd_vfs_kern_path_unlock(&parent_path, &path);
+		ksmbd_vfs_kern_path_unlock(&path);
 	}
 	rc = ksmbd_vfs_link(work, target_name, link_name);
 	if (rc)
diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
index 654c2f14a9e6..a5599d2e9ab0 100644
--- a/fs/smb/server/vfs.c
+++ b/fs/smb/server/vfs.c
@@ -66,10 +66,9 @@ int ksmbd_vfs_lock_parent(struct dentry *parent, struct dentry *child)
 	return 0;
 }
 
-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)
+static int ksmbd_vfs_path_lookup(struct ksmbd_share_config *share_conf,
+				 char *pathname, unsigned int flags,
+				 struct path *path, bool do_lock)
 {
 	struct qstr last;
 	struct filename *filename;
@@ -89,51 +88,58 @@ 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,
+				     path, &last, &type,
 				     root_share_path);
-	if (err) {
-		putname(filename);
+	putname(filename);
+	if (err)
 		return err;
-	}
 
 	if (unlikely(type != LAST_NORM)) {
-		path_put(parent_path);
-		putname(filename);
-		return -ENOENT;
-	}
-
-	err = mnt_want_write(parent_path->mnt);
-	if (err) {
-		path_put(parent_path);
-		putname(filename);
+		path_put(path);
 		return -ENOENT;
 	}
 
-	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;
+	if (do_lock) {
+		err = mnt_want_write(path->mnt);
+		if (err) {
+			path_put(path);
+			return -ENOENT;
+		}
 
-	path->dentry = d;
-	path->mnt = mntget(parent_path->mnt);
+		inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
+		d = lookup_one_qstr_excl(&last, path->dentry, 0);
 
-	if (test_share_config_flag(share_conf, KSMBD_SHARE_FLAG_CROSSMNT)) {
-		err = follow_down(path, 0);
-		if (err < 0) {
+		if (!IS_ERR(d)) {
+			dput(path->dentry);
+			path->dentry = d;
+			return 0;
+		}
+		inode_unlock(path->dentry->d_inode);
+		mnt_drop_write(path->mnt);
+		path_put(path);
+		return -ENOENT;
+	} else {
+		d = lookup_noperm_unlocked(&last, path->dentry);
+		if (!IS_ERR(d) && d_is_negative(d)) {
+			dput(d);
+			d = ERR_PTR(-ENOENT);
+		}
+		if (IS_ERR(d)) {
 			path_put(path);
-			goto err_out;
+			return -ENOENT;
 		}
+		dput(path->dentry);
+		path->dentry = d;
+
+		if (test_share_config_flag(share_conf, KSMBD_SHARE_FLAG_CROSSMNT)) {
+			err = follow_down(path, 0);
+			if (err < 0) {
+				path_put(path);
+				return -ENOENT;
+			}
+		}
+		return 0;
 	}
-
-	putname(filename);
-	return 0;
-
-err_out:
-	inode_unlock(d_inode(parent_path->dentry));
-	mnt_drop_write(parent_path->mnt);
-	path_put(parent_path);
-	putname(filename);
-	return -ENOENT;
 }
 
 void ksmbd_vfs_query_maximal_access(struct mnt_idmap *idmap,
@@ -1202,33 +1208,33 @@ static int ksmbd_vfs_lookup_in_dir(const struct path *dir, char *name,
  * @work:	work
  * @name:		file path that is relative to share
  * @flags:		lookup flags
- * @parent_path:	if lookup succeed, return parent_path info
  * @path:		if lookup succeed, return path info
  * @caseless:	caseless filename lookup
  *
  * Return:	0 on success, otherwise error
  */
-int ksmbd_vfs_kern_path_locked(struct ksmbd_work *work, char *filepath,
-			       unsigned int flags, struct path *parent_path,
-			       struct path *path, bool caseless)
+static
+int __ksmbd_vfs_kern_path(struct ksmbd_work *work, char *filepath,
+			  unsigned int flags,
+			  struct path *path, bool caseless, bool do_lock)
 {
 	struct ksmbd_share_config *share_conf = work->tcon->share_conf;
+	struct path parent_path;
 	size_t path_len, remain_len;
 	int err;
 
 retry:
-	err = ksmbd_vfs_path_lookup_locked(share_conf, filepath, flags, parent_path,
-					   path);
+	err = ksmbd_vfs_path_lookup(share_conf, filepath, flags, path, do_lock);
 	if (!err || !caseless)
 		return err;
 
 	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;
@@ -1237,10 +1243,10 @@ int ksmbd_vfs_kern_path_locked(struct ksmbd_work *work, char *filepath,
 		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);
-		path_put(parent_path);
+		path_put(&parent_path);
 		if (err)
 			goto out;
 		if (is_last) {
@@ -1253,7 +1259,7 @@ int ksmbd_vfs_kern_path_locked(struct ksmbd_work *work, char *filepath,
 				      share_conf->vfs_path.mnt,
 				      filepath,
 				      flags,
-				      parent_path);
+				      &parent_path);
 		next[0] = '/';
 		if (err)
 			goto out;
@@ -1262,17 +1268,59 @@ int ksmbd_vfs_kern_path_locked(struct ksmbd_work *work, char *filepath,
 	}
 
 	err = -EINVAL;
-	path_put(parent_path);
+	path_put(&parent_path);
 out:
 	return err;
 }
 
-void ksmbd_vfs_kern_path_unlock(struct path *parent_path, struct path *path)
+/**
+ * ksmbd_vfs_kern_path() - lookup a file and get path info
+ * @work:	work
+ * @name:		file path that is relative to share
+ * @flags:		lookup flags
+ * @path:		if lookup succeed, return path info
+ * @caseless:	caseless filename lookup
+ *
+ * Perform the lookup, possibly crossing over any mount point.
+ * On return no locks will be held and write-access to filesystem
+ * won't have been checked.
+ * Return:	0 if file was found, otherwise error
+ */
+int ksmbd_vfs_kern_path(struct ksmbd_work *work, char *filepath,
+			unsigned int flags,
+			struct path *path, bool caseless)
+{
+	return __ksmbd_vfs_kern_path(work, filepath, flags, path,
+				     caseless, false);
+}
+
+/**
+ * ksmbd_vfs_kern_path_locked() - lookup a file and get path info
+ * @work:	work
+ * @name:		file path that is relative to share
+ * @flags:		lookup flags
+ * @path:		if lookup succeed, return path info
+ * @caseless:	caseless filename lookup
+ *
+ * Perform the lookup, but don't cross over any mount point.
+ * On return the parent of path->dentry will be locked and write-access to
+ * filesystem will have been gained.
+ * Return:	0 on if file was found, otherwise error
+ */
+int ksmbd_vfs_kern_path_locked(struct ksmbd_work *work, char *filepath,
+			       unsigned int flags,
+			       struct path *path, bool caseless)
+{
+	return __ksmbd_vfs_kern_path(work, filepath, flags, path,
+				     caseless, true);
+}
+
+void ksmbd_vfs_kern_path_unlock(struct path *path)
 {
-	inode_unlock(d_inode(parent_path->dentry));
-	mnt_drop_write(parent_path->mnt);
+	/* While lock is still held, ->d_parent is safe */
+	inode_unlock(d_inode(path->dentry->d_parent));
+	mnt_drop_write(path->mnt);
 	path_put(path);
-	path_put(parent_path);
 }
 
 struct dentry *ksmbd_vfs_kern_path_create(struct ksmbd_work *work,
diff --git a/fs/smb/server/vfs.h b/fs/smb/server/vfs.h
index 2893f59803a6..d47472f3e30b 100644
--- a/fs/smb/server/vfs.h
+++ b/fs/smb/server/vfs.h
@@ -117,10 +117,13 @@ 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,
 			   bool get_write);
+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 *parent_path,
+			       unsigned int flags,
 			       struct path *path, bool caseless);
-void ksmbd_vfs_kern_path_unlock(struct path *parent_path, struct path *path);
+void ksmbd_vfs_kern_path_unlock(struct path *path);
 struct dentry *ksmbd_vfs_kern_path_create(struct ksmbd_work *work,
 					  const char *name,
 					  unsigned int flags,
-- 
2.49.0

Re: [PATCH 4/4] smb/server: add ksmbd_vfs_kern_path()

[Stefan Metzmacher]

Hi Neil,

for me this reliable generates the following problem, just doing a simple:
mount -t cifs -ousername=root,password=test,noperm,vers=3.1.1,mfsymlinks,actimeo=0 //172.31.9.167/test /mnt/test/

[ 2213.234061] [   T1972] ==================================================================
[ 2213.234607] [   T1972] BUG: KASAN: slab-use-after-free in lookup_noperm_common+0x237/0x2b0
[ 2213.235122] [   T1972] Read of size 1 at addr ffff88801c95b326 by task kworker/2:0/1972
[ 2213.235635] [   T1972]
[ 2213.235806] [   T1972] CPU: 2 UID: 0 PID: 1972 Comm: kworker/2:0 Kdump: loaded Tainted: G        W  OE       6.16.0-rc7-metze-kasan.01+ #1 PREEMPT(voluntary)
[ 2213.235820] [   T1972] Tainted: [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
[ 2213.235824] [   T1972] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[ 2213.235829] [   T1972] Workqueue: ksmbd-io handle_ksmbd_work [ksmbd]
[ 2213.235871] [   T1972] Call Trace:
[ 2213.235875] [   T1972]  <TASK>
[ 2213.235880] [   T1972]  dump_stack_lvl+0x76/0xa0
[ 2213.235893] [   T1972]  print_report+0xd1/0x600
[ 2213.235909] [   T1972]  ? __pfx__raw_spin_lock_irqsave+0x10/0x10
[ 2213.235920] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.235929] [   T1972]  ? kasan_complete_mode_report_info+0x72/0x210
[ 2213.235937] [   T1972]  kasan_report+0xe7/0x130
[ 2213.235944] [   T1972]  ? lookup_noperm_common+0x237/0x2b0
[ 2213.235952] [   T1972]  ? lookup_noperm_common+0x237/0x2b0
[ 2213.235963] [   T1972]  __asan_report_load1_noabort+0x14/0x30
[ 2213.235976] [   T1972]  lookup_noperm_common+0x237/0x2b0
[ 2213.235984] [   T1972]  lookup_noperm_unlocked+0x1d/0xa0
[ 2213.235991] [   T1972]  ? putname+0xfa/0x150
[ 2213.235998] [   T1972]  __ksmbd_vfs_kern_path+0x376/0xa80 [ksmbd]
[ 2213.236024] [   T1972]  ? local_clock+0x15/0x30
[ 2213.236039] [   T1972]  ? kasan_save_track+0x27/0x70
[ 2213.236047] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236054] [   T1972]  ? __entry_text_end+0x10257a/0x10257d
[ 2213.236066] [   T1972]  ? __pfx___ksmbd_vfs_kern_path+0x10/0x10 [ksmbd]
[ 2213.236097] [   T1972]  ? groups_alloc+0x41/0xe0
[ 2213.236106] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236114] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236122] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236128] [   T1972]  ? __kasan_check_write+0x14/0x30
[ 2213.236134] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236140] [   T1972]  ? __ksmbd_override_fsids+0x340/0x630 [ksmbd]
[ 2213.236188] [   T1972]  ? smb2_open+0x40b/0x9db0 [ksmbd]
[ 2213.236223] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236233] [   T1972]  ksmbd_vfs_kern_path+0x15/0x30 [ksmbd]
[ 2213.236260] [   T1972]  smb2_open+0x2de6/0x9db0 [ksmbd]
[ 2213.236292] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236299] [   T1972]  ? stack_depot_save_flags+0x28/0x840
[ 2213.236315] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236321] [   T1972]  ? enqueue_hrtimer+0x10b/0x230
[ 2213.236338] [   T1972]  ? ksm_scan_thread+0x480/0x59b0
[ 2213.236345] [   T1972]  ? ksm_scan_thread+0x480/0x59b0
[ 2213.236357] [   T1972]  ? __pfx_smb2_open+0x10/0x10 [ksmbd]
[ 2213.236401] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236407] [   T1972]  ? xas_load+0x19/0x300
[ 2213.236423] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236429] [   T1972]  ? __kasan_check_write+0x14/0x30
[ 2213.236435] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236441] [   T1972]  ? down_read_killable+0x120/0x290
[ 2213.236458] [   T1972]  handle_ksmbd_work+0x3fb/0xfe0 [ksmbd]
[ 2213.236489] [   T1972]  process_one_work+0x629/0xf80
[ 2213.236498] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236504] [   T1972]  ? __kasan_check_write+0x14/0x30
[ 2213.236526] [   T1972]  worker_thread+0x87f/0x1570
[ 2213.236534] [   T1972]  ? __pfx__raw_spin_lock_irqsave+0x10/0x10
[ 2213.236541] [   T1972]  ? __pfx_try_to_wake_up+0x10/0x10
[ 2213.236552] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236558] [   T1972]  ? kasan_print_address_stack_frame+0x227/0x280
[ 2213.236567] [   T1972]  ? __pfx_worker_thread+0x10/0x10
[ 2213.236581] [   T1972]  kthread+0x396/0x830
[ 2213.236589] [   T1972]  ? __pfx__raw_spin_lock_irq+0x10/0x10
[ 2213.236598] [   T1972]  ? __pfx_kthread+0x10/0x10
[ 2213.236604] [   T1972]  ? __kasan_check_write+0x14/0x30
[ 2213.236610] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236616] [   T1972]  ? recalc_sigpending+0x180/0x210
[ 2213.236624] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236630] [   T1972]  ? _raw_spin_unlock_irq+0xe/0x50
[ 2213.236643] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.236649] [   T1972]  ? calculate_sigpending+0x84/0xb0
[ 2213.236656] [   T1972]  ? __pfx_kthread+0x10/0x10
[ 2213.236664] [   T1972]  ret_from_fork+0x2b8/0x3b0
[ 2213.236671] [   T1972]  ? __pfx_kthread+0x10/0x10
[ 2213.236679] [   T1972]  ret_from_fork_asm+0x1a/0x30
[ 2213.236694] [   T1972]  </TASK>
[ 2213.236704] [   T1972]
[ 2213.269993] [   T1972] Allocated by task 1972 on cpu 2 at 2213.234048s:
[ 2213.270550] [   T1972]  kasan_save_stack+0x39/0x70
[ 2213.270569] [   T1972]  kasan_save_track+0x18/0x70
[ 2213.270578] [   T1972]  kasan_save_alloc_info+0x37/0x60
[ 2213.270587] [   T1972]  __kasan_slab_alloc+0x9d/0xa0
[ 2213.270606] [   T1972]  kmem_cache_alloc_noprof+0x13c/0x3f0
[ 2213.270619] [   T1972]  getname_kernel+0x55/0x390
[ 2213.270629] [   T1972]  __ksmbd_vfs_kern_path+0x1cf/0xa80 [ksmbd]
[ 2213.270712] [   T1972]  ksmbd_vfs_kern_path+0x15/0x30 [ksmbd]
[ 2213.270747] [   T1972]  smb2_open+0x2de6/0x9db0 [ksmbd]
[ 2213.270784] [   T1972]  handle_ksmbd_work+0x3fb/0xfe0 [ksmbd]
[ 2213.270814] [   T1972]  process_one_work+0x629/0xf80
[ 2213.270826] [   T1972]  worker_thread+0x87f/0x1570
[ 2213.270835] [   T1972]  kthread+0x396/0x830
[ 2213.270852] [   T1972]  ret_from_fork+0x2b8/0x3b0
[ 2213.270862] [   T1972]  ret_from_fork_asm+0x1a/0x30
[ 2213.270873] [   T1972]
[ 2213.271183] [   T1972] Freed by task 1972 on cpu 2 at 2213.234058s:
[ 2213.271707] [   T1972]  kasan_save_stack+0x39/0x70
[ 2213.271716] [   T1972]  kasan_save_track+0x18/0x70
[ 2213.271724] [   T1972]  kasan_save_free_info+0x3b/0x60
[ 2213.271732] [   T1972]  __kasan_slab_free+0x52/0x80
[ 2213.271741] [   T1972]  kmem_cache_free+0x316/0x560
[ 2213.271750] [   T1972]  putname+0xfa/0x150
[ 2213.271768] [   T1972]  __ksmbd_vfs_kern_path+0x20b/0xa80 [ksmbd]
[ 2213.271795] [   T1972]  ksmbd_vfs_kern_path+0x15/0x30 [ksmbd]
[ 2213.271829] [   T1972]  smb2_open+0x2de6/0x9db0 [ksmbd]
[ 2213.271857] [   T1972]  handle_ksmbd_work+0x3fb/0xfe0 [ksmbd]
[ 2213.271891] [   T1972]  process_one_work+0x629/0xf80
[ 2213.271901] [   T1972]  worker_thread+0x87f/0x1570
[ 2213.271910] [   T1972]  kthread+0x396/0x830
[ 2213.271918] [   T1972]  ret_from_fork+0x2b8/0x3b0
[ 2213.271926] [   T1972]  ret_from_fork_asm+0x1a/0x30
[ 2213.271935] [   T1972]
[ 2213.272236] [   T1972] The buggy address belongs to the object at ffff88801c95b300
                            which belongs to the cache names_cache of size 4096
[ 2213.273326] [   T1972] The buggy address is located 38 bytes inside of
                            freed 4096-byte region [ffff88801c95b300, ffff88801c95c300)
[ 2213.274374] [   T1972]
[ 2213.274671] [   T1972] The buggy address belongs to the physical page:
[ 2213.275233] [   T1972] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x1c958
[ 2213.275249] [   T1972] head: order:3 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
[ 2213.275259] [   T1972] anon flags: 0xfffffc0000040(head|node=0|zone=1|lastcpupid=0x1fffff)
[ 2213.275270] [   T1972] page_type: f5(slab)
[ 2213.275280] [   T1972] raw: 000fffffc0000040 ffff888001367c80 0000000000000000 dead000000000001
[ 2213.275289] [   T1972] raw: 0000000000000000 0000000080070007 00000000f5000000 0000000000000000
[ 2213.275304] [   T1972] head: 000fffffc0000040 ffff888001367c80 0000000000000000 dead000000000001
[ 2213.275316] [   T1972] head: 0000000000000000 0000000080070007 00000000f5000000 0000000000000000
[ 2213.275326] [   T1972] head: 000fffffc0000003 ffffea0000725601 00000000ffffffff 00000000ffffffff
[ 2213.275334] [   T1972] head: ffffffffffffffff 0000000000000000 00000000ffffffff 0000000000000008
[ 2213.275341] [   T1972] page dumped because: kasan: bad access detected
[ 2213.275348] [   T1972]
[ 2213.275652] [   T1972] Memory state around the buggy address:
[ 2213.276139] [   T1972]  ffff88801c95b200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 2213.276920] [   T1972]  ffff88801c95b280: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 2213.277700] [   T1972] >ffff88801c95b300: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 2213.278482] [   T1972]                                ^
[ 2213.278933] [   T1972]  ffff88801c95b380: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 2213.279723] [   T1972]  ffff88801c95b400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 2213.280496] [   T1972] ==================================================================
[ 2213.281383] [   T1972] Kernel panic - not syncing: KASAN: panic_on_warn set ...
[ 2213.281988] [   T1972] CPU: 2 UID: 0 PID: 1972 Comm: kworker/2:0 Kdump: loaded Tainted: G        W  OE       6.16.0-rc7-metze-kasan.01+ #1 PREEMPT(voluntary)
[ 2213.283165] [   T1972] Tainted: [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
[ 2213.283749] [   T1972] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[ 2213.284595] [   T1972] Workqueue: ksmbd-io handle_ksmbd_work [ksmbd]
[ 2213.285146] [   T1972] Call Trace:
[ 2213.285510] [   T1972]  <TASK>
[ 2213.285840] [   T1972]  dump_stack_lvl+0x27/0xa0
[ 2213.286276] [   T1972]  dump_stack+0x10/0x20
[ 2213.286700] [   T1972]  panic+0x538/0x610
[ 2213.287098] [   T1972]  ? __pfx_panic+0x10/0x10
[ 2213.287719] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.288223] [   T1972]  ? vprintk_default+0x1d/0x30
[ 2213.288682] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.289181] [   T1972]  ? sysvec_apic_timer_interrupt+0xa6/0xd0
[ 2213.289697] [   T1972]  ? asm_sysvec_apic_timer_interrupt+0x1b/0x20
[ 2213.290228] [   T1972]  check_panic_on_warn+0x6f/0x90
[ 2213.290691] [   T1972]  end_report+0xe6/0x100
[ 2213.291101] [   T1972]  kasan_report+0xf9/0x130
[ 2213.291537] [   T1972]  ? lookup_noperm_common+0x237/0x2b0
[ 2213.292013] [   T1972]  ? lookup_noperm_common+0x237/0x2b0
[ 2213.292524] [   T1972]  __asan_report_load1_noabort+0x14/0x30
[ 2213.293014] [   T1972]  lookup_noperm_common+0x237/0x2b0
[ 2213.293498] [   T1972]  lookup_noperm_unlocked+0x1d/0xa0
[ 2213.293961] [   T1972]  ? putname+0xfa/0x150
[ 2213.294397] [   T1972]  __ksmbd_vfs_kern_path+0x376/0xa80 [ksmbd]
[ 2213.294935] [   T1972]  ? local_clock+0x15/0x30
[ 2213.295361] [   T1972]  ? kasan_save_track+0x27/0x70
[ 2213.295823] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.296328] [   T1972]  ? __entry_text_end+0x10257a/0x10257d
[ 2213.296822] [   T1972]  ? __pfx___ksmbd_vfs_kern_path+0x10/0x10 [ksmbd]
[ 2213.297389] [   T1972]  ? groups_alloc+0x41/0xe0
[ 2213.297829] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.298318] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.298808] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.299322] [   T1972]  ? __kasan_check_write+0x14/0x30
[ 2213.299790] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.300277] [   T1972]  ? __ksmbd_override_fsids+0x340/0x630 [ksmbd]
[ 2213.300839] [   T1972]  ? smb2_open+0x40b/0x9db0 [ksmbd]
[ 2213.301333] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.301834] [   T1972]  ksmbd_vfs_kern_path+0x15/0x30 [ksmbd]
[ 2213.302358] [   T1972]  smb2_open+0x2de6/0x9db0 [ksmbd]
[ 2213.302841] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.303332] [   T1972]  ? stack_depot_save_flags+0x28/0x840
[ 2213.303829] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.304323] [   T1972]  ? enqueue_hrtimer+0x10b/0x230
[ 2213.304786] [   T1972]  ? ksm_scan_thread+0x480/0x59b0
[ 2213.305238] [   T1972]  ? ksm_scan_thread+0x480/0x59b0
[ 2213.305702] [   T1972]  ? __pfx_smb2_open+0x10/0x10 [ksmbd]
[ 2213.306225] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.306718] [   T1972]  ? xas_load+0x19/0x300
[ 2213.307141] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.307846] [   T1972]  ? __kasan_check_write+0x14/0x30
[ 2213.308330] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.308834] [   T1972]  ? down_read_killable+0x120/0x290
[ 2213.309319] [   T1972]  handle_ksmbd_work+0x3fb/0xfe0 [ksmbd]
[ 2213.309853] [   T1972]  process_one_work+0x629/0xf80
[ 2213.310308] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.310800] [   T1972]  ? __kasan_check_write+0x14/0x30
[ 2213.311262] [   T1972]  worker_thread+0x87f/0x1570
[ 2213.311700] [   T1972]  ? __pfx__raw_spin_lock_irqsave+0x10/0x10
[ 2213.312204] [   T1972]  ? __pfx_try_to_wake_up+0x10/0x10
[ 2213.312691] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.313183] [   T1972]  ? kasan_print_address_stack_frame+0x227/0x280
[ 2213.313728] [   T1972]  ? __pfx_worker_thread+0x10/0x10
[ 2213.314192] [   T1972]  kthread+0x396/0x830
[ 2213.314599] [   T1972]  ? __pfx__raw_spin_lock_irq+0x10/0x10
[ 2213.315097] [   T1972]  ? __pfx_kthread+0x10/0x10
[ 2213.315536] [   T1972]  ? __kasan_check_write+0x14/0x30
[ 2213.315993] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.316485] [   T1972]  ? recalc_sigpending+0x180/0x210
[ 2213.316954] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.317455] [   T1972]  ? _raw_spin_unlock_irq+0xe/0x50
[ 2213.317921] [   T1972]  ? srso_alias_return_thunk+0x5/0xfbef5
[ 2213.318409] [   T1972]  ? calculate_sigpending+0x84/0xb0
[ 2213.318883] [   T1972]  ? __pfx_kthread+0x10/0x10
[ 2213.319311] [   T1972]  ret_from_fork+0x2b8/0x3b0
[ 2213.319750] [   T1972]  ? __pfx_kthread+0x10/0x10
[ 2213.320188] [   T1972]  ret_from_fork_asm+0x1a/0x30
[ 2213.320656] [   T1972]  </TASK>

Can you please have a look?

Thanks!
metze

Re: [PATCH 4/4] smb/server: add ksmbd_vfs_kern_path()

[NeilBrown]

On Thu, 24 Jul 2025, Stefan Metzmacher wrote:
> Hi Neil,
> 
> for me this reliable generates the following problem, just doing a simple:
> mount -t cifs -ousername=root,password=test,noperm,vers=3.1.1,mfsymlinks,actimeo=0 //172.31.9.167/test /mnt/test/
> 
> [ 2213.234061] [   T1972] ==================================================================
> [ 2213.234607] [   T1972] BUG: KASAN: slab-use-after-free in lookup_noperm_common+0x237/0x2b0

Hi,
 thanks for testing and reporting.  Sorry about this obvious bug...

I called putname() too early.  The following should fix it.  Please test
and support.
Namjae: it would be good to squash this into the offending patch before
submitting upstream.  Can you do that?  Do you want me to resend the
whole patch?

Thanks,
NeilBrown

--- a/fs/smb/server/vfs.c
+++ b/fs/smb/server/vfs.c
@@ -53,7 +53,7 @@ static int ksmbd_vfs_path_lookup(struct ksmbd_share_config *share_conf,
 				 struct path *path, bool do_lock)
 {
 	struct qstr last;
-	struct filename *filename;
+	struct filename *filename __free(putname) = NULL;
 	struct path *root_share_path = &share_conf->vfs_path;
 	int err, type;
 	struct dentry *d;
@@ -72,7 +72,6 @@ static int ksmbd_vfs_path_lookup(struct ksmbd_share_config *share_conf,
 	err = vfs_path_parent_lookup(filename, flags,
 				     path, &last, &type,
 				     root_share_path);
-	putname(filename);
 	if (err)
 		return err;
 

Re: [PATCH 4/4] smb/server: add ksmbd_vfs_kern_path()

[Namjae Jeon]

On Thu, Jul 24, 2025 at 8:04 AM NeilBrown <neil@brown.name> wrote:
>
> On Thu, 24 Jul 2025, Stefan Metzmacher wrote:
> > Hi Neil,
> >
> > for me this reliable generates the following problem, just doing a simple:
> > mount -t cifs -ousername=root,password=test,noperm,vers=3.1.1,mfsymlinks,actimeo=0 //172.31.9.167/test /mnt/test/
> >
> > [ 2213.234061] [   T1972] ==================================================================
> > [ 2213.234607] [   T1972] BUG: KASAN: slab-use-after-free in lookup_noperm_common+0x237/0x2b0
>
> Hi,
>  thanks for testing and reporting.  Sorry about this obvious bug...
>
> I called putname() too early.  The following should fix it.  Please test
> and support.
> Namjae: it would be good to squash this into the offending patch before
> submitting upstream.  Can you do that?  Do you want me to resend the
> whole patch?
You don't need to resend the patch. I will directly update and test it.
Thanks!

>
> Thanks,
> NeilBrown
>
> --- a/fs/smb/server/vfs.c
> +++ b/fs/smb/server/vfs.c
> @@ -53,7 +53,7 @@ static int ksmbd_vfs_path_lookup(struct ksmbd_share_config *share_conf,
>                                  struct path *path, bool do_lock)
>  {
>         struct qstr last;
> -       struct filename *filename;
> +       struct filename *filename __free(putname) = NULL;
>         struct path *root_share_path = &share_conf->vfs_path;
>         int err, type;
>         struct dentry *d;
> @@ -72,7 +72,6 @@ static int ksmbd_vfs_path_lookup(struct ksmbd_share_config *share_conf,
>         err = vfs_path_parent_lookup(filename, flags,
>                                      path, &last, &type,
>                                      root_share_path);
> -       putname(filename);
>         if (err)
>                 return err;
>
>

Re: [PATCH 0/4] smb/server: various clean-ups

[NeilBrown]

Hi,
 did anyone have a chance to look at these - no replies and they don't
 appear in linux-next ??

Thanks,
NeilBrown
 

On Mon, 09 Jun 2025, NeilBrown wrote:
> I am working towards making some changes to how locking is managed for
> directory operations.  Prior to attempting to land these changes I am
> reviewing code that requests directory operations and cleaning up things
> that might cause me problems later.
> 
> These 4 patches are the result of my review of smb/server.  Note that
> patch 3 fixes what appears to be a real deadlock that should be trivial
> to hit if the client can actually set the flag which, as mentioned in
> the patch, can trigger the deadlock.
> 
> Patch 1 is trivial but the others deserve careful review by someone who
> knows the code.  I think they are correct, but I've been wrong before.
> 
> Thanks,
> NeilBrown
> 
>  [PATCH 1/4] smb/server: use lookup_one_unlocked()
>  [PATCH 2/4] smb/server: simplify ksmbd_vfs_kern_path_locked()
>  [PATCH 3/4] smb/server: avoid deadlock when linking with
>  [PATCH 4/4] smb/server: add ksmbd_vfs_kern_path()
> 
> 

Re: [PATCH 0/4] smb/server: various clean-ups

[Namjae Jeon]

On Wed, Jul 16, 2025 at 2:22 PM NeilBrown <neil@brown.name> wrote:
>
>
> Hi,
Hi Neil,
>  did anyone have a chance to look at these - no replies and they don't
>  appear in linux-next ??
Sorry, these patches are not in my mailbox for some reason I don't know...
I guess I missed these ones. I will check and apply the patches now.

Thanks.
>
> Thanks,
> NeilBrown
>
>
> On Mon, 09 Jun 2025, NeilBrown wrote:
> > I am working towards making some changes to how locking is managed for
> > directory operations.  Prior to attempting to land these changes I am
> > reviewing code that requests directory operations and cleaning up things
> > that might cause me problems later.
> >
> > These 4 patches are the result of my review of smb/server.  Note that
> > patch 3 fixes what appears to be a real deadlock that should be trivial
> > to hit if the client can actually set the flag which, as mentioned in
> > the patch, can trigger the deadlock.
> >
> > Patch 1 is trivial but the others deserve careful review by someone who
> > knows the code.  I think they are correct, but I've been wrong before.
> >
> > Thanks,
> > NeilBrown
> >
> >  [PATCH 1/4] smb/server: use lookup_one_unlocked()
> >  [PATCH 2/4] smb/server: simplify ksmbd_vfs_kern_path_locked()
> >  [PATCH 3/4] smb/server: avoid deadlock when linking with
> >  [PATCH 4/4] smb/server: add ksmbd_vfs_kern_path()
> >
> >
>