[PATCH 0/7] ZFS/other CoW FS save_env support

[PATCH 0/7] ZFS/other CoW FS save_env support

[Paul Dagnelie]

Hey all, I previously discussed my concept for this patch in my email
https://lists.gnu.org/archive/html/grub-devel/2020-01/msg00004.html .
I'm pleased to announce that I've gotten it into a working state and
it is ready to review.  There are a number of changes here, which I
will break down below.

First, the concept of "special files" is introduced into grub. These
are files that are manipulated using different functions from the
remainder of the filesystem. A filesystem advertises its support for
these files by filling in new entries in the grub_fs struct.

Second, the loadenv command was modified to use the special file
functions of the root filesystem if they are detected. If that
happens, these functions are used to open, read, and write to the
loadenv file instead of the normal grub file functions.  A variable
was also added that allows the user to force a file to be used instead
of the special files, or vice versa.

Third, the grub-editenv command was modified to teach it to probe the
root filesystem and then check in an array of structures that contain
functions that will read and modify the filesystem's special file in
userland. These functions are very similar to normal read and write
functions, and so don't result in a very different code flow.

Finally, the GRUB ZFS logic was modified to have new special_file
functions. These functions manipulate the pad2 area of the ZFS label,
which was previously unused. It now stores a version number, the raw
contents of the grubenv file, and an embedded checksum for integrity
purposes. GRUB was taught to read and verify these areas, and also to
modify them, computing the embeddded checksum appropriately.  In
addition, if GRUB is build with libzfs support, an entry is added to
the grub-editenv table that points GRUB to the appropriate libzfs
functions, and init and fini functions are built into grub-editenv
itself.

Future work:
* In the ZFS code could store a packed nvlist instead of a raw file,
but this would involve further changes to the grub environment file
code and was deferred for when it would be more useful and important.
* For the special files code, there is only one special file allowed
per filesystem, but a specification interface (using either an enum or
a set of names) could be added in the future.
* Other filesystem support was not built into this change, but
extensibility was a primary goal, so adding support for btrfs or other
filesystems should be relatively straightforward.
* I did not implement the proposed UEFI variable support, but I did
develop this with that future in mind, so adding it should be a
relatively simple extension of these changes.

This patchset has been tested on systems with and without ZFS as the boot
filesystem, and built with and without ZFS libraries installed. It
worked in each case I tried, including with manual corruption applied
to the ZFS labels to force GRUB to read from the other label.  This
was tested in conjunction with
https://github.com/zfsonlinux/zfs/pull/10009 , the ZoL patch that
enables ZFS to read from and write to the padding areas we reserved
for the data.

Paul Dagnelie (7):
  Factor out file filter function for reuse
  Add support for special envblk functions in GRUB
  Factor out envblk buffer creation for reuse
  Use envblk file and fs functions to implement reading the grubenv file
    from special FS regions
  Add ZFS envblock functions
  Refactor default envblk size out for reuse
  Update editenv to support editing zfs envblock, fix whitespace, and
    autodetect bootenv support in libzfs

 configure.ac                 |   8 +
 grub-core/commands/loadenv.c | 122 +++++++++++++--
 grub-core/fs/zfs/zfs.c       | 134 +++++++++++++++--
 grub-core/kern/file.c        |  76 +++++++---
 include/grub/file.h          |  10 ++
 include/grub/fs.h            |  12 ++
 include/grub/lib/envblk.h    |   3 +
 include/grub/util/install.h  |   3 +
 include/grub/util/libzfs.h   |   7 +
 include/grub/zfs/vdev_impl.h |  33 ++---
 util/editenv.c               |  31 ++--
 util/grub-editenv.c          | 278 +++++++++++++++++++++++++++++++++--
 12 files changed, 632 insertions(+), 85 deletions(-)

-- 
2.19.0

[PATCH 1/7] Factor out file filter function for reuse

[Paul Dagnelie]

This patch refactors out the logic that applies filters to files for reuse in
other code.

Signed-off-by: Paul Dagnelie <pcd@delphix.com>
---
 grub-core/kern/file.c | 46 +++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/grub-core/kern/file.c b/grub-core/kern/file.c
index 58454458c..75eb5e2fa 100644
--- a/grub-core/kern/file.c
+++ b/grub-core/kern/file.c
@@ -57,14 +57,37 @@ grub_file_get_device_name (const char *name)
   return 0;
 }
 
+static grub_file_t
+grub_apply_file_filters (grub_file_t file, enum grub_file_type type, const char *name)
+{
+  grub_file_filter_id_t filter;
+  grub_file_t last_file = NULL;
+
+  for (filter = 0; file && filter < ARRAY_SIZE (grub_file_filters);
+       filter++)
+    if (grub_file_filters[filter])
+      {
+	last_file = file;
+	file = grub_file_filters[filter] (file, type);
+	if (file && file != last_file)
+	  {
+	    file->name = grub_strdup (name);
+	    grub_errno = GRUB_ERR_NONE;
+	  }
+      }
+  if (!file)
+    grub_file_close (last_file);
+
+  return file;
+}
+
 grub_file_t
 grub_file_open (const char *name, enum grub_file_type type)
 {
-  grub_device_t device = 0;
-  grub_file_t file = 0, last_file = 0;
+  grub_device_t device = NULL;
+  grub_file_t file = NULL;
   char *device_name;
   const char *file_name;
-  grub_file_filter_id_t filter;
 
   device_name = grub_file_get_device_name (name);
   if (grub_errno)
@@ -113,22 +136,7 @@ grub_file_open (const char *name, enum grub_file_type type)
   file->name = grub_strdup (name);
   grub_errno = GRUB_ERR_NONE;
 
-  for (filter = 0; file && filter < ARRAY_SIZE (grub_file_filters);
-       filter++)
-    if (grub_file_filters[filter])
-      {
-	last_file = file;
-	file = grub_file_filters[filter] (file, type);
-	if (file && file != last_file)
-	  {
-	    file->name = grub_strdup (name);
-	    grub_errno = GRUB_ERR_NONE;
-	  }
-      }
-  if (!file)
-    grub_file_close (last_file);
-
-  return file;
+  return grub_apply_file_filters(file, type, name);
 
  fail:
   if (device)
-- 
2.19.0

[PATCH 2/7] Add support for special envblk functions in GRUB

[Paul Dagnelie]

This patch introducces the idea of envblk functions to GRUB. These functions
are used to interact with a grubenv file stored not in the usual grub tree,
but instead in a special filesystem-managed region that can more easily be
accessed at boot time. These functions interface between calling code and the
per-fs logic, an interface for which is also added as part of this patch.

Signed-off-by: Paul Dagnelie <pcd@delphix.com>
---
 grub-core/kern/file.c | 30 ++++++++++++++++++++++++++++--
 include/grub/file.h   | 10 ++++++++++
 include/grub/fs.h     | 12 ++++++++++++
 3 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/file.c b/grub-core/kern/file.c
index 75eb5e2fa..f929f61a0 100644
--- a/grub-core/kern/file.c
+++ b/grub-core/kern/file.c
@@ -149,6 +149,27 @@ grub_file_open (const char *name, enum grub_file_type type)
   return 0;
 }
 
+grub_file_t
+grub_file_envblk_open (grub_fs_t fs, grub_device_t device, enum grub_file_type type)
+{
+  grub_file_t file = NULL;
+  file = (grub_file_t) grub_zalloc (sizeof (*file));
+  if (! file)
+    return NULL;
+  file->device = device;
+  file->special = 1;
+
+  if ((fs->fs_envblk_open) (file) != GRUB_ERR_NONE) {
+    grub_free(file);
+    return NULL;
+  }
+
+  file->fs = fs;
+  grub_errno = GRUB_ERR_NONE;
+
+  return grub_apply_file_filters(file, type, NULL);
+}
+
 grub_disk_read_hook_t grub_file_progress_hook;
 
 grub_ssize_t
