On Thu, 16 Jan 2025 at 23:13, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > On Thu, 2025-01-16 at 14:05 -0500, James Bottomley wrote: > > On Thu, 2025-01-16 at 18:36 +0000, Al Viro wrote: > > > On Mon, Jan 06, 2025 at 06:35:23PM -0800, 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. > > > > > > Umm... I'd rather coallocate struct inode and struct efivar_entry; > > > that way once you get rid of the list you don't need - > > > >evict_inode() > > > anymore. > > > > > > It's pretty easy - see e.g. > > > https://lore.kernel.org/all/20250112080705.141166-1-viro@xxxxxxxxxxxxxxxxxx/ > > > for recent example of such conversion. > > > > OK, I can do that. Although I think since the number of variables is > > usually around 150, it would probably be overkill to give it its own > > inode cache allocator. > > OK, this is what I've got. As you can see from the diffstat it's about > 10 lines more than the previous; mostly because of the new allocation > routine, the fact that the root inode has to be special cased for the > list and the guid has to be parsed in efivarfs_create before we have > the inode. > That looks straight-forward enough. Can you send this as a proper patch? Or would you prefer me to squash this into the one that is already queued up? I'm fine with either, but note that I'd still like to target v6.14 with this. > --- > > fs/efivarfs/file.c | 6 +++--- > fs/efivarfs/inode.c | 27 +++++++++++---------------- > fs/efivarfs/internal.h | 6 ++++++ > fs/efivarfs/super.c | 45 ++++++++++++++++++++++++++---------------- > --- > 4 files changed, 46 insertions(+), 38 deletions(-) > > diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c > index 23c51d62f902..176362b73d38 100644 > --- a/fs/efivarfs/file.c > +++ b/fs/efivarfs/file.c > @@ -15,10 +15,10 @@ > static ssize_t efivarfs_file_write(struct file *file, > const char __user *userbuf, size_t count, loff_t > *ppos) > { > - struct efivar_entry *var = file->private_data; > void *data; > u32 attributes; > struct inode *inode = file->f_mapping->host; > + struct efivar_entry *var = efivar_entry(inode); > unsigned long datasize = count - sizeof(attributes); > ssize_t bytes; > bool set = false; > @@ -66,7 +66,8 @@ static ssize_t efivarfs_file_write(struct file *file, > static ssize_t efivarfs_file_read(struct file *file, char __user > *userbuf, > size_t count, loff_t *ppos) > { > - struct efivar_entry *var = file->private_data; > + struct inode *inode = file->f_mapping->host; > + struct efivar_entry *var = efivar_entry(inode); > unsigned long datasize = 0; > u32 attributes; > void *data; > @@ -107,7 +108,6 @@ static ssize_t efivarfs_file_read(struct file > *file, char __user *userbuf, > } > > const struct file_operations efivarfs_file_operations = { > - .open = simple_open, > .read = efivarfs_file_read, > .write = efivarfs_file_write, > }; > diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c > index ec23da8405ff..a13ffb01e149 100644 > --- a/fs/efivarfs/inode.c > +++ b/fs/efivarfs/inode.c > @@ -82,26 +82,23 @@ static int efivarfs_create(struct mnt_idmap *idmap, > struct inode *dir, > struct efivar_entry *var; > int namelen, i = 0, err = 0; > bool is_removable = false; > + efi_guid_t vendor; > > if (!efivarfs_valid_name(dentry->d_name.name, dentry- > >d_name.len)) > return -EINVAL; > > - var = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL); > - if (!var) > - return -ENOMEM; > - > /* length of the variable name itself: remove GUID and > separator */ > namelen = dentry->d_name.len - EFI_VARIABLE_GUID_LEN - 1; > > - err = guid_parse(dentry->d_name.name + namelen + 1, &var- > >var.VendorGuid); > + err = guid_parse(dentry->d_name.name + namelen + 1, &vendor); > if (err) > goto out; > - if (guid_equal(&var->var.VendorGuid, > &LINUX_EFI_RANDOM_SEED_TABLE_GUID)) { > + if (guid_equal(&vendor, &LINUX_EFI_RANDOM_SEED_TABLE_GUID)) { > err = -EPERM; > goto out; > } > > - if (efivar_variable_is_removable(var->var.VendorGuid, > + if (efivar_variable_is_removable(vendor, > dentry->d_name.name, > namelen)) > is_removable = true; > > @@ -110,15 +107,15 @@ static int efivarfs_create(struct mnt_idmap > *idmap, struct inode *dir, > err = -ENOMEM; > goto out; > } > + var = efivar_entry(inode); > + > + var->var.VendorGuid = vendor; > > for (i = 0; i < namelen; i++) > var->var.VariableName[i] = dentry->d_name.name[i]; > > var->var.VariableName[i] = '\0'; > > - inode->i_private = var; > - kmemleak_ignore(var); > - > err = efivar_entry_add(var, &info->efivarfs_list); > if (err) > goto out; > @@ -126,17 +123,15 @@ static int efivarfs_create(struct mnt_idmap > *idmap, struct inode *dir, > d_instantiate(dentry, inode); > dget(dentry); > out: > - if (err) { > - kfree(var); > - if (inode) > - iput(inode); > - } > + if (err && inode) > + iput(inode); > + > return err; > } > > static int efivarfs_unlink(struct inode *dir, struct dentry *dentry) > { > - struct efivar_entry *var = d_inode(dentry)->i_private; > + struct efivar_entry *var = efivar_entry(d_inode(dentry)); > > if (efivar_entry_delete(var)) > return -EINVAL; > diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h > index 8d82fc8bca31..fce7d5e5c763 100644 > --- a/fs/efivarfs/internal.h > +++ b/fs/efivarfs/internal.h > @@ -29,8 +29,14 @@ struct efi_variable { > struct efivar_entry { > struct efi_variable var; > struct list_head list; > + struct inode vfs_inode; > }; > > +static inline struct efivar_entry *efivar_entry(struct inode *inode) > +{ > + return container_of(inode, struct efivar_entry, vfs_inode); > +} > + > int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, > void *, > struct list_head *), > void *data, struct list_head *head); > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c > index d7facc99b745..cfead280534c 100644 > --- a/fs/efivarfs/super.c > +++ b/fs/efivarfs/super.c > @@ -39,15 +39,25 @@ static int efivarfs_ops_notifier(struct > notifier_block *nb, unsigned long event, > return NOTIFY_OK; > } > > -static void efivarfs_evict_inode(struct inode *inode) > +static struct inode *efivarfs_alloc_inode(struct super_block *sb) > { > - struct efivar_entry *entry = inode->i_private; > + struct efivar_entry *entry = kzalloc(sizeof(*entry), > GFP_KERNEL); > > - if (entry) { > + if (!entry) > + return NULL; > + > + inode_init_once(&entry->vfs_inode); > + > + return &entry->vfs_inode; > +} > + > +static void efivarfs_free_inode(struct inode *inode) > +{ > + struct efivar_entry *entry = efivar_entry(inode); > + > + if (!is_root_inode(inode)) > list_del(&entry->list); > - kfree(entry); > - } > - clear_inode(inode); > + kfree(entry); > } > > static int efivarfs_show_options(struct seq_file *m, struct dentry > *root) > @@ -112,7 +122,8 @@ static int efivarfs_statfs(struct dentry *dentry, > struct kstatfs *buf) > static const struct super_operations efivarfs_ops = { > .statfs = efivarfs_statfs, > .drop_inode = generic_delete_inode, > - .evict_inode = efivarfs_evict_inode, > + .alloc_inode = efivarfs_alloc_inode, > + .free_inode = efivarfs_free_inode, > .show_options = efivarfs_show_options, > }; > > @@ -233,21 +244,14 @@ static int efivarfs_callback(efi_char16_t > *name16, efi_guid_t vendor, > if (guid_equal(&vendor, &LINUX_EFI_RANDOM_SEED_TABLE_GUID)) > return 0; > > - entry = kzalloc(sizeof(*entry), GFP_KERNEL); > - if (!entry) > - return err; > - > - memcpy(entry->var.VariableName, name16, name_size); > - memcpy(&(entry->var.VendorGuid), &vendor, sizeof(efi_guid_t)); > - > name = efivar_get_utf8name(name16, &vendor); > if (!name) > - goto fail; > + return err; > > /* length of the variable name itself: remove GUID and > separator */ > len = strlen(name) - EFI_VARIABLE_GUID_LEN - 1; > > - if (efivar_variable_is_removable(entry->var.VendorGuid, name, > len)) > + if (efivar_variable_is_removable(vendor, name, len)) > is_removable = true; > > inode = efivarfs_get_inode(sb, d_inode(root), S_IFREG | 0644, > 0, > @@ -255,6 +259,11 @@ static int efivarfs_callback(efi_char16_t *name16, > efi_guid_t vendor, > if (!inode) > goto fail_name; > > + entry = efivar_entry(inode); > + > + memcpy(entry->var.VariableName, name16, name_size); > + memcpy(&(entry->var.VendorGuid), &vendor, sizeof(efi_guid_t)); > + > dentry = efivarfs_alloc_dentry(root, name); > if (IS_ERR(dentry)) { > err = PTR_ERR(dentry); > @@ -268,7 +277,6 @@ static int efivarfs_callback(efi_char16_t *name16, > efi_guid_t vendor, > kfree(name); > > inode_lock(inode); > - inode->i_private = entry; > i_size_write(inode, size + sizeof(__u32)); /* attributes + > data */ > inode_unlock(inode); > d_add(dentry, inode); > @@ -279,8 +287,7 @@ static int efivarfs_callback(efi_char16_t *name16, > efi_guid_t vendor, > iput(inode); > fail_name: > kfree(name); > -fail: > - kfree(entry); > + > return err; > } > >
