Message-ID: <CAOQ4uxhcj0Bu0WjkeHoi6Y3CL=gBKJRcsbSEb7DrteAfiZbBnw@mail.gmail.com>
Date: Sun, 18 Jan 2026 18:08:18 +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 Sun, Jan 18, 2026 at 6:07 PM Amir Goldstein <amir73il@...il.com> wrote:
>
> On Sun, Jan 18, 2026 at 12:47 PM Chunsheng Luo <luochunsheng@...c.edu> wrote:
> >
> >
> >
> > On 1/18/26 1:00 AM, Amir Goldstein wrote:
> > > 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.
> > >
> > >
> >
> > Yes, swap() isn't atomic, it's just copying structs, so it's not safe
> > when racing with lookup.
> >
> > I've updated the version to make fc->backing_files_map an rcu referenced
> > ptr. Please review the attached patch.
>
> You can also use rcu_replace_pointer() to swap old_idr <-> new_idr,
> but otherwise the patch looks fine to me.
>

Feel free to add
Reviewed-by: Amir Goldstein <amir73il@...il.com>

Thanks,
Amir.

Powered by blists - more mailing lists