[PATCH v2 0/9] Add support for external environment block on Btrfs

[PATCH v2 0/9] Add support for external environment block on Btrfs

[Michael Chang via Grub-devel]

This patch series adds support for storing the GRUB environment block in
a reserved area of the Btrfs header. On copy on write filesystems such
as Btrfs, the normal file based envblk cannot be updated safely at
runtime because block addresses are not stable. The reserved area
provides a fixed location that GRUB can write directly, allowing
commands such as grub-reboot and savedefault to work on Btrfs volumes.

The series proceeds in small chunks to keep each change buildable and
easier to review. The first patches add new data structures and helpers
for creating, opening, and writing an environment block in the reserved
area. Later patches update set_variables, unset_variables, and
list_variables so they can use the external block when it is present. An
entry is added to the Btrfs header to reserve space at 256 KiB for the
environment block. Finally, grub.cfg is modified so that load_env and
save_env use the external block automatically when env_block is defined.

v2:
- Define ENV_BTRFS_OFFSET as 256*1024
- Do not conflate type and variable definitions
- Align typedef with struct declaration to follow coding style
- Use bool as the return type of is_abstraction()
- Add "if (dev->disk != NULL)" check in is_abstraction()
- Refine the loop logic in is_abstraction() tests
- Remove extra indentation and redundant lines in read_envblk_fs
- Use off_t and size_t for offset and size variables, fix similar cases
  throughout
- Use explicit check "(fp == NULL)" instead of "(! fp)", fix similar
  cases throughout
- Use bool for the "found" field in var_lookup_ctx
- Add documentation describing the Btrfs environment block and special
  environment block variables

Michael Chang (9):
  util/grub-editenv: add basic structures and probe call for external
    envblk
  util/grub-editenv: add fs_envblk open helper
  util/grub-editenv: add fs_envblk write helper
  util/grub-editenv: wire set_variables to optional fs_envblk
  util/grub-editenv: wire unset_variables to optional fs_envblk
  util/grub-editenv: wire list_variables to optional fs_envblk
  btrfs: add environment block to reserved header area
  00_header.in: wire grub.cfg to use env_block when present
  docs: add Btrfs env block and special env vars

 docs/grub.texi           |  60 ++++++
 grub-core/fs/btrfs.c     |   3 +-
 include/grub/fs.h        |   2 +
 util/grub-editenv.c      | 393 ++++++++++++++++++++++++++++++++++++++-
 util/grub.d/00_header.in |  26 ++-
 5 files changed, 477 insertions(+), 7 deletions(-)

-- 
2.51.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 1/9] util/grub-editenv: add basic structures and probe call for external envblk

[Michael Chang via Grub-devel]

This patch prepares for using an environment block stored in a reserved
area of the filesystem. It adds a constant ENV_BTRFS_OFFSET at 256 KiB
in the Btrfs header. It also introduces the fs_envblk_spec and fs_envblk
structures together with the probe_fs_envblk function to identify the
root filesystem and to avoid configurations that involve diskfilter or
cryptodisk.

The probe is only invoked when grub-editenv is working on the default
environment file path. This restriction ensures that probing and
possible raw device access are not triggered for arbitrary user supplied
paths, but only for the standard grubenv file. In that case the code
checks if the filename equals DEFAULT_ENVBLK_PATH and then calls
probe_fs_envblk with fs_envblk_spec. The result is stored in the global
fs_envblk handle. At this stage the external environment block is only
detected and recorded, and the behavior of grub editenv is unchanged.

Signed-off-by: Michael Chang <mchang@suse.com>
---
 include/grub/fs.h   |   2 +
 util/grub-editenv.c | 153 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 154 insertions(+), 1 deletion(-)

diff --git a/include/grub/fs.h b/include/grub/fs.h
index df4c93b16..1be26dfba 100644
--- a/include/grub/fs.h
+++ b/include/grub/fs.h
@@ -132,4 +132,6 @@ grub_fs_unregister (grub_fs_t fs)
 
 grub_fs_t EXPORT_FUNC(grub_fs_probe) (grub_device_t device);
 
+#define ENV_BTRFS_OFFSET (256 * 1024)
+
 #endif /* ! GRUB_FS_HEADER */
diff --git a/util/grub-editenv.c b/util/grub-editenv.c
index db6f187cc..2302a6acf 100644
--- a/util/grub-editenv.c
+++ b/util/grub-editenv.c
@@ -23,8 +23,11 @@
 #include <grub/util/misc.h>
 #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/emu/getroot.h>
+#include <grub/fs.h>
+#include <grub/crypto.h>
 
 #include <stdio.h>
 #include <unistd.h>
