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;