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

Ard Biesheuvel
2025-01-09T14:24:47Z

On Thu, 9 Jan 2025 at 10:50, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> 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.
>

For the record,

Tested-by: Ard Biesheuvel <ardb@kernel.org>

including the hibernation pieces. It looks pretty to me solid to me.

I'd add a Reviewed-by: as well if I wasn't so clueless about VFS
stuff, so I'll gladly take one from the audience.

Thanks again, James - this is a really nice cleanup.