| Message ID | 20241208183415.21181-1-James.Bottomley@HansenPartnership.com |
|---|---|
| Headers | show |
| Series | efivarfs: bug fixes | expand |
On Sun, 8 Dec 2024 at 19:34, James Bottomley <James.Bottomley@hansenpartnership.com> 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.
On Mon, 9 Dec 2024 at 14:34, James Bottomley <James.Bottomley@hansenpartnership.com> 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@hansenpartnership.com> 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
On Mon, 9 Dec 2024 at 15:38, James Bottomley <James.Bottomley@hansenpartnership.com> 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@hansenpartnership.com> 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@hansenpartnership.com> 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.