@@ -120,6 +123,26 @@ block, use `rm %s'."),
   NULL, help_filter, NULL
 };
 
+struct fs_envblk_spec {
+  const char *fs_name;
+  off_t offset;
+  size_t size;
+};
+typedef struct fs_envblk_spec *fs_envblk_spec_t;
+
+struct fs_envblk {
+  struct fs_envblk_spec *spec;
+  const char *dev;
+};
+typedef struct fs_envblk *fs_envblk_t;
+
+static struct fs_envblk_spec fs_envblk_spec[] = {
+  { "btrfs", ENV_BTRFS_OFFSET, GRUB_DISK_SECTOR_SIZE },
+  { NULL, 0, 0 }
+};
+
+static fs_envblk_t fs_envblk = NULL;
+
 static grub_envblk_t
 open_envblk_file (const char *name)
 {
@@ -253,6 +276,131 @@ unset_variables (const char *name, int argc, char *argv[])
   grub_envblk_close (envblk);
 }
 
+static bool
+is_abstraction (grub_device_t dev)
+{
+  if (dev == NULL || dev->disk == NULL)
+    return false;
+
+  if (dev->disk->dev->id == GRUB_DISK_DEVICE_DISKFILTER_ID ||
+      dev->disk->dev->id == GRUB_DISK_DEVICE_CRYPTODISK_ID)
+    return true;
+  return false;
+}
+
+static fs_envblk_t
+probe_fs_envblk (fs_envblk_spec_t spec)
+{
+  char **grub_devices = NULL;
+  char **curdev, **curdrive;
+  size_t ndev = 0;
+  char **grub_drives = NULL;
+  grub_device_t grub_dev = NULL;
+  grub_fs_t grub_fs = NULL;
+  char *devname = NULL;
+  fs_envblk_spec_t p;
+  bool have_abstraction = false;
+
+  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 == NULL || grub_devices[0] == NULL)
+    {
+      grub_util_warn (_("cannot find a device for %s (is /dev mounted?)"), DEFAULT_DIRECTORY);
+      goto cleanup;
+    }
+
+  devname = xstrdup (grub_devices[0]);
+
+  for (curdev = grub_devices; *curdev; curdev++, ndev++)
+    grub_util_pull_device (*curdev);
+
+  grub_drives = xcalloc ((ndev + 1), sizeof (grub_drives[0]));
+
+  for (curdev = grub_devices, curdrive = grub_drives; *curdev; curdev++,
+       curdrive++)
+    {
+      *curdrive = grub_util_get_grub_dev (*curdev);
+      if (*curdrive == NULL)
+	{
+	  grub_util_warn (_("cannot find a GRUB drive for %s.  Check your device.map"),
+			  *curdev);
+	  goto cleanup;
+	}
+    }
+  *curdrive = 0;
+
+  grub_dev = grub_device_open (grub_drives[0]);
+  if (grub_dev == NULL)
+    {
+      grub_util_warn (_("cannot open device %s: %s"), grub_drives[0], grub_errmsg);
+      grub_errno = GRUB_ERR_NONE;
+      goto cleanup;
+    }
+
+  grub_fs = grub_fs_probe (grub_dev);
+  if (grub_fs == NULL)
+    {
+      grub_util_warn (_("cannot probe fs for %s: %s"), grub_drives[0], grub_errmsg);
+      grub_errno = GRUB_ERR_NONE;
+      goto cleanup;
+    }
+
+  have_abstraction = is_abstraction (grub_dev);
+  for (curdrive = grub_drives + 1; *curdrive && have_abstraction == false; curdrive++)
+    {
+      grub_device_t dev = grub_device_open (*curdrive);
+      if (dev == NULL)
+	continue;
+      have_abstraction = is_abstraction (dev);
+      grub_device_close (dev);
+    }
+
+ cleanup:
+  if (grub_devices != NULL)
+    for (curdev = grub_devices; *curdev; curdev++)
+      free (*curdev);
+  free (grub_devices);
+  free (grub_drives);
+  grub_device_close (grub_dev);
+  grub_gcry_fini_all ();
+  grub_fini_all ();
+  grub_util_biosdisk_fini ();
+
+  if (grub_fs == NULL)
+    {
+      free (devname);
+      return NULL;
+    }
+
+  for (p = spec; p->fs_name; p++)
+    {
+      if (strcmp (grub_fs->name, p->fs_name) == 0 && !have_abstraction)
+	{
+	  fs_envblk = xmalloc (sizeof (fs_envblk_t));
+	  fs_envblk->spec = p;
+	  fs_envblk->dev = devname;
+	  return fs_envblk;
+	}
+    }
+
+  free (devname);
+  return NULL;
+}
+
+
 int
 main (int argc, char *argv[])
 {
@@ -284,6 +432,9 @@ main (int argc, char *argv[])
       command  = argv[curindex++];
     }
 
+  if (strcmp (filename, DEFAULT_ENVBLK_PATH) == 0)
+    fs_envblk = probe_fs_envblk (fs_envblk_spec);
+
   if (strcmp (command, "create") == 0)
     grub_util_create_envblk_file (filename);
   else if (strcmp (command, "list") == 0)
-- 
2.51.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 2/9] util/grub-editenv: add fs_envblk open helper

[Michael Chang via Grub-devel]

This patch adds the logic to locate and open an environment block that
is stored in a reserved area on the device. It introduces the function
fs_envblk_open together with helper routines to read the block pointed
to by the env_block variable, and to create the block on disk when it
does not yet exist. When a block is created, the code records its
location inside the file based envblk by setting env_block in block list
syntax of offset plus size in sectors.

The env_block variable acts as a link from the file envblk to the raw
disk region so that later runs of grub editenv can follow it and access
the external block. The helper is exposed through a small ops table
attached to fs_envblk so that later patches can call
fs_envblk->ops->open without touching core code again. At this stage
variables are still stored in the file envblk and no redirection has
been applied.

Signed-off-by: Michael Chang <mchang@suse.com>
---
 util/grub-editenv.c | 128 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 128 insertions(+)

diff --git a/util/grub-editenv.c b/util/grub-editenv.c
index 2302a6acf..4e5dffa86 100644
--- a/util/grub-editenv.c
+++ b/util/grub-editenv.c
@@ -130,12 +130,23 @@ struct fs_envblk_spec {
 };
 typedef struct fs_envblk_spec *fs_envblk_spec_t;
 
+static grub_envblk_t fs_envblk_open (grub_envblk_t envblk);
+
+struct fs_envblk_ops {
+  grub_envblk_t (*open) (grub_envblk_t);
+};
+
 struct fs_envblk {
   struct fs_envblk_spec *spec;
+  struct fs_envblk_ops *ops;
   const char *dev;
 };
 typedef struct fs_envblk *fs_envblk_t;
 
+static struct fs_envblk_ops fs_envblk_ops = {
+  .open = fs_envblk_open
+};
+
 static struct fs_envblk_spec fs_envblk_spec[] = {
   { "btrfs", ENV_BTRFS_OFFSET, GRUB_DISK_SECTOR_SIZE },
   { NULL, 0, 0 }
@@ -143,6 +154,122 @@ static struct fs_envblk_spec fs_envblk_spec[] = {
 
 static fs_envblk_t fs_envblk = NULL;
 
+static int
+read_envblk_fs (const char *varname, const char *value, void *hook_data)
+{
+  grub_envblk_t *p_envblk = (grub_envblk_t *)hook_data;
+  off_t off;
+  size_t sz;
+  char *p, *buf;
+  FILE *fp;
+
+  if (p_envblk == NULL || fs_envblk == NULL)
+    return 1;
+
+  if (strcmp (varname, "env_block") != 0)
+    return 0;
+
+  off = strtol (value, &p, 10);
+  if (*p == '+')
+    sz = strtol (p+1, &p, 10);
+  else
+    return 0;
+
+  if (*p != '\0' || sz == 0)
+    return 0;
+
+  off <<= GRUB_DISK_SECTOR_BITS;
+  sz <<= GRUB_DISK_SECTOR_BITS;
+
+  fp = grub_util_fopen (fs_envblk->dev, "rb");
+  if (! fp)
+    grub_util_error (_("cannot open `%s': %s"), fs_envblk->dev,
+			strerror (errno));
+
+  if (fseek (fp, off, SEEK_SET) < 0)
+    grub_util_error (_("cannot seek `%s': %s"), fs_envblk->dev,
+			strerror (errno));
+
+  buf = xmalloc (sz);
+  if ((fread (buf, 1, sz, fp)) != sz)
+    grub_util_error (_("cannot read `%s': %s"), fs_envblk->dev,
+			strerror (errno));
+
+  fclose (fp);
+
+  *p_envblk = grub_envblk_open (buf, sz);
+
+  return 1;
+}
+
+static void
+create_envblk_fs (void)
+{
+  FILE *fp;
+  char *buf;
+  const char *device;
+  off_t offset;
+  size_t size;
+
+  if (fs_envblk == NULL)
+    return;
+
+  device = fs_envblk->dev;
+  offset = fs_envblk->spec->offset;
+  size = fs_envblk->spec->size;
+
+  fp = grub_util_fopen (device, "r+b");
+  if (fp == NULL)
+    grub_util_error (_("cannot open `%s': %s"), device, strerror (errno));
+
+  buf = xmalloc (size);
+  memcpy (buf, GRUB_ENVBLK_SIGNATURE, sizeof (GRUB_ENVBLK_SIGNATURE) - 1);
+  memset (buf + sizeof (GRUB_ENVBLK_SIGNATURE) - 1, '#', size - sizeof (GRUB_ENVBLK_SIGNATURE) + 1);
+
+  if (fseek (fp, offset, SEEK_SET) < 0)
+    grub_util_error (_("cannot seek `%s': %s"), device, strerror (errno));
+
+  if (fwrite (buf, 1, size, fp) != size)
+    grub_util_error (_("cannot write to `%s': %s"), device, strerror (errno));
+
+  grub_util_file_sync (fp);
+  free (buf);
+  fclose (fp);
+}
+
+static grub_envblk_t
+fs_envblk_open (grub_envblk_t envblk)
+{
+  grub_envblk_t envblk_fs = NULL;
+  char *val;
+  off_t offset;
+  size_t size;
+
+  if (envblk == NULL)
+    return NULL;
+
+  offset = fs_envblk->spec->offset;
+  size = fs_envblk->spec->size;
+
+  grub_envblk_iterate (envblk, &envblk_fs, read_envblk_fs);
+
+  if (envblk_fs && grub_envblk_size (envblk_fs) == size)
+    return envblk_fs;
+
+  create_envblk_fs ();
+
+  offset = offset >> GRUB_DISK_SECTOR_BITS;
+  size =  (size + GRUB_DISK_SECTOR_SIZE - 1) >> GRUB_DISK_SECTOR_BITS;
+
+  val = xasprintf ("%ld+%lu", offset, size);
+  if (! grub_envblk_set (envblk, "env_block", val))
+    grub_util_error ("%s", _("environment block too small"));
+  grub_envblk_iterate (envblk, &envblk_fs, read_envblk_fs);
+  free (val);
+
+  return envblk_fs;
+}
+
 static grub_envblk_t
 open_envblk_file (const char *name)
 {
@@ -392,6 +519,7 @@ probe_fs_envblk (fs_envblk_spec_t spec)
 	  fs_envblk = xmalloc (sizeof (fs_envblk_t));
 	  fs_envblk->spec = p;
 	  fs_envblk->dev = devname;
+	  fs_envblk->ops = &fs_envblk_ops;
 	  return fs_envblk;
 	}
     }
