[0/6] convert efivarfs to manage object data correctly

I've added fsdevel because I'm hopping some kind vfs person will check
the shift from efivarfs managing its own data to its data being
managed as part of the vfs object lifetimes.  The following paragraph
should describe all you need to know about the unusual features of the
filesystem.

efivarfs is a filesystem projecting the current state of the UEFI
variable store and allowing updates via write.  Because EFI variables
contain both contents and a set of attributes, which can't be mapped
to filesystem data, the u32 attribute is prepended to the output of
the file and, since UEFI variables can't be empty, this makes every
file at least 5 characters long.  EFI variables can be removed either
by doing an unlink (easy) or by doing a conventional write update that
reduces the content to zero size, which means any write update can
potentially remove the file.

Currently efivarfs has two bugs: it leaks memory and if a create is
attempted that results in an error in the write, it creates a zero
length file remnant that doesn't represent an EFI variable (i.e. the
state reflection of the EFI variable store goes out of sync).

The code uses inode->i_private to point to additionaly allocated
information but tries to maintain a global list of the shadowed
varibles for internal tracking.  Forgetting to kfree() entries in this
list when they are deleted is the source of the memory leak.

I've tried to make the patches as easily reviewable by non-EFI people
as possible, so some possible cleanups (like consolidating or removing
the efi lock handling and possibly removing the additional entry
allocation entirely in favour of simply converting the dentry name to
the variable name and guid) are left for later.

The first patch removes some unused fields in the entry; patches 2-3
eliminate the list search for duplication (some EFI variable stores
have buggy iterators) and replaces it with a dcache lookup.  Patch 4
move responsibility for freeing the entry data to inode eviction which
both fixes the memory leak and also means we no longer need to iterate
over the variable list and free its entries in kill_sb.  Since the
variable list is now unused, patch 5 removes it and its helper
functions.

Patch 6 fixes the second bug by introducing a file_operations->release
method that checks to see if the inode size is zero when the file is
closed and removes it if it is.  Since all files must be at least 5 in
length we use a zero i_size as an indicator that either the variable
was removed on write or that it wasn't correctly created in the first
place.

James

---

James Bottomley (6):
  efivarfs: remove unused efi_varaible.Attributes and .kobj
  efivarfs: add helper to convert from UC16 name and GUID to utf8 name
  efivarfs: make variable_is_present use dcache lookup
  efivarfs: move freeing of variable entry into evict_inode
  efivarfs: remove unused efivarfs_list
  efivarfs: fix error on write to new variable leaving remnants

 fs/efivarfs/file.c     |  31 ++++---
 fs/efivarfs/inode.c    |   5 --
 fs/efivarfs/internal.h |  20 ++---
 fs/efivarfs/super.c    |  59 +++++++-------
 fs/efivarfs/vars.c     | 179 ++++++++++-------------------------------
 5 files changed, 99 insertions(+), 195 deletions(-)

On Tue, Dec 10, 2024 at 9:03 AM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:

>  extern const struct file_operations efivarfs_file_operations;
>  extern const struct inode_operations efivarfs_dir_inode_operations;
> @@ -64,4 +66,6 @@ extern struct inode *efivarfs_get_inode(struct super_block *sb,
>                         const struct inode *dir, int mode, dev_t dev,
>                         bool is_removable);
>
> +
> +

