[PATCH] bcachefs: unlock parent dir if entry is not found in subvolume deletion

[PATCH] bcachefs: unlock parent dir if entry is not found in subvolume deletion

[Guoyu Ou]

Parent dir is locked by user_path_locked_at() before validating the
required dentry. It should be unlocked if we can not perform the
deletion.

This fixes the problem:

$ bcachefs subvolume delete not-exist-entry
BCH_IOCTL_SUBVOLUME_DESTROY ioctl error: No such file or directory
$ bcachefs subvolume delete not-exist-entry

the second will stuck because the parent dir is locked in the previous
deletion.

Signed-off-by: Guoyu Ou <benogy@gmail.com>
---
 fs/bcachefs/fs-ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/bcachefs/fs-ioctl.c b/fs/bcachefs/fs-ioctl.c
index 3a4c24c28e7f..3dc8630ff9fe 100644
--- a/fs/bcachefs/fs-ioctl.c
+++ b/fs/bcachefs/fs-ioctl.c
@@ -455,6 +455,7 @@ static long bch2_ioctl_subvolume_destroy(struct bch_fs *c, struct file *filp,
 	if (IS_ERR(victim))
 		return PTR_ERR(victim);
 
+	dir = d_inode(path.dentry);
 	if (victim->d_sb->s_fs_info != c) {
 		ret = -EXDEV;
 		goto err;
@@ -463,14 +464,13 @@ static long bch2_ioctl_subvolume_destroy(struct bch_fs *c, struct file *filp,
 		ret = -ENOENT;
 		goto err;
 	}
-	dir = d_inode(path.dentry);
 	ret = __bch2_unlink(dir, victim, true);
 	if (!ret) {
 		fsnotify_rmdir(dir, victim);
 		d_delete(victim);
 	}
-	inode_unlock(dir);
 err:
+	inode_unlock(dir);
 	dput(victim);
 	path_put(&path);
 	return ret;
-- 
2.43.0

Re: [PATCH] bcachefs: unlock parent dir if entry is not found in subvolume deletion

[Kent Overstreet]

On Sun, Jan 28, 2024 at 04:46:17PM +0800, Guoyu Ou wrote:
> Parent dir is locked by user_path_locked_at() before validating the
> required dentry. It should be unlocked if we can not perform the
> deletion.
> 
> This fixes the problem:
> 
> $ bcachefs subvolume delete not-exist-entry
> BCH_IOCTL_SUBVOLUME_DESTROY ioctl error: No such file or directory
> $ bcachefs subvolume delete not-exist-entry
> 
> the second will stuck because the parent dir is locked in the previous
> deletion.

Your fix works, but having user_path_locked_at() return an object that
we have a destructor for but needs extra cleanup is not ideal, and now
that I look I'm also wondering why the dput(victim) is necessary when
path_put() also does a dput().

How do we feel about the following?

Note that my fix is incomplete (and untested); path needs to be zeroed
out everywhere it's constructed.

diff --git a/fs/namei.c b/fs/namei.c
index 4e0de939fea1..d3ef21e529b2 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -559,6 +559,8 @@ EXPORT_SYMBOL(path_get);
  */
 void path_put(const struct path *path)
 {
+	if (path->locked)
+		inode_unlock(d_inode(path.dentry));
 	dput(path->dentry);
 	mntput(path->mnt);
 }
@@ -2578,6 +2580,8 @@ static struct dentry *__kern_path_locked(int dfd, struct filename *name, struct
 	struct qstr last;
 	int type, error;
 
+	memset(path, 0, sizeof(*path));
+
 	error = filename_parentat(dfd, name, 0, path, &last, &type);
 	if (error)
 		return ERR_PTR(error);
@@ -2586,11 +2590,10 @@ static struct dentry *__kern_path_locked(int dfd, struct filename *name, struct
 		return ERR_PTR(-EINVAL);
 	}
 	inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
+	path->locked = true;
 	d = lookup_one_qstr_excl(&last, path->dentry, 0);
-	if (IS_ERR(d)) {
-		inode_unlock(path->dentry->d_inode);
+	if (IS_ERR(d))
 		path_put(path);
-	}
 	return d;
 }
 
diff --git a/include/linux/path.h b/include/linux/path.h
index 475225a03d0d..bfa489996aeb 100644
--- a/include/linux/path.h
+++ b/include/linux/path.h
@@ -8,6 +8,7 @@ struct vfsmount;
 struct path {
 	struct vfsmount *mnt;
 	struct dentry *dentry;
+	bool locked;
 } __randomize_layout;
 
 extern void path_get(const struct path *);



> 
> Signed-off-by: Guoyu Ou <benogy@gmail.com>
> ---
>  fs/bcachefs/fs-ioctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/bcachefs/fs-ioctl.c b/fs/bcachefs/fs-ioctl.c
> index 3a4c24c28e7f..3dc8630ff9fe 100644
> --- a/fs/bcachefs/fs-ioctl.c
> +++ b/fs/bcachefs/fs-ioctl.c
> @@ -455,6 +455,7 @@ static long bch2_ioctl_subvolume_destroy(struct bch_fs *c, struct file *filp,
>  	if (IS_ERR(victim))
>  		return PTR_ERR(victim);
>  
> +	dir = d_inode(path.dentry);
>  	if (victim->d_sb->s_fs_info != c) {
>  		ret = -EXDEV;
>  		goto err;
> @@ -463,14 +464,13 @@ static long bch2_ioctl_subvolume_destroy(struct bch_fs *c, struct file *filp,
>  		ret = -ENOENT;
>  		goto err;
>  	}
> -	dir = d_inode(path.dentry);
>  	ret = __bch2_unlink(dir, victim, true);
>  	if (!ret) {
>  		fsnotify_rmdir(dir, victim);
>  		d_delete(victim);
>  	}
> -	inode_unlock(dir);
>  err:
> +	inode_unlock(dir);
>  	dput(victim);
>  	path_put(&path);
>  	return ret;
> -- 
> 2.43.0
> 

Re: [PATCH] bcachefs: unlock parent dir if entry is not found in subvolume deletion

[Al Viro]

On Sun, Jan 28, 2024 at 09:21:48PM -0500, Kent Overstreet wrote:

> How do we feel about the following?

No, with side of Fuck, NO.

Re: [PATCH] bcachefs: unlock parent dir if entry is not found in subvolume deletion

[Al Viro]

On Sun, Jan 28, 2024 at 09:21:48PM -0500, Kent Overstreet wrote:

> Your fix works, but having user_path_locked_at() return an object that
> we have a destructor for but needs extra cleanup is not ideal, and now
> that I look I'm also wondering why the dput(victim) is necessary when
> path_put() also does a dput().

Because victim is not equal to path.dentry, perhaps?

Re: [PATCH] bcachefs: unlock parent dir if entry is not found in subvolume deletion

[Al Viro]

On Mon, Jan 29, 2024 at 02:23:38AM +0000, Al Viro wrote:
> On Sun, Jan 28, 2024 at 09:21:48PM -0500, Kent Overstreet wrote:
> 
> > How do we feel about the following?
> 
> No, with side of Fuck, NO.

To elaborate a bit: that essentially introduces a new type (mount/dentry
pair, possibly locked) *and* converts existing instances of struct path
(file->f_path, mount->mnt_mountpoint, etc.) to that.  Suddenly we get
an extra constraint that needs to be verified - "no long-term struct
path instance with ->locked being true".  What's more, currently
one can do
	v = *p;
	path_get(&v);
	...
	path_put(&v);
with eventual
	path_put(p);
possibly from another thread.  Every place like that needs a proof that
we can't possibly get there with p->locked being true.

There's a lot of other reasons why that's a really bad idea.  Seriously,
let's not go there.