probe: Add support to determine if a device uses a GPT table or not

In accordance with issue #53, we must only use the PartUUID for root=
entries when we *know* that the partition definitely resides on a GPT
disk.

Whilst an EFI System Partition must live on a GPT disk to be considered
a valid ESP, there is no such constraint on the rootfs itself. Cases
emerged during testing of an MBR rootfs partition, with a GPT disk used
to house the ESP itself.

This change ensures we only ever write a root=PARTUUID if we're fully
certain of the topology, otherwise all bootloaders will automatically
fall back to root=UUID entries.

Signed-off-by: Ikey Doherty <michael.i.doherty@intel.com>

.gitignore

@@ -26,6 +26,7 @@ check_os_release
 check_uefi
 check_grub2
 check_select_bootloader
+check_probe
 
 *.log
 *.trs

Makefile.am

@@ -160,7 +160,8 @@ TESTS = 		\
 	check_os_release \
 	check_uefi 	\
 	check_grub2 \
-	check_select_bootloader
+	check_select_bootloader \
+	check_probe
 
 check_PROGRAMS = $(TESTS)
 
@@ -298,6 +299,24 @@ check_select_bootloader_LDADD =	\
 	$(BLKID_LIBS)	\
 	$(CHECK_LIBS)
 
+check_probe_SOURCES = \
+	tests/check-probe.c 	\
+	tests/blkid-harness.h   \
+	tests/system-harness.h  \
+	tests/harness.h		\
+	tests/harness.c
+
+check_probe_CFLAGS =		\
+	$(BLKID_CFLAGS)		\
+	$(CHECK_CFLAGS)		\
+	$(AM_CFLAGS)
+
+check_probe_LDADD =	\
+	libcbm.la	\
+	src/libnica/libnica.la	\
+	$(BLKID_LIBS)	\
+	$(CHECK_LIBS)
+
 
 @VALGRIND_CHECK_RULES@
 

src/lib/blkid_stub.c

@@ -47,6 +47,10 @@ static CbmBlkidOps default_blkid_ops = {
         .partition_get_flags = blkid_partition_get_flags,
         .partition_get_uuid = blkid_partition_get_uuid,
 
+        /* Partition table functions */
+        .partlist_get_table = blkid_partlist_get_table,
+        .parttable_get_type = blkid_parttable_get_type,
+
         /* Misc */
         .devno_to_wholedisk = cbm_blkid_devno_to_wholedisk_wrapped,
 };
@@ -85,6 +89,10 @@ void cbm_blkid_set_vtable(CbmBlkidOps *ops)
         assert(blkid_ops->partition_get_flags != NULL);
         assert(blkid_ops->partition_get_uuid != NULL);
 
+        /* partition table functions */
+        assert(blkid_ops->partlist_get_table != NULL);
+        assert(blkid_ops->parttable_get_type != NULL);
+
         /* misc */
         assert(blkid_ops->devno_to_wholedisk != NULL);
 }
@@ -160,6 +168,19 @@ const char *cbm_blkid_partition_get_uuid(blkid_partition par)
         return blkid_ops->partition_get_uuid(par);
 }
 
+/**
+ * Partition table related wrappers
+ */
+blkid_parttable cbm_blkid_partlist_get_table(blkid_partlist ls)
+{
+        return blkid_ops->partlist_get_table(ls);
+}
+
+const char *cbm_blkid_parttable_get_type(blkid_parttable tab)
+{
+        return blkid_ops->parttable_get_type(tab);
+}
+
 /**
  * Misc functions
  */

src/lib/blkid_stub.h

@@ -36,6 +36,10 @@ typedef struct CbmBlkidOps {
         unsigned long long (*partition_get_flags)(blkid_partition par);
         const char *(*partition_get_uuid)(blkid_partition par);
 
+        /* Partition table functions */
+        blkid_parttable (*partlist_get_table)(blkid_partlist ls);
+        const char *(*parttable_get_type)(blkid_parttable tab);
+
         /* Misc functions */
         int (*devno_to_wholedisk)(dev_t dev, char *diskname, size_t len, dev_t *diskdevno);
 } CbmBlkidOps;
