| Message ID | 20250107023525.11466-1-James.Bottomley@HansenPartnership.com |
|---|---|
| Headers | show |
| Series | convert efivarfs to manage object data correctly | expand |
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.
On Thu, 2025-01-09 at 10:50 +0100, Ard Biesheuvel wrote: > On Tue, 7 Jan 2025 at 03:36, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: [...] > > 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? I'm afraid I couldn't find any. I finally wrote a few shell scripts to try out multiple threads updating the same variable. I think I can probably work out how to add these to the kselftest infrastructure. Hibernation was a real pain because it doesn't work with secure boot, but I finally wrote a UEFI shell script to modify variables and reset. Unfortunately I don't think we have a testing framework I can add these to. Regards, James
On Thu, 2025-01-09 at 10:50 +0100, Ard Biesheuvel wrote: > Are there any existing test suites that cover efivarfs that you could > recommend? The good news is there is actually an existing test suite. I was writing some for selftests/filesystems/efivarfs, but it turns out they exist in selftests/efivarfs. You can run them from the kernel source tree (in a VM with your changes) as: make -C tools/testing/selftests TARGETS=efivarfs run_tests So I've merged all the testing I had here and started writing new ones. The bad news is that writing new tests I've run across another corner case in the efivarfs code: you can set the inode size to anything you want (as root) which means you can take a real variable and get it to mimic an uncommitted one (at least to stat): # ls -l /sys/firmware/efi/efivars/PK-8be4df61-93ca-11d2-aa0d-00e098032b8c -rw-r--r-- 1 root root 841 Jan 18 13:40 /sys/firmware/efi/efivars/PK-8be4df61-93ca-11d2-aa0d-00e098032b8c # chattr -i /sys/firmware/efi/efivars/PK-8be4df61-93ca-11d2-aa0d-00e098032b8c # > /sys/firmware/efi/efivars/PK-8be4df61-93ca-11d2-aa0d-00e098032b8c # ls -l /sys/firmware/efi/efivars/PK-8be4df61-93ca-11d2-aa0d-00e098032b8c -rw-r--r-- 1 root root 0 Jan 18 13:40 /sys/firmware/efi/efivars/PK-8be4df61-93ca-11d2-aa0d-00e098032b8c I'm not sure how much of a bug this is for the old code (only systemd seems to check for zero size files), and it's only in the cache inode, so if you cat the file you get the fully 841 bytes. However, obviously it becomes a huge problem with my new code because you can use the truncate inode to actually delete the variable file (even thought the variable is still there) so I need to add a fix for it to my series. I'll post it separately when I have it to see what you think. Regards, James
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 fs/efivarfs/file.c | 60 +++++++++++--- fs/efivarfs/inode.c | 5 -- fs/efivarfs/internal.h | 19 ++--- fs/efivarfs/super.c | 68 +++++++++------- fs/efivarfs/vars.c | 179 ++++++++++------------------------------- 5 files changed, 136 insertions(+), 195 deletions(-)