On Thu, 9 Jan 2025 at 10:50, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: > > On Tue, 7 Jan 2025 at 03:36, James Bottomley > <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx> 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.
