Re: [PATCH 3/6] efivarfs: make variable_is_present use dcache lookup

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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