Re: [PATCH 1/2] liveupdate: prevent double management of files

[Christian Brauner <brauner@kernel.org>]

On Mon, Mar 23, 2026 at 09:18:03AM -0400, Pasha Tatashin wrote:
> On Mon, Mar 23, 2026 at 7:55 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Sat, Mar 21, 2026 at 09:04:53PM -0400, Pasha Tatashin wrote:
> > > On Sat, Mar 21, 2026 at 1:58 PM Pasha Tatashin
> > > <pasha.tatashin@soleen.com> wrote:
> > > >
> > > > Currently, LUO does not prevent the same file from being managed twice
> > > > across different active sessions.
> > > >
> > > > Add a new i_state flag I_LUO_MANAGED and update luo_preserve_file()
> > > > to check and set this flag when a file is preserved, and clear it in
> > > > luo_file_unpreserve_files() when it is released.
> > > >
> > > > Additionally, set this flag in luo_retrieve_file() after a file is
> > > > successfully restored in the new kernel, and clear it in
> > > > luo_file_finish() when the LUO session is finalized.
> > > >
> > > > This ensures that the same file (inode) cannot be managed by multiple
> > > > sessions. If another session attempts to preserve an already managed
> > > > file, it will now fail with -EBUSY.
> > > >
> > > > Acked-by: Pratyush Yadav (Google) <pratyush@kernel.org>
> > > > Acked-by: Jan Kara <jack@suse.cz>
> > > > Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> > > > ---
> > > >  include/linux/fs.h           |  5 ++++-
> > > >  kernel/liveupdate/luo_file.c | 27 ++++++++++++++++++++++++---
> > > >  2 files changed, 28 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > > index 23f36a2613a3..692a8be56f3c 100644
> > > > --- a/include/linux/fs.h
> > > > +++ b/include/linux/fs.h
> > > > @@ -712,6 +712,8 @@ is_uncached_acl(struct posix_acl *acl)
> > > >   * I_LRU_ISOLATING     Inode is pinned being isolated from LRU without holding
> > > >   *                     i_count.
> > > >   *
> > > > + * I_LUO_MANAGED       Inode is being managed by a live update session.
> > > > + *
> > > >   * Q: What is the difference between I_WILL_FREE and I_FREEING?
> > > >   *
> > > >   * __I_{SYNC,NEW,LRU_ISOLATING} are used to derive unique addresses to wait
> > > > @@ -744,7 +746,8 @@ enum inode_state_flags_enum {
> > > >         I_CREATING              = (1U << 15),
> > > >         I_DONTCACHE             = (1U << 16),
> > > >         I_SYNC_QUEUED           = (1U << 17),
> > > > -       I_PINNING_NETFS_WB      = (1U << 18)
> > > > +       I_PINNING_NETFS_WB      = (1U << 18),
> > > > +       I_LUO_MANAGED           = (1U << 19),
> > > >  };
> > > >
> > > >  #define I_DIRTY_INODE (I_DIRTY_SYNC | I_DIRTY_DATASYNC)
> > > > diff --git a/kernel/liveupdate/luo_file.c b/kernel/liveupdate/luo_file.c
> > > > index 5acee4174bf0..86911beeff71 100644
> > > > --- a/kernel/liveupdate/luo_file.c
> > > > +++ b/kernel/liveupdate/luo_file.c
> > > > @@ -248,6 +248,7 @@ static bool luo_token_is_used(struct luo_file_set *file_set, u64 token)
> > > >   * Context: Can be called from an ioctl handler during normal system operation.
> > > >   * Return: 0 on success. Returns a negative errno on failure:
> > > >   *         -EEXIST if the token is already used.
> > > > + *         -EBUSY if the file descriptor is already preserved by another session.
> > > >   *         -EBADF if the file descriptor is invalid.
> > > >   *         -ENOSPC if the file_set is full.
> > > >   *         -ENOENT if no compatible handler is found.
> > > > @@ -276,6 +277,14 @@ int luo_preserve_file(struct luo_file_set *file_set, u64 token, int fd)
> > > >         if (err)
> > > >                 goto  err_fput;
> > > >
> > > > +       scoped_guard(spinlock, &file_inode(file)->i_lock) {
> > > > +               if (inode_state_read(file_inode(file)) & I_LUO_MANAGED) {
> > > > +                       err = -EBUSY;
> > > > +                       goto err_free_files_mem;
> > > > +               }
> > > > +               inode_state_set(file_inode(file), I_LUO_MANAGED);
> > > > +       }
> > > > +
> > > >         err = -ENOENT;
> > > >         list_private_for_each_entry(fh, &luo_file_handler_list, list) {
> > > >                 if (fh->ops->can_preserve(fh, file)) {
> > > > @@ -286,11 +295,11 @@ int luo_preserve_file(struct luo_file_set *file_set, u64 token, int fd)
> > > >
> > > >         /* err is still -ENOENT if no handler was found */
> > > >         if (err)
> > > > -               goto err_free_files_mem;
> > > > +               goto err_unpreserve_inode;
> > > >
> > > >         err = luo_flb_file_preserve(fh);
> > > >         if (err)
> > > > -               goto err_free_files_mem;
> > > > +               goto err_unpreserve_inode;
> > > >
> > > >         luo_file = kzalloc_obj(*luo_file);
> > > >         if (!luo_file) {
> > > > @@ -320,6 +329,9 @@ int luo_preserve_file(struct luo_file_set *file_set, u64 token, int fd)
> > > >         kfree(luo_file);
> > > >  err_flb_unpreserve:
> > > >         luo_flb_file_unpreserve(fh);
> > > > +err_unpreserve_inode:
> > > > +       scoped_guard(spinlock, &file_inode(file)->i_lock)
> > > > +               inode_state_clear(file_inode(file), I_LUO_MANAGED);
> > > >  err_free_files_mem:
> > > >         luo_free_files_mem(file_set);
> > > >  err_fput:
> > > > @@ -363,6 +375,9 @@ void luo_file_unpreserve_files(struct luo_file_set *file_set)
> > > >                 luo_file->fh->ops->unpreserve(&args);
> > > >                 luo_flb_file_unpreserve(luo_file->fh);
> > > >
> > > > +               scoped_guard(spinlock, &file_inode(luo_file->file)->i_lock)
> > > > +                       inode_state_clear(file_inode(luo_file->file), I_LUO_MANAGED);
> > > > +
> > > >                 list_del(&luo_file->list);
> > > >                 file_set->count--;
> > > >
> > > > @@ -609,6 +624,9 @@ int luo_retrieve_file(struct luo_file_set *file_set, u64 token,
> > > >         *filep = luo_file->file;
> > > >         luo_file->retrieve_status = 1;
> > > >
> > > > +       scoped_guard(spinlock, &file_inode(luo_file->file)->i_lock)
> > > > +               inode_state_set(file_inode(luo_file->file), I_LUO_MANAGED);
> > > > +
> > > >         return 0;
> > > >  }
> > > >
> > > > @@ -701,8 +719,11 @@ int luo_file_finish(struct luo_file_set *file_set)
> > > >
> > > >                 luo_file_finish_one(file_set, luo_file);
> > > >
> > > > -               if (luo_file->file)
> > > > +               if (luo_file->file) {
> > > > +                       scoped_guard(spinlock, &file_inode(luo_file->file)->i_lock)
> > > > +                               inode_state_clear(file_inode(luo_file->file), I_LUO_MANAGED);
> > > >                         fput(luo_file->file);
> > > > +               }
> > > >                 list_del(&luo_file->list);
> > > >                 file_set->count--;
> > > >                 mutex_destroy(&luo_file->mutex);
> > > > --
> > > > 2.43.0
> > > >
> > >
> > > > Sashiko: https://sashiko.dev/#/patchset/20260321175808.57942-1-pasha.tatashin@soleen.com
> > >
> > > Sashiko reported two problems:
> > >
> > > 1. Are there any issues with mixing goto-based error handling and scope-based
> > > cleanups like scoped_guard() in the same function?
> > >
> > > Initially, I thought that there should not be any problems, however,
> > > after looking this up  I found in include/linux/cleanup.h the
> > > following comment:
> > >
> > >  * Lastly, given that the benefit of cleanup helpers is removal of
> > >  * "goto", and that the "goto" statement can jump between scopes, the
> > >  * expectation is that usage of "goto" and cleanup helpers is never
> > >  * mixed in the same function.
> >
> > There's a compile-time switch you might want to turn on when
> > test-compiling code like this. I forget exactly what it is. Something
> > like jump-over-uninit or something.
> >
> > >
> > > Well, good to know, will not use goto inside scoped_guards.
> > >
> > > 2. Additionally, does setting I_LUO_MANAGED on the inode break the preservation
> > > of anonymous inodes? Many file types (like eventfd, epoll, timerfd,
> > > signalfd)
> > >
> > > This is actually a very good point. It looks like everyone who uses
> > > anon_inode_getfd() has one shared inode. This is not a problem for the
> > > existing LUO user memfd, or for the upcoming vfiofd and memfd, but
> > > kvm-vmfd and kvm-cpufd also use it, and that might be a problem in the
> > > future once we add support for Orphaned VMs.
> > >
> > > Therefore, we have two choices: either use a hash table, which adds
> > > performance and memory overhead, or delegate this double-check to the
> > > LUO file handlers, as they can use a private context to know if the FD
> > > is already preserved.
> >
> > So, I'm not happy about I_LUO_MANAGED. I don't think we need driver
> > specific stuff in struct inode and not in i_state. Track this in the
> > driver please. I don't want this precedent and I'd rather have you get
> 
> I am planning to use an xarray in the next version.
> 
> > used to implementing such things in the driver right away rather than
> > offloading this on general infrastructure. If we let this slide struct
> > inode will be 2MB 1 in year.
> 
> Claiming that a single flag bit precedent would cause the overall
> struct to grow by 2MB in a year is a slight exaggeration. :-)

Hm, you say that. But then you don't get ~5-10 patches a year that "just
add a new member into struct inode with 4-8 bytes"... I'm just making an
exaggerated point ofc. :)

But struct inode is used everywhere and I want it contained and small
and whatever lands in it - even flags - better be VFS generic stuff.

We sometimes do carve out exceptions for _filesystem drivers_ where no
other way is possible ofc. But I don't think this should extend to
drivers/.