Re: [PATCH v2 0/6] convert efivarfs to manage object data correctly

Ard Biesheuvel
2025-01-09T09:50:42Z

On Tue, 7 Jan 2025 at 03:36, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> I've added fsdevel because I'm hopping some kind vfs person will check
> the shift from efivarfs managing its own data to its data being
> managed as part of the vfs object lifetimes. The following paragraph
> should describe all you need to know about the unusual features of the
> filesystem.
>
> efivarfs is a filesystem projecting the current state of the UEFI
> variable store and allowing updates via write. Because EFI variables
> contain both contents and a set of attributes, which can't be mapped
> to filesystem data, the u32 attribute is prepended to the output of
> the file and, since UEFI variables can't be empty, this makes every
> file at least 5 characters long. EFI variables can be removed either
> by doing an unlink (easy) or by doing a conventional write update that
> reduces the content to zero size, which means any write update can
> potentially remove the file.
>
> Currently efivarfs has two bugs: it leaks memory and if a create is
> attempted that results in an error in the write, it creates a zero
> length file remnant that doesn't represent an EFI variable (i.e. the
> state reflection of the EFI variable store goes out of sync).
>
> The code uses inode->i_private to point to additionaly allocated
> information but tries to maintain a global list of the shadowed
> varibles for internal tracking. Forgetting to kfree() entries in this
> list when they are deleted is the source of the memory leak.
>
> I've tried to make the patches as easily reviewable by non-EFI people
> as possible, so some possible cleanups (like consolidating or removing
> the efi lock handling and possibly removing the additional entry
> allocation entirely in favour of simply converting the dentry name to
> the variable name and guid) are left for later.
>
> The first patch removes some unused fields in the entry; patches 2-3
> eliminate the list search for duplication (some EFI variable stores
> have buggy iterators) and replaces it with a dcache lookup. Patch 4
> move responsibility for freeing the entry data to inode eviction which
> both fixes the memory leak and also means we no longer need to iterate
> over the variable list and free its entries in kill_sb. Since the
> variable list is now unused, patch 5 removes it and its helper
> functions.
>
> Patch 6 fixes the second bug by introducing a file_operations->release
> method that checks to see if the inode size is zero when the file is
> closed and removes it if it is. Since all files must be at least 5 in
> length we use a zero i_size as an indicator that either the variable
> was removed on write or that it wasn't correctly created in the first
> place.
>
> v2: folded in feedback from Al Viro: check errors on lookup and delete
> zero length file on last close
>
> James
>
> ---
>
> James Bottomley (6):
> efivarfs: remove unused efi_varaible.Attributes and .kobj
> efivarfs: add helper to convert from UC16 name and GUID to utf8 name
> efivarfs: make variable_is_present use dcache lookup
> efivarfs: move freeing of variable entry into evict_inode
> efivarfs: remove unused efivarfs_list
> efivarfs: fix error on write to new variable leaving remnants
>

Thanks James,

I've tentatively queued up this series, as well as the hibernate one,
to get some coverage from the robots while I run some tests myself.

Are there any existing test suites that cover efivarfs that you could recommend?