Re: [PATCH 3/3] installer: Bind-mount /sys/firmware/efi/efivars into chroot

Hello Michael,

interesting. I understood that wiki page the other way; a "looks good to me" intuitively sounds weaker than "I have reviewed this and am okay with it" to me. Glad you mentioned this.

Thanks, and best regards, Peter Müller

Isn’t an Ack a stronger kind of Review?

https://wiki.ipfire.org/devel/git/tags

However, thanks for looking at this.

-Michael

...

On 17 Nov 2021, at 20:11, Peter Müller peter.mueller@ipfire.org wrote:

Hello Michael,

lacking C skills, this one looks good to me, but I am not confident enough to tag it as being reviewed.

Acked-by: Peter Müller peter.mueller@ipfire.org

Thanks, and best regards, Peter Müller

...

Signed-off-by: Michael Tremer michael.tremer@ipfire.org

src/installer/hw.c | 113 +++++++++++++++++++++++++++++-------------- src/installer/hw.h | 2 +- src/installer/main.c | 2 +- 3 files changed, 78 insertions(+), 39 deletions(-) diff --git a/src/installer/hw.c b/src/installer/hw.c index 71a1f1cce..265df2d8c 100644 --- a/src/installer/hw.c +++ b/src/installer/hw.c @@ -46,13 +46,6 @@ #include "hw.h" -const char* other_filesystems[] = {

• "/dev",
• "/proc",
• "/sys",
• NULL

-};

static int system_chroot(const char* output, const char* path, const char* cmd) { char chroot_cmd[STRING_SIZE]; @@ -149,14 +142,53 @@ int hw_mount(const char* source, const char* target, const char* fs, int flags) return mount(source, target, fs, flags, NULL); } -int hw_umount(const char* target) {

• int r = umount2(target, 0);

+static int hw_bind_mount(const char* source, const char* prefix) {

• if (!source || !prefix) {

• errno = EINVAL;
  

• return 1;
  

• }

• if (r && errno == EBUSY) {

• // Give it a moment to settle
  

• sleep(1);
  

• char target[PATH_MAX];
• int r;
• // Format target
• r = snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source);
• if (r < 0)

• return 1;
  

• r = umount2(target, MNT_FORCE);
  

• // Ensure target exists
• mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO);
• return hw_mount(source, target, NULL, MS_BIND);

+}

+int hw_umount(const char* source, const char* prefix) {

• char target[PATH_MAX];
• int r;
• if (prefix)

• r = snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source);
  

• else

• r = snprintf(target, sizeof(target) - 1, "%s", source);
  

• if (r < 0)

• return r;
  

• // Perform umount
• r = umount2(target, 0);
• if (r) {

• switch (errno) {
  

• 	// Try again with force if umount wasn't successful
  

• 	case EBUSY:
  

• 		sleep(1);
  

• 		r = umount2(target, MNT_FORCE);
  

• 		break;
  

• 	// target wasn't a mountpoint. Ignore.
  

• 	case EINVAL:
  

• 		r = 0;
  

• 		break;
  

• }
  

  } return r;

@@ -174,7 +206,7 @@ static int hw_test_source_medium(const char* path) { ret = access(SOURCE_TEST_FILE, R_OK); // Umount the test device.

• hw_umount(SOURCE_MOUNT_PATH);

• hw_umount(SOURCE_MOUNT_PATH, NULL); return ret;

} @@ -881,20 +913,21 @@ int hw_mount_filesystems(struct hw_destination* dest, const char* prefix) { } // bind-mount misc filesystems

• char** otherfs = other_filesystems;
• while (*otherfs) {

• snprintf(target, sizeof(target), "%s%s", prefix, *otherfs);
  

• r = hw_bind_mount("/dev", prefix);
• if (r)

• return r;
  

• mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO);
  

• r = hw_mount(*otherfs, target, NULL, MS_BIND);
  

• if (r) {
  

• 	hw_umount_filesystems(dest, prefix);
  

• r = hw_bind_mount("/proc", prefix);
• if (r)

• return r;
  

• 	return r;
  

• }
  

• r = hw_bind_mount("/sys", prefix);
• if (r)

• return r;
  

• otherfs++;
  

• }

• r = hw_bind_mount("/sys/firmware/efi/efivars", prefix);
• if (r && errno != ENOENT)

• return r;
  

  return 0;

} @@ -908,16 +941,14 @@ int hw_umount_filesystems(struct hw_destination* dest, const char* prefix) { // ESP if (*dest->part_boot_efi) {

• snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT_EFI);
  

• r = hw_umount(target);
  

• r = hw_umount(HW_PATH_BOOT_EFI, prefix);
  

  if (r) return -1; } // boot if (*dest->part_boot) {

• snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT);
  

• r = hw_umount(target);
  

• r = hw_umount(HW_PATH_BOOT, prefix);
  

  if (r) return -1; }

@@ -928,16 +959,24 @@ int hw_umount_filesystems(struct hw_destination* dest, const char* prefix) { } // misc filesystems

• char** otherfs = other_filesystems;
• while (*otherfs) {

• snprintf(target, sizeof(target), "%s%s", prefix, *otherfs++);
  

• r = hw_umount(target);
  

• if (r)
  

• 	return -1;
  

• }

• r = hw_umount("/sys/firmware/efi/efivars", prefix);
• if (r)

• return -1;
  

• r = hw_umount("/sys", prefix);
• if (r)

• return -1;
  

• r = hw_umount("/proc", prefix);
• if (r)

• return -1;
  

• r = hw_umount("/dev", prefix);
• if (r)

• return -1;
  

  // root

• r = hw_umount(prefix);

• r = hw_umount(prefix, NULL); if (r) return -1;

diff --git a/src/installer/hw.h b/src/installer/hw.h index 9fe69271e..b11dfa48f 100644 --- a/src/installer/hw.h +++ b/src/installer/hw.h @@ -108,7 +108,7 @@ struct hw* hw_init(); void hw_free(struct hw* hw); int hw_mount(const char* source, const char* target, const char* fs, int flags); -int hw_umount(const char* target); +int hw_umount(const char* source, const char* prefix); char* hw_find_source_medium(struct hw* hw); diff --git a/src/installer/main.c b/src/installer/main.c index bc0fdaa67..fabc0ef52 100644 --- a/src/installer/main.c +++ b/src/installer/main.c @@ -909,7 +909,7 @@ int main(int argc, char *argv[]) { } // Umount source drive and eject

• hw_umount(SOURCE_MOUNT_PATH);

• hw_umount(SOURCE_MOUNT_PATH, NULL); // Free downloaded ISO image if (strcmp(sourcedrive, SOURCE_TEMPFILE) == 0) {