@@ -70,6 +74,16 @@ typedef struct CbmBlkidOps {
  */
 #define CBM_BLKID_PARTITION_SET ((blkid_partition)1)
 
+/**
+ * Define a "set" blkid_parttable for testing
+ */
+#define CBM_BLKID_PARTTABLE_SET ((blkid_parttable)1)
+
+/**
+ * Define an empty blkid_parttable for testing
+ */
+#define CBM_BLKID_PARTTABLE_NULL ((blkid_parttable)0)
+
 /**
  * Reset the blkid vtable
  */
@@ -105,6 +119,12 @@ blkid_partition cbm_blkid_partlist_get_partition(blkid_partlist ls, int n);
 unsigned long long cbm_blkid_partition_get_flags(blkid_partition par);
 const char *cbm_blkid_partition_get_uuid(blkid_partition par);
 
+/**
+ * Partition table related wrappers
+ */
+blkid_parttable cbm_blkid_partlist_get_table(blkid_partlist ls);
+const char *cbm_blkid_parttable_get_type(blkid_parttable tab);
+
 /**
  * Misc related wrappers
  */

src/lib/probe.c

@@ -22,6 +22,7 @@
 #include <unistd.h>
 
 #include "blkid_stub.h"
+#include "files.h"
 #include "log.h"
 #include "probe.h"
 #include "system_stub.h"
@@ -123,6 +124,68 @@ clean:
         return ret;
 }
 
+/**
+ * Determine whether the probe lives on a GPT disk or not,
+ * which is the only instance in which we'll use PartUUID
+ */
+static bool cbm_probe_is_gpt(const char *path)
+{
+        autofree(char) *parent_disk = NULL;
+        blkid_probe probe = NULL;
+        blkid_partlist parts = NULL;
+        blkid_parttable table = NULL;
+        bool ret = false;
+        const char *table_type = NULL;
+
+        /* Could be a weird image type or --path into chroot */
+        parent_disk = get_parent_disk((char *)path);
+        if (!parent_disk) {
+                return false;
+        }
+
+        probe = cbm_blkid_new_probe_from_filename(parent_disk);
+        if (!probe) {
+                LOG_ERROR("Unable to blkid probe %s", parent_disk);
+                return NULL;
+        }
+
+        cbm_blkid_probe_enable_superblocks(probe, 1);
+        cbm_blkid_probe_set_superblocks_flags(probe, BLKID_SUBLKS_TYPE);
+        cbm_blkid_probe_enable_partitions(probe, 1);
+        cbm_blkid_probe_set_partitions_flags(probe, BLKID_PARTS_ENTRY_DETAILS);
+
+        if (cbm_blkid_do_safeprobe(probe) != 0) {
+                LOG_ERROR("Error probing filesystem of %s: %s", parent_disk, strerror(errno));
+                goto clean;
+        }
+
+        parts = cbm_blkid_probe_get_partitions(probe);
+        if (cbm_blkid_partlist_numof_partitions(parts) <= 0) {
+                /* No partitions */
+                goto clean;
+        }
+
+        /* Grab the partition table */
+        table = cbm_blkid_partlist_get_table(parts);
+        if (!table) {
+                LOG_ERROR("Unable to discover partitiojn table for %s: %s",
+                          parent_disk,
+                          strerror(errno));
+                goto clean;
+        }
+
+        /* Determine the partition table type. We only care if its GPT. */
+        table_type = cbm_blkid_parttable_get_type(table);
+        if (table_type && streq(table_type, "gpt")) {
+                ret = true;
+        }
+
+clean:
+        cbm_blkid_free_probe(probe);
+        errno = 0;
+        return ret;
+}
+
 CbmDeviceProbe *cbm_probe_path(const char *path)
 {
         CbmDeviceProbe probe = { 0 };
@@ -177,6 +240,14 @@ CbmDeviceProbe *cbm_probe_path(const char *path)
                 }
         }
 
