On Tue, 18 Mar 2025 at 20:46, James Bottomley
<James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> This relieves us of the requirement to have a struct path and use file
> operations, which greatly simplifies the code. The superblock is now
> pinned by the blocking notifier (which is why deregistration moves
> above kill_litter_super).
>
> Signed-off-by: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> ---
> fs/efivarfs/super.c | 103 +++-----------------------------------------
> 1 file changed, 7 insertions(+), 96 deletions(-)
>
I like this a lot. Assuming it gets blessed by the VFS maintainers,
Reviewed-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index 81b3c6b7e100..135b0bb9b4b5 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -393,29 +393,13 @@ static const struct fs_context_operations efivarfs_context_ops = {
> .reconfigure = efivarfs_reconfigure,
> };
>
> -struct efivarfs_ctx {
> - struct dir_context ctx;
> - struct super_block *sb;
> - struct dentry *dentry;
> -};
> -
> -static bool efivarfs_actor(struct dir_context *ctx, const char *name, int len,
> - loff_t offset, u64 ino, unsigned mode)
> +static bool efivarfs_iterate_callback(void *data, struct dentry *dentry)
> {
> unsigned long size;
> - struct efivarfs_ctx *ectx = container_of(ctx, struct efivarfs_ctx, ctx);
> - struct qstr qstr = { .name = name, .len = len };
> - struct dentry *dentry = d_hash_and_lookup(ectx->sb->s_root, &qstr);
> - struct inode *inode;
> - struct efivar_entry *entry;
> + struct inode *inode = d_inode(dentry);
> + struct efivar_entry *entry = efivar_entry(inode);
> int err;
>
> - if (IS_ERR_OR_NULL(dentry))
> - return true;
> -
> - inode = d_inode(dentry);
> - entry = efivar_entry(inode);
> -
> err = efivar_entry_size(entry, &size);
> size += sizeof(__u32); /* attributes */
> if (err)
> @@ -426,12 +410,10 @@ static bool efivarfs_actor(struct dir_context *ctx, const char *name, int len,
> inode_unlock(inode);
>
> if (!size) {
> - ectx->dentry = dentry;
> - return false;
> + pr_info("efivarfs: removing variable %pd\n", dentry);
> + simple_recursive_removal(dentry, NULL);
> }
>
> - dput(dentry);
> -
> return true;
> }
>
> @@ -474,33 +456,11 @@ static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t vendor,
> return err;
> }
>
> -static void efivarfs_deactivate_super_work(struct work_struct *work)
> -{
> - struct super_block *s = container_of(work, struct super_block,
> - destroy_work);
> - /*
> - * note: here s->destroy_work is free for reuse (which
> - * will happen in deactivate_super)
> - */
> - deactivate_super(s);
> -}
> -
> -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;
> - struct efivarfs_ctx ectx = {
> - .ctx = {
> - .actor = efivarfs_actor,
> - },
> - .sb = sfi->sb,
> - };
> - struct file *file;
> - struct super_block *s = sfi->sb;
> static bool rescan_done = true;
>
> if (action == PM_HIBERNATION_PREPARE) {
> @@ -513,64 +473,15 @@ static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
> if (rescan_done)
> return NOTIFY_DONE;
>
> - /* ensure single superblock is alive and pin it */
> - if (!atomic_inc_not_zero(&s->s_active))
> - return NOTIFY_DONE;
> -
> pr_info("efivarfs: resyncing variable state\n");
>
> - path.dentry = sfi->sb->s_root;
> -
> - /*
> - * do not add SB_KERNMOUNT which a single superblock could
> - * expose to userspace and which also causes MNT_INTERNAL, see
> - * below
> - */
> - path.mnt = vfs_kern_mount(&efivarfs_type, 0,
> - efivarfs_type.name, NULL);
> - if (IS_ERR(path.mnt)) {
> - pr_err("efivarfs: internal mount failed\n");
> - /*
> - * We may be the last pinner of the superblock but
> - * calling efivarfs_kill_sb from within the notifier
> - * here would deadlock trying to unregister it
> - */
> - INIT_WORK(&s->destroy_work, efivarfs_deactivate_super_work);
> - schedule_work(&s->destroy_work);
> - return PTR_ERR(path.mnt);
> - }
> -
> - /* path.mnt now has pin on superblock, so this must be above one */
> - atomic_dec(&s->s_active);
> -
> - file = kernel_file_open(&path, O_RDONLY | O_DIRECTORY | O_NOATIME,
> - current_cred());
> - /*
> - * safe even if last put because no MNT_INTERNAL means this
> - * will do delayed deactivate_super and not deadlock
> - */
> - mntput(path.mnt);
> - if (IS_ERR(file))
> - return NOTIFY_DONE;
> -
> rescan_done = true;
>
> /*
> * First loop over the directory and verify each entry exists,
> * removing it if it doesn't
> */
> - file->f_pos = 2; /* skip . and .. */
> - do {
> - ectx.dentry = NULL;
> - iterate_dir(file, &ectx.ctx);
> - if (ectx.dentry) {
> - pr_info("efivarfs: removing variable %pd\n",
> - ectx.dentry);
> - simple_recursive_removal(ectx.dentry, NULL);
> - dput(ectx.dentry);
> - }
> - } while (ectx.dentry);
> - fput(file);
> + simple_iterate_call(sfi->sb->s_root, NULL, efivarfs_iterate_callback);
>
> /*
> * then loop over variables, creating them if there's no matching
> @@ -609,8 +520,8 @@ static void efivarfs_kill_sb(struct super_block *sb)
> struct efivarfs_fs_info *sfi = sb->s_fs_info;
>
> blocking_notifier_chain_unregister(&efivar_ops_nh, &sfi->nb);
> - kill_litter_super(sb);
> unregister_pm_notifier(&sfi->pm_nb);
> + kill_litter_super(sb);
>
> kfree(sfi);
> }
> --
> 2.43.0
>
>
