Hello Jonas,
On 20.03.25 10:22, Jonas Licht wrote:
> scripts/bareboxenv: skip file creation to allow reading the destination
> file, when existing.
>
> In envfs_load before reading the superblock, we check if our superblock
> magic exists after an EFIVAR_ATTR_SIZE offset. This is necessary because
> linux kernel adds four byte variable attributes in front of each efi
> variable in sysfs. When the magic is found after EFIVAR_ATTR_SIZE, we
> skip reading the four bytes variable attributes.
> For envfs_save detection of an efi var is done the same way. But we need
> to keep the attributes, as they are required for writing as well.
>
> In order to write to efivars, you still need to remove immutable flag
> from the file with 'chattr -i' .
Sorry for taking so long to get back to you.
I agree this is a problem we want to fix, but I wasn't overly happy with
the complexity the fix introduced.
How about this alternative:
- We define in scripts/bareboxenv.c following function:
bool is_efivarfs_fd(int fd)
{
struct statfs env_statfs;
int ret;
ret = fstatvfs(fd, &env_statfs);
if (ret)
return false;
return bundle_statfs.f_type == EFIVARFS_MAGIC;
}
In common/environment.c, in the first #ifdef __BAREBOX__ branch,
we #define is_efivarfs_fd(fd) false.
- Instead of checking the magic, we just call is_efivarfs_fd and
then read the 4 bytes
- We get rid of the #ifdefs and just let the compiler drop dead code
when building for __BAREBOX__.
- As we limit the effect to bareboxenv as hosted tool, we don't need
to check for CONFIG_EFI either
What do you think?
Some minor comments below.
> Skip the detect code when __BAREBOX__ is defined as well.
> I've noticed that the first pread in detect_efi_vars never comes back
> in __BAREBOX__ , when the efivar file don't exists, but doing this on a
> file at /tmp works.
Oh, so that's a bug in the barebox efivarfs implementation?
> +#include <autoconf.h>
autoconf.h should be included by the buildsystem (-include option), but
with suggested changes above we can avoid it altogether.
> + void *buf = NULL, *wbuf = NULL, *buf_with_efi;
> struct envfs_entry *env;
> const char *defenv_path = default_environment_path_get();
>
> @@ -342,7 +380,9 @@ int envfs_save(const char *filename, const char *dirname,
> unsigned flags)
> }
> }
>
> - buf = xzalloc(size + sizeof(struct envfs_super));
> + buf_with_efi = xzalloc(size + sizeof(struct envfs_super) +
> + EFIVAR_ATTR_SIZE);
Alignment is wonky, but you can just keep it a single line here.
> + buf = buf_with_efi + EFIVAR_ATTR_SIZE;
> data.writep = buf + sizeof(struct envfs_super);
>
> super = buf;
> @@ -370,7 +410,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 +425,12 @@ int envfs_save(const char *filename, const char
> *dirname, unsigned flags)
> goto out;
> }
>
> + /* check if we writing efi vars */
> + ret = detect_efi_vars(envfd, buf_with_efi, &wbuf, &size);
> + if (ret != 0) {
> + goto out;
> + }
No braces for single line if clauses.
> +
> ret = erase(envfd, ERASE_SIZE_ALL, 0, ERASE_TO_WRITE);
>
> /* ENOSYS and EOPNOTSUPP aren't errors here, many devices don't need it
> */
> @@ -395,12 +441,11 @@ 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) {
> ret = -errno;
> + perror("write");
> goto out;
> }
>
> @@ -418,14 +463,14 @@ int envfs_save(const char *filename, const char
> *dirname, unsigned flags)
>
> ret = 0;
>
> -#ifdef CONFIG_NVVAR
> +#if defined CONFIG_NVVAR && defined __BAREBOX__
> if (defenv_path && !strcmp(filename, defenv_path))
> nv_var_set_clean();
> #endif
> out:
> close(envfd);
> out1:
> - free(buf);
> + free(buf_with_efi);
> #ifdef __BAREBOX__
> unlink_recursive(TMPDIR, NULL);
> #endif
> @@ -449,6 +494,9 @@ int envfs_load(const char *filename, const char *dir,
> unsigned flags)
> int envfd;
> int ret = 0;
> size_t size, rsize;
> +#if defined CONFIG_EFI && !defined __BAREBOX__
> + uint32_t magic;
> +#endif
Won't be needed with suggested rework.
>
> if (!filename)
> filename = default_environment_path_get();
> @@ -466,6 +514,28 @@ int envfs_load(const char *filename, const char *dir,
> unsigned flags)
> return -1;
> }
>
> +#if defined CONFIG_EFI && !defined __BAREBOX__
> + /* check if we reading efi vars */
> + ret = pread(envfd, &magic, sizeof(uint32_t),
> + EFIVAR_ATTR_SIZE);
Just make it a single line. With suggested changes above this becomes
a normal read.
Thanks,
Ahmad
> + if (ret < sizeof(u_int32_t)) {
> + perror("read");
> + ret = -errno;
> + goto out;
> + }
> +
> + if (ENVFS_32(magic) == ENVFS_MAGIC) {
> + pr_info("Assuming EFI variable. 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;
> + }
> + }
> +#endif
> +
> /* read superblock */
> ret = read(envfd, &super, sizeof(struct envfs_super));
> if ( ret < sizeof(struct envfs_super)) {
> diff --git a/scripts/Makefile b/scripts/Makefile
> index 6d89af7d4f..57d8ddb54d 100644
> --- a/scripts/Makefile
> +++ b/scripts/Makefile
> @@ -30,6 +30,7 @@ hostprogs-always-$(CONFIG_RK_IMAGE) +=
> rkimage
> HOSTCFLAGS_rkimage.o = `$(PKG_CONFIG) --cflags openssl`
> HOSTLDLIBS_rkimage = `$(PKG_CONFIG) --libs openssl`
> KBUILD_HOSTCFLAGS += -I$(srctree)/scripts/include/
> +KBUILD_HOSTCFLAGS += -I$(objtree)/include/generated/
> HOSTCFLAGS_mxsimage.o = `$(PKG_CONFIG) --cflags openssl`
> HOSTLDLIBS_mxsimage = `$(PKG_CONFIG) --libs openssl`
> HOSTCFLAGS_omap3-usb-loader.o = `$(PKG_CONFIG) --cflags libusb-1.0`
> @@ -57,6 +58,7 @@ omap4_usbboot-target-userccflags += `$(CROSS_PKG_CONFIG)
> --cflags libusb-1.0`
> omap4_usbboot-target-userldlibs += -lpthread `$(CROSS_PKG_CONFIG) --libs
> libusb-1.0`
> rk-usb-loader-target-userccflags += `$(CROSS_PKG_CONFIG) --cflags libusb-1.0`
> rk-usb-loader-target-userldlibs += `$(CROSS_PKG_CONFIG) --libs libusb-1.0`
> +bareboxenv-target-userccflags += -I$(objtree)/include/generated/
>
> userccflags += -I $(srctree)/$(src)/include -isystem
> $(srctree)/scripts/include
>
> 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);
> - }
> -
> if (save && pad) {
> if (truncate(filename, pad)) {
> perror("truncate");
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
