Message-ID: <mafs0cyb7mzl2.fsf@kernel.org>
Date: Fri, 13 Jun 2025 17:27:21 +0200
From: Pratyush Yadav <pratyush@...nel.org>
To: Pasha Tatashin <pasha.tatashin@...een.com>
Cc: Pratyush Yadav <pratyush@...nel.org>,  jasonmiu@...gle.com,
  graf@...zon.com,  changyuanl@...gle.com,  rppt@...nel.org,
  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,
  zhangguopeng@...inos.cn,  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
Subject: Re: [RFC v2 08/16] luo: luo_files: add infrastructure for FDs

On Sun, Jun 08 2025, Pasha Tatashin wrote:

[...]
>> > +     down_write(&luo_filesystems_list_rwsem);
>> > +     if (luo_files_xa_in_recreated)
>> > +             goto exit_unlock;
>> > +
>> > +     parent_node_offset = fdt_subnode_offset(luo_fdt_in, 0,
>> > +                                             LUO_FILES_NODE_NAME);
>> > +
>> > +     fdt_for_each_subnode(file_node_offset, luo_fdt_in, parent_node_offset) {
>> > +             bool handler_found = false;
>> > +             u64 token;
>> > +
>> > +             node_name = fdt_get_name(luo_fdt_in, file_node_offset, NULL);
>> > +             if (!node_name) {
>> > +                     panic("Skipping FDT subnode at offset %d: Cannot get name\n",
>> > +                           file_node_offset);
>>
>> Should failure to parse a specific FD really be a panic? Wouldn't it be
>> better to continue and let userspace decide if it can live with the FD
>> missing?
>
> This is not safe, the memory might be DMA or owned by a sensetive
> process, and if we proceed liveupdate reboot without properly handling
> memory, we can get corruptions, and memory leaks. Therefore, during
> liveupdate boot if there are exceptions, we should panic.

I don't get how it would result in memory leaks or corruptions, since
KHO would have marked that memory as preserved, and the new kernel won't
touch it until someone restores it.

So it can at most lead to loss of data, and in that case, userspace can
very well decide if it can live with that loss or not.

Or are you assuming here that even data in KHO is broken? In that case,
it would probably be a good idea to panic early.

[...]
>> > +             }
>> > +
>> > +             luo_file = kmalloc(sizeof(*luo_file),
>> > +                                GFP_KERNEL | __GFP_NOFAIL);
>> > +             luo_file->fs = fs;
>> > +             luo_file->file = NULL;
>> > +             memcpy(&luo_file->private_data, data_ptr, sizeof(u64));
>>
>> Why not make sure data_ptr is exactly sizeof(u64) when we parse it, and
>> then simply do luo_file->private_data = (u64)*data_ptr ?
>
> Because FDT alignment is 4 bytes, we can't simply assign it.

Hmm, good catch. Didn't think of that.

>
>> Because if the previous kernel wrote more than a u64 in data, then
>> something is broken and we should catch that error anyway.
>>
>> > +             luo_file->reclaimed = false;
>> > +             mutex_init(&luo_file->mutex);
>> > +             luo_file->state = LIVEUPDATE_STATE_UPDATED;
>> > +             ret = xa_err(xa_store(&luo_files_xa_in, token, luo_file,
>> > +                                   GFP_KERNEL | __GFP_NOFAIL));
>>
[...]
>> > +struct liveupdate_filesystem {
>> > +     int (*prepare)(struct file *file, void *arg, u64 *data);
>> > +     int (*freeze)(struct file *file, void *arg, u64 *data);
>> > +     void (*cancel)(struct file *file, void *arg, u64 data);
>> > +     void (*finish)(struct file *file, void *arg, u64 data, bool reclaimed);
>> > +     int (*retrieve)(void *arg, u64 data, struct file **file);
>> > +     bool (*can_preserve)(struct file *file, void *arg);
>> > +     const char *compatible;
>> > +     void *arg;
>>
>> What is the use for this arg? I would expect one file type/system to
>> register one set of handlers. So they can keep their arg in a global in
>> their code. I don't see why a per-filesystem arg is needed.
>
> I think, arg is useful in case we support a subsystem is registered
> multiple times with some differences: i.e. based on mount point, or
> file types handling. Let's keep it for now, but if needed, we can
> remove that in future revisions.
>
>> What I do think is needed is a per-file arg. Each callback gets 'data',
>> which is the serialized data, but there is no place to store runtime
>> state, like some flags or serialization metadata. Sure, you could make
>> place for it somewhere in the inode, but I think it would be a lot
>> cleaner to be able to store it in struct luo_file.
>>
>> So perhaps rename private_data in struct luo_file to say
>> serialized_data, and have a field called "private" that filesystems can
>> use for their runtime state?
>
> I am not against this, but let's make this change when it is actually
> needed by a registered filesystem.

Okay, fair enough.

-- 
Regards,
Pratyush Yadav

Powered by blists - more mailing lists