+        /* If the device isn't GPT, clear out the the PartUUID */
+        probe.gpt = cbm_probe_is_gpt(path);
+        if (!probe.gpt && probe.part_uuid) {
+                free(probe.part_uuid);
+                probe.part_uuid = NULL;
+        }
+
+        /* Now check we have at least one UUID value */
         if (!probe.part_uuid && !probe.uuid) {
                 LOG_ERROR("Unable to find UUID for %s: %s", devnode, strerror(errno));
         }

src/lib/probe.h

@@ -13,8 +13,11 @@
 
 #define _GNU_SOURCE
 
+#include <stdbool.h>
 #include <sys/types.h>
 
+#include "util.h"
+
 /**
  * A CbmCbmDeviceProbe is the result of a cbm_probe_path operation, caching
  * fields useful to clr-boot-manager in terms of partition analysis.
@@ -24,6 +27,7 @@ typedef struct CbmDeviceProbe {
         char *part_uuid; /**< PartUUID for GPT partitions */
         char *luks_uuid; /**< Parent LUKS UUID for the partition */
         dev_t dev;       /**< The device itself */
+        bool gpt;        /**<Whether this device belongs to a GPT disk */
 } CbmDeviceProbe;
 
 /**
@@ -36,6 +40,8 @@ CbmDeviceProbe *cbm_probe_path(const char *path);
  */
 void cbm_probe_free(CbmDeviceProbe *probe);
 
+DEF_AUTOFREE(CbmDeviceProbe, cbm_probe_free)
+
 /*
  * Editor modelines  -  https://www.wireshark.org/tools/modelines.html
  *

tests/blkid-harness.h

@@ -118,6 +118,18 @@ static inline int test_blkid_devno_to_wholedisk(__cbm_unused__ dev_t dev,
         return -1;
 }
 
+static inline blkid_parttable test_blkid_partlist_get_table(__cbm_unused__ blkid_partlist ls)
+{
+        /* Return a "valid" partition table */
+        return CBM_BLKID_PARTTABLE_SET;
+}
+
+static inline const char *test_blkid_parttable_get_type(__cbm_unused__ blkid_parttable tab)
+{
+        /* Return correct gpt identifier */
+        return "gpt";
+}
+
 /**
  * Default vtable for testing. Copy into a local struct and override specific
  * fields.
@@ -139,6 +151,10 @@ CbmBlkidOps BlkidTestOps = {
         .partition_get_flags = test_blkid_partition_get_flags,
         .partition_get_uuid = test_blkid_partition_get_uuid,
 
+        /* Partition table functions */
+        .partlist_get_table = test_blkid_partlist_get_table,
+        .parttable_get_type = test_blkid_parttable_get_type,
+
         /* Misc */
         .devno_to_wholedisk = test_blkid_devno_to_wholedisk,
 };

tests/check-probe.c

