[PATCH 0/1] inspect: Fix a bug in the *BSD root detection

[Nikos Skalkotos]

Hello,

I've been reading the *BSD detection code in check_filesystem() in
inspect-fs.c in order to write a patch for OpenBSD detection that is
missing. Both, in FreeBSD and in NetBSD you have this piece of code:


/* Ignore /dev/sda1 which is a shadow of the real root filesystem
* that is probably /dev/sda5 (see:
* http://www.freebsd.org/doc/handbook/disk-organization.html)
*/
if (m->im_type == MOUNTABLE_DEVICE &&
match (g, m->im_device, re_first_partition))
return 0;

This assumption is not always true. First of all, the MBR partition
that hosts the disklabel could be any of the primary partitions. Not
just the first one. The OpenBSD installer for example will by default
use the 4th partition. I'm pretty sure you can change this in FreeBSD
and NetBSD too.

The other thing is that /dev/sda5 will not always be there. The problem
is the weird way the kernel maps the disklabel partitions:

https://github.com/torvalds/linux/blob/master/block/partitions/msdos.c#L303-L310

The kernel will not map a disklabel subpartition if it has the same
boundaries with the parent (MBR) partition or if its boundaries exceed
the disklabel.

In the disklabel's partition table 'a' is always the root partition, 'b'
is the swap, in OpenBSD and FreeBSD 'c' is the full hard disk. In NetBSD
'c' is the disklabel partition itself and 'd' is the full hard disk.
Those partitions are invalid for the kernel. If I have a *BSD
installation with just root and swap, the kernel will show 3 partitions:
/dev/sda1 for the disklabel, /dev/sda5 for root and /dev/sda6 for swap,
but If I only have a root partition then this will likely have the same
boundaries with the MBR partition and the kernel will not map it again
to /dev/sda5. In this case inspect_os() will completely miss the OS.

I tried to fix this by removing the code above and checking again for
duplicated *BSD root partitions after all the file systems have been
scanned to unmark the is_root. It's not the prettiest thing in the
world but I couldn't think of something better.

Nikos

Nikos Skalkotos (1):
inspect: Fix a bug in the *BSD root detection

src/guestfs-internal.h | 1 +
src/inspect-fs.c | 55 +++++++++++++++++++++++++++++++++-----------------
src/inspect.c | 6 ++++++
3 files changed, 43 insertions(+), 19 deletions(-)

--
2.1.3

[Nikos Skalkotos]

The assumption that Linux will map the MBR partition to /dev/sda1
and the BSD 'a' partition to /dev/sda5 is not always correct.

Signed-off-by: Nikos Skalkotos <skal...@grnet.gr>
---

src/guestfs-internal.h | 1 +
src/inspect-fs.c | 55 +++++++++++++++++++++++++++++++++-----------------
src/inspect.c | 6 ++++++
3 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index fd0c4a1..2460d25 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -746,6 +746,7 @@ extern int guestfs___is_file_nocase (guestfs_h *g, const char *);
extern int guestfs___is_dir_nocase (guestfs_h *g, const char *);
extern int guestfs___check_for_filesystem_on (guestfs_h *g,
const char *mountable);
+extern void guestfs___check_for_dublicated_bsd_root(guestfs_h *g);
extern int guestfs___parse_unsigned_int (guestfs_h *g, const char *str);
extern int guestfs___parse_unsigned_int_ignore_trailing (guestfs_h *g, const char *str);
extern int guestfs___parse_major_minor (guestfs_h *g, struct inspect_fs *fs);
diff --git a/src/inspect-fs.c b/src/inspect-fs.c
index 539d814..99a8658 100644
--- a/src/inspect-fs.c
+++ b/src/inspect-fs.c
@@ -47,7 +47,7 @@
* multiple threads call into the libguestfs API functions below
* simultaneously.
*/
-static pcre *re_first_partition;
+static pcre *re_primary_partition;
static pcre *re_major_minor;

static void compile_regexps (void) __attribute__((constructor));
@@ -68,14 +68,14 @@ compile_regexps (void)
} \
} while (0)

- COMPILE (re_first_partition, "^/dev/(?:h|s|v)d.1$", 0);
+ COMPILE (re_primary_partition, "^/dev/(?:h|s|v)d.[1234]$", 0);
COMPILE (re_major_minor, "(\\d+)\\.(\\d+)", 0);
}

static void
free_regexps (void)
{
- pcre_free (re_first_partition);
+ pcre_free (re_primary_partition);
pcre_free (re_major_minor);
}

@@ -84,6 +84,39 @@ static int check_filesystem (guestfs_h *g, const char *mountable,
int whole_device);
static int extend_fses (guestfs_h *g);