-- 
2.51.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 3/9] util/grub-editenv: add fs_envblk write helper

[Michael Chang via Grub-devel]

This patch adds the function fs_envblk_write to update the reserved
environment block on disk. The helper takes an in memory envblk buffer
and writes it back to the device at the location defined by the
fs_envblk specification. It performs size checks and uses file sync to
ensure that the updated data is flushed.

The helper is also added into the fs_envblk ops table, together with the
open helper from the previous patch. With this change the basic input
and output path for an external environment block is complete. The
choice of which variables should be written externally will be handled
by later patches.

Signed-off-by: Michael Chang <mchang@suse.com>
---
 util/grub-editenv.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/util/grub-editenv.c b/util/grub-editenv.c
index 4e5dffa86..26a81d2d0 100644
--- a/util/grub-editenv.c
+++ b/util/grub-editenv.c
@@ -131,9 +131,11 @@ struct fs_envblk_spec {
 typedef struct fs_envblk_spec *fs_envblk_spec_t;
 
 static grub_envblk_t fs_envblk_open (grub_envblk_t envblk);
+static void fs_envblk_write (grub_envblk_t envblk);
 
 struct fs_envblk_ops {
   grub_envblk_t (*open) (grub_envblk_t);
+  void (*write) (grub_envblk_t);
 };
 
 struct fs_envblk {
@@ -144,7 +146,8 @@ struct fs_envblk {
 typedef struct fs_envblk *fs_envblk_t;
 
 static struct fs_envblk_ops fs_envblk_ops = {
-  .open = fs_envblk_open
+  .open = fs_envblk_open,
+  .write = fs_envblk_write
 };
 
 static struct fs_envblk_spec fs_envblk_spec[] = {
@@ -358,6 +361,39 @@ write_envblk (const char *name, grub_envblk_t envblk)
   fclose (fp);
 }
 
+static void
+fs_envblk_write (grub_envblk_t envblk)
+{
+  FILE *fp;
+  const char *device;
+  off_t offset;
+  size_t size;
+
+  if (envblk == NULL)
+    return;
+
+  device = fs_envblk->dev;
+  offset = fs_envblk->spec->offset;
+  size = fs_envblk->spec->size;
+
+  if (grub_envblk_size (envblk) > size)
+    grub_util_error ("%s", _("environment block too small"));
+
+  fp = grub_util_fopen (device, "r+b");
+
+  if (! fp)
+    grub_util_error (_("cannot open `%s': %s"), device, strerror (errno));
+
+  if (fseek (fp, offset, SEEK_SET) < 0)
+    grub_util_error (_("cannot seek `%s': %s"), device, strerror (errno));
+
+  if (fwrite (grub_envblk_buffer (envblk), 1, grub_envblk_size (envblk), fp) != grub_envblk_size (envblk))
+    grub_util_error (_("cannot write to `%s': %s"), device, strerror (errno));
+
+  grub_util_file_sync (fp);
+  fclose (fp);
+}
+
 static void
 set_variables (const char *name, int argc, char *argv[])
 {
-- 
2.51.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 4/9] util/grub-editenv: wire set_variables to optional fs_envblk

[Michael Chang via Grub-devel]

This patch changes set_variables so that it can use an external
environment block when one is present. The variable next_entry is
written into the external block, env_block is treated as read only, and
all other variables are written into the normal file based envblk.

A cleanup step is added to handle cases where GRUB at runtime writes
variables into the external block because file based updates are not
safe on a copy on write filesystem such as Btrfs. For example, the
savedefault command can update saved_entry, and on Btrfs GRUB will place
that update in the external block instead of the file envblk. If an
older copy remains in the external block, it would override the newer
value from the file envblk when GRUB first loads the file and then
applies the external block on top of it. To avoid this, whenever a
variable is updated in the file envblk, any same named key in the
external block is deleted.

Signed-off-by: Michael Chang <mchang@suse.com>
---
 util/grub-editenv.c | 55 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/util/grub-editenv.c b/util/grub-editenv.c
index 26a81d2d0..d47adeb5e 100644
--- a/util/grub-editenv.c
+++ b/util/grub-editenv.c
@@ -394,12 +394,33 @@ fs_envblk_write (grub_envblk_t envblk)
   fclose (fp);
 }
 
+struct var_lookup_ctx {
+  const char *varname;
+  bool found;
+};
+
+static int
+var_lookup_iter (const char *varname, const char *value __attribute__ ((unused)), void *hook_data)
+{
+  struct var_lookup_ctx *ctx = (struct var_lookup_ctx *)hook_data;
+  if (grub_strcmp (ctx->varname, varname) == 0)
+    {
+      ctx->found = true;
+      return 1;
+    }
+  return 0;
+}
+
 static void
 set_variables (const char *name, int argc, char *argv[])
 {
   grub_envblk_t envblk;
+  grub_envblk_t envblk_fs = NULL;
 
   envblk = open_envblk_file (name);
+  if (fs_envblk != NULL)
+    envblk_fs = fs_envblk->ops->open (envblk);
+
   while (argc)
     {
       char *p;
@@ -410,8 +431,32 @@ set_variables (const char *name, int argc, char *argv[])
 
       *(p++) = 0;
 
-      if (! grub_envblk_set (envblk, argv[0], p))
-        grub_util_error ("%s", _("environment block too small"));
+      if ((strcmp (argv[0], "next_entry") == 0) && envblk_fs)
+	{
+	  if (! grub_envblk_set (envblk_fs, argv[0], p))
+	    grub_util_error ("%s", _("environment block too small"));
+	}
+      else if (strcmp (argv[0], "env_block") == 0)
+	{
+	  grub_util_warn (_("can't set env_block as it's read-only"));
+	}
+      else
+	{
+	  if (! grub_envblk_set (envblk, argv[0], p))
+	    grub_util_error ("%s", _("environment block too small"));
+
+	  if (envblk_fs)
+	    {
+	      struct var_lookup_ctx ctx = {
+		.varname = argv[0],
+		.found = false
+	      };
+
+	      grub_envblk_iterate (envblk_fs, &ctx, var_lookup_iter);
+	      if (ctx.found == true)
+		grub_envblk_delete (envblk_fs, argv[0]);
+	    }
+	}
 
       argc--;
       argv++;
@@ -419,6 +464,12 @@ set_variables (const char *name, int argc, char *argv[])
 
   write_envblk (name, envblk);
   grub_envblk_close (envblk);
+
+  if (envblk_fs != NULL)
+    {
+      fs_envblk->ops->write (envblk_fs);
+      grub_envblk_close (envblk_fs);
+    }
 }
 
 static void
-- 
2.51.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 5/9] util/grub-editenv: wire unset_variables to optional fs_envblk

[Michael Chang via Grub-devel]

This patch updates unset_variables so that removals are also applied to
the external environment block when it is present. The code opens the
external block, deletes the same named keys there, and then writes the
external block back using fs_envblk_write. The file based envblk is
still updated and written as before.

Signed-off-by: Michael Chang <mchang@suse.com>
---
 util/grub-editenv.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/util/grub-editenv.c b/util/grub-editenv.c
index d47adeb5e..5fe240c42 100644
--- a/util/grub-editenv.c
+++ b/util/grub-editenv.c
@@ -476,18 +476,32 @@ static void
 unset_variables (const char *name, int argc, char *argv[])
 {
   grub_envblk_t envblk;
+  grub_envblk_t envblk_fs = NULL;
 
   envblk = open_envblk_file (name);
+
+  if (fs_envblk != NULL)
+    envblk_fs = fs_envblk->ops->open (envblk);
+
   while (argc)
     {
       grub_envblk_delete (envblk, argv[0]);
 
+      if (envblk_fs != NULL)
+	grub_envblk_delete (envblk_fs, argv[0]);
+
       argc--;
       argv++;
     }
 
   write_envblk (name, envblk);
   grub_envblk_close (envblk);
+
+  if (envblk_fs != NULL)
+    {
+      fs_envblk->ops->write (envblk_fs);
+      grub_envblk_close (envblk_fs);
+    }
 }
 
 static bool
-- 
2.51.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 6/9] util/grub-editenv: wire list_variables to optional fs_envblk

[Michael Chang via Grub-devel]

This patch updates list_variables so that it also prints entries from
the external environment block when one is present. The function first
lists all variables from the file based envblk, then iterates over the
external envblk and prints those as well.

The output format remains the same as before. The change makes it
possible to inspect variables regardless of whether they are stored in
the file envblk or in the reserved block.

Signed-off-by: Michael Chang <mchang@suse.com>
---
 util/grub-editenv.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/util/grub-editenv.c b/util/grub-editenv.c
index 5fe240c42..43c87aa73 100644
--- a/util/grub-editenv.c
+++ b/util/grub-editenv.c
@@ -335,10 +335,17 @@ static void
 list_variables (const char *name)
 {
   grub_envblk_t envblk;
+  grub_envblk_t envblk_fs = NULL;
 
   envblk = open_envblk_file (name);
+  grub_envblk_iterate (envblk, &envblk_fs, read_envblk_fs);
   grub_envblk_iterate (envblk, NULL, print_var);
   grub_envblk_close (envblk);
+  if (envblk_fs != NULL)
+    {
+      grub_envblk_iterate (envblk_fs, NULL, print_var);
+      grub_envblk_close (envblk_fs);
+    }
 }
 
 static void
-- 
2.51.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 7/9] btrfs: add environment block to reserved header area

