From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter =?utf-8?q?M=C3=BCller?= To: development@lists.ipfire.org Subject: Re: [PATCH 3/3] installer: Bind-mount /sys/firmware/efi/efivars into chroot Date: Wed, 17 Nov 2021 20:11:41 +0000 Message-ID: In-Reply-To: <20211104090554.6510-3-michael.tremer@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7727755329284918378==" List-Id: --===============7727755329284918378== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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=C3=BCller Thanks, and best regards, Peter M=C3=BCller > Signed-off-by: Michael Tremer > --- > src/installer/hw.c | 113 +++++++++++++++++++++++++++++-------------- > src/installer/hw.h | 2 +- > src/installer/main.c | 2 +- > 3 files changed, 78 insertions(+), 39 deletions(-) >=20 > 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 @@ > =20 > #include "hw.h" > =20 > -const char* other_filesystems[] =3D { > - "/dev", > - "/proc", > - "/sys", > - NULL > -}; > - > static int system_chroot(const char* output, const char* path, const char= * cmd) { > char chroot_cmd[STRING_SIZE]; > =20 > @@ -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); > } > =20 > -int hw_umount(const char* target) { > - int r =3D umount2(target, 0); > +static int hw_bind_mount(const char* source, const char* prefix) { > + if (!source || !prefix) { > + errno =3D EINVAL; > + return 1; > + } > =20 > - if (r && errno =3D=3D EBUSY) { > - // Give it a moment to settle > - sleep(1); > + char target[PATH_MAX]; > + int r; > + > + // Format target > + r =3D snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source); > + if (r < 0) > + return 1; > =20 > - r =3D 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 =3D snprintf(target, sizeof(target) - 1, "%s/%s", prefix, source); > + else > + r =3D snprintf(target, sizeof(target) - 1, "%s", source); > + if (r < 0) > + return r; > + > + // Perform umount > + r =3D umount2(target, 0); > + if (r) { > + switch (errno) { > + // Try again with force if umount wasn't successful > + case EBUSY: > + sleep(1); > + > + r =3D umount2(target, MNT_FORCE); > + break; > + > + // target wasn't a mountpoint. Ignore. > + case EINVAL: > + r =3D 0; > + break; > + } > } > =20 > return r; > @@ -174,7 +206,7 @@ static int hw_test_source_medium(const char* path) { > ret =3D access(SOURCE_TEST_FILE, R_OK); > =20 > // Umount the test device. > - hw_umount(SOURCE_MOUNT_PATH); > + hw_umount(SOURCE_MOUNT_PATH, NULL); > =20 > return ret; > } > @@ -881,20 +913,21 @@ int hw_mount_filesystems(struct hw_destination* dest,= const char* prefix) { > } > =20 > // bind-mount misc filesystems > - char** otherfs =3D other_filesystems; > - while (*otherfs) { > - snprintf(target, sizeof(target), "%s%s", prefix, *otherfs); > + r =3D hw_bind_mount("/dev", prefix); > + if (r) > + return r; > =20 > - mkdir(target, S_IRWXU|S_IRWXG|S_IRWXO); > - r =3D hw_mount(*otherfs, target, NULL, MS_BIND); > - if (r) { > - hw_umount_filesystems(dest, prefix); > + r =3D hw_bind_mount("/proc", prefix); > + if (r) > + return r; > =20 > - return r; > - } > + r =3D hw_bind_mount("/sys", prefix); > + if (r) > + return r; > =20 > - otherfs++; > - } > + r =3D hw_bind_mount("/sys/firmware/efi/efivars", prefix); > + if (r && errno !=3D ENOENT) > + return r; > =20 > return 0; > } > @@ -908,16 +941,14 @@ int hw_umount_filesystems(struct hw_destination* dest= , const char* prefix) { > =20 > // ESP > if (*dest->part_boot_efi) { > - snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT_EFI); > - r =3D hw_umount(target); > + r =3D hw_umount(HW_PATH_BOOT_EFI, prefix); > if (r) > return -1; > } > =20 > // boot > if (*dest->part_boot) { > - snprintf(target, sizeof(target), "%s%s", prefix, HW_PATH_BOOT); > - r =3D hw_umount(target); > + r =3D 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) { > } > =20 > // misc filesystems > - char** otherfs =3D other_filesystems; > - while (*otherfs) { > - snprintf(target, sizeof(target), "%s%s", prefix, *otherfs++); > - r =3D hw_umount(target); > - if (r) > - return -1; > - } > + r =3D hw_umount("/sys/firmware/efi/efivars", prefix); > + if (r) > + return -1; > + > + r =3D hw_umount("/sys", prefix); > + if (r) > + return -1; > + > + r =3D hw_umount("/proc", prefix); > + if (r) > + return -1; > + > + r =3D hw_umount("/dev", prefix); > + if (r) > + return -1; > =20 > // root > - r =3D hw_umount(prefix); > + r =3D hw_umount(prefix, NULL); > if (r) > return -1; > =20 > 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); > =20 > 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); > =20 > char* hw_find_source_medium(struct hw* hw); > =20 > 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[]) { > } > =20 > // Umount source drive and eject > - hw_umount(SOURCE_MOUNT_PATH); > + hw_umount(SOURCE_MOUNT_PATH, NULL); > =20 > // Free downloaded ISO image > if (strcmp(sourcedrive, SOURCE_TEMPFILE) =3D=3D 0) { >=20 --===============7727755329284918378==--