Unnecessary
>  #endif /* EFIVAR_FS_INTERNAL_H */
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index b22441f7f7c6..dc3870ae784b 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -181,6 +181,26 @@ static struct dentry *efivarfs_alloc_dentry(struct dentry *parent, char *name)
>         return ERR_PTR(-ENOMEM);
>  }
>
> +bool efivarfs_variable_is_present(efi_char16_t *variable_name,
> +                                 efi_guid_t *vendor, void *data)
> +{
> +       char *name = efivar_get_utf8name(variable_name, vendor);
> +       struct super_block *sb = data;
> +       struct dentry *dentry;
> +       struct qstr qstr;
> +
> +       if (!name)
> +               return true;

Why is this true? I understand the previous implementation would have
hit a null dereference trying to calculate strsize1 on null, so this
isn't worse, but if we considered its length to be 0, it would not be
found.

> +
> +       qstr.name = name;
> +       qstr.len = strlen(name);
> +       dentry = d_hash_and_lookup(sb->s_root, &qstr);
> +       kfree(name);
> +       if (dentry)
> +               dput(dentry);
> +       return dentry != NULL;
> +}
> +
>  static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
>                              unsigned long name_size, void *data,
>                              struct list_head *list)
> diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
> index 7a07b767e2cc..f6380fdbe173 100644
> --- a/fs/efivarfs/vars.c
> +++ b/fs/efivarfs/vars.c
> @@ -313,28 +313,6 @@ efivar_variable_is_removable(efi_guid_t vendor, const char *var_name,
>         return found;
>  }
>
> -static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor,
> -                               struct list_head *head)
> -{
> -       struct efivar_entry *entry, *n;
> -       unsigned long strsize1, strsize2;
> -       bool found = false;
> -
> -       strsize1 = ucs2_strsize(variable_name, EFI_VAR_NAME_LEN);
> -       list_for_each_entry_safe(entry, n, head, list) {
> -               strsize2 = ucs2_strsize(entry->var.VariableName, EFI_VAR_NAME_LEN);
> -               if (strsize1 == strsize2 &&
> -                       !memcmp(variable_name, &(entry->var.VariableName),
> -                               strsize2) &&
> -                       !efi_guidcmp(entry->var.VendorGuid,
> -                               *vendor)) {
> -                       found = true;
> -                       break;
> -               }
> -       }
> -       return found;
> -}
> -
>  /*
>   * Returns the size of variable_name, in bytes, including the
>   * terminating NULL character, or variable_name_size if no NULL
> @@ -439,8 +417,8 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *,
>                          * we'll ever see a different variable name,
>                          * and may end up looping here forever.
>                          */
> -                       if (variable_is_present(variable_name, &vendor_guid,
> -                                               head)) {
> +                       if (efivarfs_variable_is_present(variable_name,
> +                                                        &vendor_guid, data)) {
>                                 dup_variable_bug(variable_name, &vendor_guid,
>                                                  variable_name_size);
>                                 status = EFI_NOT_FOUND;
> --
> 2.35.3
>
>

On Tue, 2024-12-10 at 09:14 -0800, Dionna Amalie Glaze wrote:
> On Tue, Dec 10, 2024 at 9:03 AM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> 
> >  extern const struct file_operations efivarfs_file_operations;
> >  extern const struct inode_operations
> > efivarfs_dir_inode_operations;
> > @@ -64,4 +66,6 @@ extern struct inode *efivarfs_get_inode(struct
> > super_block *sb,
> >                         const struct inode *dir, int mode, dev_t
> > dev,
> >                         bool is_removable);
> > 
> > +
> > +
> 
> Unnecessary

I can remove the extra line.

> >  #endif /* EFIVAR_FS_INTERNAL_H */
> > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> > index b22441f7f7c6..dc3870ae784b 100644
> > --- a/fs/efivarfs/super.c
> > +++ b/fs/efivarfs/super.c
> > @@ -181,6 +181,26 @@ static struct dentry
> > *efivarfs_alloc_dentry(struct dentry *parent, char *name)
> >         return ERR_PTR(-ENOMEM);
> >  }
> > 
> > +bool efivarfs_variable_is_present(efi_char16_t *variable_name,
> > +                                 efi_guid_t *vendor, void *data)
> > +{
> > +       char *name = efivar_get_utf8name(variable_name, vendor);
> > +       struct super_block *sb = data;
> > +       struct dentry *dentry;
> > +       struct qstr qstr;
> > +
> > +       if (!name)
> > +               return true;
> 
> Why is this true? I understand the previous implementation would have
> hit a null dereference trying to calculate strsize1 on null, so this
> isn't worse, but if we considered its length to be 0, it would not be
> found.

Because for safety on failure we need to assume a collision.  kmalloc
failing will already have dropped an error message so adding another
here (particularly when the log will likely be filling up with these
because we're in a critical memory shortage situation) would seem to be
overkill.  The memory allocation will never fail ordinarily and if it
does the system will be degrading fast, so EFI filesystem variable
collision will be the least of the problem.

Regards,

James