On Sun, 2025-01-19 at 17:31 +0100, Ard Biesheuvel wrote: > On Sun, 19 Jan 2025 at 15:57, James Bottomley > <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > > > On Sun, 2025-01-19 at 15:50 +0100, Ard Biesheuvel wrote: > > > On Thu, 16 Jan 2025 at 23:13, James Bottomley > > > <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > > > On Thu, 2025-01-16 at 14:05 -0500, James Bottomley wrote: > > > > > On Thu, 2025-01-16 at 18:36 +0000, Al Viro wrote: > > > > > > On Mon, Jan 06, 2025 at 06:35:23PM -0800, James Bottomley > > > > > > wrote: > > > > > > > Make the inodes the default management vehicle for struct > > > > > > > efivar_entry, so they are now all freed automatically if > > > > > > > the file is removed and on unmount in > > > > > > > kill_litter_super(). Remove the now superfluous iterator > > > > > > > to free the entries after kill_litter_super(). > > > > > > > > > > > > > > Also fixes a bug where some entry freeing was missing > > > > > > > causing efivarfs to leak memory. > > > > > > > > > > > > Umm... I'd rather coallocate struct inode and struct > > > > > > efivar_entry; that way once you get rid of the list you > > > > > > don't need - evict_inode() anymore. > > > > > > > > > > > > It's pretty easy - see e.g. > > > > > > https://lore.kernel.org/all/20250112080705.141166-1-viro@xxxxxxxxxxxxxxxxxx/ > > > > > > for recent example of such conversion. > > > > > > > > > > OK, I can do that. Although I think since the number of > > > > > variables is usually around 150, it would probably be > > > > > overkill to give it its own inode cache allocator. > > > > > > > > OK, this is what I've got. As you can see from the diffstat > > > > it's about 10 lines more than the previous; mostly because of > > > > the new allocation routine, the fact that the root inode has to > > > > be special cased for the list and the guid has to be parsed in > > > > efivarfs_create before we have the inode. > > > > > > > > > > That looks straight-forward enough. > > > > > > Can you send this as a proper patch? Or would you prefer me to > > > squash this into the one that is already queued up? > > > > Sure; I've got a slightly different version because after talking > > with Al he thinks it's OK still to put a pointer to the > > efivar_entry in i_private, which means less disruption. But there > > is enough disruption that the whole of the series needs redoing to > > avoid conflicts. > > > > > I'm fine with either, but note that I'd still like to target > > > v6.14 with this. > > > > Great, but I'm afraid the fix for the zero size problem also could > > do with being a precursor which is making the timing pretty tight. > > > > OK, I'll queue up your v3 about I won't send it out with the initial > pull request, so we can make up our minds later. > > I take it the setattr series is intended for merging straight away? Yes, it could be treated as a bug fix, although since only root could truncate an EFI variable in a normal installation, it's not a huge issue. Regards, James
