On Tue, 28 Oct 2025 at 22:08, Al Viro [off-list ref] wrote:

On Tue, Oct 28, 2025 at 05:45:40PM +0000, Al Viro wrote:

quoted

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@kernel.org>

but I have to confess I am slightly out of my depth when it comes to VFS stuff.