On Tue, 2025-03-11 at 09:01 -0400, James Bottomley wrote:
> On Tue, 2025-03-11 at 09:45 +0100, Christian Brauner wrote:
[...]
> > But since efivars does only ever have a single global superblock,
> > one possibility is to an internal superblock that always exits and
> > is resurfaced whenever userspace mounts efivarfs. That's
> > essentially the devtmpfs model.
> >
> > Then you can stash:
> >
> > static struct vfsmount *efivarfs_mnt;
> >
> > globally and use that in efivarfs_pm_notify() to fill in struct
> > path.
>
> I didn't see devtmpfs when looking for examples, since it's hiding
> outside of the fs/ directory. However, it does seem to be a bit
> legacy nasty as an example to copy. However, I get the basics: we'd
> instantiate the mnt and superblock on init (stashing mnt in the sfi
> so the notifier gets it). Then we can do the variable population on
> reconfigure, just in case an EFI system doesn't want to mount
> efivarfs to save memory.
>
> I can code that up if I can get an answer to the uid/gid parameter
> question above.
I coded up the naive implementation and it definitely works, but it
suffers from the problem that everything that pins in the module init
routine (like configfs) does in that once inserted the module can never
be removed. Plus, for efivarfs, we would allocate all resources on
module insertion not on first mount. The final problem we'd have is
that the uid/gid parameters for variable creation would be taken from
the kernel internal mount, so if they got specified on a user mount,
they'd be ignored (because the variable inodes are already created).
To answer some of your other questions:
> (1) Is it guaranteed that efivarfs_pm_notify() is only called once a
> superblock exists?
Yes, as you realized.
> (2) Is it guaranteed that efivarfs_pm_notify() is only called when
> and while a mount for the superblock exists?
No, but the behaviour is correct because the notifier needs to update
the variable list and we create the variable list in
efivarfs_fill_super. Now you can argue this is suboptimal because if
userspace didn't ever mount, we'd simply destroy it all again on last
put of the superblock so it's wasted effort, but its function is
correct.
> Another question is whether the superblock can be freed while
> efivarfs_pm_notify() is running? I think that can't happen because
> blocking_notifier_chain_unregister(&efivar_ops_nh, &sfi->nb) will
> block in efivarfs_kill_sb() until all outstanding calls to
> efivarfs_pm_notify() are finished?
That's right: a blocking notifier is called under the notifier list
rwsem. It's taken read for calls but write for register/unregister, so
efivarfs_kill_sb would block in the unregister until the call chain was
executed.
Taking into account the module removal issue, the simplest way I found
to fix the issue was to call vfs_kern_mount() from the notifier to get
a struct vfsmount before opening the path. We ensure it's gone by
calling mntput immediately after open, but, by that time, the open file
is pinning the vfsmnt if the open was successful.
If this looks OK to everyone I'll code it up as a fix which can be cc'd
to stable.
Regards,
James
---
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 6eae8cf655c1..e2e6575b5abf 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -474,12 +474,14 @@ static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t vendor,
return err;
}
+static struct file_system_type efivarfs_type;
+
static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
void *ptr)
{
struct efivarfs_fs_info *sfi = container_of(nb, struct efivarfs_fs_info,
pm_nb);
- struct path path = { .mnt = NULL, .dentry = sfi->sb->s_root, };
+ struct path path;
struct efivarfs_ctx ectx = {
.ctx = {
.actor = efivarfs_actor,
@@ -501,9 +503,17 @@ static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
pr_info("efivarfs: resyncing variable state\n");
- /* O_NOATIME is required to prevent oops on NULL mnt */
+ path.dentry = sfi->sb->s_root;
+ path.mnt = vfs_kern_mount(&efivarfs_type, SB_KERNMOUNT,
+ efivarfs_type.name, NULL);
+ if (IS_ERR(path.mnt)) {
+ pr_err("efivarfs: internal mount failed\n");
+ return PTR_ERR(path.mnt);
+ }
+
file = kernel_file_open(&path, O_RDONLY | O_DIRECTORY | O_NOATIME,
current_cred());
+ mntput(path.mnt);
if (IS_ERR(file))
return NOTIFY_DONE;