+/* On *BSD systems, sometimes /dev/sda[1234] is a shadow of the real root
+ * filesystem that is probably /dev/sda5
+ * (see: http://www.freebsd.org/doc/handbook/disk-organization.html)
+ */
+void
+guestfs___check_for_dublicated_bsd_root(guestfs_h *g)
+{
+ size_t i;
+ bool is_primary, is_bsd;
+ struct inspect_fs *fs, *bsd_primary = NULL;
+
+ for (i = 0; i < g->nr_fses; ++i) {
+ fs = &g->fses[i];
+
+ is_primary = match (g, fs->mountable, re_primary_partition);
+ is_bsd = ((fs->type == OS_TYPE_FREEBSD) || \
+ (fs->type == OS_TYPE_NETBSD) || \
+ (fs->type == OS_TYPE_OPENBSD));
+
+ if (fs->is_root && is_primary && is_bsd) {
+ bsd_primary = fs;
+ continue;
+ }
+
+ if (fs->is_root && bsd_primary && (bsd_primary->type == fs->type)) {
+ /* remove the is root flag from the bsd_primary */
+ bsd_primary->is_root = 0;
+ bsd_primary->format = OS_FORMAT_UNKNOWN;
+ return;
+ }
+ }
+}
+
/* Find out if 'device' contains a filesystem. If it does, add
* another entry in g->fses.
*/
@@ -207,14 +240,6 @@ check_filesystem (guestfs_h *g, const char *mountable,
is_dir_bin &&
guestfs_is_file (g, "/etc/freebsd-update.conf") > 0 &&
guestfs_is_file (g, "/etc/fstab") > 0) {
- /* Ignore /dev/sda1 which is a shadow of the real root filesystem
- * that is probably /dev/sda5 (see:
- * http://www.freebsd.org/doc/handbook/disk-organization.html)
- */
- if (m->im_type == MOUNTABLE_DEVICE &&
- match (g, m->im_device, re_first_partition))
- return 0;
-
fs->is_root = 1;
fs->format = OS_FORMAT_INSTALLED;
if (guestfs___check_freebsd_root (g, fs) == -1)
@@ -225,14 +250,6 @@ check_filesystem (guestfs_h *g, const char *mountable,
guestfs_is_file (g, "/netbsd") > 0 &&
guestfs_is_file (g, "/etc/fstab") > 0 &&
guestfs_is_file (g, "/etc/release") > 0) {
- /* Ignore /dev/sda1 which is a shadow of the real root filesystem
- * that is probably /dev/sda5 (see:
- * http://www.freebsd.org/doc/handbook/disk-organization.html)
- */
- if (m->im_type == MOUNTABLE_DEVICE &&
- match (g, m->im_device, re_first_partition))
- return 0;
-
fs->is_root = 1;
fs->format = OS_FORMAT_INSTALLED;
if (guestfs___check_netbsd_root (g, fs) == -1)
diff --git a/src/inspect.c b/src/inspect.c
index 9248b06..048b059 100644
--- a/src/inspect.c
+++ b/src/inspect.c
@@ -64,6 +64,12 @@ guestfs__inspect_os (guestfs_h *g)
}
}

+ /* Check if the same filesystem was listed twice as root in g->fses.
+ * This may happen for the *BSD root partition where an MBR partition
+ * is a shadow of the real root partition probably /dev/sda5
+ */
+ guestfs___check_for_dublicated_bsd_root(g);
+
/* At this point we have, in the handle, a list of all filesystems
* found and data about each one. Now we assemble the list of
* filesystems which are root devices and return that to the user.
--
2.1.3

[Richard W.M. Jones]

How about the attached patch? It's basically the same as your patch
but I moved the code between files and tidied up some whitespace
issues.

Rich.

--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

[Richard W.M. Jones]

On Fri, Nov 28, 2014 at 03:42:58PM +0100, Pino Toscano wrote:

> On Friday 28 November 2014 14:31:01 Richard W.M. Jones wrote:
> > How about the attached patch? It's basically the same as your patch
> > but I moved the code between files and tidied up some whitespace
> > issues.
>

> Present in both the patches:

>
> > +/* On *BSD systems, sometimes /dev/sda[1234] is a shadow of the real root
> > + * filesystem that is probably /dev/sda5
> > + * (see: http://www.freebsd.org/doc/handbook/disk-organization.html)
> > + */

> > +static void
> > +check_for_duplicated_bsd_root (guestfs_h *g)