@@ -0,0 +1,267 @@
+/*
+ * This file is part of clr-boot-manager.
+ *
+ * This test suite is designed to resolve issue #53:
+ * https://github.com/ikeydoherty/clr-boot-manager/issues/53
+ *
+ * When writing the bootloader entries, clr-boot-manager will attempt to use
+ * the PartUUID where applicable. Originally we would use if blkid reported
+ * a valid PartUUID, resulting in root=PARTUUID= kernel parameters.
+ *
+ * However, it has been seen that blkid can and will report a PartUUID on a
+ * non GPT system, resulting in a broken boot. i.e. /dev/disk/by-partuuid/$UUID
+ * does not exist.
+ *
+ * For UEFI, GPT is only required for the EFI System Partition itself, however
+ * no such limitation is placed on the rootfs. Thus, clr-boot-manager must
+ * determine whether the disk for the rootfs is GPT or not, and *only* use
+ * a PartUUID when it is definitely GPT.
+ *
+ * Copyright © 2016-2017 Intel Corporation
+ *
+ * clr-boot-manager is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1
+ * of the License, or (at your option) any later version.
+ */
+
+#define _GNU_SOURCE
+#include <check.h>
+#include <errno.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "blkid_stub.h"
+#include "bootloader.h"
+#include "bootman.h"
+#include "config.h"
+#include "files.h"
+#include "log.h"
+#include "probe.h"
+
+#include "blkid-harness.h"
+#include "harness.h"
+#include "system-harness.h"
+
+#define PLAYGROUND_ROOT TOP_BUILD_DIR "/tests/update_playground"
+
+/**
+ * Restore the default testing vtables
+ */
+static void bootman_probe_set_default_vtables(void)
+{
+        cbm_blkid_set_vtable(&BlkidTestOps);
+        cbm_system_set_vtable(&SystemTestOps);
+}
+
+/**
+ * Coerce GPT lookup
+ */
+static inline int gpt_devno_to_wholedisk(__cbm_unused__ dev_t dev, __cbm_unused__ char *diskname,
+                                         __cbm_unused__ size_t len, __cbm_unused__ dev_t *diskdevno)
+{
+        *diskdevno = makedev(8, 8);
+        return 0;
+}
+
+/**
+ * This will force the tests to use the GPT detection codepaths
+ */
+static CbmBlkidOps gpt_blkid_ops = {
+        .probe_new_from_filename = test_blkid_new_probe_from_filename,
+        .probe_enable_superblocks = test_blkid_probe_enable_superblocks,
+        .probe_set_superblocks_flags = test_blkid_probe_set_superblocks_flags,
+        .probe_enable_partitions = test_blkid_probe_enable_partitions,
+        .probe_set_partitions_flags = test_blkid_probe_set_partitions_flags,
+        .probe_lookup_value = test_blkid_probe_lookup_value,
+        .do_safeprobe = test_blkid_do_safeprobe,
+        .free_probe = test_blkid_free_probe,
+        .probe_get_partitions = test_blkid_probe_get_partitions,
+        .partlist_numof_partitions = test_blkid_partlist_numof_partitions,
+        .partlist_get_partition = test_blkid_partlist_get_partition,
+        .partition_get_flags = test_blkid_partition_get_flags,
+        .partition_get_uuid = test_blkid_partition_get_uuid,
+        .partlist_get_table = test_blkid_partlist_get_table,
+        .parttable_get_type = test_blkid_parttable_get_type,
+        .devno_to_wholedisk = gpt_devno_to_wholedisk,
+};
+
+static inline const char *mbr_parttable_get_type(__cbm_unused__ blkid_parttable tab)
+{
+        /* Return correct mbr identifier */
+        return "mbr";
+}
+/**
+ * This will force the tests to use the MBR detection codepaths
+ */
+static CbmBlkidOps mbr_blkid_ops = {
+        .probe_new_from_filename = test_blkid_new_probe_from_filename,
+        .probe_enable_superblocks = test_blkid_probe_enable_superblocks,
+        .probe_set_superblocks_flags = test_blkid_probe_set_superblocks_flags,
+        .probe_enable_partitions = test_blkid_probe_enable_partitions,
+        .probe_set_partitions_flags = test_blkid_probe_set_partitions_flags,
+        .probe_lookup_value = test_blkid_probe_lookup_value,
+        .do_safeprobe = test_blkid_do_safeprobe,
+        .free_probe = test_blkid_free_probe,
+        .probe_get_partitions = test_blkid_probe_get_partitions,
+        .partlist_numof_partitions = test_blkid_partlist_numof_partitions,
+        .partlist_get_partition = test_blkid_partlist_get_partition,
+        .partition_get_flags = test_blkid_partition_get_flags,
+        .partition_get_uuid = test_blkid_partition_get_uuid,
+        .partlist_get_table = test_blkid_partlist_get_table,
+        .parttable_get_type = mbr_parttable_get_type,
+        .devno_to_wholedisk = gpt_devno_to_wholedisk,
+};
+
+static void bootman_probe_set_gpt_vtables(void)
+{
+        /* override test ops for legacy testing */
+        cbm_blkid_set_vtable(&gpt_blkid_ops);
+        cbm_system_set_vtable(&SystemTestOps);
+}
+
+static void bootman_probe_set_mbr_vtables(void)
+{
+        /* override test ops for legacy testing */
+        cbm_blkid_set_vtable(&mbr_blkid_ops);
+        cbm_system_set_vtable(&SystemTestOps);
+}
+
+/**
+ * Ensure we can detect GPT rootfs for a UEFI system.
+ * Note that `set_test_system_legacy` is only responsible for creating the
+ * root nodes, when we initialise the playground the UEFI nodes are also
+ * created.
+ */
+START_TEST(bootman_probe_basic_gpt)
+{
+        static PlaygroundConfig config = { "4.2.1-121.kvm", NULL, 0, .uefi = true };
+        autofree(BootManager) *m = NULL;
+        bootman_probe_set_gpt_vtables();
+        autofree(CbmDeviceProbe) *probe = NULL;
+
+        /* Let harness prep the root */
+        m = prepare_playground(&config);
+
+        set_test_system_legacy();
+        probe = cbm_probe_path(PLAYGROUND_ROOT);
+
+        fail_if(!probe, "Failed to get probe for a valid rootfs");
+        fail_if(!probe->gpt, "GPT UEFI root not detected as GPT");
+        fail_if(!probe->part_uuid, "GPT UEFI root has no PartUUID detected");
+        fail_if(!streq(probe->part_uuid, DEFAULT_PART_UUID),
+                "Expected PartUUID '%s', got '%s'",
+                DEFAULT_PART_UUID,
+                probe->part_uuid);
+}
+END_TEST
+
+/**
+ * Much like the previous test, except that we force MBR detection for the root
+ * disk
+ */
+START_TEST(bootman_probe_basic_mbr)
+{
+        static PlaygroundConfig config = { "4.2.1-121.kvm", NULL, 0, .uefi = true };
+        autofree(BootManager) *m = NULL;
+        bootman_probe_set_mbr_vtables();
+        autofree(CbmDeviceProbe) *probe = NULL;
+
+        /* Let harness prep the root */
+        m = prepare_playground(&config);
+
+        set_test_system_legacy();
+        probe = cbm_probe_path(PLAYGROUND_ROOT);
+
+        fail_if(!probe, "Failed to get probe for a valid rootfs");
+        fail_if(probe->gpt, "MBR UEFI root not detected as MBR");
+        fail_if(probe->part_uuid, "MBR UEFI root has a PartUUID detected");
+        fail_if(!streq(probe->uuid, DEFAULT_UUID),
+                "Expected UUID '%s', got '%s'",
+                DEFAULT_UUID,
+                probe->uuid);
+}
+END_TEST
+
+/**
+ * UEFI + unknown partition table
+ */
+START_TEST(bootman_probe_basic_none)
+{
+        static PlaygroundConfig config = { "4.2.1-121.kvm", NULL, 0, .uefi = true };
+        autofree(BootManager) *m = NULL;
+        bootman_probe_set_default_vtables();
+        autofree(CbmDeviceProbe) *probe = NULL;
+
+        /* Let harness prep the root */
+        m = prepare_playground(&config);
+
+        set_test_system_legacy();
+        probe = cbm_probe_path(PLAYGROUND_ROOT);
+
+        fail_if(!probe, "Failed to get probe for a valid rootfs");
+        fail_if(probe->gpt, "Unknown UEFI root not detected as MBR");
+        fail_if(probe->part_uuid, "Unknown UEFI root has a PartUUID detected");
+        fail_if(!streq(probe->uuid, DEFAULT_UUID),
+                "Expected UUID '%s', got '%s'",
+                DEFAULT_UUID,
+                probe->uuid);
+}
+END_TEST
+
+static Suite *core_suite(void)
+{
+        Suite *s = NULL;
+        TCase *tc = NULL;
+
+        s = suite_create("bootman_probe");
+
+        /* UEFI tests */
+        tc = tcase_create("bootman_probe_basic_functions");
+        tcase_add_test(tc, bootman_probe_basic_gpt);
+        tcase_add_test(tc, bootman_probe_basic_mbr);
+        tcase_add_test(tc, bootman_probe_basic_none);
+        suite_add_tcase(s, tc);
+
+        return s;
+}
+
+int main(void)
+{
+        Suite *s;
+        SRunner *sr;
+        int fail;
+
+        /* syncing can be problematic during test suite runs */
+        cbm_set_sync_filesystems(false);
+
+        /* Ensure that logging is set up properly. */
+        setenv("CBM_DEBUG", "1", 1);
+        cbm_log_init(stderr);
+
+        s = core_suite();
+        sr = srunner_create(s);
+        srunner_run_all(sr, CK_VERBOSE);
+        fail = srunner_ntests_failed(sr);
+        srunner_free(sr);
+
+        if (fail > 0) {
+                return EXIT_FAILURE;
+        }
+
+        return EXIT_SUCCESS;
+}
+
+/*
+ * Editor modelines  -  https://www.wireshark.org/tools/modelines.html
+ *
+ * Local variables:
+ * c-basic-offset: 8
+ * tab-width: 8
+ * indent-tabs-mode: nil
+ * End:
+ *
+ * vi: set shiftwidth=8 tabstop=8 expandtab:
+ * :indentSize=8:tabSize=8:noTabs=true:
+ */

