On Thu, Oct 02, 2025 at 04:50:45PM +0200, Daniel Kiper wrote: > On Thu, Oct 02, 2025 at 02:46:14PM +0800, Michael Chang via Grub-devel wrote: > > 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. > > s/grub editenv/grub-editenv/
OK, will fix next version. > > > Signed-off-by: Michael Chang <[email protected]> > > Reviewed-by: Neal Gompa <[email protected]> > > Reviewed-by: Avnish Chouhan <[email protected]> > > --- > > include/grub/fs.h | 2 + > > util/grub-editenv.c | 155 +++++++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 156 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) > > This change does not belong to this patch and should be moved to the > patch #7. OK, will make the change. This probably requires adjusting the order so that patch #7 comes earlier, since the definition needs to be in place first. > > > #endif /* ! GRUB_FS_HEADER */ > > diff --git a/util/grub-editenv.c b/util/grub-editenv.c > > index db6f187cc..1f055ca43 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; > > If you drop "*" from typedf then you can use the fs_envblk_spec_t > everywhere... Ok, will do it. > > > +struct fs_envblk { > > + struct fs_envblk_spec *spec; > > s/struct fs_envblk_spec/fs_envblk_spec_t/ Ok. > > > + const char *dev; > > +}; > > +typedef struct fs_envblk *fs_envblk_t; > > + > > +static struct fs_envblk_spec fs_envblk_spec[] = { > > s/struct fs_envblk_spec/fs_envblk_spec_t/ Ok. > > > + { "btrfs", ENV_BTRFS_OFFSET, GRUB_DISK_SECTOR_SIZE }, > > Again, this does not belong to this patch... Additionally, I think you > should put a comment here and in grub-core/fs/btrfs.c around btrfs_head > variable initialization saying that both fs_envblk_spec and btrfs_head > should be kept in sync. Yes, makes sense. Will work on the comment. > > > + { NULL, 0, 0 } > > +}; > > + > > +static fs_envblk_t fs_envblk = NULL; > > + > > static grub_envblk_t > > open_envblk_file (const char *name) > > { > > @@ -253,6 +276,133 @@ 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++) > > *curdev != NULL OK, will fix next version. > > In general please compare with NULL explicitly. Still I can see this > issue in many places in this patch set. Sorry about that. I'll take another look and make sure I didn't miss anything. > > > + grub_util_pull_device (*curdev); > > + > > + grub_drives = xcalloc ((ndev + 1), sizeof (grub_drives[0])); > > + > > + for (curdev = grub_devices, curdrive = grub_drives; *curdev; curdev++, > > Ditto... OK, will fix next version. > > > + 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; > > *curdrive = NULL; OK, will fix next version. > > > + > > + 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); > > Please add en empty line after variables definitions. This should be > fixed in other places too... OK, will fix next version. > > > + if (dev == NULL) > > + continue; > > + have_abstraction = is_abstraction (dev); > > + grub_device_close (dev); > > + } > > + > > + cleanup: > > s/cleanup/cleanup_0/ OK, will fix next version. > > > + 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; > > + } > > goto cleanup_1; > > > + > > + for (p = spec; p->fs_name; p++) > > p->fs_name != NULL OK, will fix next version. > > > + { > > + if (strcmp (grub_fs->name, p->fs_name) == 0 && !have_abstraction) > > have_abstraction == false OK, will fix next version. > > > + { > > + fs_envblk = xmalloc (sizeof (fs_envblk_t)); > > + fs_envblk->spec = p; > > + fs_envblk->dev = devname; > > + return fs_envblk; > > + } > > + } > > + > > cleanup_1: OK, will fix next version. Thanks, Michael > > > + free (devname); > > + > > + return NULL; > > +} > > Daniel _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