[Michael Chang via Grub-devel]

This patch reserves space for the GRUB environment block inside the
Btrfs header. The block is placed at an offset of 256 KiB from the start
of the device and occupies one sector. To protect the space, overflow
guard sectors are placed before and after the reserved block.

The Btrfs header already defines regions for bootloader use. By adding
this entry, GRUB gains a fixed and safe location to store the
environment block without conflicting with other structures in the
header.

Signed-off-by: Michael Chang <mchang@suse.com>
---
 grub-core/fs/btrfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 7bf8d922f..d35ff5fa5 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -2341,7 +2341,7 @@ struct embed_region {
 
 static const struct {
   struct embed_region available;
-  struct embed_region used[6];
+  struct embed_region used[7];
 } btrfs_head = {
   .available = {0, GRUB_DISK_KiB_TO_SECTORS (1024)}, /* The first 1 MiB. */
   .used = {
@@ -2349,6 +2349,7 @@ static const struct {
     {GRUB_DISK_KiB_TO_SECTORS (64) - 1, 1},                        /* Overflow guard. */
     {GRUB_DISK_KiB_TO_SECTORS (64), GRUB_DISK_KiB_TO_SECTORS (4)}, /* 4 KiB superblock. */
     {GRUB_DISK_KiB_TO_SECTORS (68), 1},                            /* Overflow guard. */
+    {(ENV_BTRFS_OFFSET >> GRUB_DISK_SECTOR_BITS) - 1, 3},          /* Environment Block. */
     {GRUB_DISK_KiB_TO_SECTORS (1024) - 1, 1},                      /* Overflow guard. */
     {0, 0}                                                         /* Array terminator. */
   }
-- 
2.51.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 8/9] 00_header.in: wire grub.cfg to use env_block when present

[Michael Chang via Grub-devel]

This patch extends the generated grub.cfg so that it can use the
external environment block when the variable env_block is defined.
During boot, if env_block is set, grub.cfg builds a device path for it,
exports the variable, and then loads its contents in addition to the
normal grubenv file.

When GRUB writes variables such as next_entry or saved_entry, the save
commands are changed to write into env_block if it is set, and to fall
back to the grubenv file otherwise. In this way the external environment
block is used automatically, and existing commands like savedefault or
save_env do not need to change.

Signed-off-by: Michael Chang <mchang@suse.com>
---
 util/grub.d/00_header.in | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/util/grub.d/00_header.in b/util/grub.d/00_header.in
index f86b69bad..9d36feda3 100644
--- a/util/grub.d/00_header.in
+++ b/util/grub.d/00_header.in
@@ -46,6 +46,13 @@ cat << EOF
 if [ -s \$prefix/grubenv ]; then
   load_env
 fi
+
+if [ "\${env_block}" ] ; then
+  set env_block="(\${root})\${env_block}"
+  export env_block
+  load_env -f "\${env_block}"
+fi
+
 EOF
 if [ "x$GRUB_BUTTON_CMOS_ADDRESS" != "x" ]; then
     cat <<EOF
@@ -54,7 +61,11 @@ if cmostest $GRUB_BUTTON_CMOS_ADDRESS ; then
 elif [ "\${next_entry}" ] ; then
    set default="\${next_entry}"
    set next_entry=
-   save_env next_entry
+   if [ "\${env_block}" ] ; then
+     save_env -f "\${env_block}" next_entry
+   else
+     save_env next_entry
+   fi
    set boot_once=true
 else
    set default="${GRUB_DEFAULT}"
@@ -65,7 +76,11 @@ else
 if [ "\${next_entry}" ] ; then
    set default="\${next_entry}"
    set next_entry=
-   save_env next_entry
+   if [ "\${env_block}" ] ; then
+     save_env -f "\${env_block}" next_entry
+   else
+     save_env next_entry
+   fi
    set boot_once=true
 else
    set default="${GRUB_DEFAULT}"
@@ -93,7 +108,12 @@ fi
 function savedefault {
   if [ -z "\${boot_once}" ]; then
     saved_entry="\${chosen}"
-    save_env saved_entry
+    if [ "\${env_block}" ] ; then
+      save_env -f "\${env_block}" saved_entry
+    else
+      save_env saved_entry
+    fi
+
   fi
 }
 
-- 
2.51.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 9/9] docs: add Btrfs env block and special env vars

[Michael Chang via Grub-devel]

Update grub.texi to describe the external environment block in the
reserved area of Btrfs header used for grub-reboot and savedefault, and
add a section documenting the saved_entry, next_entry, and env_block
variables.

Signed-off-by: Michael Chang <mchang@suse.com>
---
 docs/grub.texi | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/docs/grub.texi b/docs/grub.texi
index 34b3484dc..6ffb3a370 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3271,6 +3271,7 @@ GRUB.  Others may be used freely in GRUB configuration files.
 @menu
 * Special environment variables::
 * Environment block::
+* Special environment block variables::
 @end menu
 
 
@@ -3868,9 +3869,68 @@ For safety reasons, this storage is only available when installed on a plain
 disk (no LVM or RAID), using a non-checksumming filesystem (no ZFS), and
 using BIOS or EFI functions (no ATA, USB or IEEE1275).
 
+On Btrfs filesystems, a reserved area in the filesystem header may be used to
+store the environment block. This static block avoids the problems of updating
+a normal file on a copy-on-write filesystem, where writing raw block is not
+stable and requires metadata update. The reserved area provides a fixed
+location that GRUB can update directly, allowing commands such as
+@command{grub-reboot} and @samp{GRUB_SAVEDEFAULT} to function correctly on
+Btrfs volumes.
+
 @command{grub-mkconfig} uses this facility to implement
 @samp{GRUB_SAVEDEFAULT} (@pxref{Simple configuration}).
 
+
+@node Special environment block variables
+@section Special environment block variables
+
+These special variables are usually written to the environment block
+(@pxref{Environment block}) to customize the behavior of @file{grub.cfg}
+generated by @command{grub-mkconfig}.
+
+@menu
+* saved_entry::
+* next_entry::
+* env_block::
+@end menu
+
+
+@node saved_entry
+@subsection saved_entry
+
+The @var{saved_entry} variable sets the default boot entry in @file{grub.cfg}
+created by @command{grub-mkconfig}. It can be set with
+@command{grub-set-default} to choose a default entry, or at runtime with the
+@code{savedefault} function in grub.cfg to save the current entry as the new
+default. This may require write access by GRUB.
+
+
+@node next_entry
+@subsection next_entry
+
+The @var{next_entry} variable sets the boot entry for the next boot only. After
+it is used, GRUB clears the value so it is not reused. This requires write
+access to the environment block (@pxref{Environment block}) at runtime. The
+@command{grub-reboot} command is usually used instead of changing this variable
+directly.
+
+
+@node env_block
+@subsection env_block
+
+If the filesystem is Btrfs and the disk is not an abstracted device such as
+LVM, RAID, or encryption, the reserved space in the Btrfs header can be used as
+the environment block (@pxref{Environment block}). This provides a fixed raw
+block that GRUB can reliably write to. The @var{env_block} records this
+location in GRUB blocklist syntax (@pxref{Block list syntax}) so that
+@command{grub-editenv} and @file{grub.cfg} know how to access and use the
+external raw block.
+
+This variable is initialized when @file{grubenv} is first created by
+@command{grub-editenv} and is treated as read-only to avoid being overwritten
+with an unpredictable value.
+
+
 @node Modules
 @chapter Modules
 
-- 
2.51.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 0/9] Add support for external environment block on Btrfs

