On Mon, 9 Dec 2024 at 15:38, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > On Mon, 2024-12-09 at 14:49 +0100, Ard Biesheuvel wrote: > > 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? > > 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? > > I don't think it's a race that can be mediated in the current > mechanism, although the efivar lock seems to do some of it. In the > ordinary course of filesystems it should be mediated by the dentry > objects (any racing open/write with delete can either succeed or get an > error). > > > > 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. > > Yes, that's the same issue I was complaining about and trying to fix in > patch 3/3. > > > 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? > > Yes. If we simply let the dentries behave correctly as filesystem > objects, they'll mediate the race and if the inode gets evicted then we > also free the entry (so we tie efivarfs entry lifetimes to the inode). > There would still be a corner case where you could call open(O_CREAT) > but *not* write on the file. That would still create a zero length > variable entry which would disappear on reboot because it would have no > corresponding EFI variable. This is an inevitable consequence of our > required semantics of not creating the variable until something is > written to it although it may be possible to flag the dentry in create > as being not visible to others until it gets written (i.e. effectively > open it as a deleted but open file and promote it to not deleted on a > successful write). > > If you want me to look into doing the above instead of patches 2-3, I > can. I think patch 1 can be safely applied. > That would be much appreciated. I'll keep only patch #1 for now.
