| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+CK2bAoZ9GhL+SCsrzWL-eG1XcRai0h9QMrL-fsZJFzSxSt6g@mail.gmail.com>
Date: Tue, 25 Nov 2025 10:13:13 -0500
From: Pasha Tatashin <pasha.tatashin@...een.com>
To: Mike Rapoport <rppt@...nel.org>
Cc: pratyush@...nel.org, jasonmiu@...gle.com, graf@...zon.com,
dmatlack@...gle.com, rientjes@...gle.com, corbet@....net,
rdunlap@...radead.org, ilpo.jarvinen@...ux.intel.com, kanie@...ux.alibaba.com,
ojeda@...nel.org, aliceryhl@...gle.com, masahiroy@...nel.org,
akpm@...ux-foundation.org, tj@...nel.org, yoann.congal@...le.fr,
mmaurer@...gle.com, roman.gushchin@...ux.dev, chenridong@...wei.com,
axboe@...nel.dk, mark.rutland@....com, jannh@...gle.com,
vincent.guittot@...aro.org, hannes@...xchg.org, dan.j.williams@...el.com,
david@...hat.com, joel.granados@...nel.org, rostedt@...dmis.org,
anna.schumaker@...cle.com, song@...nel.org, linux@...ssschuh.net,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, linux-mm@...ck.org,
gregkh@...uxfoundation.org, tglx@...utronix.de, mingo@...hat.com,
bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
rafael@...nel.org, dakr@...nel.org, bartosz.golaszewski@...aro.org,
cw00.choi@...sung.com, myungjoo.ham@...sung.com, yesanishhere@...il.com,
Jonathan.Cameron@...wei.com, quic_zijuhu@...cinc.com,
aleksander.lobakin@...el.com, ira.weiny@...el.com,
andriy.shevchenko@...ux.intel.com, leon@...nel.org, lukas@...ner.de,
bhelgaas@...gle.com, wagi@...nel.org, djeffery@...hat.com,
stuart.w.hayes@...il.com, ptyadav@...zon.de, lennart@...ttering.net,
brauner@...nel.org, linux-api@...r.kernel.org, linux-fsdevel@...r.kernel.org,
saeedm@...dia.com, ajayachandra@...dia.com, jgg@...dia.com, parav@...dia.com,
leonro@...dia.com, witu@...dia.com, hughd@...gle.com, skhawaja@...gle.com,
chrisl@...nel.org
Subject: Re: [PATCH v7 06/22] liveupdate: luo_file: implement file systems callbacks
On Mon, Nov 24, 2025 at 3:18 AM Mike Rapoport <rppt@...nel.org> wrote:
>
> On Sat, Nov 22, 2025 at 05:23:33PM -0500, 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@...een.com>
>
> With some comments below
> Reviewed-by: Mike Rapoport (Microsoft) <rppt@...nel.org>
>
> > ---
> > include/linux/kho/abi/luo.h | 39 +-
> > include/linux/liveupdate.h | 98 ++++
> > kernel/liveupdate/Makefile | 1 +
> > kernel/liveupdate/luo_file.c | 882 +++++++++++++++++++++++++++++++
> > kernel/liveupdate/luo_internal.h | 38 ++
> > 5 files changed, 1057 insertions(+), 1 deletion(-)
> > create mode 100644 kernel/liveupdate/luo_file.c
> >
>
> ...
>
> > +int luo_preserve_file(struct luo_file_set *file_set, 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;
> > +
> > + if (luo_token_is_used(file_set, token))
> > + return -EEXIST;
> > +
> > + file = fget(fd);
> > + if (!file)
> > + return -EBADF;
> > +
> > + err = luo_alloc_files_mem(file_set);
> > + if (err)
> > + goto err_files_mem;
> > +
> > + if (file_set->count == LUO_FILE_MAX) {
>
> This can be checked before getting the file and allocating memory, can't it?
Moved up.
>
> > + err = -ENOSPC;
> > + goto err_files_mem;
>
> The goto label should say what it does, not what the error was.
Changed to err_free_files_mem;
>
> > + }
> > +
> > + err = -ENOENT;
> > + luo_list_for_each_private(fh, &luo_file_handler_list, list) {
> > + if (fh->ops->can_preserve(fh, file)) {
> > + err = 0;
> > + break;
> > + }
> > + }
> > +
> > + /* err is still -ENOENT if no handler was found */
> > + if (err)
> > + goto err_files_mem;
> > +
> > + luo_file = kzalloc(sizeof(*luo_file), GFP_KERNEL);
> > + if (!luo_file) {
> > + err = -ENOMEM;
> > + goto err_files_mem;
> > + }
> > +
> > + 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.file = file;
> > + err = fh->ops->preserve(&args);
> > + if (err)
> > + goto err_kfree;
> > +
> > + luo_file->serialized_data = args.serialized_data;
> > + list_add_tail(&luo_file->list, &file_set->files_list);
> > + file_set->count++;
> > +
> > + return 0;
> > +
> > +err_kfree:
> > + mutex_destroy(&luo_file->mutex);
>
> Don't think we need this, luo_file is freed in the next line.
Removed.
>
> > + kfree(luo_file);
> > +err_files_mem:
> > + fput(file);
> > + luo_free_files_mem(file_set);
>
> I'd have the error path as
>
> err_free_luo_file:
> kfree(luo_file);
> err_free_files_mem:
> luo_free_files_mem(file_set);
> err_put_file:
> fput(file);
Yeap, done like this.
>
> > +
> > + return err;
> > +}
>
> ...
>
> > +void luo_file_unpreserve_files(struct luo_file_set *file_set)
> > +{
> > + struct luo_file *luo_file;
> > +
> > + while (!list_empty(&file_set->files_list)) {
>
> list_for_each_entry_safe_reverse()?
In this case I prefer while(!list_empty(...))
It emphasizes to a reader that we are emptying the full list. _safe
is good to use when some items are removed from a list while
traversing.
>
> > + struct liveupdate_file_op_args args = {0};
> > +
> > + luo_file = list_last_entry(&file_set->files_list,
> > + struct luo_file, list);
> > +
> > + args.handler = luo_file->fh;
> > + args.file = luo_file->file;
> > + args.serialized_data = luo_file->serialized_data;
> > + luo_file->fh->ops->unpreserve(&args);
> > +
> > + list_del(&luo_file->list);
> > + file_set->count--;
> > +
> > + fput(luo_file->file);
> > + mutex_destroy(&luo_file->mutex);
> > + kfree(luo_file);
> > + }
> > +
> > + luo_free_files_mem(file_set);
> > +}
>
> ...
>
> > +int luo_file_finish(struct luo_file_set *file_set)
> > +{
> > + struct list_head *files_list = &file_set->files_list;
> > + struct luo_file *luo_file;
> > + int err;
> > +
> > + if (!file_set->count)
> > + return 0;
> > +
> > + list_for_each_entry(luo_file, files_list, list) {
> > + err = luo_file_can_finish_one(file_set, luo_file);
> > + if (err)
> > + return err;
> > + }
> > +
> > + while (!list_empty(&file_set->files_list)) {
>
> list_for_each_entry_safe_reverse()?
Same
>
> > + luo_file = list_last_entry(&file_set->files_list,
> > + struct luo_file, list);
> > +
> > + luo_file_finish_one(file_set, luo_file);
> > +
> > + if (luo_file->file)
> > + fput(luo_file->file);
> > + list_del(&luo_file->list);
> > + file_set->count--;
> > + mutex_destroy(&luo_file->mutex);
> > + kfree(luo_file);
> > + }
> > +
>
> ...
>
> > diff --git a/kernel/liveupdate/luo_internal.h b/kernel/liveupdate/luo_internal.h
> > index 1292ac47eef8..c8973b543d1d 100644
> > --- a/kernel/liveupdate/luo_internal.h
> > +++ b/kernel/liveupdate/luo_internal.h
> > @@ -40,6 +40,28 @@ static inline int luo_ucmd_respond(struct luo_ucmd *ucmd,
> > */
> > #define luo_restore_fail(__fmt, ...) panic(__fmt, ##__VA_ARGS__)
> >
> > +/* Mimics list_for_each_entry() but for private list head entries */
> > +#define luo_list_for_each_private(pos, head, member) \
> > + for (struct list_head *__iter = (head)->next; \
> > + __iter != (head) && \
> > + ({ pos = container_of(__iter, typeof(*(pos)), member); 1; }); \
> > + __iter = __iter->next)
>
> Ideally something like this should go to include/linux/list.h, but it can
> be done later to avoid bikeshedding about the name :)
Exactly, I am planning to propose this as a separate change for
list.h, but I suspect we will need to take care of other variants as
well, reverse, cont, safe etc.
>
> And you can reuse most of list_for_each_entry, just replace the line that
> accesses __private member:
>
> #define luo_list_for_each_private(pos, head, member) \
> for (pos = list_first_entry(head, typeof(*pos), member); \
> &ACCESS_PRIVATE(pos, member) != head; \
> pos = list_next_entry(pos, member))
>
This does not work, because list_next_entry also accesses private,
what works is this:
#define luo_list_for_each_private(pos, head, member) \
for (pos = list_first_entry(head, typeof(*pos), member); \
&ACCESS_PRIVATE(pos, member) != head; \
pos = list_entry(ACCESS_PRIVATE(pos, member).next, typeof(*pos), member))
But that extra ACCESS_PRIVATE bothers me, so let's keep it as-is for
now. And solve it once in list.h, and then update the private macro.
> --
> Sincerely yours,
> Mike.
Powered by blists - more mailing lists