[Neal Gompa]

On Mon, Sep 15, 2025 at 5:08 AM Michael Chang <mchang@suse.com> wrote:
>
> This patch series adds support for storing the GRUB environment block in
> a reserved area of the Btrfs header. On copy on write filesystems such
> as Btrfs, the normal file based envblk cannot be updated safely at
> runtime because block addresses are not stable. The reserved area
> provides a fixed location that GRUB can write directly, allowing
> commands such as grub-reboot and savedefault to work on Btrfs volumes.
>
> The series proceeds in small chunks to keep each change buildable and
> easier to review. The first patches add new data structures and helpers
> for creating, opening, and writing an environment block in the reserved
> area. Later patches update set_variables, unset_variables, and
> list_variables so they can use the external block when it is present. An
> entry is added to the Btrfs header to reserve space at 256 KiB for the
> environment block. Finally, grub.cfg is modified so that load_env and
> save_env use the external block automatically when env_block is defined.
>
> v2:
> - Define ENV_BTRFS_OFFSET as 256*1024
> - Do not conflate type and variable definitions
> - Align typedef with struct declaration to follow coding style
> - Use bool as the return type of is_abstraction()
> - Add "if (dev->disk != NULL)" check in is_abstraction()
> - Refine the loop logic in is_abstraction() tests
> - Remove extra indentation and redundant lines in read_envblk_fs
> - Use off_t and size_t for offset and size variables, fix similar cases
>   throughout
> - Use explicit check "(fp == NULL)" instead of "(! fp)", fix similar
>   cases throughout
> - Use bool for the "found" field in var_lookup_ctx
> - Add documentation describing the Btrfs environment block and special
>   environment block variables
>
> Michael Chang (9):
>   util/grub-editenv: add basic structures and probe call for external
>     envblk
>   util/grub-editenv: add fs_envblk open helper
>   util/grub-editenv: add fs_envblk write helper
>   util/grub-editenv: wire set_variables to optional fs_envblk
>   util/grub-editenv: wire unset_variables to optional fs_envblk
>   util/grub-editenv: wire list_variables to optional fs_envblk
>   btrfs: add environment block to reserved header area
>   00_header.in: wire grub.cfg to use env_block when present
>   docs: add Btrfs env block and special env vars
>
>  docs/grub.texi           |  60 ++++++
>  grub-core/fs/btrfs.c     |   3 +-
>  include/grub/fs.h        |   2 +
>  util/grub-editenv.c      | 393 ++++++++++++++++++++++++++++++++++++++-
>  util/grub.d/00_header.in |  26 ++-
>  5 files changed, 477 insertions(+), 7 deletions(-)
>
> --
> 2.51.0
>

