Re: [PATCH 3/4] luks2: set up dummy sector size during scan

[Fabian Vogt <fvogt@suse.de>]

Hi,

Am Mittwoch, 22. Dezember 2021, 19:17:37 CET schrieb Josselin Poiret via Grub-devel:
> Hello everyone,
> 
> Fabian Vogt <fvogt@suse.de> writes:
> > It looks like we have a third patch (series) for this feature meanwhile:
> > [PATCH 0/2] Have LUKS2 cryptomounts be useable with grub-probe
> >
> > I CC'd the author, let's try to coordinate.

And there's a forth one now (author CC'd)!
("[PATCH 3/3] grub-core/kern/disk.c: handle LUKS2 devices")

So we have:

"[PATCH 3/4] luks2: set up dummy sector size during scan", which hardcodes 512,

"[PATCH 1/2] disk/cryptodisk: When cheatmounting, use the sector info of the
 cheat device", which queries the sector size of the underlying host device,

"[PATCH v2 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from
 DM parameters", which parses the DM table to get the sector_size, and now

"[PATCH 3/3] grub-core/kern/disk.c: handle LUKS2 devices", which changes the
grub core code to accept a sector size of 0 for LUKS2 devices.

Should be enough options to pick a good one ;-)

> > Thanks,
> > Fabian
> 
> Let me just say that I had not found this patch series while searching
> beforehand.  Let me just recap what my patches do differently (in
> relation to patches 3 and 4 of this series):
> 
> Because cheat-mounting cryptodisks only happens (from my understanding)
> when pulling devmapper devices, we can simply ask the kernel for the dm
> and dm-crypt parameters that it's opened with, and populate our
> cheat-mounted device from that.  This completely circumvents the multiple
> segments issue, as this will always yield the parameters corresponding
> to the user-specified mountpoint of `grub-probe` or `grub-install`.

Yup.

Did you have a look at my approach? That effectively does the same, but using a
single ioctl instead of anything complex with DM directly.

> I also opted not to add a GRUB_DEV_ABSTRACTION_LUKS2 abstraction, so as
> to reuse all existing code that supports LUKS1, although that can be
> confusing.  We could simply rename GRUB_DEV_ABSTRACTION_LUKS1 to
> GRUB_DEV_ABSTRACTION_CRYTODISK, as LUKS1 and LUKS2 only differ in how
> they're unlocked, not in underlying algorithms.
> 
> What do you think?

Sounds good to me, though I'd count that as a separate refactoring step for
the future.

Cheers,
Fabian