Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 4 Dec 2018 16:43:50 +0000 (UTC)
From:      Ian Lepore <ian@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r341473 - in head/stand/i386: common gptboot loader
Message-ID:  <201812041643.wB4GhoSV025590@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: ian
Date: Tue Dec  4 16:43:50 2018
New Revision: 341473
URL: https://svnweb.freebsd.org/changeset/base/341473

Log:
  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.
  
  X-MFC after:	a few days, along with prior related changes.

Modified:
  head/stand/i386/common/bootargs.h
  head/stand/i386/gptboot/gptboot.c
  head/stand/i386/loader/main.c

Modified: head/stand/i386/common/bootargs.h
==============================================================================
--- head/stand/i386/common/bootargs.h	Tue Dec  4 16:12:43 2018	(r341472)
+++ head/stand/i386/common/bootargs.h	Tue Dec  4 16:43:50 2018	(r341473)
@@ -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) */

Modified: head/stand/i386/gptboot/gptboot.c
==============================================================================
--- head/stand/i386/gptboot/gptboot.c	Tue Dec  4 16:12:43 2018	(r341472)
+++ head/stand/i386/gptboot/gptboot.c	Tue Dec  4 16:43:50 2018	(r341473)
@@ -490,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: head/stand/i386/loader/main.c
==============================================================================
--- head/stand/i386/loader/main.c	Tue Dec  4 16:12:43 2018	(r341472)
+++ head/stand/i386/loader/main.c	Tue Dec  4 16:43:50 2018	(r341473)
@@ -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,24 +170,46 @@ 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->size > offsetof(struct zfs_boot_args, gelidata)) {
-	    import_geli_boot_data(&zargs->gelidata);
-	}
     }
-#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 */
+
+    if (zargs != NULL) {
+	if (zargs->size > offsetof(struct zfs_boot_args, gelidata)) {
+	    gbdata = &zargs->gelidata;
+	}
+    } else if ((kargs->bootflags & KARGS_FLAGS_EXTARG) != 0) {
 	gargs = (struct geli_boot_args *)(kargs + 1);
-	if (gargs->size >= offsetof(struct geli_boot_args, gelidata)) {
-	    import_geli_boot_data(&gargs->gelidata);
+	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.
@@ -258,11 +281,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 */



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201812041643.wB4GhoSV025590>