From 3bd22a9cc814764cfa89ac54f1a3289ad9eeebfa Mon Sep 17 00:00:00 2001 From: Marcel Moolenaar Date: Sun, 9 Jun 2013 23:34:26 +0000 Subject: [PATCH] Change the set and unset ctlreqs by making the index argument optional. This allows setting attributes on tables. One simply does not provide an index in that case. Otherwise the entry corresponding the index has the attribute set or unset. Use this change to fix a relatively longstanding bug in our GPT scheme that's the result of rev 198097 (relatively harmless) followed by rev 237057 (damaging). The damaging part being that our GPT scheme always has the active flag set on the PMBR slice. This is in violation with EFI. Existing EFI implementions for both x86 and ia64 reject the GPT. As such, GPT disks created by us aren't usable under EFI because of that. After this change, GPT disks never have the active flag set on the PMBR slice. In order to make the GPT disk bootable under some x86 BIOSes, the reason of rev 198097, one must now set the active attribute on the gpt table. The kernel will apply this to the PMBR slice For (S)ATA: gpart set -a active ada0 To fix an existing GPT disk that has the active flag set in the PMBR, and that does not need the flag, use (again for (S)ATA): gpart unset -a active ada0 The EBR, MBR & PC98 schemes, which also impement at least 1 attribute, now check to make sure the entry passed is valid. They do not have attributes that apply to the table. --- sbin/geom/class/part/geom_part.c | 8 ++-- sys/geom/part/g_part.c | 39 ++++++++++++-------- sys/geom/part/g_part_ebr.c | 2 + sys/geom/part/g_part_gpt.c | 63 ++++++++++++++++++++++---------- sys/geom/part/g_part_mbr.c | 2 + sys/geom/part/g_part_pc98.c | 3 ++ 6 files changed, 78 insertions(+), 39 deletions(-) diff --git a/sbin/geom/class/part/geom_part.c b/sbin/geom/class/part/geom_part.c index 8a57ca44ebf6..9b3e0e5e451b 100644 --- a/sbin/geom/class/part/geom_part.c +++ b/sbin/geom/class/part/geom_part.c @@ -147,10 +147,10 @@ struct g_command PUBSYM(class_commands)[] = { }, { "set", 0, gpart_issue, { { 'a', "attrib", NULL, G_TYPE_STRING }, - { 'i', GPART_PARAM_INDEX, NULL, G_TYPE_NUMBER }, + { 'i', GPART_PARAM_INDEX, G_VAL_OPTIONAL, G_TYPE_NUMBER }, { 'f', "flags", GPART_FLAGS, G_TYPE_STRING }, G_OPT_SENTINEL }, - "-a attrib -i index [-f flags] geom" + "-a attrib [-i index] [-f flags] geom" }, { "show", 0, gpart_show, { { 'l', "show_label", NULL, G_TYPE_BOOL }, @@ -164,10 +164,10 @@ struct g_command PUBSYM(class_commands)[] = { }, { "unset", 0, gpart_issue, { { 'a', "attrib", NULL, G_TYPE_STRING }, - { 'i', GPART_PARAM_INDEX, NULL, G_TYPE_NUMBER }, + { 'i', GPART_PARAM_INDEX, G_VAL_OPTIONAL, G_TYPE_NUMBER }, { 'f', "flags", GPART_FLAGS, G_TYPE_STRING }, G_OPT_SENTINEL }, - "-a attrib -i index [-f flags] geom" + "-a attrib [-i index] [-f flags] geom" }, { "resize", 0, gpart_issue, { { 'a', "alignment", GPART_AUTOFILL, G_TYPE_STRING }, diff --git a/sys/geom/part/g_part.c b/sys/geom/part/g_part.c index 1583122e9443..31b535aa2a01 100644 --- a/sys/geom/part/g_part.c +++ b/sys/geom/part/g_part.c @@ -1352,16 +1352,20 @@ g_part_ctl_setunset(struct gctl_req *req, struct g_part_parms *gpp, table = gp->softc; - LIST_FOREACH(entry, &table->gpt_entry, gpe_entry) { - if (entry->gpe_deleted || entry->gpe_internal) - continue; - if (entry->gpe_index == gpp->gpp_index) - break; - } - if (entry == NULL) { - gctl_error(req, "%d index '%d'", ENOENT, gpp->gpp_index); - return (ENOENT); - } + if (gpp->gpp_parms & G_PART_PARM_INDEX) { + LIST_FOREACH(entry, &table->gpt_entry, gpe_entry) { + if (entry->gpe_deleted || entry->gpe_internal) + continue; + if (entry->gpe_index == gpp->gpp_index) + break; + } + if (entry == NULL) { + gctl_error(req, "%d index '%d'", ENOENT, + gpp->gpp_index); + return (ENOENT); + } + } else + entry = NULL; error = G_PART_SETUNSET(table, entry, gpp->gpp_attrib, set); if (error) { @@ -1374,8 +1378,11 @@ g_part_ctl_setunset(struct gctl_req *req, struct g_part_parms *gpp, sb = sbuf_new_auto(); sbuf_printf(sb, "%s %sset on ", gpp->gpp_attrib, (set) ? "" : "un"); - G_PART_FULLNAME(table, entry, sb, gp->name); - sbuf_printf(sb, "\n"); + if (entry) + G_PART_FULLNAME(table, entry, sb, gp->name); + else + sbuf_cat(sb, gp->name); + sbuf_cat(sb, "\n"); sbuf_finish(sb); gctl_set_param(req, "output", sbuf_data(sb), sbuf_len(sb) + 1); sbuf_delete(sb); @@ -1581,8 +1588,8 @@ g_part_ctlreq(struct gctl_req *req, struct g_class *mp, const char *verb) case 's': if (!strcmp(verb, "set")) { ctlreq = G_PART_CTL_SET; - mparms |= G_PART_PARM_ATTRIB | G_PART_PARM_GEOM | - G_PART_PARM_INDEX; + mparms |= G_PART_PARM_ATTRIB | G_PART_PARM_GEOM; + oparms |= G_PART_PARM_INDEX; } break; case 'u': @@ -1592,8 +1599,8 @@ g_part_ctlreq(struct gctl_req *req, struct g_class *mp, const char *verb) modifies = 0; } else if (!strcmp(verb, "unset")) { ctlreq = G_PART_CTL_UNSET; - mparms |= G_PART_PARM_ATTRIB | G_PART_PARM_GEOM | - G_PART_PARM_INDEX; + mparms |= G_PART_PARM_ATTRIB | G_PART_PARM_GEOM; + oparms |= G_PART_PARM_INDEX; } break; } diff --git a/sys/geom/part/g_part_ebr.c b/sys/geom/part/g_part_ebr.c index 889f26378113..03b9aac72c54 100644 --- a/sys/geom/part/g_part_ebr.c +++ b/sys/geom/part/g_part_ebr.c @@ -540,6 +540,8 @@ g_part_ebr_setunset(struct g_part_table *table, struct g_part_entry *baseentry, struct g_part_ebr_entry *entry; int changed; + if (baseentry == NULL) + return (ENODEV); if (strcasecmp(attrib, "active") != 0) return (EINVAL); diff --git a/sys/geom/part/g_part_gpt.c b/sys/geom/part/g_part_gpt.c index 405b49212e26..12b0f8920b90 100644 --- a/sys/geom/part/g_part_gpt.c +++ b/sys/geom/part/g_part_gpt.c @@ -260,6 +260,16 @@ gpt_map_type(struct uuid *t) return (0); } +static void +gpt_create_pmbr(struct g_part_gpt_table *table, struct g_provider *pp) +{ + + bzero(table->mbr + DOSPARTOFF, DOSPARTSIZE * NDOSPART); + gpt_write_mbr_entry(table->mbr, 0, 0xee, 1, + MIN(pp->mediasize / pp->sectorsize - 1, UINT32_MAX)); + le16enc(table->mbr + DOSMAGICOFFSET, DOSMAGIC); +} + /* * Under Boot Camp the PMBR partition (type 0xEE) doesn't cover the * whole disk anymore. Rather, it covers the GPT table and the EFI @@ -284,7 +294,7 @@ gpt_is_bootcamp(struct g_part_gpt_table *table, const char *provname) } static void -gpt_update_bootcamp(struct g_part_table *basetable) +gpt_update_bootcamp(struct g_part_table *basetable, struct g_provider *pp) { struct g_part_entry *baseentry; struct g_part_gpt_entry *entry; @@ -341,6 +351,7 @@ gpt_update_bootcamp(struct g_part_table *basetable) disable: table->bootcamp = 0; + gpt_create_pmbr(table, pp); } static struct gpt_hdr * @@ -609,6 +620,8 @@ g_part_gpt_create(struct g_part_table *basetable, struct g_part_parms *gpp) pp->sectorsize) return (ENOSPC); + gpt_create_pmbr(table, pp); + /* Allocate space for the header */ table->hdr = g_malloc(sizeof(struct gpt_hdr), M_WAITOK | M_ZERO); @@ -936,9 +949,13 @@ g_part_gpt_read(struct g_part_table *basetable, struct g_consumer *cp) static int g_part_gpt_recover(struct g_part_table *basetable) { + struct g_part_gpt_table *table; + struct g_provider *pp; - g_gpt_set_defaults(basetable, - LIST_FIRST(&basetable->gpt_gp->consumer)->provider); + table = (struct g_part_gpt_table *)basetable; + pp = LIST_FIRST(&basetable->gpt_gp->consumer)->provider; + gpt_create_pmbr(table, pp); + g_gpt_set_defaults(basetable, pp); basetable->gpt_corrupt = 0; return (0); } @@ -949,6 +966,7 @@ g_part_gpt_setunset(struct g_part_table *basetable, { struct g_part_gpt_entry *entry; struct g_part_gpt_table *table; + uint8_t *p; uint64_t attr; int i; @@ -956,15 +974,32 @@ g_part_gpt_setunset(struct g_part_table *basetable, entry = (struct g_part_gpt_entry *)baseentry; if (strcasecmp(attrib, "active") == 0) { - if (!table->bootcamp || baseentry->gpe_index > NDOSPART) - return (EINVAL); - for (i = 0; i < NDOSPART; i++) { - table->mbr[DOSPARTOFF + i * DOSPARTSIZE] = - (i == baseentry->gpe_index - 1) ? 0x80 : 0; + if (table->bootcamp) { + /* The active flag must be set on a valid entry. */ + if (entry == NULL) + return (ENXIO); + if (baseentry->gpe_index > NDOSPART) + return (EINVAL); + for (i = 0; i < NDOSPART; i++) { + p = &table->mbr[DOSPARTOFF + i * DOSPARTSIZE]; + p[0] = (i == baseentry->gpe_index - 1) + ? ((set) ? 0x80 : 0) : 0; + } + } else { + /* The PMBR is marked as active without an entry. */ + if (entry != NULL) + return (ENXIO); + for (i = 0; i < NDOSPART; i++) { + p = &table->mbr[DOSPARTOFF + i * DOSPARTSIZE]; + p[0] = (p[4] == 0xee) ? ((set) ? 0x80 : 0) : 0; + } } return (0); } + if (entry == NULL) + return (ENODEV); + attr = 0; if (strcasecmp(attrib, "bootme") == 0) { attr |= GPT_ENT_ATTR_BOOTME; @@ -1032,17 +1067,7 @@ g_part_gpt_write(struct g_part_table *basetable, struct g_consumer *cp) /* Reconstruct the MBR from the GPT if under Boot Camp. */ if (table->bootcamp) - gpt_update_bootcamp(basetable); - - /* Update partition entries in the PMBR if Boot Camp disabled. */ - if (!table->bootcamp) { - bzero(table->mbr + DOSPARTOFF, DOSPARTSIZE * NDOSPART); - gpt_write_mbr_entry(table->mbr, 0, 0xee, 1, - MIN(pp->mediasize / pp->sectorsize - 1, UINT32_MAX)); - /* Mark the PMBR active since some BIOS require it. */ - table->mbr[DOSPARTOFF] = 0x80; - } - le16enc(table->mbr + DOSMAGICOFFSET, DOSMAGIC); + gpt_update_bootcamp(basetable, pp); /* Write the PMBR */ buf = g_malloc(pp->sectorsize, M_WAITOK | M_ZERO); diff --git a/sys/geom/part/g_part_mbr.c b/sys/geom/part/g_part_mbr.c index d522d023da13..459aee67363b 100644 --- a/sys/geom/part/g_part_mbr.c +++ b/sys/geom/part/g_part_mbr.c @@ -497,6 +497,8 @@ g_part_mbr_setunset(struct g_part_table *table, struct g_part_entry *baseentry, struct g_part_mbr_entry *entry; int changed; + if (baseentry == NULL) + return (ENODEV); if (strcasecmp(attrib, "active") != 0) return (EINVAL); diff --git a/sys/geom/part/g_part_pc98.c b/sys/geom/part/g_part_pc98.c index 0d81a0e0c84f..4d2b60c36444 100644 --- a/sys/geom/part/g_part_pc98.c +++ b/sys/geom/part/g_part_pc98.c @@ -498,6 +498,9 @@ g_part_pc98_setunset(struct g_part_table *table, struct g_part_entry *baseentry, struct g_part_pc98_entry *entry; int changed, mid, sid; + if (baseentry == NULL) + return (ENODEV); + mid = sid = 0; if (strcasecmp(attrib, "active") == 0) sid = 1;