[bug#73654,v4] mapped-devices: luks: Support passing --allow-discards during open

  Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> tag 73654 + moreinfo
> quit
>
> Hi!
>
> soeren@soeren-tempel.net writes:
>
>> From: Sören Tempel <soeren@soeren-tempel.net>
>>
>> * gnu/system/mapped-devices.scm (open-luks-device): Support opening
>> LUKS devices with the --allow-discards option.
>> * gnu/system/mapped-devices.scm (luks-device-mapping-with-options):
>> Pass through the allow-discards? keyword argument.
>> * doc/guix.texi (Mapped Devices): Update documentation for the
>> luks-device-mapping-with-options procedure.
>>
>> Co-authored-by: Sisiutl <sisiutl@egregore.fun>
>
> I was about to apply it with the following cosmetic changes (mostly to
> meet the 80 max column width):
>
>> ---
>> Changes since v3: Fix replacement of “Solid State Disks” with “solid
>> state disks” in doc/guix.texi.  That is, only perform this replacement
>> locally on the added text and not the whole document.
>>
>>  doc/guix.texi                 | 11 +++++++++-
>>  gnu/system/mapped-devices.scm | 39 +++++++++++++++++++++--------------
>>  2 files changed, 33 insertions(+), 17 deletions(-)
>>
>> diff --git a/doc/guix.texi b/doc/guix.texi
>> index b1b6d98e74..6eb9fcb8ee 100644
>> --- a/doc/guix.texi
>> +++ b/doc/guix.texi
>> @@ -18402,7 +18402,7 @@ Mapped Devices
>>  @code{dm-crypt} Linux kernel module.
>>  @end defvar
>>  
>> -@deffn {Procedure} luks-device-mapping-with-options [#:key-file]
>> +@deffn {Procedure} luks-device-mapping-with-options [#:key-file #:allow-discards?]
>>  Return a @code{luks-device-mapping} object, which defines LUKS block
>>  device encryption using the @command{cryptsetup} command from the
>>  package with the same name.  It relies on the @code{dm-crypt} Linux
>> @@ -18424,6 +18424,15 @@ Mapped Devices
>>   (type (luks-device-mapping-with-options
>>          #:key-file "/crypto.key")))
>>  @end lisp
>> +
>> +
>> +@code{allow-discards?} allows the use of discard (TRIM) requests for the
>> +underlying device.  This is useful for solid state drives.  However,
>> +this option can have a negative security impact because it can make
>> +file system level operations visible on the physical device.  For more
>> +information, refer to the description of the @code{--allow-discards}
>> +option in the @code{cryptsetup-open(8)} man page.
>> +
>>  @end deffn
>>  
>>  @defvar raid-device-mapping
>> diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
>> index 931c371425..3a8f0d66fe 100644
>> --- a/gnu/system/mapped-devices.scm
>> +++ b/gnu/system/mapped-devices.scm
>> @@ -194,9 +194,10 @@ (define (check-device-initrd-modules device linux-modules location)
>>  ;;; Common device mappings.
>>  ;;;
>>  
>> -(define* (open-luks-device source targets #:key key-file)
>> +(define* (open-luks-device source targets #:key key-file allow-discards?)
>>    "Return a gexp that maps SOURCE to TARGET as a LUKS device, using
>> -'cryptsetup'."
>> +'cryptsetup'.  When ALLOW-DISCARDS? is true, the use of discard (TRIM) requests is
>> +allowed for the underlying device."
>>    (with-imported-modules (source-module-closure
>>                            '((gnu build file-systems)
>>                              (guix build utils))) ;; For mkdir-p
>> @@ -234,17 +235,21 @@ (define* (open-luks-device source targets #:key key-file)
>>                                              (loop (- tries-left 1))))))
>>                            (error "LUKS partition not found" source))
>>                        source)))
>> -             ;; We want to fallback to the password unlock if the keyfile fails.
>> -             (or (and keyfile
>> -                      (zero? (system*/tty
>> -                              #$(file-append cryptsetup-static "/sbin/cryptsetup")
>> -                              "open" "--type" "luks"
>> -                              "--key-file" keyfile
>> -                              partition #$target)))
>> -                 (zero? (system*/tty
>> -                         #$(file-append cryptsetup-static "/sbin/cryptsetup")
>> -                         "open" "--type" "luks"
>> -                         partition #$target)))))))))
>> +             (let ((cryptsetup-flags (cons*
>> +                                       "open" "--type" "luks" partition #$target
>> +                                       (if allow-discards?
>> +                                           '("--allow-discards")
>> +                                           '()))))
>> +               ;; We want to fallback to the password unlock if the keyfile fails.
>> +               (or (and keyfile
>> +                        (zero?
>> +                          (apply system*/tty
>> +                                 #$(file-append cryptsetup-static "/sbin/cryptsetup")
>> +                                 "--key-file" keyfile
>> +                                 cryptsetup-flags)))
>> +                   (zero? (apply system*/tty
>> +                                 #$(file-append cryptsetup-static "/sbin/cryptsetup")
>> +                                 cryptsetup-flags))))))))))
>>  
>>  (define (close-luks-device source targets)
>>    "Return a gexp that closes TARGET, a LUKS device."
>> @@ -286,13 +291,15 @@ (define luks-device-mapping
>>                ((gnu build file-systems)
>>                 #:select (find-partition-by-luks-uuid system*/tty))))))
>>  
>> -(define* (luks-device-mapping-with-options #:key key-file)
>> +(define* (luks-device-mapping-with-options #:key key-file allow-discards?)
>>    "Return a luks-device-mapping object with open modified to pass the arguments
>>  into the open-luks-device procedure."
>>    (mapped-device-kind
>>     (inherit luks-device-mapping)
>> -   (open (λ (source targets) (open-luks-device source targets
>> -                                               #:key-file key-file)))))
>> +   (open (λ (source targets)
>> +           (open-luks-device source targets
>> +                             #:key-file key-file
>> +                             #:allow-discards? allow-discards?)))))
>>  
>>  (define (open-raid-device sources targets)
>>    "Return a gexp that assembles SOURCES (a list of devices) to the RAID device
>>
>> base-commit: f2b3c36bee8c232b026a66de93db38e13fbd7076
>
>
> But unfortunately it appears to hang at least the 'encrypted-root-os'
> system test, which you can run like:
>
> $ make check-system TESTS=encrypted-root-os
> [...]
> cSeaBIOS (version 1.16.2/GNU Guix)
>
>
> iPXE (https://ipxe.org) 00:03.0 CA00 PCI2.10 PnP PMM+0EFCB030+0EF0B030 CA00
>                                                                                
>
>
> Booting from Hard Disk...
> GRUB loading..
> Welcome to GRUB!
>
> Enter passphrase for hd0,gpt2 (12345678-1234-1234-1234-123456789abc): 
> Attempting to decrypt master key...
> lot 0 opened
>   C-c C-cmake: *** [Makefile:7562: check-system] Interrompre
>
> Would you have an idea of why this happens and how we could avoid the
> hang in the test?

I have deployed the patch to my secondary laptop, it hangs on real
hardware as well.  I am not sure it was testing before sending it.

--8<---------------cut here---------------start------------->8---
Unbound variable: allow-discards?
--8<---------------cut here---------------end--------------->8---

I assume #$ is missing.  And indeed, this is enough to get my system to
boot again:

--8<---------------cut here---------------start------------->8---
--8<---------------cut here---------------end--------------->8---

I did not run the test case with the fix (it takes really long and I
should go to sleep), I will leave it as an exercise to the author.

>
> Thanks,