Message-ID: <CAOQ4uxhgOk2Ati81vqEkgWFODkW_gkB7Z7wj0x1A8RX38wLSRA@mail.gmail.com>
Date: Sat, 17 Jan 2026 18:00:56 +0100
From: Amir Goldstein <amir73il@...il.com>
To: Chunsheng Luo <luochunsheng@...c.edu>
Cc: miklos@...redi.hu, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] fuse: add ioctl to cleanup all backing files

On Sat, Jan 17, 2026 at 5:14 PM Chunsheng Luo <luochunsheng@...c.edu> wrote:
>
>
>
> On 1/16/26 11:39 PM, Amir Goldstein wrote:
> > On Fri, Jan 16, 2026 at 3:28 PM Chunsheng Luo <luochunsheng@...c.edu> wrote:
> >>
> >> To simplify crash recovery and reduce performance impact, backing_ids
> >> are not persisted across daemon restarts. After crash recovery, this
> >> may lead to resource leaks if backing file resources are not properly
> >> cleaned up.
> >>
> >> Add FUSE_DEV_IOC_BACKING_CLOSE_ALL ioctl to release all backing_ids
> >> and put backing files. When the FUSE daemon restarts, it can use this
> >> ioctl to cleanup all backing file resources.
> >>
> >> Signed-off-by: Chunsheng Luo <luochunsheng@...c.edu>
> >> ---
> >>   fs/fuse/backing.c         | 19 +++++++++++++++++++
> >>   fs/fuse/dev.c             | 16 ++++++++++++++++
> >>   fs/fuse/fuse_i.h          |  1 +
> >>   include/uapi/linux/fuse.h |  1 +
> >>   4 files changed, 37 insertions(+)
> >>
> >> diff --git a/fs/fuse/backing.c b/fs/fuse/backing.c
> >> index 4afda419dd14..e93d797a2cde 100644
> >> --- a/fs/fuse/backing.c
> >> +++ b/fs/fuse/backing.c
> >> @@ -166,6 +166,25 @@ int fuse_backing_close(struct fuse_conn *fc, int backing_id)
> >>          return err;
> >>   }
> >>
> >> +static int fuse_backing_close_one(int id, void *p, void *data)
> >> +{
> >> +       struct fuse_conn *fc = data;
> >> +
> >> +       fuse_backing_close(fc, id);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +int fuse_backing_close_all(struct fuse_conn *fc)
> >> +{
> >> +       if (!fc->passthrough || !capable(CAP_SYS_ADMIN))
> >> +               return -EPERM;
> >> +
> >> +       idr_for_each(&fc->backing_files_map, fuse_backing_close_one, fc);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >
> > This is not safe and not efficient.
> > For safety from racing with _open/_close, iteration needs at least
> > rcu_read_lock(),
>
> Yes, you're absolutely right. Additionally, calling idr_remove within
> idr_for_each maybe presents safety risks.
>
> > but I think it will be much more efficient to zap the entire map with
> > fuse_backing_files_free()/fuse_backing_files_init().
> >
> > This of course needs to be synchronized with concurrent _open/_close/_lookup.
> > This could be done by making c->backing_files_map a struct idr __rcu *
> > and replace the old and new backing_files_map under spin_lock(&fc->lock);
> >
> > Then you can call fuse_backing_files_free() on the old backing_files_map
> > without a lock.
> >
> > As a side note, fuse_backing_files_free() iteration looks like it may need
> > cond_resched() if there are a LOT of backing ids, but I am not sure and
> > this is orthogonal to your change.
> >
> > Thanks,
> > Amir.
> >
> >
>
> Thank you for your helpful suggestions. However, it cannot use
> fuse_backing_files_free() in the close_all implementation because it
> directly frees backing files without respecting reference counts. This
> function requires that no one is actively using the backing file (it
> even has WARN_ON_ONCE(refcount_read(&fb->count) != 1)), which cannot be
> guaranteed after a crash recovery scenario where backing files may still
> be in use.

Right.

>
> Instead, the implementation uses fuse_backing_put() to safely decrement
> the reference count and allow the backing file to be freed when no
> longer in use.

OK.

>
> Additionally, the implementation addresses two race conditions:
>
> - Race between idr_for_each and lookup: Uses synchronize_rcu() to ensure
> all concurrent RCU readers (i.e., in-flight fuse_backing_lookup() calls)
> complete before releasing backing files, preventing use-after-free issues.

Almost. See below.

>
> - Race with open/close operations: Uses fc->lock to atomically swap the
> old and new IDR maps, ensuring consistency with concurrent
> fuse_backing_open() and fuse_backing_close() operations.
>
> This approach provides the same as the RCU pointer suggestion, but with
> less code and no changes to the struct fuse_conn data structures.
>
> I've updated it and verified the implementation. Could you please review it?
>
>
> diff --git a/fs/fuse/backing.c b/fs/fuse/backing.c
> index 4afda419dd14..047d373684f9 100644
> --- a/fs/fuse/backing.c
> +++ b/fs/fuse/backing.c
> @@ -166,6 +166,45 @@ int fuse_backing_close(struct fuse_conn *fc, int
> backing_id)
>          return err;
>   }
>
> +static int fuse_backing_release_one(int id, void *p, void *data)
> +{
> +       struct fuse_backing *fb = p;
> +
> +       fuse_backing_put(fb);
> +
> +       return 0;
> +}
> +
> +int fuse_backing_close_all(struct fuse_conn *fc)
> +{
> +       struct idr old_map;
> +
> +       if (!fc->passthrough || !capable(CAP_SYS_ADMIN))
> +               return -EPERM;
> +
> +       /*
> +        * Swap out the old backing_files_map with a new empty one under
> lock,
> +        * then release all backing files outside the lock. This avoids long
> +        * lock hold times and potential races with concurrent open/close
> +        * operations.
> +        */
> +       idr_init(&old_map);
> +       spin_lock(&fc->lock);
> +       swap(fc->backing_files_map, old_map);
> +       spin_unlock(&fc->lock);
> +
> +       /*
> +        * Ensure all concurrent RCU readers complete before releasing
> backing
> +        * files, so any in-flight lookups can safely take references.
> +        */
> +       synchronize_rcu();
> +
> +       idr_for_each(&old_map, fuse_backing_release_one, NULL);
> +       idr_destroy(&old_map);
> +
> +       return 0;
> +}
> +

That's almost safe but not enough.
This lookup code is not safe against the swap():

  rcu_read_lock();
  fb = idr_find(&fc->backing_files_map, backing_id);

That is the reason you need to make fc->backing_files_map
an rcu referenced ptr.

Instead of swap() you use xchg() to atomically exchange the
old and new struct idr pointers and for lookup:

  rcu_read_lock();
  fb = idr_find(rcu_dereference(fc->backing_files_map), backing_id);

Thanks,
Amir.

Powered by blists - more mailing lists