Re: [PATCH] environment: support reading and writing of efivar

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

Thanks for your patch. Please see the comments inline.

On 3/11/25 15:17, Jonas Licht wrote:
> scripts/bareboxenv: skip file creation to allow reading the destination
> file, when existing.

Can you elaborate a bit on what's done here? e.g. On EFI, barebox
environment can be stored into an EFI variable. An EFI variable's value
as read from Linux efivarfs is preceded by four bytes of metadata, which
we need to maintain when rewriting the variable.

> Signed-off-by: Jonas Licht <jonas.licht@gmail.com>
> Fixes #29

Link: https://github.com/barebox/barebox/issues/29

> ---
>  common/environment.c | 62 +++++++++++++++++++++++++++++++++++++++-----
>  scripts/bareboxenv.c | 11 +-------
>  2 files changed, 57 insertions(+), 16 deletions(-)
> 
> diff --git a/common/environment.c b/common/environment.c
> index 37adb5d678..dee746a6b2 100644
> --- a/common/environment.c
> +++ b/common/environment.c
> @@ -305,9 +305,10 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
>  	struct envfs_super *super;
>  	int envfd, size, ret;
>  	struct action_data data = {};
> -	void *buf = NULL, *wbuf;
> +	void *buf = NULL, *wbuf, *bufWithEfi;

barebox is written in kernel coding style, so please use snake case
for variables.

>  	struct envfs_entry *env;
>  	const char *defenv_path = default_environment_path_get();
> +	uint32_t magic;
>  
>  	if (!filename)
>  		filename = defenv_path;
> @@ -342,7 +343,9 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
>  		}
>  	}
>  
> -	buf = xzalloc(size + sizeof(struct envfs_super));
> +	bufWithEfi = xzalloc(size + sizeof(struct envfs_super) +
> +			     4); // four byte efi attributes

Nit: #define EFIVAR_ATTR_SIZE 4 at the top of the file and use it here
instead of the comment.

> +	buf = bufWithEfi + 4;
>  	data.writep = buf + sizeof(struct envfs_super);
>  
>  	super = buf;
> @@ -370,7 +373,7 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
>  	super->crc = ENVFS_32(crc32(0, buf + sizeof(struct envfs_super), size));
>  	super->sb_crc = ENVFS_32(crc32(0, buf, sizeof(struct envfs_super) - 4));
>  
> -	envfd = open(filename, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR);
> +	envfd = open(filename, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
>  	if (envfd < 0) {
>  		printf("could not open %s: %m\n", filename);
>  		ret = -errno;
> @@ -385,6 +388,34 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
>  		goto out;
>  	}
>  
> +	wbuf = buf;
> +
> +	/* check if we writing efi vars */

I think it will be more readable to factor this out into a helper
function. That function should also have an early exit with:

	if (!IS_ENABLED(CONFIG_EFI))
		return buf;

So, non-EFI platforms can skip this code altogether.

> +	ret = pread(envfd, &magic, sizeof(uint32_t),
> +		    4); // four byte efi var attributes
> +	if (ret == -1 && errno == ENOENT) {
> +		// skip as file don't exist
> +		goto skip_efi_read;
> +	}
> +	if (ret < sizeof(u_int32_t)) {
> +		perror("read of destination file failed");

perror("pread") is enough given that there are no instances of perror.

> +		ret = -errno;
> +		goto skip_efi_read;
> +	}
> +
> +	if (ENVFS_32(magic) == ENVFS_MAGIC) {
> +		pr_info("looks like we writing efi vars, keep attributes\n");

"Assuming EFI variable. Keeping attributes\n"

> +		ret = pread(envfd, bufWithEfi, sizeof(uint32_t), 0);
> +		if (ret < sizeof(uint32_t)) {
> +			perror("read of efi attributes failed");
> +			ret = -errno;

errno could be clobbered by perror before, so you need to record it
before doing anything else (including calling perror).


> +			goto out;
> +		}
> +		size += sizeof(uint32_t);
> +		wbuf = bufWithEfi;
> +	}
> +
> +skip_efi_read:
>  	ret = erase(envfd, ERASE_SIZE_ALL, 0, ERASE_TO_WRITE);
>  
>  	/* ENOSYS and EOPNOTSUPP aren't errors here, many devices don't need it */
> @@ -395,8 +426,6 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
>  
>  	size += sizeof(struct envfs_super);
>  
> -	wbuf = buf;
> -
>  	while (size) {
>  		ssize_t now = write(envfd, wbuf, size);
>  		if (now < 0) {
> @@ -425,7 +454,7 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
>  out:
>  	close(envfd);
>  out1:
> -	free(buf);
> +	free(bufWithEfi);
>  #ifdef __BAREBOX__
>  	unlink_recursive(TMPDIR, NULL);
>  #endif
> @@ -449,6 +478,7 @@ int envfs_load(const char *filename, const char *dir, unsigned flags)
>  	int envfd;
>  	int ret = 0;
>  	size_t size, rsize;
> +	uint32_t magic;
>  
>  	if (!filename)
>  		filename = default_environment_path_get();
> @@ -466,6 +496,26 @@ int envfs_load(const char *filename, const char *dir, unsigned flags)
>  		return -1;
>  	}
>  
> +	/* check if we reading efi vars */
> +	ret = pread(envfd, &magic, sizeof(uint32_t),
> +		    4); // four byte efi var attributes
> +	if (ret < sizeof(u_int32_t)) {
> +		perror("read");
> +		ret = -errno;
> +		goto out;
> +	}
> +
> +	if (ENVFS_32(magic) == ENVFS_MAGIC) {
> +		pr_info("looks like we reading efi vars, skip attributes\n");
> +		ret = read(envfd, &magic,
> +			   sizeof(uint32_t)); // simply reuse the memory
> +		if (ret < sizeof(uint32_t)) {
> +			perror("read");
> +			ret = -errno;
> +			goto out;
> +		}
> +	}
> +
>  	/* read superblock */
>  	ret = read(envfd, &super, sizeof(struct envfs_super));
>  	if ( ret < sizeof(struct envfs_super)) {
> diff --git a/scripts/bareboxenv.c b/scripts/bareboxenv.c
> index e954447015..6b9b8d90c4 100644
> --- a/scripts/bareboxenv.c
> +++ b/scripts/bareboxenv.c
> @@ -117,7 +117,7 @@ static void usage(char *prgname)
>  int main(int argc, char *argv[])
>  {
>  	int opt;
> -	int save = 0, load = 0, pad = 0, err = 0, fd;
> +	int save = 0, load = 0, pad = 0, err = 0;
>  	char *filename = NULL, *dirname = NULL;
>  	unsigned envfs_flags = 0;
>  	int verbose = 0;
> @@ -156,15 +156,6 @@ int main(int argc, char *argv[])
>  		exit(1);
>  	}
>  
> -	if (save) {
> -		fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0644);
> -		if (fd < 0) {
> -			perror("open");
> -			exit(1);
> -		}
> -		close(fd);
> -	}

Did you verify proper operation in the non-EFI case?

Thanks,
Ahmad

> -
>  	if (save && pad) {
>  		if (truncate(filename, pad)) {
>  			perror("truncate");