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

["Peter Müller" <peter.mueller@ipfire.org>]

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

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) {
>