Re: [PATCH v5 08/22] liveupdate: luo_file: implement file systems callbacks

[Pasha Tatashin <pasha.tatashin@soleen.com>]

On Mon, Nov 10, 2025 at 12:27 PM Pratyush Yadav <pratyush@kernel.org> wrote:
>
> Hi Pasha,
>
> Caught a small bug during some of my testing.
>
> On Fri, Nov 07 2025, Pasha Tatashin wrote:
>
> > This patch implements the core mechanism for managing preserved
> > files throughout the live update lifecycle. It provides the logic to
> > invoke the file handler callbacks (preserve, unpreserve, freeze,
> > unfreeze, retrieve, and finish) at the appropriate stages.
> >
> > During the reboot phase, luo_file_freeze() serializes the final
> > metadata for each file (handler compatible string, token, and data
> > handle) into a memory region preserved by KHO. In the new kernel,
> > luo_file_deserialize() reconstructs the in-memory file list from this
> > data, preparing the session for retrieval.
> >
> > Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> [...]
> > +int luo_preserve_file(struct luo_session *session, u64 token, int fd)
> > +{
> > +     struct liveupdate_file_op_args args = {0};
> > +     struct liveupdate_file_handler *fh;
> > +     struct luo_file *luo_file;
> > +     struct file *file;
> > +     int err = -ENOENT;
> > +
> > +     lockdep_assert_held(&session->mutex);
> > +
> > +     if (luo_token_is_used(session, token))
> > +             return -EEXIST;
> > +
> > +     file = fget(fd);
> > +     if (!file)
> > +             return -EBADF;
> > +
> > +     err = luo_session_alloc_files_mem(session);
>
> err gets set to 0 here...
>
> > +     if (err)
> > +             goto  exit_err;
> > +
> > +     if (session->count == LUO_FILE_MAX) {
> > +             err = -ENOSPC;
> > +             goto exit_err;
> > +     }
> > +
> > +     list_for_each_entry(fh, &luo_file_handler_list, list) {
> > +             if (fh->ops->can_preserve(fh, file)) {
> > +                     err = 0;
> > +                     break;
> > +             }
> > +     }
>
> ... say no file handler can preserve this file ...
>
> > +
> > +     /* err is still -ENOENT if no handler was found */
> > +     if (err)
>
> ... err is not ENOENT, but 0. So this function does not error but, but
> goes ahead with fh == luo_file_handler_list (since end of list). This
> causes an out-of-bounds access. It eventually causes a kernel fault and
> panic.
>
> You should drop the ENOENT at initialization time and set it right
> before list_for_each_entry().

Right, thank you for reporting this. Should add it to self-tests,
where we try to preserve FD that does not have a file handler.

Pasha

>
> > +             goto exit_err;
> > +
> > +     luo_file = kzalloc(sizeof(*luo_file), GFP_KERNEL);
> > +     if (!luo_file) {
> > +             err = -ENOMEM;
> > +             goto exit_err;
> > +     }
> > +
> > +     luo_file->file = file;
> > +     luo_file->fh = fh;
> > +     luo_file->token = token;
> > +     luo_file->retrieved = false;
> > +     mutex_init(&luo_file->mutex);
> > +
> > +     args.handler = fh;
> > +     args.session = (struct liveupdate_session *)session;
> > +     args.file = file;
> > +     err = fh->ops->preserve(&args);
> > +     if (err) {
> > +             mutex_destroy(&luo_file->mutex);
> > +             kfree(luo_file);
> > +             goto exit_err;
> > +     } else {
> > +             luo_file->serialized_data = args.serialized_data;
> > +             list_add_tail(&luo_file->list, &session->files_list);
> > +             session->count++;
> > +     }
> > +
> > +     return 0;
> > +
> > +exit_err:
> > +     fput(file);
> > +     luo_session_free_files_mem(session);
> > +
> > +     return err;
> > +}
> [...]
>
> --
> Regards,
> Pratyush Yadav