[PATCH 0/2] Add efivarfs support.

DoneSubmitted by Mathieu Othacehe.
Details
3 participants
  • Mathieu Othacehe
  • Mathieu Othacehe
  • pelzflorian (Florian Pelz)
Owner
unassigned
Severity
normal
M
M
Mathieu Othacehe wrote on 16 Apr 2020 17:04
(address . guix-patches@gnu.org)(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)
20200416150436.3274-1-m.othacehe@gmail.com
Hello,

Here's a small serie to add support for efivarfs and fix the issue reported

The first patch is a bit hacky, so I didn't bother to write the documentation,
in case we find another solution.

Thanks,

Mathieu

Mathieu Othacehe (2):
file-system: Add mount-may-fail? option.
file-system: Add efivarfs support.

gnu/build/file-systems.scm | 49 +++++++++++++++++++++----------------
gnu/system/file-systems.scm | 23 ++++++++++++++---
gnu/system/install.scm | 1 +
3 files changed, 49 insertions(+), 24 deletions(-)

--
2.26.0
M
M
Mathieu Othacehe wrote on 16 Apr 2020 17:05
[PATCH 1/2] file-system: Add mount-may-fail? option.
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)
20200416150548.3426-1-m.othacehe@gmail.com
* gnu/system/file-systems.scm (<file-system>): Add a mount-may-fail? field.
(file-system->spec): adapt accordingly,
(spec->file-system): ditto.
* gnu/build/file-systems.scm (mount-file-system): If 'system-error is raised
and mount-may-fail? is true, ignore it. Otherwise, re-raise the exception.
---
gnu/build/file-systems.scm | 49 +++++++++++++++++++++----------------
gnu/system/file-systems.scm | 11 ++++++---
2 files changed, 36 insertions(+), 24 deletions(-)

Toggle diff (113 lines)
diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
index 902563b219..2f7987f334 100644
--- a/gnu/build/file-systems.scm
+++ b/gnu/build/file-systems.scm
@@ -673,26 +673,33 @@ corresponds to the symbols listed in FLAGS."
     (when (file-system-check? fs)
       (check-file-system source type))
 
