On Mon, 9 Dec 2024 at 14:34, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > On Mon, 2024-12-09 at 10:20 +0100, Ard Biesheuvel wrote: > > On Sun, 8 Dec 2024 at 19:34, James Bottomley > > <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > Patch 1 is stand alone, but 2 depends on 3 > > > > > > Regards, > > > > > > James > > > > > > --- > > > > > > James Bottomley (3): > > > efivarfs: fix error on non-existent file > > > efivarfs: fix memory leak on variable removal > > > efivarfs: fix incorrect variable creation > > > > > > > Thanks James, > > > > I've queued these up now. > > Thanks, but I need to redo 3/3: there's a bug where if the variable is > created to do a write which fails, it remains on the list even though > the entry is freed. > OK. I'm a bit out of my depth here with the VFS stuff. So efivarfs_create() creates the file and efivarfs_file_write() writes the contents of the variables, right? So what is the correct behavior here if the write fails? If the file exists as an empty file, but might still be open, surely we cannot just drop it from the list? > It also begs the question: why does this list of variables exist? All > it does is cause management complexity and overhead and its only > function seems to be to free the entries when the filesystem is > unmounted, which could much more easily be accomplished by implementing > a superblock evict_inode() callback that kfree's i_private, which would > mean the entry was freed when the inode was and thus wouldn't have to > be explicitly freed anywhere. > I have no idea - this code existed before I got involved with EFI. However, a related issue came up here [0]: the list may get out of sync after a resume from hibernate, and so it needs to be brought up to date. Not having a list in the first place seems like a step in the right direction, as we'd need to synchronize that with the updated varstore as well. So IIUC, you are suggesting to keep the caching behavior of the name and attributes, but not keep a list at all? Would that work with the needed resync above? [0] https://github.com/systemd/systemd/pull/35497
