[PATCH v2 22/50] convert efivarfs

[Ard Biesheuvel]

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.

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.