| Message ID | 20250107023525.11466-5-James.Bottomley@HansenPartnership.com |
|---|---|
| State | New |
| Headers | show |
| Series | convert efivarfs to manage object data correctly | expand |
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@zeniv.linux.org.uk/ for recent example of such conversion.
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@zeniv.linux.org.uk/ > 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. Regards, James
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@zeniv.linux.org.uk/ > > 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; }
On Thu, 16 Jan 2025 at 23:13, James Bottomley <James.Bottomley@hansenpartnership.com> 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@zeniv.linux.org.uk/ > > > 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; > } > >
On Sun, 2025-01-19 at 15:50 +0100, Ard Biesheuvel wrote: > On Thu, 16 Jan 2025 at 23:13, James Bottomley > <James.Bottomley@hansenpartnership.com> 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@zeniv.linux.org.uk/ > > > > 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? Sure; I've got a slightly different version because after talking with Al he thinks it's OK still to put a pointer to the efivar_entry in i_private, which means less disruption. But there is enough disruption that the whole of the series needs redoing to avoid conflicts. > I'm fine with either, but note that I'd still like to target v6.14 > with this. Great, but I'm afraid the fix for the zero size problem also could do with being a precursor which is making the timing pretty tight. Regards, James
On Sun, 19 Jan 2025 at 15:57, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > On Sun, 2025-01-19 at 15:50 +0100, Ard Biesheuvel wrote: > > On Thu, 16 Jan 2025 at 23:13, James Bottomley > > <James.Bottomley@hansenpartnership.com> 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@zeniv.linux.org.uk/ > > > > > 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? > > Sure; I've got a slightly different version because after talking with > Al he thinks it's OK still to put a pointer to the efivar_entry in > i_private, which means less disruption. But there is enough disruption > that the whole of the series needs redoing to avoid conflicts. > > > I'm fine with either, but note that I'd still like to target v6.14 > > with this. > > Great, but I'm afraid the fix for the zero size problem also could do > with being a precursor which is making the timing pretty tight. > OK, I'll queue up your v3 about I won't send it out with the initial pull request, so we can make up our minds later. I take it the setattr series is intended for merging straight away?
On Sun, 2025-01-19 at 17:31 +0100, Ard Biesheuvel wrote: > On Sun, 19 Jan 2025 at 15:57, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > > > On Sun, 2025-01-19 at 15:50 +0100, Ard Biesheuvel wrote: > > > On Thu, 16 Jan 2025 at 23:13, James Bottomley > > > <James.Bottomley@hansenpartnership.com> 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@zeniv.linux.org.uk/ > > > > > > 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? > > > > Sure; I've got a slightly different version because after talking > > with Al he thinks it's OK still to put a pointer to the > > efivar_entry in i_private, which means less disruption. But there > > is enough disruption that the whole of the series needs redoing to > > avoid conflicts. > > > > > I'm fine with either, but note that I'd still like to target > > > v6.14 with this. > > > > Great, but I'm afraid the fix for the zero size problem also could > > do with being a precursor which is making the timing pretty tight. > > > > OK, I'll queue up your v3 about I won't send it out with the initial > pull request, so we can make up our minds later. > > I take it the setattr series is intended for merging straight away? Yes, it could be treated as a bug fix, although since only root could truncate an EFI variable in a normal installation, it's not a huge issue. Regards, James
diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h index 597ccaa60d37..8d82fc8bca31 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 9e90823f8009..d7facc99b745 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); } @@ -278,13 +284,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, }; @@ -407,7 +406,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 b2fc5bdc759a..bb9406e03a10 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);
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@HansenPartnership.com> --- fs/efivarfs/internal.h | 1 - fs/efivarfs/super.c | 15 +++++++-------- fs/efivarfs/vars.c | 39 +++------------------------------------ 3 files changed, 10 insertions(+), 45 deletions(-)