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

James Bottomley
2025-01-16T22:13:09Z

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