The patches look reasonable to me and things still seem to work. It is
much easier to understand all the pieces with this series breakout. :)

Reviewed-by: Neal Gompa <ngompa13@gmail.com>


-- 
真実はいつも一つ！/ Always, there's only one truth!

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 2/9] util/grub-editenv: add fs_envblk open helper

[Avnish Chouhan]

On 2025-09-15 14:39, grub-devel-request@gnu.org wrote:
> Message: 3
> Date: Mon, 15 Sep 2025 17:08:41 +0800
> From: Michael Chang <mchang@suse.com>
> To: The development of GNU GRUB <grub-devel@gnu.org>
> Cc: Neal Gompa <ngompa13@gmail.com>,	Marta Lewandowska
> 	<mlewando@redhat.com>
> Subject: [PATCH v2 2/9] util/grub-editenv: add fs_envblk open helper
> Message-ID: <20250915090848.131937-3-mchang@suse.com>
> 
> This patch adds the logic to locate and open an environment block that
> is stored in a reserved area on the device. It introduces the function
> fs_envblk_open together with helper routines to read the block pointed
> to by the env_block variable, and to create the block on disk when it
> does not yet exist. When a block is created, the code records its
> location inside the file based envblk by setting env_block in block 
> list
> syntax of offset plus size in sectors.
> 
> The env_block variable acts as a link from the file envblk to the raw
> disk region so that later runs of grub editenv can follow it and access
> the external block. The helper is exposed through a small ops table
> attached to fs_envblk so that later patches can call
> fs_envblk->ops->open without touching core code again. At this stage
> variables are still stored in the file envblk and no redirection has
> been applied.
> 
> Signed-off-by: Michael Chang <mchang@suse.com>
> ---
>  util/grub-editenv.c | 128 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 128 insertions(+)
> 
> diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> index 2302a6acf..4e5dffa86 100644
> --- a/util/grub-editenv.c
> +++ b/util/grub-editenv.c
> @@ -130,12 +130,23 @@ struct fs_envblk_spec {
>  };
>  typedef struct fs_envblk_spec *fs_envblk_spec_t;
> 
> +static grub_envblk_t fs_envblk_open (grub_envblk_t envblk);
> +
> +struct fs_envblk_ops {
> +  grub_envblk_t (*open) (grub_envblk_t);
> +};
> +
>  struct fs_envblk {
>    struct fs_envblk_spec *spec;
> +  struct fs_envblk_ops *ops;
>    const char *dev;
>  };
>  typedef struct fs_envblk *fs_envblk_t;
> 
> +static struct fs_envblk_ops fs_envblk_ops = {
> +  .open = fs_envblk_open
> +};
> +
>  static struct fs_envblk_spec fs_envblk_spec[] = {
>    { "btrfs", ENV_BTRFS_OFFSET, GRUB_DISK_SECTOR_SIZE },
>    { NULL, 0, 0 }
> @@ -143,6 +154,122 @@ static struct fs_envblk_spec fs_envblk_spec[] = {
> 
>  static fs_envblk_t fs_envblk = NULL;
> 
> +static int
> +read_envblk_fs (const char *varname, const char *value, void 
> *hook_data)
> +{
> +  grub_envblk_t *p_envblk = (grub_envblk_t *)hook_data;
> +  off_t off;
> +  size_t sz;
> +  char *p, *buf;
> +  FILE *fp;
> +
> +  if (p_envblk == NULL || fs_envblk == NULL)
> +    return 1;
> +
> +  if (strcmp (varname, "env_block") != 0)
> +    return 0;
> +
> +  off = strtol (value, &p, 10);
> +  if (*p == '+')
> +    sz = strtol (p+1, &p, 10);
> +  else
> +    return 0;
> +
> +  if (*p != '\0' || sz == 0)
> +    return 0;
> +
> +  off <<= GRUB_DISK_SECTOR_BITS;
> +  sz <<= GRUB_DISK_SECTOR_BITS;
> +
> +  fp = grub_util_fopen (fs_envblk->dev, "rb");
> +  if (! fp)

Hi Michael,

(fp == NULL) would be better here!

> +    grub_util_error (_("cannot open `%s': %s"), fs_envblk->dev,
> +			strerror (errno));
> +
> +  if (fseek (fp, off, SEEK_SET) < 0)

fclose (fp) seems missing here.

> +    grub_util_error (_("cannot seek `%s': %s"), fs_envblk->dev,
> +			strerror (errno));
> +
> +  buf = xmalloc (sz);
> +  if ((fread (buf, 1, sz, fp)) != sz)

fclose (fp) seems missing as well as grub_free (buf).

> +    grub_util_error (_("cannot read `%s': %s"), fs_envblk->dev,
> +			strerror (errno));
> +
> +  fclose (fp);
> +
> +  *p_envblk = grub_envblk_open (buf, sz);
> +
> +  return 1;
> +}
> +
> +static void
> +create_envblk_fs (void)
> +{
> +  FILE *fp;
> +  char *buf;
> +  const char *device;
> +  off_t offset;
> +  size_t size;
> +
> +  if (fs_envblk == NULL)
> +    return;
> +
> +  device = fs_envblk->dev;
> +  offset = fs_envblk->spec->offset;
> +  size = fs_envblk->spec->size;
> +
> +  fp = grub_util_fopen (device, "r+b");
> +  if (fp == NULL)
> +    grub_util_error (_("cannot open `%s': %s"), device, strerror 
> (errno));
> +
> +  buf = xmalloc (size);
> +  memcpy (buf, GRUB_ENVBLK_SIGNATURE, sizeof (GRUB_ENVBLK_SIGNATURE) - 
> 1);
> +  memset (buf + sizeof (GRUB_ENVBLK_SIGNATURE) - 1, '#', size -
> sizeof (GRUB_ENVBLK_SIGNATURE) + 1);
> +
> +  if (fseek (fp, offset, SEEK_SET) < 0)

fclose (fp) seems missing as well as grub_free (buf).

> +    grub_util_error (_("cannot seek `%s': %s"), device, strerror 
> (errno));
> +
> +  if (fwrite (buf, 1, size, fp) != size)

fclose (fp) seems missing as well as grub_free (buf).

> +    grub_util_error (_("cannot write to `%s': %s"), device, strerror 
> (errno));
> +
> +  grub_util_file_sync (fp);
> +  free (buf);
> +  fclose (fp);
> +}
> +
> +static grub_envblk_t
> +fs_envblk_open (grub_envblk_t envblk)
> +{
> +  grub_envblk_t envblk_fs = NULL;
> +  char *val;
> +  off_t offset;
> +  size_t size;
> +
> +  if (envblk == NULL)
> +    return NULL;
> +
> +  offset = fs_envblk->spec->offset;
> +  size = fs_envblk->spec->size;
> +
> +  grub_envblk_iterate (envblk, &envblk_fs, read_envblk_fs);
> +
> +  if (envblk_fs && grub_envblk_size (envblk_fs) == size)
> +    return envblk_fs;
> +
> +  create_envblk_fs ();
> +
> +  offset = offset >> GRUB_DISK_SECTOR_BITS;
> +  size =  (size + GRUB_DISK_SECTOR_SIZE - 1) >> GRUB_DISK_SECTOR_BITS;
> +
> +  val = xasprintf ("%ld+%lu", offset, size);
> +  if (! grub_envblk_set (envblk, "env_block", val))

grub_free (val) missing here.

Thank you!

Regards,
Avnish Chouhan

> +    grub_util_error ("%s", _("environment block too small"));
> +  grub_envblk_iterate (envblk, &envblk_fs, read_envblk_fs);
> +  free (val);
> +
> +  return envblk_fs;
> +}
> +
>  static grub_envblk_t
>  open_envblk_file (const char *name)
>  {
> @@ -392,6 +519,7 @@ probe_fs_envblk (fs_envblk_spec_t spec)
>  	  fs_envblk = xmalloc (sizeof (fs_envblk_t));
>  	  fs_envblk->spec = p;
>  	  fs_envblk->dev = devname;
> +	  fs_envblk->ops = &fs_envblk_ops;
>  	  return fs_envblk;
>  	}
>      }
> --
> 2.51.0

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 6/9] util/grub-editenv: wire list_variables to optional fs_envblk

