New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 901479 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Last visit > 30 days ago
Components:
EstimatedDays: 2
NextAction: ----
OS: Chrome
Pri: 3
Type: Feature



Sign in to add a comment

Package sys-fs/dosfstools: upgrade to latest stable upstream version

Project Member Reported by ncrews@google.com, Nov 2

Issue description

Upgrade the package for the dosfstools library.
Latest upstream stable is 4.1 vs current 3.0.26
 
Worked with bmgordon@ on this bug.

# Background

The CrOS disk image is comprised of 12 different partitions: some are for different versions of the kernel, the OS, some of the stateful info on the drive, etc. To format the filesystems onto the partitions, we rely on various libraries. However, these libraries are designed to operate on devices, whereas we want them to be performed onto partitions. Therefore, we use something called loop devices. Loop devices look like any other device (eg usb, hdd) on the host filesystem, but when you write or read them you are actually modifying some block of memory on the machine. These loop devices appear as any other device, for instance as /dev/loop0, /dev/loop1, /dev/loop3, etc. To make a disk image, we create a binary file which will be the contents of the final disk image, and then we delineate 12 chunks within that file, one for each partition. When we want to modify a partition, we associate a loop device to each of these blocks of memory. For instance, as we format each of these partitions, we could point the loopback device /dev/loop3 to point to one of the chunks within our binary file:

https://cs.corp.google.com/chromeos_public/src/scripts/build_library/disk_layout_util.sh?l=403

Then when we read or write to the device, we are actually just reading or writing to that chunk of memory within the binary file.

# Error

One of the partitions is in the vfat filesystem format. We rely on the dosfstools library (github.com/dosfstools/dosfstools/) to do the formatting for this partition. We are currently using version 3.0.27, but wanted to upgrade to the 4.1 version. However, after this upgrade, the mkfs.fat utility within dosfstools throws an error during the disk image build process, saying that it doesn't want to format the loopback device, since the device appears to contain partitions:

> ./build_image --board=veyron_minnie test
> ...
> ...
> INFO    : Creating FS for partition 8 with format ext4.
> tune2fs 1.44.1 (24-Mar-2018)
> Setting maximal mount count to -1
> Setting error behavior to 2
> Setting interval between checks to 0 seconds
> Setting reserved blocks percentage to 0% (0 blocks)
> Setting reserved blocks count to 0
> Setting time filesystem last checked to Thu Nov 19 11:00:00 2009
> 
> INFO    : Creating FS for partition 12 with format vfat.
> mkfs.fat 4.1 (2017-01-24)
> mkfs.vfat: Partitions or virtual mappings on device '/dev/loop1', not making > filesystem (use -I to override)
> An error occurred in your build so your latest output directory is invalid.
> Would you like to delete the output directory (y/N)? 

This is a fairly inconsistent error though. Maybe every 4th try mkfs.vfat completes with no error. On every failed attempt a new /dev/loopN device is created but not cleaned up, so the /dev/ directory slowly gets filled with garbage devices. This error happens while building the image for the grunt board as well, so I'm guessing it will happen for all the boards. 

# Discussion

The main build_image script calls the disk_layout_util.sh script which calls the mkfs.fat command line tool:

https://cs.corp.google.com/chromeos_public/src/scripts/build_library/disk_layout_util.sh?l=447

Within mkfs.fat, the error happens at:

https://github.com/dosfstools/dosfstools/blob/ca549534762c794e5a4cdc36143d17635d196c6a/src/mkfs.fat.c#L1710

In earlier versions of dosfstools, this safety check was not performed, so it was possible to format a filesystem over the top of a device which already has partitions. We aren't sure of the exact cause of the error: Did we discover a bug from earlier where we are accidentally partitioning the vfat partition, or is everything fine and the error is a false alarm?

This error was introduced in dosfstools 4.0 when udev support was added. This seems to be what is causing the error: udev believes that /dev/loopN possesses "children" AKA partitions. The question is whether udev is wrong about this, or if our partition was itself partitioned. The code that actually determines the number of children is happening here:

https://github.com/dosfstools/dosfstools/tree/master/src/device_info.c#L143

# Solutions
There are two possible solutions that we came up with. If we turn off the USE=udev flag for the dosfstools gentoo package, then this extra probing of the partition using udev does not happen, and there is never the safety check for the device being partitioned. The other option is to pass the -I option to mkfs.vfat, which tells it to not perform the safety check.

