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. Are there any existing test suites that cover efivarfs that you could recommend?