@@ -185,7 +206,10 @@ grub_file_read (grub_file_t file, void *buf, grub_size_t len)
       file->read_hook_data = file;
       file->progress_offset = file->offset;
     }
-  res = (file->fs->fs_read) (file, buf, len);
+  if (grub_file_envblk (file))
+    res = (file->fs->fs_envblk_read) (file, buf, len);
+  else
+    res = (file->fs->fs_read) (file, buf, len);
   file->read_hook = read_hook;
   file->read_hook_data = read_hook_data;
   if (res > 0)
@@ -197,7 +221,9 @@ grub_file_read (grub_file_t file, void *buf, grub_size_t len)
 grub_err_t
 grub_file_close (grub_file_t file)
 {
-  if (file->fs->fs_close)
+  if (grub_file_envblk (file) && file->fs->fs_envblk_close)
+    (file->fs->fs_envblk_close) (file);
+  else if (file->fs->fs_close)
     (file->fs->fs_close) (file);
 
   if (file->device)
diff --git a/include/grub/file.h b/include/grub/file.h
index 31567483c..1cc688f55 100644
--- a/include/grub/file.h
+++ b/include/grub/file.h
@@ -170,6 +170,9 @@ struct grub_file
 
   /* Caller-specific data passed to the read hook.  */
   void *read_hook_data;
+
+  /* If the file is an FS's special file, which uses separate functions for interaction. */
+  int special;
 };
 typedef struct grub_file *grub_file_t;
 
@@ -211,6 +214,7 @@ grub_ssize_t EXPORT_FUNC(grub_file_read) (grub_file_t file, void *buf,
 					  grub_size_t len);
 grub_off_t EXPORT_FUNC(grub_file_seek) (grub_file_t file, grub_off_t offset);
 grub_err_t EXPORT_FUNC(grub_file_close) (grub_file_t file);
+grub_file_t EXPORT_FUNC(grub_file_envblk_open) (grub_fs_t fs, grub_device_t device, enum grub_file_type type);
 
 /* Return value of grub_file_size() in case file size is unknown. */
 #define GRUB_FILE_SIZE_UNKNOWN	 0xffffffffffffffffULL
@@ -233,6 +237,12 @@ grub_file_seekable (const grub_file_t file)
   return !file->not_easily_seekable;
 }
 
+static inline int
+grub_file_envblk (const grub_file_t file)
+{
+  return file->special;
+}
+
 grub_file_t
 grub_file_offset_open (grub_file_t parent, enum grub_file_type type,
 		       grub_off_t start, grub_off_t size);
diff --git a/include/grub/fs.h b/include/grub/fs.h
index 302e48d4b..ec1a7a50e 100644
--- a/include/grub/fs.h
+++ b/include/grub/fs.h
@@ -96,6 +96,18 @@ struct grub_fs
   /* Whether blocklist installs have a chance to work.  */
   int blocklist_install;
 #endif
+
+  /*
+   * The envblk functions are defined on filesystems that need to handle
+   * grub-writable files in a special way. This is most commonly the case for
+   * CoW filesystems like btrfs and ZFS.  The normal read and close functions
+   * should detect that they are being called on a special file and act
+   * appropriately.
+   */
+  grub_err_t (*fs_envblk_open) (struct grub_file *file);
+  grub_ssize_t (*fs_envblk_read) (struct grub_file *file, char *buf, grub_size_t len);
+  grub_err_t (*fs_envblk_write) (struct grub_file *file, char *buf, grub_size_t len);
+  grub_err_t (*fs_envblk_close) (struct grub_file *file);
 };
 typedef struct grub_fs *grub_fs_t;
 
-- 
2.19.0

[PATCH 3/7] Factor out envblk buffer creation for reuse

[Paul Dagnelie]

This patch factors out the filling of the grubenv buffer into a separate
function for reuse.

Signed-off-by: Paul Dagnelie <pcd@delphix.com>
---
 include/grub/util/install.h |  3 +++
 util/editenv.c              | 26 +++++++++++++++++---------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/include/grub/util/install.h b/include/grub/util/install.h
index 2631b1074..2ca349503 100644
--- a/include/grub/util/install.h
+++ b/include/grub/util/install.h
@@ -246,6 +246,9 @@ grub_install_get_blocklist (grub_device_t root_dev,
 					      void *data),
 			    void *hook_data);
 
+void
+grub_util_create_envblk_buffer (char *, size_t);
+
 void
 grub_util_create_envblk_file (const char *name);
 
diff --git a/util/editenv.c b/util/editenv.c
index 81f68bd10..45aeba259 100644
--- a/util/editenv.c
+++ b/util/editenv.c
@@ -32,13 +32,29 @@
 #define DEFAULT_ENVBLK_SIZE	1024
 #define GRUB_ENVBLK_MESSAGE	"# WARNING: Do not edit this file by tools other than "PACKAGE"-editenv!!!\n"
 
+void
+grub_util_create_envblk_buffer (char *buf, size_t size)
+{
+  if (size < DEFAULT_ENVBLK_SIZE)
+    grub_util_error (_("Envblock buffer too small"));
+  char *pbuf;
+  pbuf = buf;
+  memcpy (pbuf, GRUB_ENVBLK_SIGNATURE, sizeof (GRUB_ENVBLK_SIGNATURE) - 1);
+  pbuf += sizeof (GRUB_ENVBLK_SIGNATURE) - 1;
+  memcpy (pbuf, GRUB_ENVBLK_MESSAGE, sizeof (GRUB_ENVBLK_MESSAGE) - 1);
+  pbuf += sizeof (GRUB_ENVBLK_MESSAGE) - 1;
+  memset (pbuf , '#',
+          size - sizeof (GRUB_ENVBLK_SIGNATURE) - sizeof (GRUB_ENVBLK_MESSAGE) + 2);
+}
+
 void
 grub_util_create_envblk_file (const char *name)
 {
   FILE *fp;
-  char *buf, *pbuf, *namenew;
+  char *buf, *namenew;
 
   buf = xmalloc (DEFAULT_ENVBLK_SIZE);
+  grub_util_create_envblk_buffer(buf, DEFAULT_ENVBLK_SIZE);
 
   namenew = xasprintf ("%s.new", name);
   fp = grub_util_fopen (namenew, "wb");
@@ -46,14 +62,6 @@ grub_util_create_envblk_file (const char *name)
     grub_util_error (_("cannot open `%s': %s"), namenew,
 		     strerror (errno));
 
-  pbuf = buf;
-  memcpy (pbuf, GRUB_ENVBLK_SIGNATURE, sizeof (GRUB_ENVBLK_SIGNATURE) - 1);
-  pbuf += sizeof (GRUB_ENVBLK_SIGNATURE) - 1;
-  memcpy (pbuf, GRUB_ENVBLK_MESSAGE, sizeof (GRUB_ENVBLK_MESSAGE) - 1);
-  pbuf += sizeof (GRUB_ENVBLK_MESSAGE) - 1;
-  memset (pbuf , '#',
-          DEFAULT_ENVBLK_SIZE - sizeof (GRUB_ENVBLK_SIGNATURE) - sizeof (GRUB_ENVBLK_MESSAGE) + 2);
-
   if (fwrite (buf, 1, DEFAULT_ENVBLK_SIZE, fp) != DEFAULT_ENVBLK_SIZE)
     grub_util_error (_("cannot write to `%s': %s"), namenew,
 		     strerror (errno));
-- 
2.19.0

[PATCH 4/7] Use envblk file and fs functions to implement reading the grubenv file from special FS regions

[Paul Dagnelie]

This patch modifies the loadenv command to take advantage of the new envblock
functions provided by a previous patch. These functions will be used instead
of the normal file-based functions if the boot filesystem is detected to
support them. There is also a config variable, grubenv_src, that can be used
to force the file or envblock logic to be used.

Signed-off-by: Paul Dagnelie <pcd@delphix.com>
---
 grub-core/commands/loadenv.c | 122 ++++++++++++++++++++++++++++++-----
 include/grub/lib/envblk.h    |   2 +
 2 files changed, 109 insertions(+), 15 deletions(-)

diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
index 3fd664aac..66f5345ec 100644
--- a/grub-core/commands/loadenv.c
+++ b/grub-core/commands/loadenv.c
@@ -79,6 +79,94 @@ open_envblk_file (char *filename,
   return file;
 }
 
+static grub_file_t
+open_envblk_block (grub_fs_t fs, grub_device_t dev, enum grub_file_type type)
+{
+  if (fs->fs_envblk_open)
+    return grub_file_envblk_open(fs, dev, type);
+  return NULL;
+}
+
+static grub_file_t
+open_envblk (grub_extcmd_context_t ctxt, enum grub_file_type type)
+{
+  struct grub_arg_list *state = ctxt->state;
+  grub_file_t file = NULL;
+  grub_device_t device = NULL;
+  const char *source;
+  grub_fs_t fs = NULL;
+  char *filename = state[0].set ? state[0].arg : NULL;
+
+  source = grub_env_get ("grubenv_src");
+  if (! source || ! grub_strcmp (source, GRUB_ENVBLK_SRC_BLK))
+    {
+      char *device_name;
+      const char *prefix;
+      grub_dprintf ("loadenv", "checking for envblk\n");
+
+      prefix = grub_env_get ("prefix");
+      if (! prefix)
+        {
+          grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("variable `%s' isn't set"), "prefix");
+          return NULL;
+        }
+
+      device_name = grub_file_get_device_name (prefix);
+      if (grub_errno)
+	return NULL;
+
+      device = grub_device_open (device_name);
+      if (! device)
+	return NULL;
+
+      fs = grub_fs_probe (device);
+      if (! fs) {
+        grub_device_close (device);
+	return NULL;
+      }
+
+      /* We have to reopen the device here because it was closed in grub_fs_probe. */
+      device = grub_device_open (device_name);
+      grub_free (device_name);
+      if (! device)
+	return NULL;
+
+    }
+  if (! source)
+    {
+      if (fs->fs_envblk_open)
+	{
+	  source = GRUB_ENVBLK_SRC_BLK;
+	  grub_dprintf ("loadenv", "envblk support detected\n");
+	}
+      else
+	{
+	  source = GRUB_ENVBLK_SRC_FILE;
+	  grub_dprintf ("loadenv", "envblk support not detected\n");
+	}
+    }
+
+  if (! grub_strcmp (source, GRUB_ENVBLK_SRC_FILE))
+    {
+      if (device)
+        grub_device_close (device);
+      file = open_envblk_file (filename, type);
+
+      if (! file)
+	return NULL;
+    }
+  else if (! grub_strcmp (source, GRUB_ENVBLK_SRC_BLK))
+    {
+      file = open_envblk_block (fs, device, type);
+      if (! file)
+	{
+	  grub_device_close (device);
+	  return NULL;
+	}
+    }
+  return file;
+}
+
 static grub_envblk_t
 read_envblk_file (grub_file_t file)
 {
@@ -165,11 +253,10 @@ grub_cmd_load_env (grub_extcmd_context_t ctxt, int argc, char **args)
   whitelist.len = argc;
   whitelist.list = args;
 
-  /* state[0] is the -f flag; state[1] is the --skip-sig flag */
-  file = open_envblk_file ((state[0].set) ? state[0].arg : 0,
-			   GRUB_FILE_TYPE_LOADENV
-			   | (state[1].set
-			      ? GRUB_FILE_TYPE_SKIP_SIGNATURE : GRUB_FILE_TYPE_NONE));
+  file = open_envblk (ctxt, GRUB_FILE_TYPE_LOADENV
+                            | (state[1].set
+			       ? GRUB_FILE_TYPE_SKIP_SIGNATURE :
+			       GRUB_FILE_TYPE_NONE));
   if (! file)
     return grub_errno;
 
@@ -204,10 +291,9 @@ grub_cmd_list_env (grub_extcmd_context_t ctxt,
   grub_file_t file;
   grub_envblk_t envblk;
 
-  file = open_envblk_file ((state[0].set) ? state[0].arg : 0,
-			   GRUB_FILE_TYPE_LOADENV
-			   | (state[1].set
-			      ? GRUB_FILE_TYPE_SKIP_SIGNATURE : GRUB_FILE_TYPE_NONE));
+  file = open_envblk (ctxt, GRUB_FILE_TYPE_LOADENV
+			    | (state[1].set
+			       ? GRUB_FILE_TYPE_SKIP_SIGNATURE : GRUB_FILE_TYPE_NONE));
   if (! file)
     return grub_errno;
 
@@ -379,7 +465,6 @@ save_env_read_hook (grub_disk_addr_t sector, unsigned offset, unsigned length,
 static grub_err_t
 grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
 {
-  struct grub_arg_list *state = ctxt->state;
   grub_file_t file;
   grub_envblk_t envblk;
   struct grub_cmd_save_env_ctx ctx = {
@@ -390,9 +475,8 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
   if (! argc)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "no variable is specified");
 
-  file = open_envblk_file ((state[0].set) ? state[0].arg : 0,
-			   GRUB_FILE_TYPE_SAVEENV
-			   | GRUB_FILE_TYPE_SKIP_SIGNATURE);
+  file = open_envblk (ctxt, GRUB_FILE_TYPE_SAVEENV
+			    | GRUB_FILE_TYPE_SKIP_SIGNATURE);
   if (! file)
     return grub_errno;
 
@@ -409,7 +493,7 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
   if (! envblk)
     goto fail;
 
-  if (check_blocklists (envblk, ctx.head, file))
+  if (! grub_file_envblk (file) && check_blocklists (envblk, ctx.head, file))
     goto fail;
 
   while (argc)
@@ -432,7 +516,15 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
       args++;
     }
 
-  write_blocklists (envblk, ctx.head, file);
+  if (grub_file_envblk (file))
+    {
+      grub_file_seek (file, 0);
+      file->fs->fs_envblk_write (file, grub_envblk_buffer (envblk), grub_envblk_size (envblk));
+    }
+  else
+    {
+      write_blocklists (envblk, ctx.head, file);
+    }
 
  fail:
   if (envblk)
diff --git a/include/grub/lib/envblk.h b/include/grub/lib/envblk.h
index c3e655921..287c87105 100644
--- a/include/grub/lib/envblk.h
+++ b/include/grub/lib/envblk.h
@@ -21,6 +21,8 @@
 
 #define GRUB_ENVBLK_SIGNATURE	"# GRUB Environment Block\n"
 #define GRUB_ENVBLK_DEFCFG	"grubenv"
+#define GRUB_ENVBLK_SRC_BLK	"block"
+#define GRUB_ENVBLK_SRC_FILE	"file"
 
 #ifndef ASM_FILE
 
-- 
2.19.0

[PATCH 5/7] Add ZFS envblock functions

[Paul Dagnelie]

This patch adds a ZFS implementation of the fs_envblk_* functions. These
functions will be used to load the grubenv block from a padding area in the
label of ZFS. This padding area is protected by an embedded checksum, and
multiple copies are stored on each disk. This should provide sufficient
reliability for most use cases. Since this area is not part of the block tree,
it can be easily modified at boot time with a simple recalculation of the
embedded checksum, enabling save_env to work for ZFS boot filesystems.

Note that the open() function is doing the read; this is because the ZFS
envblk logic doesn't store the file size, so the only way to retrieve it is to
determine the length of the file using strlen. If this is considered too poor
of an approach to be acceptable, format changes can still be made to the
structure of the padding area.

Signed-off-by: Paul Dagnelie <pcd@delphix.com>
---
 grub-core/fs/zfs/zfs.c       | 134 ++++++++++++++++++++++++++++++++---
 include/grub/zfs/vdev_impl.h |  33 +++++----
 2 files changed, 139 insertions(+), 28 deletions(-)

diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
index 2f72e42bf..d741426cd 100644
--- a/grub-core/fs/zfs/zfs.c
+++ b/grub-core/fs/zfs/zfs.c
@@ -30,6 +30,7 @@
  *
  */
 
+#include <stddef.h>
 #include <grub/err.h>
 #include <grub/file.h>
 #include <grub/mm.h>
@@ -1146,7 +1147,6 @@ scan_disk (grub_device_t dev, struct grub_zfs_data *data,
 {
   int label = 0;
   uberblock_phys_t *ub_array, *ubbest = NULL;
-  vdev_boot_header_t *bh;
   grub_err_t err;
   int vdevnum;
   struct grub_zfs_device_desc desc;
@@ -1155,13 +1155,6 @@ scan_disk (grub_device_t dev, struct grub_zfs_data *data,
   if (!ub_array)
     return grub_errno;
 
-  bh = grub_malloc (VDEV_BOOT_HEADER_SIZE);
-  if (!bh)
-    {
-      grub_free (ub_array);
-      return grub_errno;
-    }
-
   vdevnum = VDEV_LABELS;
 
   desc.dev = dev;
@@ -1175,7 +1168,7 @@ scan_disk (grub_device_t dev, struct grub_zfs_data *data,
     {
       desc.vdev_phys_sector
 	= label * (sizeof (vdev_label_t) >> SPA_MINBLOCKSHIFT)
-	+ ((VDEV_SKIP_SIZE + VDEV_BOOT_HEADER_SIZE) >> SPA_MINBLOCKSHIFT)
+	+ ((VDEV_PAD_SIZE * 2) >> SPA_MINBLOCKSHIFT)
 	+ (label < VDEV_LABELS / 2 ? 0 : 
 	   ALIGN_DOWN (grub_disk_get_size (dev->disk), sizeof (vdev_label_t))
 	   - VDEV_LABELS * (sizeof (vdev_label_t) >> SPA_MINBLOCKSHIFT));
@@ -1184,6 +1177,7 @@ scan_disk (grub_device_t dev, struct grub_zfs_data *data,
       err = grub_disk_read (dev->disk, desc.vdev_phys_sector
 			    + (VDEV_PHYS_SIZE >> SPA_MINBLOCKSHIFT),
 			    0, VDEV_UBERBLOCK_RING, (char *) ub_array);
+
       if (err)
 	{
 	  grub_errno = GRUB_ERR_NONE;
@@ -1219,12 +1213,10 @@ scan_disk (grub_device_t dev, struct grub_zfs_data *data,
 	continue;
 #endif
       grub_free (ub_array);
-      grub_free (bh);
       return GRUB_ERR_NONE;
     }
   
   grub_free (ub_array);
-  grub_free (bh);
 
   return grub_error (GRUB_ERR_BAD_FS, "couldn't find a valid label");
 }
@@ -3765,6 +3757,60 @@ zfs_mtime (grub_device_t device, grub_int32_t *mt)
   return GRUB_ERR_NONE;
 }
 
+static grub_err_t
+zfs_devs_read_zbt (struct grub_zfs_data *data, grub_uint64_t offset, char *buf, grub_size_t len)
+{
+  grub_err_t err = GRUB_ERR_NONE;
+  zio_cksum_t zc;
+  unsigned int i;
+  ZIO_SET_CHECKSUM(&zc, offset, 0, 0, 0);
+
+  for (i = 0; i < data->n_devices_attached; i++)
+    {
+      err = grub_disk_read (data->devices_attached[i].dev->disk,
+			    offset >> SPA_MINBLOCKSHIFT,
+			    offset & ((1 << SPA_MINBLOCKSHIFT) - 1),
+			    len, buf);
+      if (err)
+	continue;
+      ZIO_SET_CHECKSUM(&zc, offset, 0, 0, 0);
+      err = zio_checksum_verify (zc, ZIO_CHECKSUM_LABEL, GRUB_ZFS_LITTLE_ENDIAN,
+				 buf, len);
+      if (!err)
+	return GRUB_ERR_NONE;
+    }
+  return err;
+}
+
+static grub_err_t
+grub_zfs_envblk_open (struct grub_file *file)
+{
+  grub_err_t err;
+  struct grub_zfs_data *data;
+  vdev_boot_envblock_t *vbe;
+  int l;
+
+  file->offset = 0;
+  data = zfs_mount (file->device);
+  file->data = data;
+  data->file_buf = grub_malloc (sizeof (vdev_boot_envblock_t));
+  for (l = 0; l < VDEV_LABELS / 2; l++)
+    {
+      grub_uint64_t offset = l * sizeof (vdev_label_t) + offsetof (vdev_label_t, vl_be);
+
+      err = zfs_devs_read_zbt (data, offset, data->file_buf,
+			       sizeof (vdev_boot_envblock_t));
+      if (err == GRUB_ERR_NONE)
+	{
+	  vbe = (vdev_boot_envblock_t *)data->file_buf;
+	  file->size = grub_strlen (vbe->vbe_bootenv);
+	  return err;
+	}
+    }
+  zfs_unmount (data);
+  return err;
+}
+
 /*
  * zfs_open() locates a file in the rootpool by following the
  * MOS and places the dnode of the file in the memory address DNODE.
@@ -3850,6 +3896,19 @@ grub_zfs_open (struct grub_file *file, const char *fsfilename)
   return GRUB_ERR_NONE;
 }
 
+static grub_ssize_t
+grub_zfs_envblk_read (grub_file_t file, char *buf, grub_size_t len)
+{
+  struct grub_zfs_data *data = (struct grub_zfs_data *) file->data;
+  grub_ssize_t olen = len;
+  grub_uint64_t offset = file->offset + offsetof (vdev_boot_envblock_t, vbe_bootenv);
+
+  if (len + file->offset > file->size)
+    olen = file->size - file->offset;
+  grub_memmove (buf, data->file_buf + offset, olen);
+  return olen;
+}
+
 static grub_ssize_t
 grub_zfs_read (grub_file_t file, char *buf, grub_size_t len)
 {
@@ -3859,6 +3918,9 @@ grub_zfs_read (grub_file_t file, char *buf, grub_size_t len)
   grub_size_t read;
   grub_err_t err;
 
+  if (grub_file_envblk (file))
+	  return grub_zfs_envblk_read (file, buf, len);
+
   /*
    * If offset is in memory, move it into the buffer provided and return.
    */
@@ -3924,6 +3986,52 @@ grub_zfs_read (grub_file_t file, char *buf, grub_size_t len)
   return len;
 }
 
+static grub_err_t
+grub_zfs_envblk_write (struct grub_file *file, char *buf, grub_size_t len)
+{
+  grub_uint64_t offset = file->offset + offsetof (vdev_boot_envblock_t, vbe_bootenv);
+  grub_err_t err = GRUB_ERR_NONE;
+  struct grub_zfs_data *data = file->data;
+  unsigned int i, l;
+  zio_cksum_t cksum;
+  vdev_boot_envblock_t *vbe = (vdev_boot_envblock_t *)data->file_buf;
+
+  if (len + file->offset > file->size)
+    return GRUB_ERR_OUT_OF_RANGE;
+
+  grub_memmove (data->file_buf + offset, buf, len);
+
+  for (l = 0; l < VDEV_LABELS / 2; l++)
+    {
+      offset = l * sizeof (vdev_label_t) + offsetof (vdev_label_t, vl_be);
+      vbe->vbe_zbt.zec_magic = ZEC_MAGIC;
+      ZIO_SET_CHECKSUM(&vbe->vbe_zbt.zec_cksum, offset, 0, 0, 0);
+      vbe->vbe_zbt.zec_magic = ZEC_MAGIC;
+      zio_checksum_SHA256(vbe, VDEV_PAD_SIZE, GRUB_ZFS_LITTLE_ENDIAN, &cksum);
+      vbe->vbe_zbt.zec_cksum = cksum;
+
+      for (i = 0; i < data->n_devices_attached; i++)
+	{
+	  struct grub_zfs_device_desc *desc = &data->devices_attached[i];
+	  int num_sectors = VDEV_PAD_SIZE >> desc->ashift;
+	  int label_seek = l * sizeof (vdev_label_t) >> desc->ashift;
+	  int j;
+
+	  for (j = 0; j < num_sectors; j++)
+	    {
+	      grub_err_t err2;
+
+	      err2 = grub_disk_write (desc->dev->disk, label_seek + num_sectors + j, 0,
+				      1 << desc->ashift,
+				      data->file_buf + (j << desc->ashift));
+	      if (err2 != GRUB_ERR_NONE)
+		err = err2;
+	    }
+	}
+    }
+  return err;
+}
+
 static grub_err_t
 grub_zfs_close (grub_file_t file)
 {
@@ -4360,6 +4468,10 @@ static struct grub_fs grub_zfs_fs = {
   .reserved_first_sector = 1,
   .blocklist_install = 0,
 #endif
+  .fs_envblk_open = grub_zfs_envblk_open,
+  .fs_envblk_read = grub_zfs_read,
+  .fs_envblk_write = grub_zfs_envblk_write,
+  .fs_envblk_close = grub_zfs_close,
   .next = 0
 };
 
diff --git a/include/grub/zfs/vdev_impl.h b/include/grub/zfs/vdev_impl.h
index 9b5f0a7a8..48ec59003 100644
--- a/include/grub/zfs/vdev_impl.h
+++ b/include/grub/zfs/vdev_impl.h
@@ -23,34 +23,33 @@
 #ifndef _SYS_VDEV_IMPL_H
 #define	_SYS_VDEV_IMPL_H
 
-#define	VDEV_SKIP_SIZE		(8 << 10)
-#define	VDEV_BOOT_HEADER_SIZE	(8 << 10)
+#define	VDEV_PAD_SIZE		(8 << 10)
 #define	VDEV_PHYS_SIZE		(112 << 10)
 #define	VDEV_UBERBLOCK_RING	(128 << 10)
 
-/* ZFS boot block */
-#define	VDEV_BOOT_MAGIC		0x2f5b007b10cULL
-#define	VDEV_BOOT_VERSION	1		/* version number	*/
-
-typedef struct vdev_boot_header {
-	grub_uint64_t	vb_magic;		/* VDEV_BOOT_MAGIC	*/
-	grub_uint64_t	vb_version;		/* VDEV_BOOT_VERSION	*/
-	grub_uint64_t	vb_offset;		/* start offset	(bytes) */
-	grub_uint64_t	vb_size;		/* size (bytes)		*/
-	char		vb_pad[VDEV_BOOT_HEADER_SIZE - 4 * sizeof (grub_uint64_t)];
-} vdev_boot_header_t;
-
 typedef struct vdev_phys {
 	char		vp_nvlist[VDEV_PHYS_SIZE - sizeof (zio_eck_t)];
 	zio_eck_t	vp_zbt;
 } vdev_phys_t;
 
+typedef enum vbe_vers {
+	VB_RAW,
+	VB_NVLIST
+} vb_evers_t;
+
+typedef struct vdev_boot_envblock {
+	grub_uint64_t	vbe_version;
+	char		vbe_bootenv[VDEV_PAD_SIZE - sizeof (grub_uint64_t) -
+			sizeof (zio_eck_t)];
+	zio_eck_t	vbe_zbt;
+} vdev_boot_envblock_t;
+
 typedef struct vdev_label {
-	char		vl_pad[VDEV_SKIP_SIZE];			/*   8K	*/
-	vdev_boot_header_t vl_boot_header;			/*   8K	*/
+	char		vl_pad1[VDEV_PAD_SIZE];			/*  8K */
+	vdev_boot_envblock_t	vl_be;				/*  8K */
 	vdev_phys_t	vl_vdev_phys;				/* 112K	*/
 	char		vl_uberblock[VDEV_UBERBLOCK_RING];	/* 128K	*/
-} vdev_label_t;							/* 256K total */
+} vdev_label_t;						/* 256K total */
 
 /*
  * Size and offset of embedded boot loader region on each label.
-- 
2.19.0

[PATCH 6/7] Refactor default envblk size out for reuse

[Paul Dagnelie]

This patch refactors the DEFAULT_ENVBLK_SIZE definition into a header file for
reuse in other code, and renames it to match other definitions in that header.

Signed-off-by: Paul Dagnelie <pcd@delphix.com>
---
 include/grub/lib/envblk.h | 1 +
 util/editenv.c            | 9 ++++-----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/grub/lib/envblk.h b/include/grub/lib/envblk.h
index 287c87105..b9067dc37 100644
--- a/include/grub/lib/envblk.h
+++ b/include/grub/lib/envblk.h
@@ -23,6 +23,7 @@
 #define GRUB_ENVBLK_DEFCFG	"grubenv"
 #define GRUB_ENVBLK_SRC_BLK	"block"
 #define GRUB_ENVBLK_SRC_FILE	"file"
+#define GRUB_ENVBLK_DEFAULT_SIZE      1024
 
 #ifndef ASM_FILE
 
diff --git a/util/editenv.c b/util/editenv.c
index 45aeba259..ef1aba25b 100644
--- a/util/editenv.c
+++ b/util/editenv.c
@@ -29,13 +29,12 @@
 #include <errno.h>
 #include <string.h>
 
-#define DEFAULT_ENVBLK_SIZE	1024
 #define GRUB_ENVBLK_MESSAGE	"# WARNING: Do not edit this file by tools other than "PACKAGE"-editenv!!!\n"
 
 void
 grub_util_create_envblk_buffer (char *buf, size_t size)
 {
-  if (size < DEFAULT_ENVBLK_SIZE)
+  if (size < GRUB_ENVBLK_DEFAULT_SIZE)
     grub_util_error (_("Envblock buffer too small"));
   char *pbuf;
   pbuf = buf;
@@ -53,8 +52,8 @@ grub_util_create_envblk_file (const char *name)
   FILE *fp;
   char *buf, *namenew;
 
-  buf = xmalloc (DEFAULT_ENVBLK_SIZE);
-  grub_util_create_envblk_buffer(buf, DEFAULT_ENVBLK_SIZE);
+  buf = xmalloc (GRUB_ENVBLK_DEFAULT_SIZE);
+  grub_util_create_envblk_buffer(buf, GRUB_ENVBLK_DEFAULT_SIZE);
 
   namenew = xasprintf ("%s.new", name);
   fp = grub_util_fopen (namenew, "wb");
@@ -62,7 +61,7 @@ grub_util_create_envblk_file (const char *name)
     grub_util_error (_("cannot open `%s': %s"), namenew,
 		     strerror (errno));
 
-  if (fwrite (buf, 1, DEFAULT_ENVBLK_SIZE, fp) != DEFAULT_ENVBLK_SIZE)
+  if (fwrite (buf, 1, GRUB_ENVBLK_DEFAULT_SIZE, fp) != GRUB_ENVBLK_DEFAULT_SIZE)
     grub_util_error (_("cannot write to `%s': %s"), namenew,
 		     strerror (errno));
 
-- 
2.19.0

[PATCH 7/7] Update editenv to support editing zfs envblock, fix whitespace, and autodetect bootenv support in libzfs

[Paul Dagnelie]

This patch updates the grub-editenv command to support editing the zfs
envblock by using specific libzfs functions. In order to ensure that GRUB will
continue to build against both old and new versions of libzfs, logic was also
added to the configure script to detect if the bootenv functions are present
in the version of libzfs present at build time.  There is also a small
whitespace fix in grub-editenv unrelated to the content of these changes.

Signed-off-by: Paul Dagnelie <pcd@delphix.com>
---
 configure.ac               |   8 ++
 include/grub/util/libzfs.h |   7 +
 util/grub-editenv.c        | 278 +++++++++++++++++++++++++++++++++++--
 3 files changed, 284 insertions(+), 9 deletions(-)

diff --git a/configure.ac b/configure.ac
index 7d74eba66..7107cc3b8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1853,6 +1853,14 @@ if test x"$libzfs_excuse" = x ; then
   LIBNVPAIR="-lnvpair"
   AC_DEFINE([HAVE_LIBNVPAIR], [1],
             [Define to 1 if you have the NVPAIR library.])
+
+  AC_CHECK_LIB([zfs], [zpool_get_bootenv],
+               [libzfs_have_bootenv="yes"],
+	       [])
+  if test x"$libzfs_have_bootenv" = xyes ; then
+    AC_DEFINE([HAVE_LIBZFS_BOOTENV], [1],
+              [Define to 1 if you have a version of the ZFS library with bootenv support.])
+  fi
 fi
 
 AC_SUBST([LIBZFS])
diff --git a/include/grub/util/libzfs.h b/include/grub/util/libzfs.h
index a02caa335..5ebc085d8 100644
--- a/include/grub/util/libzfs.h
+++ b/include/grub/util/libzfs.h
@@ -40,6 +40,13 @@ extern int zpool_get_physpath (zpool_handle_t *, const char *);
 
 extern nvlist_t *zpool_get_config (zpool_handle_t *, nvlist_t **);
 
+extern libzfs_handle_t *zpool_get_handle (zpool_handle_t *);
+
+extern int zpool_set_bootenv (zpool_handle_t *, const char *);
+extern int zpool_get_bootenv (zpool_handle_t *, char *, size_t, off_t);
+
+extern int libzfs_errno (libzfs_handle_t *);
+
 #endif /* ! HAVE_LIBZFS_H */
 
 libzfs_handle_t *grub_get_libzfs_handle (void);
diff --git a/util/grub-editenv.c b/util/grub-editenv.c
index f3662c95b..a282f45ee 100644
--- a/util/grub-editenv.c
+++ b/util/grub-editenv.c
@@ -24,7 +24,12 @@
 #include <grub/lib/envblk.h>
 #include <grub/i18n.h>
 #include <grub/emu/hostfile.h>
+#include <grub/emu/hostdisk.h>
 #include <grub/util/install.h>
+#include <grub/util/libzfs.h>
+#include <grub/emu/getroot.h>
+#include <grub/fs.h>
+#include <grub/crypto.h>
 
 #include <stdio.h>
 #include <unistd.h>
@@ -36,7 +41,6 @@
 #pragma GCC diagnostic error "-Wmissing-prototypes"
 #pragma GCC diagnostic error "-Wmissing-declarations"
 
-
 #include "progname.h"
 
 #define DEFAULT_ENVBLK_PATH DEFAULT_DIRECTORY "/" GRUB_ENVBLK_DEFCFG
@@ -120,6 +124,79 @@ block, use `rm %s'."),
   NULL, help_filter, NULL
 };
 
+struct fs_envblk_spec {
+  const char *fs_name;
+  int (*fs_read) (void *, char *, size_t, off_t);
+  int (*fs_write) (void *, const char *);
+  void *(*fs_init) (grub_device_t);
+  void (*fs_fini) (void *);
+};
+typedef struct fs_envblk_spec fs_envblk_spec_t;
+
+struct fs_envblk {
+  struct fs_envblk_spec *spec;
+  grub_fs_t fs;
+  void *data;
+};
+typedef struct fs_envblk fs_envblk_t;
+
+fs_envblk_t *fs_envblk = NULL;
+
+#if defined(HAVE_LIBZFS) && defined(HAVE_LIBNVPAIR) && defined(HAVE_LIBZFS_BOOTENV)
+static void *
+grub_zfs_init (grub_device_t dev)
+{
+  libzfs_handle_t *g_zfs = libzfs_init();
+  int err;
+  char *name;
+  zpool_handle_t *zhp;
+
+  if (g_zfs == NULL)
+    return NULL;
+
+  err = fs_envblk->fs->fs_label(dev, &name);
+  if (err != GRUB_ERR_NONE) {
+    libzfs_fini(g_zfs);
+    return NULL;
+  }
+  zhp = zpool_open(g_zfs, name);
+  if (zhp == NULL)
+    {
+      libzfs_fini(g_zfs);
+      return NULL;
+    }
+  return zhp;
+}
+
+static void
+grub_zfs_fini (void *arg)
+{
+  zpool_handle_t *zhp = arg;
+  libzfs_handle_t *g_zfs = zpool_get_handle(zhp);
+  zpool_close(zhp);
+  libzfs_fini(g_zfs);
+}
+
+/* We need to convert ZFS's error returning pattern to the one we expect */
+static int
+grub_zfs_get_bootenv (void *arg, char *buf, size_t size, off_t offset)
+{
+  zpool_handle_t *zhp = arg;
+  int error = zpool_get_bootenv (zhp, buf, size, offset);
+  if (error != -1)
+    return error;
+  error = libzfs_errno(zpool_get_handle(zhp));
+  return error;
+}
+#endif
+
+fs_envblk_spec_t fs_envblk_table[] = {
+#if defined(HAVE_LIBZFS) && defined(HAVE_LIBNVPAIR) && defined(HAVE_LIBZFS_BOOTENV)
+  { "zfs", grub_zfs_get_bootenv, zpool_set_bootenv, grub_zfs_init, grub_zfs_fini},
+#endif
+  { NULL, NULL, NULL, NULL, NULL }
+};
+
 static grub_envblk_t
 open_envblk_file (const char *name)
 {
@@ -164,6 +241,58 @@ open_envblk_file (const char *name)
   return envblk;
 }
 
+static grub_envblk_t
+open_envblk (const char *name)
+{
+  char *buf = NULL;
+  off_t off = 0;
+  size_t size = GRUB_ENVBLK_DEFAULT_SIZE;
+  grub_envblk_t envblk;
+
+  if (fs_envblk == NULL)
+    return open_envblk_file(name);
+
+  /*
+   * For normal grubenv files, we can just use the size of the file to
+   * allocate our buffer, but envblks don't necessarily advertise their size
+   * directly. We need to instead read and realloc a larger buffer until we
+   * have read in the entire file.
+   */
+  while (1)
+    {
+      int rc;
+
+      buf = xrealloc(buf, size);
+      rc = fs_envblk->spec->fs_read(fs_envblk->data, buf + off, size, off);
+      if (rc < 0)
+	{
+	  grub_util_error (_("cannot read envblock: %s"), strerror (errno));
+	  free(buf);
+	  return NULL;
+	}
+      if (rc < size)
+	{
+	  envblk = grub_envblk_open (buf, rc + off);
+	  if (! envblk)
+	    grub_util_error ("%s", _("invalid environment block"));
+	  return envblk;
+	}
+      off += size;
+      size *= 2;
+    }
+}
+
+static void
+close_envblk (grub_envblk_t envblk)
+{
+  grub_envblk_close (envblk);
+  if (fs_envblk != NULL)
+    {
+      fs_envblk->spec->fs_fini (fs_envblk->data);
+      fs_envblk->data = NULL;
+    }
+}
+
 static int
 print_var (const char *varname, const char *value,
            void *hook_data __attribute__ ((unused)))
@@ -177,13 +306,13 @@ list_variables (const char *name)
 {
   grub_envblk_t envblk;
 
-  envblk = open_envblk_file (name);
+  envblk = open_envblk (name);
   grub_envblk_iterate (envblk, NULL, print_var);
-  grub_envblk_close (envblk);
+  close_envblk (envblk);
 }
 
 static void
-write_envblk (const char *name, grub_envblk_t envblk)
+write_envblk_file (const char *name, grub_envblk_t envblk)
 {
   FILE *fp;
 
@@ -202,12 +331,38 @@ write_envblk (const char *name, grub_envblk_t envblk)
   fclose (fp);
 }
 
+static void
+write_envblk (const char *name, grub_envblk_t envblk)
+{
+  if (fs_envblk == NULL)
+    return write_envblk_file(name, envblk);
+
+  if (fs_envblk->spec->fs_write (fs_envblk->data, grub_envblk_buffer (envblk)) != 0)
+    grub_util_error (_("cannot write to envblock: %s"), strerror (errno));
+}
+
+static void
+create_envblk (const char *name)
+{
+  char *buf;
+  grub_envblk_t envblk;
+
+  if (fs_envblk == NULL)
+    grub_util_create_envblk_file (name);
+  buf = xmalloc (GRUB_ENVBLK_DEFAULT_SIZE);
+  grub_util_create_envblk_buffer(buf, GRUB_ENVBLK_DEFAULT_SIZE);
+
+  envblk = grub_envblk_open (buf, GRUB_ENVBLK_DEFAULT_SIZE);
+  write_envblk (name, envblk);
+  close_envblk (envblk);
+}
+
 static void
 set_variables (const char *name, int argc, char *argv[])
 {
   grub_envblk_t envblk;
 
-  envblk = open_envblk_file (name);
+  envblk = open_envblk (name);
   while (argc)
     {
       char *p;
@@ -226,7 +381,7 @@ set_variables (const char *name, int argc, char *argv[])
     }
 
   write_envblk (name, envblk);
-  grub_envblk_close (envblk);
+  close_envblk (envblk);
 }
 
 static void
@@ -234,7 +389,7 @@ unset_variables (const char *name, int argc, char *argv[])
 {
   grub_envblk_t envblk;
 
-  envblk = open_envblk_file (name);
+  envblk = open_envblk (name);
   while (argc)
     {
       grub_envblk_delete (envblk, argv[0]);
@@ -244,7 +399,109 @@ unset_variables (const char *name, int argc, char *argv[])
     }
 
   write_envblk (name, envblk);
-  grub_envblk_close (envblk);
+  close_envblk (envblk);
+}
+
+static int
+probe_abstraction (grub_disk_t disk)
+{
+  if (disk->partition == NULL)
+    grub_util_info ("no partition map found for %s", disk->name);
+
+  if (disk->dev->id == GRUB_DISK_DEVICE_DISKFILTER_ID ||
+      disk->dev->id == GRUB_DISK_DEVICE_CRYPTODISK_ID)
+    return 1;
+  return 0;
+}
+
+static fs_envblk_t *
+probe_fs_envblk (fs_envblk_spec_t *spec)
+{
+  char **grub_devices;
+  char **curdev, **curdrive;
+  size_t ndev = 0;
+  char **grub_drives;
+  grub_device_t grub_dev = NULL;
+  grub_fs_t grub_fs;
+  const char *fs_envblk_device;
+  int have_abstraction = 0;
+  fs_envblk_spec_t *p;
+
+  grub_util_biosdisk_init (DEFAULT_DEVICE_MAP);
+  grub_init_all ();
+  grub_gcry_init_all ();
+
+  grub_lvm_fini ();
+  grub_mdraid09_fini ();
+  grub_mdraid1x_fini ();
+  grub_diskfilter_fini ();
+  grub_diskfilter_init ();
+  grub_mdraid09_init ();
+  grub_mdraid1x_init ();
+  grub_lvm_init ();
+
+  grub_devices = grub_guess_root_devices (DEFAULT_DIRECTORY);
+
+  if (!grub_devices || !grub_devices[0])
+    grub_util_error (_("cannot find a device for %s (is /dev mounted?)"), DEFAULT_DIRECTORY);
+
+  fs_envblk_device = grub_devices[0];
+
+  for (curdev = grub_devices; *curdev; curdev++)
+    {
+      grub_util_pull_device (*curdev);
+      ndev++;
+    }
+
+  grub_drives = xmalloc (sizeof (grub_drives[0]) * (ndev + 1));
+
+  for (curdev = grub_devices, curdrive = grub_drives; *curdev; curdev++,
+       curdrive++)
+    {
+      *curdrive = grub_util_get_grub_dev (*curdev);
+      if (! *curdrive)
+	grub_util_error (_("cannot find a GRUB drive for %s.  Check your device.map"),
+			 *curdev);
+    }
+  *curdrive = NULL;
+
+  grub_dev = grub_device_open (grub_drives[0]);
+
+  grub_fs = grub_fs_probe (grub_dev);
+
+  if (grub_dev->disk)
+    have_abstraction = probe_abstraction (grub_dev->disk);
+  for (curdrive = grub_drives + 1; *curdrive; curdrive++)
+    {
+      grub_device_t dev = grub_device_open (*curdrive);
+      if (!dev)
+	continue;
+      if (dev->disk)
+	have_abstraction |= probe_abstraction (dev->disk);
+      grub_device_close (dev);
+    }
+
+  free (grub_drives);
+  grub_gcry_fini_all ();
+  grub_fini_all ();
+  grub_util_biosdisk_fini ();
+
+  for (p = spec; p->fs_name; p++)
+    {
+      if (strcmp (grub_fs->name, p->fs_name) == 0 && !have_abstraction)
+	{
+	  grub_util_info ("Detected envblock support in %s, leveraging", grub_fs->name);
+	  fs_envblk = xmalloc (sizeof (fs_envblk_t));
+	  fs_envblk->spec = p;
+	  fs_envblk->fs = grub_fs;
+	  fs_envblk->data = p->fs_init (grub_dev);
+	  grub_device_close (grub_dev);
+	  return fs_envblk;
+	}
+    }
+
+  grub_device_close (grub_dev);
+  return NULL;
 }
 
 int
@@ -278,8 +535,11 @@ main (int argc, char *argv[])
       command  = argv[curindex++];
     }
 
+  if (strcmp (filename, DEFAULT_ENVBLK_PATH) == 0)
+    fs_envblk = probe_fs_envblk (fs_envblk_table);
+
   if (strcmp (command, "create") == 0)
-    grub_util_create_envblk_file (filename);
+    create_envblk (filename);
   else if (strcmp (command, "list") == 0)
     list_variables (filename);
   else if (strcmp (command, "set") == 0)
-- 
2.19.0

Re: [PATCH 0/7] ZFS/other CoW FS save_env support

[Daniel Kiper]

Hi Paul,

On Wed, Mar 11, 2020 at 10:37:08AM -0700, Paul Dagnelie wrote:
> Hey all, I previously discussed my concept for this patch in my email
> https://lists.gnu.org/archive/html/grub-devel/2020-01/msg00004.html .
> I'm pleased to announce that I've gotten it into a working state and
> it is ready to review.  There are a number of changes here, which I
> will break down below.
>
> First, the concept of "special files" is introduced into grub. These
> are files that are manipulated using different functions from the
> remainder of the filesystem. A filesystem advertises its support for
> these files by filling in new entries in the grub_fs struct.
>
> Second, the loadenv command was modified to use the special file
> functions of the root filesystem if they are detected. If that
> happens, these functions are used to open, read, and write to the
> loadenv file instead of the normal grub file functions.  A variable
> was also added that allows the user to force a file to be used instead
> of the special files, or vice versa.
>
> Third, the grub-editenv command was modified to teach it to probe the
> root filesystem and then check in an array of structures that contain
> functions that will read and modify the filesystem's special file in
> userland. These functions are very similar to normal read and write
> functions, and so don't result in a very different code flow.
>
> Finally, the GRUB ZFS logic was modified to have new special_file
> functions. These functions manipulate the pad2 area of the ZFS label,
> which was previously unused. It now stores a version number, the raw
> contents of the grubenv file, and an embedded checksum for integrity
> purposes. GRUB was taught to read and verify these areas, and also to
> modify them, computing the embeddded checksum appropriately.  In
> addition, if GRUB is build with libzfs support, an entry is added to
> the grub-editenv table that points GRUB to the appropriate libzfs
> functions, and init and fini functions are built into grub-editenv
> itself.
>
> Future work:
> * In the ZFS code could store a packed nvlist instead of a raw file,
> but this would involve further changes to the grub environment file
> code and was deferred for when it would be more useful and important.
> * For the special files code, there is only one special file allowed
> per filesystem, but a specification interface (using either an enum or
> a set of names) could be added in the future.
> * Other filesystem support was not built into this change, but
> extensibility was a primary goal, so adding support for btrfs or other
> filesystems should be relatively straightforward.
> * I did not implement the proposed UEFI variable support, but I did
> develop this with that future in mind, so adding it should be a
> relatively simple extension of these changes.
>
> This patchset has been tested on systems with and without ZFS as the boot
> filesystem, and built with and without ZFS libraries installed. It
> worked in each case I tried, including with manual corruption applied
> to the ZFS labels to force GRUB to read from the other label.  This
> was tested in conjunction with
> https://github.com/zfsonlinux/zfs/pull/10009 , the ZoL patch that
> enables ZFS to read from and write to the padding areas we reserved
> for the data.

First of all I would like to thank you for doing this work. This is very
interesting feature. I went quickly through the code and it looks quite
nice. However, I realized that it touches a lot of critical places in the
GRUB code. ...and we are in freeze right now. So, at this point I think
it is dangerous to merge that code. Simply we can make a lot of fallout
which we may not be able to catch and clear before the release. Hence,
I would like to suggest to defer this work until the release. Sorry about
that but I do not want to risk GRUB code breakage at this point. I hope
this is not a problem for you.

Daniel

Re: [PATCH 0/7] ZFS/other CoW FS save_env support

[Paul Dagnelie]

Not a problem, I suspected as much. We don't have any strong need to
get this work integrated ASAP, since we are going to have to build a
custom version of GRUB for internal use until this makes its way into
a release that lands in an Ubuntu LTS. We just want to make sure that
we get some good reviews on the changes from people with experience in
the codebase, and that one day we will be able to stop maintaining our
fork.

On Wed, Mar 25, 2020 at 10:06 AM Daniel Kiper <dkiper@net-space.pl> wrote:
>
> Hi Paul,
>
> On Wed, Mar 11, 2020 at 10:37:08AM -0700, Paul Dagnelie wrote:
> > Hey all, I previously discussed my concept for this patch in my email
> > https://lists.gnu.org/archive/html/grub-devel/2020-01/msg00004.html .
> > I'm pleased to announce that I've gotten it into a working state and
> > it is ready to review.  There are a number of changes here, which I
> > will break down below.
> >
> > First, the concept of "special files" is introduced into grub. These
> > are files that are manipulated using different functions from the
> > remainder of the filesystem. A filesystem advertises its support for
> > these files by filling in new entries in the grub_fs struct.
> >
> > Second, the loadenv command was modified to use the special file
> > functions of the root filesystem if they are detected. If that
> > happens, these functions are used to open, read, and write to the
> > loadenv file instead of the normal grub file functions.  A variable
> > was also added that allows the user to force a file to be used instead
> > of the special files, or vice versa.
> >
> > Third, the grub-editenv command was modified to teach it to probe the
> > root filesystem and then check in an array of structures that contain
> > functions that will read and modify the filesystem's special file in
> > userland. These functions are very similar to normal read and write
> > functions, and so don't result in a very different code flow.
> >
> > Finally, the GRUB ZFS logic was modified to have new special_file
> > functions. These functions manipulate the pad2 area of the ZFS label,
> > which was previously unused. It now stores a version number, the raw
> > contents of the grubenv file, and an embedded checksum for integrity
> > purposes. GRUB was taught to read and verify these areas, and also to
> > modify them, computing the embeddded checksum appropriately.  In
> > addition, if GRUB is build with libzfs support, an entry is added to
> > the grub-editenv table that points GRUB to the appropriate libzfs
> > functions, and init and fini functions are built into grub-editenv
> > itself.
> >
> > Future work:
> > * In the ZFS code could store a packed nvlist instead of a raw file,
> > but this would involve further changes to the grub environment file
> > code and was deferred for when it would be more useful and important.
> > * For the special files code, there is only one special file allowed
> > per filesystem, but a specification interface (using either an enum or
> > a set of names) could be added in the future.
> > * Other filesystem support was not built into this change, but
> > extensibility was a primary goal, so adding support for btrfs or other
> > filesystems should be relatively straightforward.
> > * I did not implement the proposed UEFI variable support, but I did
> > develop this with that future in mind, so adding it should be a
> > relatively simple extension of these changes.
> >
> > This patchset has been tested on systems with and without ZFS as the boot
> > filesystem, and built with and without ZFS libraries installed. It
> > worked in each case I tried, including with manual corruption applied
> > to the ZFS labels to force GRUB to read from the other label.  This
> > was tested in conjunction with
> > https://github.com/zfsonlinux/zfs/pull/10009 , the ZoL patch that
> > enables ZFS to read from and write to the padding areas we reserved
> > for the data.
>
> First of all I would like to thank you for doing this work. This is very
> interesting feature. I went quickly through the code and it looks quite
> nice. However, I realized that it touches a lot of critical places in the
> GRUB code. ...and we are in freeze right now. So, at this point I think
> it is dangerous to merge that code. Simply we can make a lot of fallout
> which we may not be able to catch and clear before the release. Hence,
> I would like to suggest to defer this work until the release. Sorry about
> that but I do not want to risk GRUB code breakage at this point. I hope
> this is not a problem for you.
>
> Daniel



-- 
Paul Dagnelie