We think the better solution is to use the -I flag:
1. This requires changing one line of code in the disk_layout_util.sh script, and doesn't change the underlying intended behavior of the upstream package. It could be more compatible with future versions of dosfstools.
2. The biggest concern we thought of against the -I solution was that it was covering up an underlying issue, but we don't see any evidence of an underlying issue. The code that sets up the partitions is the same as before, so we probably haven't introduced a bug there. Perhaps with this new filesystem formatting code, we are accidentally looking at and then formatting the wrong area within the disk image binary, but the code that points the loopback device to the respective chunks within the disk image also hasn't changed. And if the rest of the build_image works, and then the image works on the chromebook, it seems like something as major as overwriting an entire other filesystem would show up.

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosutils/+/639a155b78ef2c44980d54d130d7defe180a4b6e

commit 639a155b78ef2c44980d54d130d7defe180a4b6e
Author: Nick Crews <ncrews@google.com>
Date: Tue Dec 04 04:31:57 2018

disk_layout_util.sh: Pass -I flag to mkfs.fat

This was to be compatible with the upgrade of the portage package
sys-fs/dosfstools from 3.0.27 to 4.1.
This ignores a *possibly* wrong error message about formatting
a device which is already partitioned. See crbug.com/901479 for
more discussion.

BUG=chromium:901479
TEST=building seems to work as before, the error we are covering
   up seems to be a problem with udev, possibly because we're
   inside chroot.
CQ-DEPEND=CL:1323910

Change-Id: I4d53b05036a285eb3b54af84f1486cc3bb532cf1
Reviewed-on: https://chromium-review.googlesource.com/1323474
Commit-Ready: Nick Crews <ncrews@chromium.org>
Tested-by: Nick Crews <ncrews@chromium.org>
Reviewed-by: Benjamin Gordon <bmgordon@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/639a155b78ef2c44980d54d130d7defe180a4b6e/build_library/disk_layout_util.sh

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/portage-stable/+/5184268f3953b3d166f705eac33b4825b9853bc6

commit 5184268f3953b3d166f705eac33b4825b9853bc6
Author: Nick Crews <ncrews@google.com>
Date: Tue Dec 04 04:31:57 2018

dosfstools: upgraded package to upstream

Upgraded sys-fs/dosfstools to version 4.1 on amd64, arm.
This seemed to cause an error when trying to create the vfat filesystem
on one of the partitions of the disk image, such that udev thinks
that the partition is itself partitioned. To fix this we had to change
the build script to ginore this error, and submit this as a separate CL.
See the linked bug for more discussion.

BUG=chromium:901479
TEST=See the bug for more info
CQ-DEPEND=CL:1323474

Change-Id: I9cbbbf8242e9b6073f5ca822eac1d40ff0938801
Reviewed-on: https://chromium-review.googlesource.com/1323910
Commit-Ready: Nick Crews <ncrews@chromium.org>
Tested-by: Nick Crews <ncrews@chromium.org>
Reviewed-by: Benjamin Gordon <bmgordon@chromium.org>

[add] https://crrev.com/5184268f3953b3d166f705eac33b4825b9853bc6/sys-fs/dosfstools/files/dosfstools-4.0-udevlibs.patch
[delete] https://crrev.com/91de605f9681b5db74edb284a8e6372f70bd7692/sys-fs/dosfstools/dosfstools-3.0.26.ebuild
[add] https://crrev.com/5184268f3953b3d166f705eac33b4825b9853bc6/metadata/md5-cache/sys-fs/dosfstools-4.1
[delete] https://crrev.com/91de605f9681b5db74edb284a8e6372f70bd7692/sys-fs/dosfstools/files/dosfstools-3.0.20-name-ext.patch
[modify] https://crrev.com/5184268f3953b3d166f705eac33b4825b9853bc6/sys-fs/dosfstools/metadata.xml
[modify] https://crrev.com/5184268f3953b3d166f705eac33b4825b9853bc6/sys-fs/dosfstools/Manifest
[delete] https://crrev.com/91de605f9681b5db74edb284a8e6372f70bd7692/metadata/md5-cache/sys-fs/dosfstools-3.0.26
[add] https://crrev.com/5184268f3953b3d166f705eac33b4825b9853bc6/sys-fs/dosfstools/dosfstools-4.1.ebuild

Sign in to add a comment