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. Regards, James --- 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; }
