GNU bug report logs - #73654
[PATCH] gnu: luks-device-mapping-with-options: Add allow-discards? argument.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 73654 in the body.
You can then email your comments to 73654 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Sisiutl <sisiutl <at> egregore.fun>
To: guix-patches <at> gnu.org
Cc: Sisiutl <sisiutl <at> egregore.fun>
Subject: [PATCH] gnu: luks-device-mapping-with-options: Add allow-discards?
 argument.
Date: Sun,  6 Oct 2024 11:42:28 +0200

* gnu/system/mapped-devices.scm (luks-device-mapping-with-options): Add allow-discards? argument.

Change-Id: I0a43c13570a223d17698c7fe9ef4607e587bb8d0
---
 gnu/system/mapped-devices.scm | 36 +++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
index 931c371425..674e8708a4 100644
--- a/gnu/system/mapped-devices.scm
+++ b/gnu/system/mapped-devices.scm
@@ -189,12 +189,12 @@ (define missing
              (&error-location
               (location (source-properties->location 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'."
   (with-imported-modules (source-module-closure
@@ -234,17 +234,19 @@ (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 (list "open" "--type" "luks" partition #$target))
+                    (cryptsetup-flags (if allow-discards?
+                                          (cons "--allow-discards" cryptsetup-flags)
+                                          cryptsetup-flags)))
+               ;; 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 +288,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
-- 
2.46.0

Message #8 received at 73654 <at> debbugs.gnu.org (full text, mbox):

From: Hilton Chain <hako <at> ultrarare.space>
To: Sisiutl <sisiutl <at> egregore.fun>
Cc: 73654 <at> debbugs.gnu.org
Subject: Re: [PATCH] gnu: luks-device-mapping-with-options: Add
 allow-discards? argument.
Date: Wed, 06 Nov 2024 21:57:16 +0800

Hi Sisiutl,

On Sun, 06 Oct 2024 17:42:28 +0800,
Sisiutl wrote:
>
> * gnu/system/mapped-devices.scm (luks-device-mapping-with-options): Add allow-discards? argument.
>
> Change-Id: I0a43c13570a223d17698c7fe9ef4607e587bb8d0
> ---
>  gnu/system/mapped-devices.scm | 36 +++++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
> index 931c371425..674e8708a4 100644
> --- a/gnu/system/mapped-devices.scm
> +++ b/gnu/system/mapped-devices.scm
> @@ -189,12 +189,12 @@ (define missing
>               (&error-location
>                (location (source-properties->location location))))))))
>
> -
> +


This character (‘’) is a form feed, please leave it here :)


>  ;;;
>  ;;; 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'."
>    (with-imported-modules (source-module-closure
> @@ -234,17 +234,19 @@ (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 (list "open" "--type" "luks" partition #$target))
> +                    (cryptsetup-flags (if allow-discards?
> +                                          (cons "--allow-discards" cryptsetup-flags)
> +                                          cryptsetup-flags)))
> +               ;; 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 +288,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
> --
> 2.46.0


Can you also add documentation for this option in doc/guix.texi?


Thanks

Message #11 received at 73654 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Sisiutl <sisiutl <at> egregore.fun>
Cc: 73654 <at> debbugs.gnu.org, Tomas Volf <~@wolfsden.cz>
Subject: Re: [bug#73654] [PATCH] gnu: luks-device-mapping-with-options: Add
 allow-discards? argument.
Date: Sun, 15 Dec 2024 17:31:10 +0100

Hi,

(Cc: Tomas, who I believe initially worked on this.)

Sisiutl <sisiutl <at> egregore.fun> skribis:

> * gnu/system/mapped-devices.scm (luks-device-mapping-with-options): Add allow-discards? argument.
>
> Change-Id: I0a43c13570a223d17698c7fe9ef4607e587bb8d0

> -
> +

This is a linefeed and it facilitates navigation in the file; please
preserve it.  :-)

> +(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'."

Please briefly document ‘allow-discards?’ in the docstring…

> +(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."

… also here, and also in a bit more detail in the relevant place in
‘doc/guix.texi’.

Thanks in advance!

Ludo’.

Message #14 received at 73654 <at> debbugs.gnu.org (full text, mbox):

From: soeren <at> soeren-tempel.net
To: 73654 <at> debbugs.gnu.org
Cc: sisiutl <at> egregore.fun, hako <at> ultrarare.space, ludo <at> gnu.org
Subject: [PATCH v2] mapped-devices: luks: Support passing --allow-discards
 during open
Date: Sun,  9 Mar 2025 16:55:49 +0100

From: Sisiutl <sisiutl <at> egregore.fun>

* 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.

Signed-off-by: Sören Tempel <soeren <at> soeren-tempel.net>
---
Not the author of the original patchset, but I needed this for my
own setup as well so I might as well pick up the slack.  I made
the following changes since the v1:

* Mention allow-discards? in the docstring of open-luks-device.
* Reference the new option in luks-device-mapping-with-options.
* Expand the related documentation in doc/guix.texi.
* Revise the commit message slightly.
* Restore the linefeed.

 doc/guix.texi                 | 11 +++++++++-
 gnu/system/mapped-devices.scm | 39 ++++++++++++++++++++---------------
 2 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 05c855c5ea..bc3ba1f2ed 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -18461,7 +18461,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
@@ -18483,6 +18483,15 @@ Mapped Devices
  (type (luks-device-mapping-with-options
         #:key-file "/crypto.key")))
 @end lisp
+
+If @code{allow-discards?} is provided, then the use of discard (TRIM)
+requests is allowed for the underlying device.  This is useful for
+Solid State Drives.  However, this option can have a negative security
+impact because it can make filesystem-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..c3eaf9ff6e 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,19 @@ (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 (list "open" "--type" "luks" partition #$target))
+                    (cryptsetup-flags (if allow-discards?
+                                          (cons "--allow-discards" cryptsetup-flags)
+                                          cryptsetup-flags)))
+               ;; 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 +289,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."
+(key-file and allow-discards?) 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: c4f297a664869a18126b66eb5209de1fcceb42d8

Message #17 received at 73654 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: soeren <at> soeren-tempel.net
Cc: 73654 <at> debbugs.gnu.org, sisiutl <at> egregore.fun, ludo <at> gnu.org,
 hako <at> ultrarare.space
Subject: Re: [bug#73654] [PATCH v2] mapped-devices: luks: Support passing
 --allow-discards during open
Date: Mon, 10 Mar 2025 11:49:56 +0900

Hi,

soeren <at> soeren-tempel.net writes:

> From: Sisiutl <sisiutl <at> egregore.fun>
>
> * 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.
>
> Signed-off-by: Sören Tempel <soeren <at> soeren-tempel.net>

I'd use a 'Co-authored-by' if significantly modified or 'Modified-by' if
lightly touched git trailers here.  Signed-off-by is currently used in
Guix to denote someone else's work pushed by a committer.

> ---
> Not the author of the original patchset, but I needed this for my
> own setup as well so I might as well pick up the slack.  I made
> the following changes since the v1:
>
> * Mention allow-discards? in the docstring of open-luks-device.
> * Reference the new option in luks-device-mapping-with-options.
> * Expand the related documentation in doc/guix.texi.
> * Revise the commit message slightly.
> * Restore the linefeed.

Sounds good.

>  doc/guix.texi                 | 11 +++++++++-
>  gnu/system/mapped-devices.scm | 39 ++++++++++++++++++++---------------
>  2 files changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 05c855c5ea..bc3ba1f2ed 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -18461,7 +18461,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
> @@ -18483,6 +18483,15 @@ Mapped Devices
>   (type (luks-device-mapping-with-options
>          #:key-file "/crypto.key")))
>  @end lisp
> +
> +If @code{allow-discards?} is provided, then the use of discard (TRIM)
> +requests is allowed for the underlying device.

I'd streamline this sentence into:

--8<---------------cut here---------------start------------->8---
@code{allow-discards?} allows the use of discard (TRIM) requests for the
underlying device.
--8<---------------cut here---------------end--------------->8---

> + This is useful for
> +Solid State Drives.

I'd use 'solid state drives', un-capitalized or @acronym{SSD, Solid
State Drives}.

> However, this option can have a negative security
> +impact because it can make filesystem-level operations visible on the

The GNU convention is to use 'file system', not filesystem.

> +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..c3eaf9ff6e 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,19 @@ (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 (list "open" "--type" "luks" partition #$target))
> +                    (cryptsetup-flags (if allow-discards?
> +                                          (cons "--allow-discards" cryptsetup-flags)
> +                                          cryptsetup-flags)))

Theres' not need for a let* and reusing the same variable; you can
instead use the following list splicing trick:

--8<---------------cut here---------------start------------->8---
(let ((options `(,@(if allow-discards?
                       "--allow-discards"
                       '())
                 "open" "--type" "luks" partition #$target)))
  [...])
--8<---------------cut here---------------end--------------->8---

> +               ;; 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))))))))))

You'll want to nest the apply under the (zero? ... call and ensure it
fits under 80 characters, which is in our coding style guidelines.

>  (define (close-luks-device source targets)
>    "Return a gexp that closes TARGET, a LUKS device."
> @@ -286,13 +289,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."
> +(key-file and allow-discards?) into the open-luks-device procedure."

I would drop the above doc change.  'Arguments' already cover it in a
more abstract (and maintainable) fashion.

>    (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?)))))

The rest LGTM.  Could you please send a new revision taking into account
my review comments?

-- 
Thanks,
Maxim

Message #20 received at 73654 <at> debbugs.gnu.org (full text, mbox):

From: soeren <at> soeren-tempel.net
To: 73654 <at> debbugs.gnu.org
Cc: sisiutl <at> egregore.fun, hako <at> ultrarare.space, ludo <at> gnu.org,
 maxim.cournoyer <at> gmail.com
Subject: [PATCH v3] mapped-devices: luks: Support passing --allow-discards
 during open
Date: Fri, 14 Mar 2025 21:27:06 +0100

From: Sören Tempel <soeren <at> 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 <at> egregore.fun>
---
Change since v2:

* Revert doc change in luks-device-mapping-with-options procedure
* Reformat zero? expression to make it fit into the 80 characters
* Do not use let* expression
* Reword "filesystem" to "file system"
* Reword "Solid State Drives" to "solid state drives"
* Streamline description of new feature in documentation
* Use co-authored-by and swap author and co-author

 doc/guix.texi                 | 13 ++++++++++--
 gnu/system/mapped-devices.scm | 39 +++++++++++++++++++++--------------
 2 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index b1b6d98e74..91588ca02f 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -18402,7 +18402,7 @@ command from the package with the same name.  It relies on the
 @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 @@ given location at the time of the unlock attempt.
  (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
@@ -18591,7 +18600,7 @@ priority after prioritized spaces, and in the order that they appeared in
 @item @code{discard?} (default: @code{#f})
 Only supported by the Linux kernel.  When true, the kernel will notify
 the disk controller of discarded pages, for example with the TRIM
-operation on Solid State Drives.
+operation on solid state drives.
 
 @end table
 @end deftp
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 missing
 ;;; 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

Message #23 received at 73654 <at> debbugs.gnu.org (full text, mbox):

From: Sören Tempel <soeren <at> soeren-tempel.net>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 73654 <at> debbugs.gnu.org, sisiutl <at> egregore.fun, ludo <at> gnu.org,
 hako <at> ultrarare.space
Subject: Re: [bug#73654] [PATCH v2] mapped-devices: luks: Support passing
 --allow-discards during open
Date: Fri, 14 Mar 2025 21:38:29 +0100

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> wrote:
> Hi,

Hi Maxim,

Thanks for taking a look at the patch, I revised it as requested.

> Theres' not need for a let* and reusing the same variable; you can
> instead use the following list splicing trick:
> 
> --8<---------------cut here---------------start------------->8---
> (let ((options `(,@(if allow-discards?
>                        "--allow-discards"
>                        '())
>                  "open" "--type" "luks" partition #$target)))
>   [...])
> --8<---------------cut here---------------end--------------->8---

Implemented this slightly differently using a cons* expression, I hope
that is fine as well (I find it slightly more readable), if not let me
know.

Greetings,
Sören

Message #26 received at 73654 <at> debbugs.gnu.org (full text, mbox):

From: soeren <at> soeren-tempel.net
To: 73654 <at> debbugs.gnu.org
Cc: sisiutl <at> egregore.fun, hako <at> ultrarare.space, ludo <at> gnu.org,
 maxim.cournoyer <at> gmail.com
Subject: [PATCH v4] mapped-devices: luks: Support passing --allow-discards
 during open
Date: Sun, 16 Mar 2025 12:49:50 +0100

From: Sören Tempel <soeren <at> 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 <at> egregore.fun>
---
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

Message #29 received at 73654 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: soeren <at> soeren-tempel.net
Cc: 73654 <at> debbugs.gnu.org, sisiutl <at> egregore.fun, ludo <at> gnu.org,
 GNU Debbugs <control <at> debbugs.gnu.org>, hako <at> ultrarare.space
Subject: Re: [PATCH v4] mapped-devices: luks: Support passing
 --allow-discards during open
Date: Thu, 20 Mar 2025 11:54:28 +0900

tag 73654 + moreinfo
quit

Hi!

soeren <at> soeren-tempel.net writes:

> From: Sören Tempel <soeren <at> 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 <at> egregore.fun>

I was about to apply it with the following cosmetic changes (mostly to
meet the 80 max column width):

--8<---------------cut here---------------start------------->8---
> ---
> 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
--8<---------------cut here---------------end--------------->8---

But unfortunately it appears to hang at least the 'encrypted-root-os'
system test, which you can run like:

--8<---------------cut here---------------start------------->8---
$ 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
--8<---------------cut here---------------end--------------->8---

Would you have an idea of why this happens and how we could avoid the
hang in the test?

Thanks,

-- 
Maxim

Message #34 received at 73654 <at> debbugs.gnu.org (full text, mbox):

From: Tomas Volf <~@wolfsden.cz>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: soeren <at> soeren-tempel.net, sisiutl <at> egregore.fun, ludo <at> gnu.org,
 73654 <at> debbugs.gnu.org, hako <at> ultrarare.space,
 GNU Debbugs <control <at> debbugs.gnu.org>
Subject: Re: [bug#73654] [PATCH v4] mapped-devices: luks: Support passing
 --allow-discards during open
Date: Fri, 21 Mar 2025 00:08:03 +0100

[Message part 1 (text/plain, inline)]

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

> tag 73654 + moreinfo
> quit
>
> Hi!
>
> soeren <at> soeren-tempel.net writes:
>
>> From: Sören Tempel <soeren <at> 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 <at> 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---
--- a/gnu/system/mapped-devices.scm
+++ b/gnu/system/mapped-devices.scm
@@ -239,7 +239,7 @@ (define* (open-luks-device source targets #:key key-file allow-discards?)
                       source)))
              (let ((cryptsetup-flags (cons*
                                        "open" "--type" "luks" partition #$target
-                                       (if allow-discards?
+                                       (if #$allow-discards?
                                            '("--allow-discards")
                                            '()))))
                ;; We want to fallback to the password unlock if the keyfile fails.
--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,

-- 
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.

[signature.asc (application/pgp-signature, inline)]

Message #37 received at 73654 <at> debbugs.gnu.org (full text, mbox):

From: Tomas Volf <~@wolfsden.cz>
To: soeren <at> soeren-tempel.net
Cc: 73654 <at> debbugs.gnu.org, sisiutl <at> egregore.fun, ludo <at> gnu.org,
 maxim.cournoyer <at> gmail.com, hako <at> ultrarare.space
Subject: Re: [bug#73654] [PATCH v4] mapped-devices: luks: Support passing
 --allow-discards during open
Date: Fri, 21 Mar 2025 00:14:12 +0100

soeren <at> soeren-tempel.net writes:

> From: Sören Tempel <soeren <at> 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 <at> egregore.fun>
> ---
> 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)))

I am not sure about passing the --key-file before the `open' command.
It does seem to work (currently), but I am not sure we should assume it
always will.

Is this type of usage documented somewhere?  All manuals I found are
passing the arguments after `open'.

You could rewrite this into a lambda returning the argument list, the
lambda would splice them (both keyfile and discard) into the correct
places.

> +                   (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

Message #40 received at 73654 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Tomas Volf <~@wolfsden.cz>
Cc: 73654 <at> debbugs.gnu.org, sisiutl <at> egregore.fun, ludo <at> gnu.org,
 hako <at> ultrarare.space, soeren <at> soeren-tempel.net
Subject: Re: [bug#73654] [PATCH v4] mapped-devices: luks: Support passing
 --allow-discards during open
Date: Sat, 22 Mar 2025 22:36:27 +0900

Hi,

Tomas Volf <~@wolfsden.cz> writes:

[...]

>> +               ;; 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)))
>
> I am not sure about passing the --key-file before the `open' command.
> It does seem to work (currently), but I am not sure we should assume it
> always will.

It's documented as such, per 'cryptsetup --help':

--8<---------------cut here---------------start------------->8---
cryptsetup 2.6.1 flags: UDEV BLKID KEYRING KERNEL_CAPI 
Usage: cryptsetup [OPTION...] <action> <action-specific>

Help options:
  -?, --help                            Show this help message
      --usage                           Display brief usage
  -V, --version                         Print package version
      --active-name=STRING              Override device autodetection of dm device to be reencrypted
      --align-payload=SECTORS           Align payload at <n> sector boundaries - for luksFormat
      --allow-discards                  Allow discards (aka TRIM) requests for device
--8<---------------cut here---------------end--------------->8---

There are many options though perhaps we should just provide a
#:extra-args escape hatch.


-- 
Thanks,
Maxim

Message #45 received at 73654-done <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Tomas Volf <~@wolfsden.cz>
Cc: 73654-done <at> debbugs.gnu.org, sisiutl <at> egregore.fun, ludo <at> gnu.org,
 soeren <at> soeren-tempel.net, hako <at> ultrarare.space
Subject: Re: bug#73654: [PATCH] gnu: luks-device-mapping-with-options: Add
 allow-discards? argument.
Date: Sun, 23 Mar 2025 00:02:10 +0900

Hello,

Pushed as commit 7aa855b05b.

-- 
Thanks,
Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.