-    ;; Create the mount point.  Most of the time this is a directory, but
-    ;; in the case of a bind mount, a regular file or socket may be needed.
-    (if (and (= MS_BIND (logand flags MS_BIND))
-             (not (file-is-directory? source)))
-        (unless (file-exists? mount-point)
-          (mkdir-p (dirname mount-point))
-          (call-with-output-file mount-point (const #t)))
-        (mkdir-p mount-point))
-
-    (cond
-     ((string-prefix? "nfs" type)
-      (mount-nfs source mount-point type flags options))
-     (else
-      (mount source mount-point type flags options)))
-
-    ;; For read-only bind mounts, an extra remount is needed, as per
-    ;; <http://lwn.net/Articles/281157/>, which still applies to Linux 4.0.
-    (when (and (= MS_BIND (logand flags MS_BIND))
-               (= MS_RDONLY (logand flags MS_RDONLY)))
-      (let ((flags (logior MS_BIND MS_REMOUNT MS_RDONLY)))
-        (mount source mount-point type flags #f)))))
+    (catch 'system-error
+      (lambda ()
+        ;; Create the mount point.  Most of the time this is a directory, but
+        ;; in the case of a bind mount, a regular file or socket may be
+        ;; needed.
+        (if (and (= MS_BIND (logand flags MS_BIND))
+                 (not (file-is-directory? source)))
+            (unless (file-exists? mount-point)
+              (mkdir-p (dirname mount-point))
+              (call-with-output-file mount-point (const #t)))
+            (mkdir-p mount-point))
+
+        (cond
+         ((string-prefix? "nfs" type)
+          (mount-nfs source mount-point type flags options))
+         (else
+          (mount source mount-point type flags options)))
+
+        ;; For read-only bind mounts, an extra remount is needed, as per
+        ;; <http://lwn.net/Articles/281157/>, which still applies to Linux
+        ;; 4.0.
+        (when (and (= MS_BIND (logand flags MS_BIND))
+                   (= MS_RDONLY (logand flags MS_RDONLY)))
+          (let ((flags (logior MS_BIND MS_REMOUNT MS_RDONLY)))
+            (mount source mount-point type flags #f))))
+      (lambda args
+        (or (file-system-mount-may-fail? fs)
+            (apply throw args))))))
 
 ;;; file-systems.scm ends here
diff --git a/gnu/system/file-systems.scm b/gnu/system/file-systems.scm
index 3b599efa8e..5180eca8d9 100644
--- a/gnu/system/file-systems.scm
+++ b/gnu/system/file-systems.scm
@@ -38,6 +38,7 @@
             file-system-flags
             file-system-options
             file-system-mount?
+            file-system-mount-may-fail?
             file-system-check?
             file-system-create-mount-point?
             file-system-dependencies
@@ -101,6 +102,8 @@
                     (default #f))
   (mount?           file-system-mount?            ; Boolean
                     (default #t))
+  (mount-may-fail?  file-system-mount-may-fail?   ; Boolean
+                    (default #f))
   (needed-for-boot? %file-system-needed-for-boot? ; Boolean
                     (default #f))
   (check?           file-system-check?            ; Boolean
@@ -261,18 +264,19 @@ store--e.g., if FS is the root file system."
   "Return a list corresponding to file-system FS that can be passed to the
 initrd code."
   (match fs
-    (($ <file-system> device mount-point type flags options _ _ check?)
+    (($ <file-system> device mount-point type flags options mount?
+                      mount-may-fail? needed-for-boot? check?)
      (list (cond ((uuid? device)
                   `(uuid ,(uuid-type device) ,(uuid-bytevector device)))
                  ((file-system-label? device)
                   `(file-system-label ,(file-system-label->string device)))
                  (else device))
-           mount-point type flags options check?))))
+           mount-point type flags options mount-may-fail? check?))))
 
 (define (spec->file-system sexp)
   "Deserialize SEXP, a list, to the corresponding <file-system> object."
   (match sexp
-    ((device mount-point type flags options check?)
+    ((device mount-point type flags options mount-may-fail? check?)
      (file-system
        (device (match device
                  (('uuid (? symbol? type) (? bytevector? bv))
@@ -283,6 +287,7 @@ initrd code."
                   device)))
        (mount-point mount-point) (type type)
        (flags flags) (options options)
+       (mount-may-fail? mount-may-fail?)
        (check? check?)))))
 
 (define (specification->file-system-mapping spec writable?)
-- 
2.26.0
M
M
Mathieu Othacehe wrote on 16 Apr 2020 17:05
[PATCH 2/2] file-system: Add efivarfs support.
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)
20200416150548.3426-2-m.othacehe@gmail.com
Tools such as efibootmgr rely on the deprecated /sys/firmware/efi/vars API as
well as on the new /sys/firmware/efi/efivars API. The later needs to be
mounted.

Reported by Keyhenge here:

Here is the efivarfs documentation:

* gnu/system/file-systems.scm (%efivars-file-system): New exported variable,
(%base-file-systems): add it.
* gnu/system/install.scm (%efivars-file-system): Add it.
---
gnu/system/file-systems.scm | 12 ++++++++++++
gnu/system/install.scm | 1 +
2 files changed, 13 insertions(+)

Toggle diff (51 lines)
diff --git a/gnu/system/file-systems.scm b/gnu/system/file-systems.scm
index 5180eca8d9..644a90d42b 100644
--- a/gnu/system/file-systems.scm
+++ b/gnu/system/file-systems.scm
@@ -57,6 +57,7 @@
             %pseudo-file-system-types
             %fuse-control-file-system
             %binary-format-file-system
+            %efivars-file-system
             %shared-memory-file-system
             %pseudo-terminal-file-system
             %tty-gid
@@ -334,6 +335,16 @@ TARGET in the other system."
     (type "binfmt_misc")
     (check? #f)))
 
+(define %efivars-file-system
+  ;; Support for EFI variables file system.
+  (file-system
+    (device "efivarfs")
+    (mount-point "/sys/firmware/efi/efivars")
+    (type "efivarfs")
+    (mount-may-fail? #t)
+    (needed-for-boot? #f)
+    (check? #f)))
+
 (define %tty-gid
   ;; ID of the 'tty' group.  Allocate it statically to make it easy to refer
   ;; to it from here and from the 'tty' group definitions.
@@ -434,6 +445,7 @@ TARGET in the other system."
   ;; currently mounted by the initrd.
   (list %pseudo-terminal-file-system
         %shared-memory-file-system
+        %efivars-file-system
         %immutable-store))
 
 ;; File systems for Linux containers differ from %base-file-systems in that
diff --git a/gnu/system/install.scm b/gnu/system/install.scm
index 0965c4d237..e78c61ed62 100644
--- a/gnu/system/install.scm
+++ b/gnu/system/install.scm
@@ -501,6 +501,7 @@ Access documentation at any time by pressing Alt-F2.\x1b[0m
             ;; elogind's cgroup file systems.
             (list %pseudo-terminal-file-system
                   %shared-memory-file-system
+                  %efivars-file-system
                   %immutable-store)))
 
     (users (list (user-account
-- 
2.26.0
P
P
pelzflorian (Florian Pelz) wrote on 30 Apr 2020 14:46
Re: [bug#40662] [PATCH 0/2] Add efivarfs support.
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)(address . 40662@debbugs.gnu.org)
20200430124618.r4yck2mmenup6p5n@pelzflorian.localdomain
On Thu, Apr 16, 2020 at 05:04:36PM +0200, Mathieu Othacehe wrote:
Toggle quote (5 lines)
> Hello,
>
> Here's a small serie to add support for efivarfs and fix the issue reported
> here: https://lists.gnu.org/archive/html/bug-guix/2020-04/msg00274.html.

I have no idea why I only ever got the [Patch 0/2] e-mail delivered to


Toggle quote (6 lines)
> [PATCH 2/2] file-system: Add efivarfs support.
> […]
> Tools such as efibootmgr rely on the deprecated /sys/firmware/efi/vars API as
> well as on the new /sys/firmware/efi/efivars API. The later needs to be
> mounted.

Typo: The latter needs to be mounted.


Well I had no issues with efivars on install without your patch. I
will try to provoke the error by adding more efivars. I did not yet
manage to create a new efivar via dd or via the efivar program though.

Regards,
Florian
P
P
pelzflorian (Florian Pelz) wrote on 30 Apr 2020 17:41
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)(address . 40662@debbugs.gnu.org)
20200430154150.eogbhn6crje4tho6@pelzflorian.localdomain
On Thu, Apr 30, 2020 at 02:46:18PM +0200, pelzflorian (Florian Pelz) wrote:
Toggle quote (7 lines)
> On Thu, Apr 16, 2020 at 05:04:36PM +0200, Mathieu Othacehe wrote:
> > Here's a small serie to add support for efivarfs and fix the issue reported
> > here: https://lists.gnu.org/archive/html/bug-guix/2020-04/msg00274.html.
> Well I had no issues with efivars on install without your patch. I
> will try to provoke the error by adding more efivars. I did not yet
> manage to create a new efivar via dd or via the efivar program though.

Actually I see no reason to test after provoking Keyhenge’s error.
How does your patch fix a full NVRAM? I thought Keyhenge resolved
their error by deleting “files” from the full NVRAM. Do I
misunderstand Keyhenge’s issue?

But /sys/firmware/efi/efivars probably should be mounted anyway.

Do you know why

efivarfs on /sys/firmware/efi/efivars type efivarfs (rw,nosuid,nodev,noexec,relatime)

is mounted even without your patch once I add %desktop-services to my
config.scm?

Regards,
Florian
P
P
pelzflorian (Florian Pelz) wrote on 30 Apr 2020 18:06
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)(address . 40662@debbugs.gnu.org)
20200430160632.xh5wzhxwoylvd44s@pelzflorian.localdomain
On Thu, Apr 16, 2020 at 05:04:36PM +0200, Mathieu Othacehe wrote:
Toggle quote (3 lines)
> Here's a small serie to add support for efivarfs and fix the issue reported
> here: https://lists.gnu.org/archive/html/bug-guix/2020-04/msg00274.html.

When I reconfigure the first time with your patch, I get a harmless error:

building /gnu/store/klnbdxbw2jqglrdqyslv87c9y72g80f7-upgrade-shepherd-services.scm.drv
guix system: error: exception caught while executing 'start' on service 'file-system-/sys/firmware/efi/efivars':
Throw to key `match-error' with args `("match" "no matching pattern" ("efivarfs" "/sys/firmware/efi/efivars" "efivarfs" () #f #t #f))

I don’t know why it happens.

Regards,
Florian
M
M
Mathieu Othacehe wrote on 1 May 2020 11:16
(name . pelzflorian (Florian Pelz))(address . pelzflorian@pelzflorian.de)(address . 40662@debbugs.gnu.org)
875zdguia5.fsf@gmail.com
Hey Florian,

Thanks a lot for having a look.

Toggle quote (5 lines)
> Actually I see no reason to test after provoking Keyhenge’s error.
> How does your patch fix a full NVRAM? I thought Keyhenge resolved
> their error by deleting “files” from the full NVRAM. Do I
> misunderstand Keyhenge’s issue?

I think the full NVRAM is only one part of the issue. There could also
be errors for variables > 1024 bytes using the legacy sysfs API, see:

Toggle quote (3 lines)
> is mounted even without your patch once I add %desktop-services to my
> config.scm?

Oh, it seems that elogind has support to mount efivarfs.

Mathieu
M
M
Mathieu Othacehe wrote on 1 May 2020 11:39
(name . pelzflorian (Florian Pelz))(address . pelzflorian@pelzflorian.de)(address . 40662@debbugs.gnu.org)
87wo5wt2n5.fsf@gmail.com
Toggle quote (5 lines)
> guix system: error: exception caught while executing 'start' on service 'file-system-/sys/firmware/efi/efivars':
> Throw to key `match-error' with args `("match" "no matching pattern" ("efivarfs" "/sys/firmware/efi/efivars" "efivarfs" () #f #t #f))
>
> I don’t know why it happens.

Seems like an ABI incompatibility between the running shepherd and this
patch adding a new field in "file-system->spec" procedure. Not sure it
can be worked around.

Mathieu
M
M
Mathieu Othacehe wrote on 31 Jul 2020 14:19
(address . 40662-done@debbugs.gnu.org)(name . pelzflorian (Florian Pelz))(address . pelzflorian@pelzflorian.de)
87k0yj27xf.fsf@gnu.org
Hello,

I just pushed this one to master. Closing!

Mathieu
Closed
?
Your comment

This issue is archived.

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