[PATCH] gnu: luks-device-mapping-with-options: Add allow-discards? argument.

  • Open
  • quality assurance status badge
Details
5 participants
  • Hilton Chain
  • Ludovic Courtès
  • Maxim Cournoyer
  • Sisiutl
  • soeren
Owner
unassigned
Submitted by
Sisiutl
Severity
normal

Debbugs page

Sisiutl wrote 5 months ago
(address . guix-patches@gnu.org)(name . Sisiutl)(address . sisiutl@egregore.fun)
20241006094239.7157-1-sisiutl@egregore.fun
* 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(-)

Toggle diff (71 lines)
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
Hilton Chain wrote 4 months ago
(name . Sisiutl)(address . sisiutl@egregore.fun)(address . 73654@debbugs.gnu.org)
87ikt0ihxf.wl-hako@ultrarare.space
Hi Sisiutl,

On Sun, 06 Oct 2024 17:42:28 +0800,
Sisiutl wrote:
Toggle quote (20 lines)
>
> * 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 :)


Toggle quote (62 lines)
> ;;;
> ;;; 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
Ludovic Courtès wrote 3 months ago
Re: [bug#73654] [PATCH] gnu: luks-device-mapping-with-options: Add allow-discards? argument.
(name . Sisiutl)(address . sisiutl@egregore.fun)(address . 73654@debbugs.gnu.org)(name . Tomas Volf)(address . ~@wolfsden.cz)
87msgwj4c1.fsf@gnu.org
Hi,

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

Sisiutl <sisiutl@egregore.fun> skribis:

Toggle quote (7 lines)
> * 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. :-)

Toggle quote (4 lines)
> +(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…

Toggle quote (4 lines)
> +(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’.
soeren wrote 1 weeks ago
[PATCH v2] mapped-devices: luks: Support passing --allow-discards during open
(address . 73654@debbugs.gnu.org)(address . sisiutl@egregore.fun)(address . hako@ultrarare.space)(address . ludo@gnu.org)
175e49381c046e500d69a4dc655258d7692d84df.1741535749.git.soeren@soeren-tempel.net
From: Sisiutl <sisiutl@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@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(-)

Toggle diff (99 lines)
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
Maxim Cournoyer wrote 1 weeks ago
(address . soeren@soeren-tempel.net)(address . 73654@debbugs.gnu.org)(address . sisiutl@egregore.fun)(address . ludo@gnu.org)(address . hako@ultrarare.space)
871pv5sip7.fsf@gmail.com
Hi,

soeren@soeren-tempel.net writes:

Toggle quote (11 lines)
> From: Sisiutl <sisiutl@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@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.

Toggle quote (11 lines)
> ---
> 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.

Toggle quote (25 lines)
> 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:

Toggle snippet (4 lines)
@code{allow-discards?} allows the use of discard (TRIM) requests for the
underlying device.

Toggle quote (3 lines)
> + This is useful for
> +Solid State Drives.

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

Toggle quote (3 lines)
> 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.

Toggle quote (44 lines)
> +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:

Toggle snippet (7 lines)
(let ((options `(,@(if allow-discards?
"--allow-discards"
'())
"open" "--type" "luks" partition #$target)))
[...])

Toggle quote (10 lines)
> + ;; 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.

Toggle quote (12 lines)
> (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.

Toggle quote (9 lines)
> (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
soeren wrote 3 days ago
[PATCH v3] mapped-devices: luks: Support passing --allow-discards during open
(address . 73654@debbugs.gnu.org)(address . sisiutl@egregore.fun)(address . hako@ultrarare.space)(address . ludo@gnu.org)(address . maxim.cournoyer@gmail.com)
20250314203029.13613-2-soeren@soeren-tempel.net
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>
---
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(-)

Toggle diff (107 lines)
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
Sören Tempel wrote 3 days ago
Re: [bug#73654] [PATCH v2] mapped-devices: luks: Support passing --allow-discards during open
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 73654@debbugs.gnu.org)(address . sisiutl@egregore.fun)(address . ludo@gnu.org)(address . hako@ultrarare.space)
2FK4LQGI02BYM.322YYEZ2J10BG@8pit.net
Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
Toggle quote (2 lines)
> Hi,

Hi Maxim,

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

Toggle quote (11 lines)
> 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
soeren wrote 41 hours ago
[PATCH v4] mapped-devices: luks: Support passing --allow-discards during open
(address . 73654@debbugs.gnu.org)(address . sisiutl@egregore.fun)(address . hako@ultrarare.space)(address . ludo@gnu.org)(address . maxim.cournoyer@gmail.com)
94e28c2091f319bfdb681055b7e5bdafa0cb9120.1742125790.git.soeren@soeren-tempel.net
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>
---
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(-)

Toggle diff (100 lines)
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
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 73654@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 73654
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch
You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags
mumi command -t +confirmed -t +easy
Or, remove the moreinfo tag and set the help tag
mumi command -t -moreinfo -t +help