Re: [PATCH v2 4/6] fuse: clean up device cloning

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Mar 12, 2026 at 08:40:02PM +0100, Miklos Szeredi wrote:
>  - fuse_mutex is not needed for device cloning, because fuse_dev_install()
>    uses cmpxcg() to set fud->fc, which prevents races between clone/mount
>    or clone/clone.  This makes the logic simpler
> 
>  - Drop fc->dev_count.  This is only used to check in release if the device
>    is the last clone, but checking list_empty(&fc->devices) is equivalent
>    after removing the released device from the list.  Removing the fuse_dev
>    before calling fuse_abort_conn() is okay, since the processing and io
>    lists are now empty for this device.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/fuse/dev.c    | 44 ++++++++++++++++----------------------------
>  fs/fuse/fuse_i.h |  3 ---
>  fs/fuse/inode.c  |  1 -
>  3 files changed, 16 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 3adf6bd38c9b..18cc844cf290 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -2543,14 +2543,15 @@ int fuse_dev_release(struct inode *inode, struct file *file)
>  
>  		fuse_dev_end_requests(&to_end);
>  
> +		spin_lock(&fc->lock);
> +		list_del(&fud->entry);
> +		spin_unlock(&fc->lock);
> +
>  		/* Are we the last open device? */
> -		if (atomic_dec_and_test(&fc->dev_count)) {
> +		if (list_empty(&fc->devices)) {

If you have two threads closing their cloned fuse dev fds, can they both
see an empty list here and abort the connection?  I think they can, but
it's benign because the abort takes its own spinlock.

Aside from that question, this looks ok to me.

--D

>  			WARN_ON(fc->iq.fasync != NULL);
>  			fuse_abort_conn(fc);
>  		}
> -		spin_lock(&fc->lock);
> -		list_del(&fud->entry);
> -		spin_unlock(&fc->lock);
>  		fuse_conn_put(fc);
>  	}
>  	fuse_dev_put(fud);
> @@ -2569,24 +2570,10 @@ static int fuse_dev_fasync(int fd, struct file *file, int on)
>  	return fasync_helper(fd, file, on, &fud->fc->iq.fasync);
>  }
>  
> -static int fuse_device_clone(struct fuse_conn *fc, struct file *new)
> -{
> -	struct fuse_dev *new_fud = fuse_file_to_fud(new);
> -
> -	if (fuse_dev_fc_get(new_fud))
> -		return -EINVAL;
> -
> -	fuse_dev_install(new_fud, fc);
> -	atomic_inc(&fc->dev_count);
> -
> -	return 0;
> -}
> -
>  static long fuse_dev_ioctl_clone(struct file *file, __u32 __user *argp)
>  {
> -	int res;
>  	int oldfd;
> -	struct fuse_dev *fud = NULL;
> +	struct fuse_dev *fud, *new_fud;
>  
>  	if (get_user(oldfd, argp))
>  		return -EFAULT;
> @@ -2599,17 +2586,18 @@ static long fuse_dev_ioctl_clone(struct file *file, __u32 __user *argp)
>  	 * Check against file->f_op because CUSE
>  	 * uses the same ioctl handler.
>  	 */
> -	if (fd_file(f)->f_op == file->f_op)
> -		fud = __fuse_get_dev(fd_file(f));
> +	if (fd_file(f)->f_op != file->f_op)
> +		return -EINVAL;
>  
> -	res = -EINVAL;
> -	if (fud) {
> -		mutex_lock(&fuse_mutex);
> -		res = fuse_device_clone(fud->fc, file);
> -		mutex_unlock(&fuse_mutex);
> -	}
> +	fud = __fuse_get_dev(fd_file(f));
> +	if (!fud)
> +		return -EINVAL;
>  
> -	return res;
> +	new_fud = fuse_file_to_fud(file);
> +	if (!fuse_dev_install(new_fud, fud->fc))
> +		return -EINVAL;
> +
> +	return 0;
>  }
>  
>  static long fuse_dev_ioctl_backing_open(struct file *file,
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index b77b384b0385..92576e28f8ac 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -647,9 +647,6 @@ struct fuse_conn {
>  	/** Refcount */
>  	refcount_t count;
>  
> -	/** Number of fuse_dev's */
> -	atomic_t dev_count;
> -
>  	/** Current epoch for up-to-date dentries */
>  	atomic_t epoch;
>  
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 45abcfec03a4..e42356d60f7a 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -995,7 +995,6 @@ void fuse_conn_init(struct fuse_conn *fc, struct fuse_mount *fm,
>  	spin_lock_init(&fc->bg_lock);
>  	init_rwsem(&fc->killsb);
>  	refcount_set(&fc->count, 1);
> -	atomic_set(&fc->dev_count, 1);
>  	atomic_set(&fc->epoch, 1);
>  	INIT_WORK(&fc->epoch_work, fuse_epoch_work);
>  	init_waitqueue_head(&fc->blocked_waitq);
> -- 
> 2.53.0
> 
>