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

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 7152 bytes --]

Hmm, difficult. Reading it again it is a little bit unclear.

I suppose the Ack-by doesn’t quite fit into a bucket like Reported-by and Tested-by which are very clear.

It is indeed maybe a more relaxed review, but with a “I want this change” in it. So it makes sense that you used that.

Never mind me.

-Michael

> On 19 Nov 2021, at 06:30, Peter Müller <peter.mueller(a)ipfire.org> wrote:
> 
> 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(a)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(a)ipfire.org>
>>> 
>>> Thanks, and best regards,
>>> Peter Müller
>>> 
>>>> Signed-off-by: Michael Tremer <michael.tremer(a)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) {
>>