Re: [PATCH v18 12/21] dm: add finalize hook to target_type

[Mikulas Patocka]

On Fri, 17 May 2024, Fan Wu wrote:

> > So, it seems that the preresume callback provides the guarantee that you
> > looking for.
> > 
> >> -Fan
> > 
> > Mikulas
> 
> Thanks for the info. I have tested and verified that the preresume() hook can
> also work for our case.
> 
> From the source code
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/md/dm-ioctl.c#n1149,
> the whole resume process appears to be:
> 
> 1. Check if there is a new map for the device. If so, attempt to activate the
> new map using dm_swap_table() (where the finalize() callback occurs).
> 
> 2. Check if the device is suspended. If so, use dm_resume() (where the
> preresume() callback occurs) to resume the device.
> 
> 3. If a new map is activated, use dm_table_destroy() to destroy the old map.
> 
> For our case:
> 
> - Using the finalize() callback, the metadata of the dm-verity target inside
> the table is attached to the mapped device every time a new table is
> activated.
> - Using the preresume() callback, the same metadata is attached every time the
> device resumes from suspension.
> 
> If I understand the code correctly, resuming from suspension is a necessary
> step for loading a new mapping table. Thus, the preresume() callback covers
> all conditions where the finalize() callback would be triggered.

Yes.

> However, the preresume() callback can also be triggered when the device 
> resumes from suspension without loading a new table, in which case there 
> is no new metadata in the table to attach to the mapped device.

Yes.

> In the scenario where the finalize() callback succeeds but the preresume()
> callback fails, it seems the device will remain in a suspended state, the
> newly activated table will be kept, and the old table will be destroyed, so it
> seems there is no inconsistency using finalize() even preresume() potentially
> fails.

What does your security module do when the verification of the dm-verity 
hash fails? Does it halt the whole system? Does it destroy just the 
failing dm device? Or does it attempt to recover somehow from this 
situation?

> I believe both the finalize() callback proposed by Mike and the preresume()
> callback suggested by Mikulas can work for our case. I am fine with either
> approach, but I would like to know which one is preferred by the maintainers
> and would appreciate an ACK for the chosen approach.
> 
> -Fan

I would prefer preresume - we shouldn't add new callbacks unless it's 
necessary.

Mikulas