tests/check-select-bootloader.c

@@ -211,6 +211,8 @@ static CbmBlkidOps legacy_blkid_ops = {
         .partlist_get_partition = test_blkid_partlist_get_partition,
         .partition_get_flags = legacy_partition_get_flags,
         .partition_get_uuid = legacy_partition_get_uuid,
+        .partlist_get_table = test_blkid_partlist_get_table,
+        .parttable_get_type = test_blkid_parttable_get_type,
         .devno_to_wholedisk = legacy_devno_to_wholedisk,
 };
 
@@ -304,6 +306,8 @@ static CbmBlkidOps grub2_blkid_ops = {
         .partlist_get_partition = test_blkid_partlist_get_partition,
         .partition_get_flags = test_blkid_partition_get_flags,
         .partition_get_uuid = test_blkid_partition_get_uuid,
+        .partlist_get_table = test_blkid_partlist_get_table,
+        .parttable_get_type = test_blkid_parttable_get_type,
         .devno_to_wholedisk = test_blkid_devno_to_wholedisk,
 };
 

tests/harness.c

@@ -566,6 +566,8 @@ void set_test_system_legacy(void)
         autofree(char) *ddir = NULL;
         autofree(char) *diskdir = NULL;
         autofree(char) *diskfile = NULL;
+        autofree(char) *diskdir_uuid = NULL;
+        autofree(char) *diskfile_uuid = NULL;
         autofree(char) *dfile = NULL;
         autofree(char) *dlink = NULL;
         const char *devfs_path = cbm_system_get_devfs_path();
@@ -594,11 +596,21 @@ void set_test_system_legacy(void)
         diskdir = string_printf("%s/disk/by-partuuid", cbm_system_get_devfs_path());
         diskfile = string_printf("%s/%s", diskdir, "Test-PartUUID");
 
+        /* Create /dev/disk/by-uuid portions */
+        diskdir_uuid = string_printf("%s/disk/by-uuid", cbm_system_get_devfs_path());
+        diskfile_uuid = string_printf("%s/%s", diskdir, "Test-UUID");
+
         nc_mkdir_p(diskdir, 00755);
         if (!file_set_text(diskfile, "clr-boot-manager Legacy testing")) {
                 DECLARE_OOM();
                 abort();
         }
+
+        nc_mkdir_p(diskdir_uuid, 00755);
+        if (!file_set_text(diskfile_uuid, "clr-boot-manager Legacy testing")) {
+                DECLARE_OOM();
+                abort();
+        }
 }
 
 /*