[PATCH v2 22/50] convert efivarfs

[James Bottomley]

On Tue, 2025-10-28 at 22:34 +0100, Ard Biesheuvel wrote:
> On Tue, 28 Oct 2025 at 22:08, Al Viro <viro at zeniv.linux.org.uk>
> wrote:
> > 
> > On Tue, Oct 28, 2025 at 05:45:40PM +0000, Al Viro wrote:
> > 
> > > FWIW, having a special path for "we are in foofs_fill_super(),
> > > fuck the locking - nobody's going to access it anyway" is not a
> > > great idea, simply because the helpers tend to get reused on
> > > codepaths where we can't cut corners that way.
> > 
> >         BTW, looking through efivarfs codebase now... *both*
> > callers of efivarfs_create_dentry() end up doing dcache lookups,
> > with variously convoluted call chains.  Look:
> > efivarfs_check_missing() has an explicit try_lookup_noperm() before
> > the call of efivarfs_create_dentry(). efivarfs_callback() doesn't,
> > but it's called via
> >         efivar_init(efivarfs_callback, sb, true)
> > and with the last argument being true efivar_init() will precede
> > the call of the callback with efivarfs_variable_is_present(). 
> > Guess what does that thing (never used anywhere else) do?  Right,
> > the call of try_lookup_noperm().
> > 
> > Why do we bother with that?  What's wrong with having
> > efivarfs_create_dentry() returning -EEXIST in case of dentry
> > already being there and turning the chunk in efivar_init() into
> >                         err = func(variable_name, vendor_guid,
> >                                    variable_name_size, data);
> >                         if (err == -EEXIST) {
> >                                 if (duplicate_check)
> >                                        
> > dup_variable_bug(variable_name,
> >                                                         
> > &vendor_guid,
> >                                                         
> > variable_name_size);
> >                                 else
> >                                         err = 0;
> >                         }
> >                         if (err)
> >                                 status = EFI_NOT_FOUND;
> > Note that both possible callbacks become almost identical and I
> > wouldn't be surprised if that "almost" is actually "completely"... 
> > <checks> yep.
> > 
> 
> I'll let James respond to the specifics of your suggestion, but I'll
> just note that this code has a rather convoluted history, as we used
> to have two separate pseudo-filesystem drivers, up until a few years
> ago: the sysfs based 'efivars' and this efivarfs driver. Given that
> modifications in one needed to be visible in the other, they shared a
> linked list that shadowed the state of the underlying variable store.
> 'efivars' was removed years ago, but it was only recently that James
> replaced the linked list in this driver with the dentry cache as the
> shadow mechanism.

I think this all looks OK.  The reason for the convolution is that
simple_start/done_creating() didn't exist when I did the conversion ...
although if they had, I'm not sure I'd have thought of reworking
efivarfs_create_dentry to use them.  I tried to update some redundant
bits, but it wasn't the focus of what I was trying to fix.

So I think the cleanup works and looks nice.

> 
> Relying on the -EEXIST return value to detect duplicates, and
> combining the two callbacks seem like neat optimizations to me, so
> 
> Acked-by: Ard Biesheuvel <ardb at kernel.org>
> 
> but I have to confess I am slightly out of my depth when it comes to
> VFS stuff.

Yes, ack too.

Regards,

James