Re: [PATCH v2 4/6] efivarfs: move freeing of variable entry into evict_inode

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

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;
 }