On Tue, Dec 10, 2024 at 12:02:22PM -0500, James Bottomley wrote:
> Make the inodes the default management vehicle for struct
> efivar_entry, so they are now all freed automatically if the file is
> removed and on unmount in kill_litter_super(). Remove the now
> superfluous iterator to free the entries after kill_litter_super().
>
> Also fixes a bug where some entry freeing was missing causing efivarfs
> to leak memory.
>
> Signed-off-by: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> ---
This looks like a good idea to me.
> fs/efivarfs/internal.h | 1 -
> fs/efivarfs/super.c | 15 +++++++--------
> fs/efivarfs/vars.c | 39 +++------------------------------------
> 3 files changed, 10 insertions(+), 45 deletions(-)
>
> diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
> index 84a36e6fb653..d768bfa7f12b 100644
> --- a/fs/efivarfs/internal.h
> +++ b/fs/efivarfs/internal.h
> @@ -37,7 +37,6 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *,
>
> int efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
> void __efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
> -void efivar_entry_remove(struct efivar_entry *entry);
> int efivar_entry_delete(struct efivar_entry *entry);
>
> int efivar_entry_size(struct efivar_entry *entry, unsigned long *size);
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index dc3870ae784b..70b99f58c906 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -41,6 +41,12 @@ static int efivarfs_ops_notifier(struct notifier_block *nb, unsigned long event,
>
> static void efivarfs_evict_inode(struct inode *inode)
> {
> + struct efivar_entry *entry = inode->i_private;
> +
> + if (entry) {
> + list_del(&entry->list);
> + kfree(entry);
> + }
> clear_inode(inode);
> }
>
> @@ -269,13 +275,6 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
> return err;
> }
>
> -static int efivarfs_destroy(struct efivar_entry *entry, void *data)
> -{
> - efivar_entry_remove(entry);
> - kfree(entry);
> - return 0;
> -}
> -
> enum {
> Opt_uid, Opt_gid,
> };
> @@ -398,7 +397,7 @@ static void efivarfs_kill_sb(struct super_block *sb)
> kill_litter_super(sb);
>
> /* Remove all entries and destroy */
> - efivar_entry_iter(efivarfs_destroy, &sfi->efivarfs_list, NULL);
> + WARN_ON(!list_empty(&sfi->efivarfs_list));
> kfree(sfi);
> }
>
> diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
> index f6380fdbe173..bda8e8b869e8 100644
> --- a/fs/efivarfs/vars.c
> +++ b/fs/efivarfs/vars.c
> @@ -485,34 +485,6 @@ void __efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
> list_add(&entry->list, head);
> }
>
> -/**
> - * efivar_entry_remove - remove entry from variable list
> - * @entry: entry to remove from list
> - *
> - * Returns 0 on success, or a kernel error code on failure.
> - */
> -void efivar_entry_remove(struct efivar_entry *entry)
> -{
> - list_del(&entry->list);
> -}
> -
> -/*
> - * efivar_entry_list_del_unlock - remove entry from variable list
> - * @entry: entry to remove
> - *
> - * Remove @entry from the variable list and release the list lock.
> - *
> - * NOTE: slightly weird locking semantics here - we expect to be
> - * called with the efivars lock already held, and we release it before
> - * returning. This is because this function is usually called after
> - * set_variable() while the lock is still held.
> - */
> -static void efivar_entry_list_del_unlock(struct efivar_entry *entry)
> -{
> - list_del(&entry->list);
> - efivar_unlock();
> -}
> -
> /**
> * efivar_entry_delete - delete variable and remove entry from list
> * @entry: entry containing variable to delete
> @@ -536,12 +508,10 @@ int efivar_entry_delete(struct efivar_entry *entry)
> status = efivar_set_variable_locked(entry->var.VariableName,
> &entry->var.VendorGuid,
> 0, 0, NULL, false);
> - if (!(status == EFI_SUCCESS || status == EFI_NOT_FOUND)) {
> - efivar_unlock();
> + efivar_unlock();
> + if (!(status == EFI_SUCCESS || status == EFI_NOT_FOUND))
> return efi_status_to_err(status);
> - }
>
> - efivar_entry_list_del_unlock(entry);
> return 0;
> }
>
> @@ -679,10 +649,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
> &entry->var.VendorGuid,
> NULL, size, NULL);
>
> - if (status == EFI_NOT_FOUND)
> - efivar_entry_list_del_unlock(entry);
> - else
> - efivar_unlock();
> + efivar_unlock();
>
> if (status && status != EFI_BUFFER_TOO_SMALL)
> return efi_status_to_err(status);
> --
> 2.35.3
>