[Sudhakar Kuppusamy]

> On 15 Sep 2025, at 2:38 PM, Michael Chang via Grub-devel <grub-devel@gnu.org> wrote:
> 
> This patch updates list_variables so that it also prints entries from
> the external environment block when one is present. The function first
> lists all variables from the file based envblk, then iterates over the
> external envblk and prints those as well.
> 
> The output format remains the same as before. The change makes it
> possible to inspect variables regardless of whether they are stored in
> the file envblk or in the reserved block.
> 
> Signed-off-by: Michael Chang <mchang@suse.com>

Reviewed-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

> ---
> util/grub-editenv.c | 7 +++++++
> 1 file changed, 7 insertions(+)
> 
> diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> index 5fe240c42..43c87aa73 100644
> --- a/util/grub-editenv.c
> +++ b/util/grub-editenv.c
> @@ -335,10 +335,17 @@ static void
> list_variables (const char *name)
> {
>   grub_envblk_t envblk;
> +  grub_envblk_t envblk_fs = NULL;
> 
>   envblk = open_envblk_file (name);
> +  grub_envblk_iterate (envblk, &envblk_fs, read_envblk_fs);
>   grub_envblk_iterate (envblk, NULL, print_var);
>   grub_envblk_close (envblk);
> +  if (envblk_fs != NULL)
> +    {
> +      grub_envblk_iterate (envblk_fs, NULL, print_var);
> +      grub_envblk_close (envblk_fs);
> +    }
> }
> 
> static void
> -- 
> 2.51.0
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 2/9] util/grub-editenv: add fs_envblk open helper

[Michael Chang via Grub-devel]

Hi Avnish,

Sorry for the late reply. Please check my comments below:

On Wed, Sep 17, 2025 at 04:13:21PM +0530, Avnish Chouhan wrote:
> On 2025-09-15 14:39, grub-devel-request@gnu.org wrote:
> > Message: 3
> > Date: Mon, 15 Sep 2025 17:08:41 +0800
> > From: Michael Chang <mchang@suse.com>
> > To: The development of GNU GRUB <grub-devel@gnu.org>
> > Cc: Neal Gompa <ngompa13@gmail.com>,	Marta Lewandowska
> > 	<mlewando@redhat.com>
> > Subject: [PATCH v2 2/9] util/grub-editenv: add fs_envblk open helper
> > Message-ID: <20250915090848.131937-3-mchang@suse.com>
> > 
> > This patch adds the logic to locate and open an environment block that
> > is stored in a reserved area on the device. It introduces the function
> > fs_envblk_open together with helper routines to read the block pointed
> > to by the env_block variable, and to create the block on disk when it
> > does not yet exist. When a block is created, the code records its
> > location inside the file based envblk by setting env_block in block list
> > syntax of offset plus size in sectors.
> > 
> > The env_block variable acts as a link from the file envblk to the raw
> > disk region so that later runs of grub editenv can follow it and access
> > the external block. The helper is exposed through a small ops table
> > attached to fs_envblk so that later patches can call
> > fs_envblk->ops->open without touching core code again. At this stage
> > variables are still stored in the file envblk and no redirection has
> > been applied.
> > 
> > Signed-off-by: Michael Chang <mchang@suse.com>
> > ---
> >  util/grub-editenv.c | 128 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 128 insertions(+)
> > 
> > diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> > index 2302a6acf..4e5dffa86 100644
> > --- a/util/grub-editenv.c
> > +++ b/util/grub-editenv.c
> > @@ -130,12 +130,23 @@ struct fs_envblk_spec {
> >  };
> >  typedef struct fs_envblk_spec *fs_envblk_spec_t;
> > 
> > +static grub_envblk_t fs_envblk_open (grub_envblk_t envblk);
> > +
> > +struct fs_envblk_ops {
> > +  grub_envblk_t (*open) (grub_envblk_t);
> > +};
> > +
> >  struct fs_envblk {
> >    struct fs_envblk_spec *spec;
> > +  struct fs_envblk_ops *ops;
> >    const char *dev;
> >  };
> >  typedef struct fs_envblk *fs_envblk_t;
> > 
> > +static struct fs_envblk_ops fs_envblk_ops = {
> > +  .open = fs_envblk_open
> > +};
> > +
> >  static struct fs_envblk_spec fs_envblk_spec[] = {
> >    { "btrfs", ENV_BTRFS_OFFSET, GRUB_DISK_SECTOR_SIZE },
> >    { NULL, 0, 0 }
> > @@ -143,6 +154,122 @@ static struct fs_envblk_spec fs_envblk_spec[] = {
> > 
> >  static fs_envblk_t fs_envblk = NULL;
> > 
> > +static int
> > +read_envblk_fs (const char *varname, const char *value, void
> > *hook_data)
> > +{
> > +  grub_envblk_t *p_envblk = (grub_envblk_t *)hook_data;
> > +  off_t off;
> > +  size_t sz;
> > +  char *p, *buf;
> > +  FILE *fp;
> > +
> > +  if (p_envblk == NULL || fs_envblk == NULL)
> > +    return 1;
> > +
> > +  if (strcmp (varname, "env_block") != 0)
> > +    return 0;
> > +
> > +  off = strtol (value, &p, 10);
> > +  if (*p == '+')
> > +    sz = strtol (p+1, &p, 10);
> > +  else
> > +    return 0;
> > +
> > +  if (*p != '\0' || sz == 0)
> > +    return 0;
> > +
> > +  off <<= GRUB_DISK_SECTOR_BITS;
> > +  sz <<= GRUB_DISK_SECTOR_BITS;
> > +
> > +  fp = grub_util_fopen (fs_envblk->dev, "rb");
> > +  if (! fp)
> 
> Hi Michael,
> 
> (fp == NULL) would be better here!

OK. I'll change it next version.

> 
> > +    grub_util_error (_("cannot open `%s': %s"), fs_envblk->dev,
> > +			strerror (errno));
> > +
> > +  if (fseek (fp, off, SEEK_SET) < 0)
> 
> fclose (fp) seems missing here.

It is not a mistake. The housekeeping cleanup is not required because
the ensuing grub_util_error() aborts and terminates the program
completely.

> 
> > +    grub_util_error (_("cannot seek `%s': %s"), fs_envblk->dev,
> > +			strerror (errno));
> > +
> > +  buf = xmalloc (sz);
> > +  if ((fread (buf, 1, sz, fp)) != sz)
> 
> fclose (fp) seems missing as well as grub_free (buf).

It is not required. Please check the comment above.
> 
> > +    grub_util_error (_("cannot read `%s': %s"), fs_envblk->dev,
> > +			strerror (errno));
> > +
> > +  fclose (fp);
> > +
> > +  *p_envblk = grub_envblk_open (buf, sz);
> > +
> > +  return 1;
> > +}
> > +
> > +static void
> > +create_envblk_fs (void)
> > +{
> > +  FILE *fp;
> > +  char *buf;
> > +  const char *device;
> > +  off_t offset;
> > +  size_t size;
> > +
> > +  if (fs_envblk == NULL)
> > +    return;
> > +
> > +  device = fs_envblk->dev;
> > +  offset = fs_envblk->spec->offset;
> > +  size = fs_envblk->spec->size;
> > +
> > +  fp = grub_util_fopen (device, "r+b");
> > +  if (fp == NULL)
> > +    grub_util_error (_("cannot open `%s': %s"), device, strerror
> > (errno));
> > +
> > +  buf = xmalloc (size);
> > +  memcpy (buf, GRUB_ENVBLK_SIGNATURE, sizeof (GRUB_ENVBLK_SIGNATURE) -
> > 1);
> > +  memset (buf + sizeof (GRUB_ENVBLK_SIGNATURE) - 1, '#', size -
> > sizeof (GRUB_ENVBLK_SIGNATURE) + 1);
> > +
> > +  if (fseek (fp, offset, SEEK_SET) < 0)
> 
> fclose (fp) seems missing as well as grub_free (buf).

It is not required. Please check the comment above.
> 
> > +    grub_util_error (_("cannot seek `%s': %s"), device, strerror
> > (errno));
> > +
> > +  if (fwrite (buf, 1, size, fp) != size)
> 
> fclose (fp) seems missing as well as grub_free (buf).

It is not required. Please check the comment above.
> 
> > +    grub_util_error (_("cannot write to `%s': %s"), device, strerror
> > (errno));
> > +
> > +  grub_util_file_sync (fp);
> > +  free (buf);
> > +  fclose (fp);
> > +}
> > +
> > +static grub_envblk_t
> > +fs_envblk_open (grub_envblk_t envblk)
> > +{
> > +  grub_envblk_t envblk_fs = NULL;
> > +  char *val;
> > +  off_t offset;
> > +  size_t size;
> > +
> > +  if (envblk == NULL)
> > +    return NULL;
> > +
> > +  offset = fs_envblk->spec->offset;
> > +  size = fs_envblk->spec->size;
> > +
> > +  grub_envblk_iterate (envblk, &envblk_fs, read_envblk_fs);
> > +
> > +  if (envblk_fs && grub_envblk_size (envblk_fs) == size)
> > +    return envblk_fs;
> > +
> > +  create_envblk_fs ();
> > +
> > +  offset = offset >> GRUB_DISK_SECTOR_BITS;
> > +  size =  (size + GRUB_DISK_SECTOR_SIZE - 1) >> GRUB_DISK_SECTOR_BITS;
> > +
> > +  val = xasprintf ("%ld+%lu", offset, size);
> > +  if (! grub_envblk_set (envblk, "env_block", val))
> 
> grub_free (val) missing here.
It is not required. Please check the comment above.

Thanks,
Michael

> 
> Thank you!
> 
> Regards,
> Avnish Chouhan
> 
> > +    grub_util_error ("%s", _("environment block too small"));
> > +  grub_envblk_iterate (envblk, &envblk_fs, read_envblk_fs);
> > +  free (val);
> > +
> > +  return envblk_fs;
> > +}
> > +
> >  static grub_envblk_t
> >  open_envblk_file (const char *name)
> >  {
> > @@ -392,6 +519,7 @@ probe_fs_envblk (fs_envblk_spec_t spec)
> >  	  fs_envblk = xmalloc (sizeof (fs_envblk_t));
> >  	  fs_envblk->spec = p;
> >  	  fs_envblk->dev = devname;
> > +	  fs_envblk->ops = &fs_envblk_ops;
> >  	  return fs_envblk;
> >  	}
> >      }
> > --
> > 2.51.0

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 2/9] util/grub-editenv: add fs_envblk open helper

[Avnish Chouhan]

On 2025-10-02 11:14, Michael Chang wrote:
> Hi Avnish,
> 
> Sorry for the late reply. Please check my comments below:
> 
> On Wed, Sep 17, 2025 at 04:13:21PM +0530, Avnish Chouhan wrote:
>> On 2025-09-15 14:39, grub-devel-request@gnu.org wrote:
>> > Message: 3
>> > Date: Mon, 15 Sep 2025 17:08:41 +0800
>> > From: Michael Chang <mchang@suse.com>
>> > To: The development of GNU GRUB <grub-devel@gnu.org>
>> > Cc: Neal Gompa <ngompa13@gmail.com>,	Marta Lewandowska
>> > 	<mlewando@redhat.com>
>> > Subject: [PATCH v2 2/9] util/grub-editenv: add fs_envblk open helper
>> > Message-ID: <20250915090848.131937-3-mchang@suse.com>
>> >
>> > This patch adds the logic to locate and open an environment block that
>> > is stored in a reserved area on the device. It introduces the function
>> > fs_envblk_open together with helper routines to read the block pointed
>> > to by the env_block variable, and to create the block on disk when it
>> > does not yet exist. When a block is created, the code records its
>> > location inside the file based envblk by setting env_block in block list
>> > syntax of offset plus size in sectors.
>> >
>> > The env_block variable acts as a link from the file envblk to the raw
>> > disk region so that later runs of grub editenv can follow it and access
>> > the external block. The helper is exposed through a small ops table
>> > attached to fs_envblk so that later patches can call
>> > fs_envblk->ops->open without touching core code again. At this stage
>> > variables are still stored in the file envblk and no redirection has
>> > been applied.
>> >
>> > Signed-off-by: Michael Chang <mchang@suse.com>

Reviewed-by: Avnish Chouhan <avnish@linux.ibm.com>

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel