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: 6106 bytes --]

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