Date: Sun, 21 Apr 2019 22:13:07 +0000 (UTC) From: Ian Lepore <ian@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: svn commit: r346501 - in stable/12/stand: i386/common i386/gptboot i386/loader i386/zfsboot libsa/zfs Message-ID: <201904212213.x3LMD7wZ020221@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: ian Date: Sun Apr 21 22:13:07 2019 New Revision: 346501 URL: https://svnweb.freebsd.org/changeset/base/346501 Log: MFC r341420, r341473, r341651 r341420: Eliminate duplicated code and struct member definitions in the handoff of args data between gptboot/zfsboot and loader(8). Despite what seems like a lot of changes here, there are no actual changes in behavior, or in the data layout in the structures involved. This is just eliminating identical code pasted into multiple locations. In detail, the changes are... - Move struct zfs_boot_args definition from libsa/zfs/libzfs.h to i386/common/bootargs.h because it is specific to x86 booting and the handoff between zfsboot and loader, and has no relation to the zfs library code in general. - The geli_boot_args and zfs_boot_args structs both contain an identical set of member variables containing geli information. Extract this out to a new geli_boot_data struct, and embed it in the arg-passing structs. - Provide new routines geli_import_boot_data() and geli_export_boot_data() that can be shared between gptboot, zfsboot, and loader instead of pasting identical code into several different .c files. - Remove some checks for a NULL pointer that can never be true because the pointer being tested was set using pointer math (kargs + 1) and that can never result in NULL in this code. r341473: Fix args cross-threading between gptboot(8) and loader(8) with zfs support. When loader(8) is built with zfs support enabled, it assumes that any extarg data present is a zfs_boot_args struct, but if the first-stage loader was gptboot(8) the extarg data is actually a geli_boot_args struct. Luckily, zfsboot(8) and gptzfsboot(8) have always passed KARGS_FLAGS_ZFS along with KARGS_FLAGS_EXTARG, so we can use KARGS_FLAGS_ZFS to decide whether the extarg data is a zfs_boot_args struct. To avoid similar problems in the future, gptboot(8) now passes a new KARGS_FLAGS_GELI to indicate that extarg data is geli_boot_args. In loader(8), if the neither KARGS_FLAGS_ZFS nor KARGS_FLAGS_GELI is set but extarg data is present (which will be the case for gptboot compiled before this change), we now check for the known size of the geli_boot_args struct passed by the older versions of gptboot as a way of confirming what type of extarg data is present. In a semi-related tidying up, since loader's main() has already decided what type of extarg data is present and set the global 'zargs' var accordingly, don't repeat the check in extract_currdev, just check whether zargs is NULL or not. r341651: Don't reference zfs-specific variables if LOADER_ZFS_SUPPORT is undefined because the variables will be undefined too. Modified: stable/12/stand/i386/common/bootargs.h stable/12/stand/i386/gptboot/gptboot.c stable/12/stand/i386/loader/main.c stable/12/stand/i386/zfsboot/zfsboot.c stable/12/stand/libsa/zfs/libzfs.h Directory Properties: stable/12/ (props changed) Modified: stable/12/stand/i386/common/bootargs.h ============================================================================== --- stable/12/stand/i386/common/bootargs.h Sun Apr 21 20:55:33 2019 (r346500) +++ stable/12/stand/i386/common/bootargs.h Sun Apr 21 22:13:07 2019 (r346501) @@ -18,10 +18,11 @@ #ifndef _BOOT_I386_ARGS_H_ #define _BOOT_I386_ARGS_H_ -#define KARGS_FLAGS_CD 0x1 -#define KARGS_FLAGS_PXE 0x2 -#define KARGS_FLAGS_ZFS 0x4 -#define KARGS_FLAGS_EXTARG 0x8 /* variably sized extended argument */ +#define KARGS_FLAGS_CD 0x0001 /* .bootdev is a bios CD dev */ +#define KARGS_FLAGS_PXE 0x0002 /* .pxeinfo is valid */ +#define KARGS_FLAGS_ZFS 0x0004 /* .zfspool is valid, EXTARG is zfs_boot_args */ +#define KARGS_FLAGS_EXTARG 0x0008 /* variably sized extended argument */ +#define KARGS_FLAGS_GELI 0x0010 /* EXTARG is geli_boot_args */ #define BOOTARGS_SIZE 24 /* sizeof(struct bootargs) */ #define BA_BOOTFLAGS 8 /* offsetof(struct bootargs, bootflags) */ @@ -84,11 +85,15 @@ struct bootargs #ifdef LOADER_GELI_SUPPORT #include <crypto/intake.h> +#include "geliboot.h" #endif -struct geli_boot_args +/* + * geli_boot_data is embedded in geli_boot_args (passed from gptboot to loader) + * and in zfs_boot_args (passed from zfsboot and gptzfsboot to loader). + */ +struct geli_boot_data { - uint32_t size; union { char gelipw[256]; struct { @@ -104,6 +109,49 @@ struct geli_boot_args #endif }; }; +}; + +#ifdef LOADER_GELI_SUPPORT + +static inline void +export_geli_boot_data(struct geli_boot_data *gbdata) +{ + + gbdata->notapw = '\0'; + gbdata->keybuf_sentinel = KEYBUF_SENTINEL; + gbdata->keybuf = malloc(sizeof(struct keybuf) + + (GELI_MAX_KEYS * sizeof(struct keybuf_ent))); + geli_export_key_buffer(gbdata->keybuf); +} + +static inline void +import_geli_boot_data(struct geli_boot_data *gbdata) +{ + + if (gbdata->gelipw[0] != '\0') { + setenv("kern.geom.eli.passphrase", gbdata->gelipw, 1); + explicit_bzero(gbdata->gelipw, sizeof(gbdata->gelipw)); + } else if (gbdata->keybuf_sentinel == KEYBUF_SENTINEL) { + geli_import_key_buffer(gbdata->keybuf); + } +} +#endif /* LOADER_GELI_SUPPORT */ + +struct geli_boot_args +{ + uint32_t size; + struct geli_boot_data gelidata; +}; + +struct zfs_boot_args +{ + uint32_t size; + uint32_t reserved; + uint64_t pool; + uint64_t root; + uint64_t primary_pool; + uint64_t primary_vdev; + struct geli_boot_data gelidata; }; #endif /*__ASSEMBLER__*/ Modified: stable/12/stand/i386/gptboot/gptboot.c ============================================================================== --- stable/12/stand/i386/gptboot/gptboot.c Sun Apr 21 20:55:33 2019 (r346500) +++ stable/12/stand/i386/gptboot/gptboot.c Sun Apr 21 22:13:07 2019 (r346501) @@ -114,7 +114,6 @@ static int vdev_read(void *vdev __unused, void *priv, #ifdef LOADER_GELI_SUPPORT #include "geliboot.h" static char gelipw[GELI_PW_MAXLEN]; -static struct keybuf *gelibuf; #endif struct gptdsk { @@ -480,12 +479,7 @@ load(void) #ifdef LOADER_GELI_SUPPORT geliargs.size = sizeof(geliargs); explicit_bzero(gelipw, sizeof(gelipw)); - gelibuf = malloc(sizeof(struct keybuf) + - (GELI_MAX_KEYS * sizeof(struct keybuf_ent))); - geli_export_key_buffer(gelibuf); - geliargs.notapw = '\0'; - geliargs.keybuf_sentinel = KEYBUF_SENTINEL; - geliargs.keybuf = gelibuf; + export_geli_boot_data(&geliargs.gelidata); #endif /* * Note that the geliargs struct is passed by value, not by pointer. @@ -496,7 +490,7 @@ load(void) __exec((caddr_t)addr, RB_BOOTINFO | (opts & RBX_MASK), MAKEBOOTDEV(dev_maj[gdsk.dsk.type], gdsk.dsk.part + 1, gdsk.dsk.unit, 0xff), #ifdef LOADER_GELI_SUPPORT - KARGS_FLAGS_EXTARG, 0, 0, VTOP(&bootinfo), geliargs + KARGS_FLAGS_GELI | KARGS_FLAGS_EXTARG, 0, 0, VTOP(&bootinfo), geliargs #else 0, 0, 0, VTOP(&bootinfo) #endif Modified: stable/12/stand/i386/loader/main.c ============================================================================== --- stable/12/stand/i386/loader/main.c Sun Apr 21 20:55:33 2019 (r346500) +++ stable/12/stand/i386/loader/main.c Sun Apr 21 22:13:07 2019 (r346501) @@ -73,6 +73,7 @@ void exit(int code); #ifdef LOADER_GELI_SUPPORT #include "geliboot.h" struct geli_boot_args *gargs; +struct geli_boot_data *gbdata; #endif #ifdef LOADER_ZFS_SUPPORT struct zfs_boot_args *zargs; @@ -169,37 +170,49 @@ main(void) #ifdef LOADER_ZFS_SUPPORT archsw.arch_zfs_probe = i386_zfs_probe; -#ifdef LOADER_GELI_SUPPORT - if ((kargs->bootflags & KARGS_FLAGS_EXTARG) != 0) { + /* + * zfsboot and gptzfsboot have always passed KARGS_FLAGS_ZFS, so if that is + * set along with KARGS_FLAGS_EXTARG we know we can interpret the extarg + * data as a struct zfs_boot_args. + */ +#define KARGS_EXTARGS_ZFS (KARGS_FLAGS_EXTARG | KARGS_FLAGS_ZFS) + + if ((kargs->bootflags & KARGS_EXTARGS_ZFS) == KARGS_EXTARGS_ZFS) { zargs = (struct zfs_boot_args *)(kargs + 1); - if (zargs != NULL && zargs->size >= offsetof(struct zfs_boot_args, gelipw)) { - if (zargs->size >= offsetof(struct zfs_boot_args, keybuf_sentinel) && - zargs->keybuf_sentinel == KEYBUF_SENTINEL) { - geli_import_key_buffer(zargs->keybuf); - } - if (zargs->gelipw[0] != '\0') { - setenv("kern.geom.eli.passphrase", zargs->gelipw, 1); - explicit_bzero(zargs->gelipw, sizeof(zargs->gelipw)); - } - } } -#endif /* LOADER_GELI_SUPPORT */ -#else /* !LOADER_ZFS_SUPPORT */ +#endif /* LOADER_ZFS_SUPPORT */ + #ifdef LOADER_GELI_SUPPORT - if ((kargs->bootflags & KARGS_FLAGS_EXTARG) != 0) { + /* + * If we decided earlier that we have zfs_boot_args extarg data, and it is + * big enough to contain the embedded geli data (the early zfs_boot_args + * structs weren't), then init the gbdata pointer accordingly. If there is + * extarg data which isn't zfs_boot_args data, determine whether it is + * geli_boot_args data. Recent versions of gptboot set KARGS_FLAGS_GELI to + * indicate that. Earlier versions didn't, but we presume that's what we + * have if the extarg size exactly matches the size of the geli_boot_args + * struct during that pre-flag era. + */ +#define LEGACY_GELI_ARGS_SIZE 260 /* This can never change */ + +#ifdef LOADER_ZFS_SUPPORT + if (zargs != NULL) { + if (zargs->size > offsetof(struct zfs_boot_args, gelidata)) { + gbdata = &zargs->gelidata; + } + } else +#endif /* LOADER_ZFS_SUPPORT */ + if ((kargs->bootflags & KARGS_FLAGS_EXTARG) != 0) { gargs = (struct geli_boot_args *)(kargs + 1); - if (gargs != NULL && gargs->size >= offsetof(struct geli_boot_args, gelipw)) { - if (gargs->keybuf_sentinel == KEYBUF_SENTINEL) { - geli_import_key_buffer(gargs->keybuf); - } - if (gargs->gelipw[0] != '\0') { - setenv("kern.geom.eli.passphrase", gargs->gelipw, 1); - explicit_bzero(gargs->gelipw, sizeof(gargs->gelipw)); - } + if ((kargs->bootflags & KARGS_FLAGS_GELI) || + gargs->size == LEGACY_GELI_ARGS_SIZE) { + gbdata = &gargs->gelidata; } } + + if (gbdata != NULL) + import_geli_boot_data(gbdata); #endif /* LOADER_GELI_SUPPORT */ -#endif /* LOADER_ZFS_SUPPORT */ /* * March through the device switch probing for things. @@ -271,11 +284,7 @@ extract_currdev(void) } #ifdef LOADER_ZFS_SUPPORT } else if ((kargs->bootflags & KARGS_FLAGS_ZFS) != 0) { - zargs = NULL; - /* check for new style extended argument */ - if ((kargs->bootflags & KARGS_FLAGS_EXTARG) != 0) - zargs = (struct zfs_boot_args *)(kargs + 1); - + /* zargs was set in main() if we have new style extended argument */ if (zargs != NULL && zargs->size >= offsetof(struct zfs_boot_args, primary_pool)) { /* sufficient data is provided */ Modified: stable/12/stand/i386/zfsboot/zfsboot.c ============================================================================== --- stable/12/stand/i386/zfsboot/zfsboot.c Sun Apr 21 20:55:33 2019 (r346500) +++ stable/12/stand/i386/zfsboot/zfsboot.c Sun Apr 21 22:13:07 2019 (r346501) @@ -129,7 +129,6 @@ int main(void); #ifdef LOADER_GELI_SUPPORT #include "geliboot.h" static char gelipw[GELI_PW_MAXLEN]; -static struct keybuf *gelibuf; #endif struct zfsdsk { @@ -993,13 +992,7 @@ load(void) zfsargs.primary_pool = primary_spa->spa_guid; #ifdef LOADER_GELI_SUPPORT explicit_bzero(gelipw, sizeof(gelipw)); - gelibuf = malloc(sizeof(struct keybuf) + (GELI_MAX_KEYS * sizeof(struct keybuf_ent))); - geli_export_key_buffer(gelibuf); - zfsargs.notapw = '\0'; - zfsargs.keybuf_sentinel = KEYBUF_SENTINEL; - zfsargs.keybuf = gelibuf; -#else - zfsargs.gelipw[0] = '\0'; + export_geli_boot_data(&zfsargs.gelidata); #endif if (primary_vdev != NULL) zfsargs.primary_vdev = primary_vdev->v_guid; Modified: stable/12/stand/libsa/zfs/libzfs.h ============================================================================== --- stable/12/stand/libsa/zfs/libzfs.h Sun Apr 21 20:55:33 2019 (r346500) +++ stable/12/stand/libsa/zfs/libzfs.h Sun Apr 21 22:13:07 2019 (r346501) @@ -44,31 +44,6 @@ struct zfs_devdesc { #include <crypto/intake.h> #endif -struct zfs_boot_args -{ - uint32_t size; - uint32_t reserved; - uint64_t pool; - uint64_t root; - uint64_t primary_pool; - uint64_t primary_vdev; - union { - char gelipw[256]; - struct { - char notapw; /* - * single null byte to stop keybuf - * being interpreted as a password - */ - uint32_t keybuf_sentinel; -#ifdef LOADER_GELI_SUPPORT - struct keybuf *keybuf; -#else - void *keybuf; -#endif - }; - }; -}; - int zfs_parsedev(struct zfs_devdesc *dev, const char *devspec, const char **path); char *zfs_fmtdev(void *vdev);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201904212213.x3LMD7wZ020221>