> > +{
> > + size_t i;
> > + bool is_primary, is_bsd;
> > + struct inspect_fs *fs, *bsd_primary = NULL;
> > +
> > + for (i = 0; i < g->nr_fses; ++i) {
> > + fs = &g->fses[i];
> > +
> > + is_primary = match (g, fs->mountable, re_primary_partition);
> > + is_bsd =

> > + fs->type == OS_TYPE_FREEBSD ||
> > + fs->type == OS_TYPE_NETBSD ||
> > + fs->type == OS_TYPE_OPENBSD;

> > +
> > + if (fs->is_root && is_primary && is_bsd) {
> > + bsd_primary = fs;
> > + continue;
> > + }
>

> This will run the regexp matching for every filesystem found; what
> about inlining the match call as last part of the if, like:
>
> is_bsd =
> fs->type == OS_TYPE_FREEBSD ||
> fs->type == OS_TYPE_NETBSD ||
> fs->type == OS_TYPE_OPENBSD;
>
> if (fs->is_root && is_bsd && is_primary
> && match (g, fs->mountable, re_primary_partition)) {
> bsd_primary = fs;
> continue;
> }
>
> This way it is done only for *BSD filesystems, and is_primary is
> used only in that if anyway.
>
> Also, is_bsd and fs could be declared just inside the for, as they are
> not needed outside of it.

Good points.

See updated patch below.

Rich.

--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com

virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

[Pino Toscano]

On Friday 28 November 2014 14:31:01 Richard W.M. Jones wrote:

> How about the attached patch? It's basically the same as your patch
> but I moved the code between files and tidied up some whitespace
> issues.

Present in both the patches:

> +/* On *BSD systems, sometimes /dev/sda[1234] is a shadow of the real root
> + * filesystem that is probably /dev/sda5
> + * (see: http://www.freebsd.org/doc/handbook/disk-organization.html)
> + */

> +static void
> +check_for_duplicated_bsd_root (guestfs_h *g)

> +{
> + size_t i;
> + bool is_primary, is_bsd;
> + struct inspect_fs *fs, *bsd_primary = NULL;
> +
> + for (i = 0; i < g->nr_fses; ++i) {
> + fs = &g->fses[i];
> +
> + is_primary = match (g, fs->mountable, re_primary_partition);
> + is_bsd =

> + fs->type == OS_TYPE_FREEBSD ||
> + fs->type == OS_TYPE_NETBSD ||
> + fs->type == OS_TYPE_OPENBSD;

> +
> + if (fs->is_root && is_primary && is_bsd) {
> + bsd_primary = fs;
> + continue;
> + }

This will run the regexp matching for every filesystem found; what
about inlining the match call as last part of the if, like:

is_bsd =
fs->type == OS_TYPE_FREEBSD ||
fs->type == OS_TYPE_NETBSD ||
fs->type == OS_TYPE_OPENBSD;

if (fs->is_root && is_bsd && is_primary
&& match (g, fs->mountable, re_primary_partition)) {
bsd_primary = fs;
continue;
}

This way it is done only for *BSD filesystems, and is_primary is
used only in that if anyway.

Also, is_bsd and fs could be declared just inside the for, as they are
not needed outside of it.

--
Pino Toscano

[Nikos Skalkotos]

LGTM

Another thing concerning the disklabel partitions is that
list_filesystems() will print both /dev/sda1 and /dev/sda5 as ufs file
systems. I don't know if you care to change this. The fact is that both
are mountable.

P.S. I wish the kernel folks would stop treating the disklabel
partitions as logical partitions. Logical partitions are sequential,
there are no gaps in the numbering. On the other hand disklabel may
have gaps like the MBR partitions. Having defined partitions 'a', 'b'
and 'c' is completely different than having defined partitions 'a', 'c'
and 'd'. The kernel in both cases will show /dev/sda5 and /dev/sda6. I
wonder if they could get convinced to change that...

[Richard W.M. Jones]

On Fri, Nov 28, 2014 at 06:03:15PM +0200, Nikos Skalkotos wrote:
> LGTM
>
> Another thing concerning the disklabel partitions is that
> list_filesystems() will print both /dev/sda1 and /dev/sda5 as ufs file
> systems. I don't know if you care to change this. The fact is that both
> are mountable.

While this could confuse some clients, I'm not sure that attempting to
fix it would improve the situation, so I think it's best to leave it.

> P.S. I wish the kernel folks would stop treating the disklabel
> partitions as logical partitions. Logical partitions are sequential,
> there are no gaps in the numbering. On the other hand disklabel may
> have gaps like the MBR partitions. Having defined partitions 'a', 'b'
> and 'c' is completely different than having defined partitions 'a', 'c'
> and 'd'. The kernel in both cases will show /dev/sda5 and /dev/sda6. I
> wonder if they could get convinced to change that...

Agreed on the problem.

Thanks - I will push the modified patch shortly.

Rich.

--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com

Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW