Re: [PATCH v3 3/7] fuse: add refcount to fuse_dev

[Bernd Schubert <bernd@bsbernd.com>]

On 3/16/26 17:53, Miklos Szeredi wrote:
> This will make it possible to grab the fuse_dev and subsequently release
> the file that it came from.
> 
> In the above case, fud->fc will be set to FUSE_DEV_FC_DISCONNECTED to
> indicate that this is no longer a functional device.
> 
> When trying to assign an fc to such a disconnected fuse_dev, the fc is set
> to the disconnected state.
> 
> Use atomic operations xchg() and cmpxchg() to prevent races.

So the actual motivation is in fuse_fill_super_common() which checks if
the file descriptor got closed during the mount? I.e. one thread mounts
and another thread closes the fd for the mount?

> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/fuse/cuse.c       |  2 +-
>  fs/fuse/dev.c        |  9 +++++++--
>  fs/fuse/fuse_dev_i.h | 15 ++++++++-------
>  fs/fuse/fuse_i.h     |  7 +++++--
>  fs/fuse/inode.c      | 44 ++++++++++++++++++++++++++++++++++++--------
>  fs/fuse/virtio_fs.c  |  2 +-
>  6 files changed, 58 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c
> index dfcb98a654d8..174333633471 100644
> --- a/fs/fuse/cuse.c
> +++ b/fs/fuse/cuse.c
> @@ -527,7 +527,7 @@ static int cuse_channel_open(struct inode *inode, struct file *file)
>  	cc->fc.initialized = 1;
>  	rc = cuse_send_init(cc);
>  	if (rc) {
> -		fuse_dev_free(fud);
> +		fuse_dev_put(fud);
>  		return rc;
>  	}
>  	file->private_data = fud;
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index fe453634897b..4c926c9c4f39 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -2531,7 +2531,8 @@ void fuse_wait_aborted(struct fuse_conn *fc)
>  int fuse_dev_release(struct inode *inode, struct file *file)
>  {
>  	struct fuse_dev *fud = fuse_file_to_fud(file);
> -	struct fuse_conn *fc = fuse_dev_fc_get(fud);
> +	/* Pairs with cmpxchg() in fuse_dev_install() */
> +	struct fuse_conn *fc = xchg(&fud->fc, FUSE_DEV_FC_DISCONNECTED);
>  
>  	if (fc) {
>  		struct fuse_pqueue *fpq = &fud->pq;
> @@ -2551,8 +2552,12 @@ int fuse_dev_release(struct inode *inode, struct file *file)
>  			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_free(fud);
> +	fuse_dev_put(fud);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(fuse_dev_release);
> diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> index 522b2012cd1f..910f883cd090 100644
> --- a/fs/fuse/fuse_dev_i.h
> +++ b/fs/fuse/fuse_dev_i.h
> @@ -39,22 +39,23 @@ struct fuse_copy_state {
>  	} ring;
>  };
>  
> +/* fud->fc gets assigned to this value when /dev/fuse is closed */
> +#define FUSE_DEV_FC_DISCONNECTED ((struct fuse_conn *) 1)

Hmm, these things always give me a headache, because now all callers
need to check for FUSE_DEV_FC_DISCONNECTED? Main protection is then vfs,
which disallows device release when there are competing operations?


> +
>  /*
>   * Lockless access is OK, because fud->fc is set once during mount and is valid
>   * until the file is released.
> + *
> + * fud->fc is set to FUSE_DEV_FC_DISCONNECTED only after the containing file is
> + * released, so result is safe to dereference in most cases.  Exceptions are:
> + * fuse_dev_put() and fuse_fill_super_common().
>   */
>  static inline struct fuse_conn *fuse_dev_fc_get(struct fuse_dev *fud)
>  {
> -	/* Pairs with smp_store_release() in fuse_dev_fc_set() */
> +	/* Pairs with xchg() in fuse_dev_install() */
>  	return smp_load_acquire(&fud->fc);
>  }

Minor nit: Pairs with cmpxchg?

>  
> -static inline void fuse_dev_fc_set(struct fuse_dev *fud, struct fuse_conn *fc)
> -{
> -	/* Pairs with smp_load_acquire() in fuse_dev_fc_get() */
> -	smp_store_release(&fud->fc, fc);
> -}
> -
>  static inline struct fuse_dev *fuse_file_to_fud(struct file *file)
>  {
>  	return file->private_data;
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 94b49384a2f7..230201dc3f90 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -577,6 +577,9 @@ struct fuse_pqueue {
>   * Fuse device instance
>   */
>  struct fuse_dev {
> +	/** Reference count of this object */
> +	refcount_t ref;
> +
>  	/** Issue FUSE_INIT synchronously */
>  	bool sync_init;
>  
> @@ -1343,8 +1346,8 @@ void fuse_conn_put(struct fuse_conn *fc);
>  
>  struct fuse_dev *fuse_dev_alloc_install(struct fuse_conn *fc);
>  struct fuse_dev *fuse_dev_alloc(void);
> -void fuse_dev_install(struct fuse_dev *fud, struct fuse_conn *fc);
> -void fuse_dev_free(struct fuse_dev *fud);
> +bool fuse_dev_install(struct fuse_dev *fud, struct fuse_conn *fc);
> +void fuse_dev_put(struct fuse_dev *fud);
>  int fuse_send_init(struct fuse_mount *fm);
>  
>  /**
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index c2d1184d5ba5..0e848e3a13c2 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1623,6 +1623,7 @@ struct fuse_dev *fuse_dev_alloc(void)
>  	if (!fud)
>  		return NULL;
>  
> +	refcount_set(&fud->ref, 1);
>  	pq = kzalloc_objs(struct list_head, FUSE_PQ_HASH_SIZE);
>  	if (!pq) {
>  		kfree(fud);
> @@ -1636,12 +1637,32 @@ struct fuse_dev *fuse_dev_alloc(void)
>  }
>  EXPORT_SYMBOL_GPL(fuse_dev_alloc);
>  
> -void fuse_dev_install(struct fuse_dev *fud, struct fuse_conn *fc)
> +bool fuse_dev_install(struct fuse_dev *fud, struct fuse_conn *fc)
>  {
> -	fuse_dev_fc_set(fud, fuse_conn_get(fc));
> +	struct fuse_conn *old_fc;
> +
> +	fuse_conn_get(fc);
>  	spin_lock(&fc->lock);
> +	/*
> +	 * Pairs with:
> +	 *  - xchg() in fuse_dev_release()
> +	 *  - smp_load_acquire() in fuse_dev_fc_get()
> +	 */
> +	old_fc = cmpxchg(&fud->fc, NULL, fc);
> +	if (old_fc) {
> +		/*
> +		 * failed to set fud->fc because
> +		 *  - it was already set to a different fc
> +		 *  - it was set to disconneted
> +		 */
> +		spin_unlock(&fc->lock);
> +		fuse_conn_put(fc);
> +		return false;
> +	}
>  	list_add_tail(&fud->entry, &fc->devices);
>  	spin_unlock(&fc->lock);
> +
> +	return true;
>  }
>  EXPORT_SYMBOL_GPL(fuse_dev_install);
>  
> @@ -1658,11 +1679,16 @@ struct fuse_dev *fuse_dev_alloc_install(struct fuse_conn *fc)

Shouldn't all callers, i.e. also  fuse_dev_alloc_install() and
fuse_device_clone() check for success?


>  }
>  EXPORT_SYMBOL_GPL(fuse_dev_alloc_install);
>  
> -void fuse_dev_free(struct fuse_dev *fud)
> +void fuse_dev_put(struct fuse_dev *fud)
>  {
> -	struct fuse_conn *fc = fuse_dev_fc_get(fud);
> +	struct fuse_conn *fc;
>  
> -	if (fc) {
> +	if (!refcount_dec_and_test(&fud->ref))
> +		return;
> +
> +	fc = fuse_dev_fc_get(fud);
> +	if (fc && fc != FUSE_DEV_FC_DISCONNECTED) {
> +		/* This is the virtiofs case (fuse_dev_release() not called) */
>  		spin_lock(&fc->lock);
>  		list_del(&fud->entry);
>  		spin_unlock(&fc->lock);
> @@ -1672,7 +1698,7 @@ void fuse_dev_free(struct fuse_dev *fud)
>  	kfree(fud->pq.processing);
>  	kfree(fud);
>  }
> -EXPORT_SYMBOL_GPL(fuse_dev_free);
> +EXPORT_SYMBOL_GPL(fuse_dev_put);
>  
>  static void fuse_fill_attr_from_inode(struct fuse_attr *attr,
>  				      const struct fuse_inode *fi)
> @@ -1901,8 +1927,10 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>  	list_add_tail(&fc->entry, &fuse_conn_list);
>  	sb->s_root = root_dentry;
>  	if (fud) {
> -		fuse_dev_install(fud, fc);
> -		wake_up_all(&fuse_dev_waitq);
> +		if (!fuse_dev_install(fud, fc))
> +			fc->connected = 0; /* device file got closed */
> +		else
> +			wake_up_all(&fuse_dev_waitq);
>  	}
>  	mutex_unlock(&fuse_mutex);
>  	return 0;
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index f685916754ad..12300651a0f1 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -486,7 +486,7 @@ static void virtio_fs_free_devs(struct virtio_fs *fs)
>  		if (!fsvq->fud)
>  			continue;
>  
> -		fuse_dev_free(fsvq->fud);
> +		fuse_dev_put(fsvq->fud);
>  		fsvq->fud = NULL;
>  	}
>  }

Thanks,
Bernd