Re: [PATCH v2 4/6] efivarfs: move freeing of variable entry into evict_inode

James Bottomley
2025-01-19T16:46:23Z

On Sun, 2025-01-19 at 17:31 +0100, Ard Biesheuvel wrote:
> On Sun, 19 Jan 2025 at 15:57, James Bottomley
> <James.Bottomley@hansenpartnership.com> 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@hansenpartnership.com> 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@zeniv.linux.org.uk/
> > > > > > 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