[PATCH v2 0/6] convert efivarfs to manage object data correctly

[PATCH v2 0/6] convert efivarfs to manage object data correctly

[James Bottomley]

I've added fsdevel because I'm hopping some kind vfs person will check
the shift from efivarfs managing its own data to its data being
managed as part of the vfs object lifetimes.  The following paragraph
should describe all you need to know about the unusual features of the
filesystem.

efivarfs is a filesystem projecting the current state of the UEFI
variable store and allowing updates via write.  Because EFI variables
contain both contents and a set of attributes, which can't be mapped
to filesystem data, the u32 attribute is prepended to the output of
the file and, since UEFI variables can't be empty, this makes every
file at least 5 characters long.  EFI variables can be removed either
by doing an unlink (easy) or by doing a conventional write update that
reduces the content to zero size, which means any write update can
potentially remove the file.

Currently efivarfs has two bugs: it leaks memory and if a create is
attempted that results in an error in the write, it creates a zero
length file remnant that doesn't represent an EFI variable (i.e. the
state reflection of the EFI variable store goes out of sync).

The code uses inode->i_private to point to additionaly allocated
information but tries to maintain a global list of the shadowed
varibles for internal tracking.  Forgetting to kfree() entries in this
list when they are deleted is the source of the memory leak.

I've tried to make the patches as easily reviewable by non-EFI people
as possible, so some possible cleanups (like consolidating or removing
the efi lock handling and possibly removing the additional entry
allocation entirely in favour of simply converting the dentry name to
the variable name and guid) are left for later.

The first patch removes some unused fields in the entry; patches 2-3
eliminate the list search for duplication (some EFI variable stores
have buggy iterators) and replaces it with a dcache lookup.  Patch 4
move responsibility for freeing the entry data to inode eviction which
both fixes the memory leak and also means we no longer need to iterate
over the variable list and free its entries in kill_sb.  Since the
variable list is now unused, patch 5 removes it and its helper
functions.

Patch 6 fixes the second bug by introducing a file_operations->release
method that checks to see if the inode size is zero when the file is
closed and removes it if it is.  Since all files must be at least 5 in
length we use a zero i_size as an indicator that either the variable
was removed on write or that it wasn't correctly created in the first
place.

v2: folded in feedback from Al Viro: check errors on lookup and delete
    zero length file on last close

James

---

James Bottomley (6):
  efivarfs: remove unused efi_varaible.Attributes and .kobj
  efivarfs: add helper to convert from UC16 name and GUID to utf8 name
  efivarfs: make variable_is_present use dcache lookup
  efivarfs: move freeing of variable entry into evict_inode
  efivarfs: remove unused efivarfs_list
  efivarfs: fix error on write to new variable leaving remnants

 fs/efivarfs/file.c     |  60 +++++++++++---
 fs/efivarfs/inode.c    |   5 --
 fs/efivarfs/internal.h |  19 ++---
 fs/efivarfs/super.c    |  68 +++++++++-------
 fs/efivarfs/vars.c     | 179 ++++++++++-------------------------------
 5 files changed, 136 insertions(+), 195 deletions(-)

-- 
2.35.3

[PATCH v2 1/6] efivarfs: remove unused efi_varaible.Attributes and .kobj

[James Bottomley]

These fields look to be remnants of older code: Attributes was likely
meant to stash the variable attributes, but doesn't because we always
read them from the variable store and kobj was likely left over from
an older iteration of code where we manually created the objects
instead of using a filesystem.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 fs/efivarfs/internal.h | 2 --
 fs/efivarfs/super.c    | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
index 74f0602a9e01..64d15d1bb6bf 100644
--- a/fs/efivarfs/internal.h
+++ b/fs/efivarfs/internal.h
@@ -24,13 +24,11 @@ struct efivarfs_fs_info {
 struct efi_variable {
 	efi_char16_t  VariableName[EFI_VAR_NAME_LEN/sizeof(efi_char16_t)];
 	efi_guid_t    VendorGuid;
-	__u32         Attributes;
 };
 
 struct efivar_entry {
 	struct efi_variable var;
 	struct list_head list;
-	struct kobject kobj;
 };
 
 int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *,
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index beba15673be8..d3c8528274aa 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -245,7 +245,7 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 
 	inode_lock(inode);
 	inode->i_private = entry;
-	i_size_write(inode, size + sizeof(entry->var.Attributes));
+	i_size_write(inode, size + sizeof(__u32)); /* attributes + data */
 	inode_unlock(inode);
 	d_add(dentry, inode);
 
-- 
2.35.3

[PATCH v2 2/6] efivarfs: add helper to convert from UC16 name and GUID to utf8 name

[James Bottomley]

These will be used by a later patch to check for uniqueness on initial
EFI variable iteration.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 fs/efivarfs/internal.h |  1 +
 fs/efivarfs/super.c    | 17 +++--------------
 fs/efivarfs/vars.c     | 25 +++++++++++++++++++++++++
 3 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
index 64d15d1bb6bf..c10efc1ad0a7 100644
--- a/fs/efivarfs/internal.h
+++ b/fs/efivarfs/internal.h
@@ -55,6 +55,7 @@ bool efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
 		     unsigned long data_size);
 bool efivar_variable_is_removable(efi_guid_t vendor, const char *name,
 				  size_t len);
+char *efivar_get_utf8name(const efi_char16_t *name16, efi_guid_t *vendor);
 
 extern const struct file_operations efivarfs_file_operations;
 extern const struct inode_operations efivarfs_dir_inode_operations;
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index d3c8528274aa..b22441f7f7c6 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -205,27 +205,16 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 	memcpy(entry->var.VariableName, name16, name_size);
 	memcpy(&(entry->var.VendorGuid), &vendor, sizeof(efi_guid_t));
 
-	len = ucs2_utf8size(entry->var.VariableName);
-
-	/* name, plus '-', plus GUID, plus NUL*/
-	name = kmalloc(len + 1 + EFI_VARIABLE_GUID_LEN + 1, GFP_KERNEL);
+	name = efivar_get_utf8name(name16, &vendor);
 	if (!name)
 		goto fail;
 
-	ucs2_as_utf8(name, entry->var.VariableName, len);
+	/* 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))
 		is_removable = true;
 
-	name[len] = '-';
-
-	efi_guid_to_str(&entry->var.VendorGuid, name + len + 1);
-
-	name[len + EFI_VARIABLE_GUID_LEN+1] = '\0';
-
-	/* replace invalid slashes like kobject_set_name_vargs does for /sys/firmware/efi/vars. */
-	strreplace(name, '/', '!');
-
 	inode = efivarfs_get_inode(sb, d_inode(root), S_IFREG | 0644, 0,
 				   is_removable);
 	if (!inode)
diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
index f7d43c847ee9..13594087d9ee 100644
--- a/fs/efivarfs/vars.c
+++ b/fs/efivarfs/vars.c
@@ -225,6 +225,31 @@ variable_matches(const char *var_name, size_t len, const char *match_name,
 	}
 }
 
+char *
+efivar_get_utf8name(const efi_char16_t *name16, efi_guid_t *vendor)
+{
+	int len = ucs2_utf8size(name16);
+	char *name;
+
+	/* name, plus '-', plus GUID, plus NUL*/
+	name = kmalloc(len + 1 + EFI_VARIABLE_GUID_LEN + 1, GFP_KERNEL);
+	if (!name)
+		return NULL;
+
+	ucs2_as_utf8(name, name16, len);
+
+	name[len] = '-';
+
+	efi_guid_to_str(vendor, name + len + 1);
+
+	name[len + EFI_VARIABLE_GUID_LEN+1] = '\0';
+
+	/* replace invalid slashes like kobject_set_name_vargs does for /sys/firmware/efi/vars. */
+	strreplace(name, '/', '!');
+
+	return name;
+}
+
 bool
 efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
 		unsigned long data_size)
-- 
2.35.3

[PATCH v2 3/6] efivarfs: make variable_is_present use dcache lookup

[James Bottomley]

Instead of searching the variable entry list for a variable, use the
dcache lookup functions to find it instead.  Also add an efivarfs_
prefix to the function now it is no longer static.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---
v2: add IS_ERR_OR_NULL check before doing dput
---
 fs/efivarfs/internal.h |  2 ++
 fs/efivarfs/super.c    | 29 +++++++++++++++++++++++++++++
 fs/efivarfs/vars.c     | 26 ++------------------------
 3 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
index c10efc1ad0a7..597ccaa60d37 100644
--- a/fs/efivarfs/internal.h
+++ b/fs/efivarfs/internal.h
@@ -56,6 +56,8 @@ bool efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
 bool efivar_variable_is_removable(efi_guid_t vendor, const char *name,
 				  size_t len);
 char *efivar_get_utf8name(const efi_char16_t *name16, efi_guid_t *vendor);
+bool efivarfs_variable_is_present(efi_char16_t *variable_name,
+				  efi_guid_t *vendor, void *data);
 
 extern const struct file_operations efivarfs_file_operations;
 extern const struct inode_operations efivarfs_dir_inode_operations;
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index b22441f7f7c6..9e90823f8009 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -181,6 +181,35 @@ static struct dentry *efivarfs_alloc_dentry(struct dentry *parent, char *name)
 	return ERR_PTR(-ENOMEM);
 }
 
+bool efivarfs_variable_is_present(efi_char16_t *variable_name,
+				  efi_guid_t *vendor, void *data)
+{
+	char *name = efivar_get_utf8name(variable_name, vendor);
+	struct super_block *sb = data;
+	struct dentry *dentry;
+	struct qstr qstr;
+
+	if (!name)
+		/*
+		 * If the allocation failed there'll already be an
+		 * error in the log (and likely a huge and growing
+		 * number of them since they system will be under
+		 * extreme memory pressure), so simply assume
+		 * collision for safety but don't add to the log
+		 * flood.
+		 */
+		return true;
+
+	qstr.name = name;
+	qstr.len = strlen(name);
+	dentry = d_hash_and_lookup(sb->s_root, &qstr);
+	kfree(name);
+	if (!IS_ERR_OR_NULL(dentry))
+		dput(dentry);
+
+	return dentry != NULL;
+}
+
 static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 			     unsigned long name_size, void *data,
 			     struct list_head *list)
diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
index 13594087d9ee..b2fc5bdc759a 100644
--- a/fs/efivarfs/vars.c
+++ b/fs/efivarfs/vars.c
@@ -313,28 +313,6 @@ efivar_variable_is_removable(efi_guid_t vendor, const char *var_name,
 	return found;
 }
 
-static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor,
-				struct list_head *head)
-{
-	struct efivar_entry *entry, *n;
-	unsigned long strsize1, strsize2;
-	bool found = false;
-
-	strsize1 = ucs2_strsize(variable_name, EFI_VAR_NAME_LEN);
-	list_for_each_entry_safe(entry, n, head, list) {
-		strsize2 = ucs2_strsize(entry->var.VariableName, EFI_VAR_NAME_LEN);
-		if (strsize1 == strsize2 &&
-			!memcmp(variable_name, &(entry->var.VariableName),
-				strsize2) &&
-			!efi_guidcmp(entry->var.VendorGuid,
-				*vendor)) {
-			found = true;
-			break;
-		}
-	}
-	return found;
-}
-
 /*
  * Returns the size of variable_name, in bytes, including the
  * terminating NULL character, or variable_name_size if no NULL
@@ -439,8 +417,8 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *,
 			 * we'll ever see a different variable name,
 			 * and may end up looping here forever.
 			 */
-			if (variable_is_present(variable_name, &vendor_guid,
-						head)) {
+			if (efivarfs_variable_is_present(variable_name,
+							 &vendor_guid, data)) {
 				dup_variable_bug(variable_name, &vendor_guid,
 						 variable_name_size);
 				status = EFI_NOT_FOUND;
-- 
2.35.3

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

[James Bottomley]

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(-)

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);
-- 
2.35.3

[PATCH v2 5/6] efivarfs: remove unused efivarfs_list

[James Bottomley]

Remove all function helpers and mentions of the efivarfs_list now that
all consumers of the list have been removed and entry management goes
exclusively through the inode.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 fs/efivarfs/inode.c    |  5 ---
 fs/efivarfs/internal.h | 12 +-----
 fs/efivarfs/super.c    | 15 ++-----
 fs/efivarfs/vars.c     | 89 ++++++------------------------------------
 4 files changed, 16 insertions(+), 105 deletions(-)

diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c
index ec23da8405ff..7fe1b5b60902 100644
--- a/fs/efivarfs/inode.c
+++ b/fs/efivarfs/inode.c
@@ -77,7 +77,6 @@ static bool efivarfs_valid_name(const char *str, int len)
 static int efivarfs_create(struct mnt_idmap *idmap, struct inode *dir,
 			   struct dentry *dentry, umode_t mode, bool excl)
 {
-	struct efivarfs_fs_info *info = dir->i_sb->s_fs_info;
 	struct inode *inode = NULL;
 	struct efivar_entry *var;
 	int namelen, i = 0, err = 0;
@@ -119,10 +118,6 @@ static int efivarfs_create(struct mnt_idmap *idmap, struct inode *dir,
 	inode->i_private = var;
 	kmemleak_ignore(var);
 
-	err = efivar_entry_add(var, &info->efivarfs_list);
-	if (err)
-		goto out;
-
 	d_instantiate(dentry, inode);
 	dget(dentry);
 out:
diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
index 8d82fc8bca31..18a600f80992 100644
--- a/fs/efivarfs/internal.h
+++ b/fs/efivarfs/internal.h
@@ -6,7 +6,6 @@
 #ifndef EFIVAR_FS_INTERNAL_H
 #define EFIVAR_FS_INTERNAL_H
 
-#include <linux/list.h>
 #include <linux/efi.h>
 
 struct efivarfs_mount_opts {
@@ -16,7 +15,6 @@ struct efivarfs_mount_opts {
 
 struct efivarfs_fs_info {
 	struct efivarfs_mount_opts mount_opts;
-	struct list_head efivarfs_list;
 	struct super_block *sb;
 	struct notifier_block nb;
 };
@@ -28,15 +26,11 @@ struct efi_variable {
 
 struct efivar_entry {
 	struct efi_variable var;
-	struct list_head list;
 };
 
-int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *,
-			    struct list_head *),
-		void *data, struct list_head *head);
+int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
+		void *data);
 
-int efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
-void __efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
 int efivar_entry_delete(struct efivar_entry *entry);
 
 int efivar_entry_size(struct efivar_entry *entry, unsigned long *size);
@@ -47,8 +41,6 @@ int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
 int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 			      unsigned long *size, void *data, bool *set);
 
-int efivar_entry_iter(int (*func)(struct efivar_entry *, void *),
-		      struct list_head *head, void *data);
 
 bool efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
 		     unsigned long data_size);
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index d7facc99b745..2523e74dbcfd 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -43,10 +43,7 @@ static void efivarfs_evict_inode(struct inode *inode)
 {
 	struct efivar_entry *entry = inode->i_private;
 
-	if (entry)  {
-		list_del(&entry->list);
-		kfree(entry);
-	}
+	kfree(entry);
 	clear_inode(inode);
 }
 
@@ -217,8 +214,7 @@ bool efivarfs_variable_is_present(efi_char16_t *variable_name,
 }
 
 static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
-			     unsigned long name_size, void *data,
-			     struct list_head *list)
+			     unsigned long name_size, void *data)
 {
 	struct super_block *sb = (struct super_block *)data;
 	struct efivar_entry *entry;
@@ -262,7 +258,6 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 	}
 
 	__efivar_entry_get(entry, NULL, &size, NULL);
-	__efivar_entry_add(entry, list);
 
 	/* copied by the above to local storage in the dentry. */
 	kfree(name);
@@ -353,7 +348,7 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	if (err)
 		return err;
 
-	return efivar_init(efivarfs_callback, sb, &sfi->efivarfs_list);
+	return efivar_init(efivarfs_callback, sb);
 }
 
 static int efivarfs_get_tree(struct fs_context *fc)
@@ -388,8 +383,6 @@ static int efivarfs_init_fs_context(struct fs_context *fc)
 	if (!sfi)
 		return -ENOMEM;
 
-	INIT_LIST_HEAD(&sfi->efivarfs_list);
-
 	sfi->mount_opts.uid = GLOBAL_ROOT_UID;
 	sfi->mount_opts.gid = GLOBAL_ROOT_GID;
 
@@ -405,8 +398,6 @@ static void efivarfs_kill_sb(struct super_block *sb)
 	blocking_notifier_chain_unregister(&efivar_ops_nh, &sfi->nb);
 	kill_litter_super(sb);
 
-	/* Remove all entries and destroy */
-	WARN_ON(!list_empty(&sfi->efivarfs_list));
 	kfree(sfi);
 }
 
diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
index bb9406e03a10..d0beecbf9441 100644
--- a/fs/efivarfs/vars.c
+++ b/fs/efivarfs/vars.c
@@ -364,16 +364,14 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
  * efivar_init - build the initial list of EFI variables
  * @func: callback function to invoke for every variable
  * @data: function-specific data to pass to @func
- * @head: initialised head of variable list
  *
  * Get every EFI variable from the firmware and invoke @func. @func
- * should call efivar_entry_add() to build the list of variables.
+ * should populate the initial dentry and inode tree.
  *
  * Returns 0 on success, or a kernel error code on failure.
  */
-int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *,
-			    struct list_head *),
-		void *data, struct list_head *head)
+int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
+		void *data)
 {
 	unsigned long variable_name_size = 512;
 	efi_char16_t *variable_name;
@@ -424,7 +422,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *,
 				status = EFI_NOT_FOUND;
 			} else {
 				err = func(variable_name, vendor_guid,
-					   variable_name_size, data, head);
+					   variable_name_size, data);
 				if (err)
 					status = EFI_NOT_FOUND;
 			}
@@ -456,42 +454,12 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *,
 }
 
 /**
- * efivar_entry_add - add entry to variable list
- * @entry: entry to add to list
- * @head: list head
- *
- * Returns 0 on success, or a kernel error code on failure.
- */
-int efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
-{
-	int err;
-
-	err = efivar_lock();
-	if (err)
-		return err;
-	list_add(&entry->list, head);
-	efivar_unlock();
-
-	return 0;
-}
-
-/**
- * __efivar_entry_add - add entry to variable list
- * @entry: entry to add to list
- * @head: list head
- */
-void __efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
-{
-	list_add(&entry->list, head);
-}
-
-/**
- * efivar_entry_delete - delete variable and remove entry from list
+ * efivar_entry_delete - delete variable
  * @entry: entry containing variable to delete
  *
- * Delete the variable from the firmware and remove @entry from the
- * variable list. It is the caller's responsibility to free @entry
- * once we return.
+ * Delete the variable from the firmware. It is the caller's
+ * responsibility to free @entry (by deleting the dentry/inode) once
+ * we return.
  *
  * Returns 0 on success, -EINTR if we can't grab the semaphore,
  * converted EFI status code if set_variable() fails.
@@ -605,7 +573,7 @@ int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
  * get_variable() fail.
  *
  * If the EFI variable does not exist when calling set_variable()
- * (EFI_NOT_FOUND), @entry is removed from the variable list.
+ * (EFI_NOT_FOUND).
  */
 int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 			      unsigned long *size, void *data, bool *set)
@@ -621,9 +589,8 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 		return -EINVAL;
 
 	/*
-	 * The lock here protects the get_variable call, the conditional
-	 * set_variable call, and removal of the variable from the efivars
-	 * list (in the case of an authenticated delete).
+	 * The lock here protects the get_variable call and the
+	 * conditional set_variable call
 	 */
 	err = efivar_lock();
 	if (err)
@@ -661,37 +628,3 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	return err;
 
 }
-
-/**
- * efivar_entry_iter - iterate over variable list
- * @func: callback function
- * @head: head of variable list
- * @data: function-specific data to pass to callback
- *
- * Iterate over the list of EFI variables and call @func with every
- * entry on the list. It is safe for @func to remove entries in the
- * list via efivar_entry_delete() while iterating.
- *
- * Some notes for the callback function:
- *  - a non-zero return value indicates an error and terminates the loop
- *  - @func is called from atomic context
- */
-int efivar_entry_iter(int (*func)(struct efivar_entry *, void *),
-		      struct list_head *head, void *data)
-{
-	struct efivar_entry *entry, *n;
-	int err = 0;
-
-	err = efivar_lock();
-	if (err)
-		return err;
-
-	list_for_each_entry_safe(entry, n, head, list) {
-		err = func(entry, data);
-		if (err)
-			break;
-	}
-	efivar_unlock();
-
-	return err;
-}
-- 
2.35.3

[PATCH v2 6/6] efivarfs: fix error on write to new variable leaving remnants

[James Bottomley]

Make variable cleanup go through the fops release mechanism and use
zero inode size as the indicator to delete the file.  Since all EFI
variables must have an initial u32 attribute, zero size occurs either
because the update deleted the variable or because an unsuccessful
write after create caused the size never to be set in the first place.
In the case of multiple racing opens and closes, the open is counted
to ensure that the zero size check is done on the last close.

Even though this fixes the bug that a create either not followed by a
write or followed by a write that errored would leave a remnant file
for the variable, the file will appear momentarily globally visible
until the last close of the fd deletes it.  This is safe because the
normal filesystem operations will mediate any races; however, it is
still possible for a directory listing at that instant between create
and close contain a zero size variable that doesn't exist in the EFI
table.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---
v2: implement counter for last close
---
 fs/efivarfs/file.c     | 60 +++++++++++++++++++++++++++++++++++-------
 fs/efivarfs/internal.h |  1 +
 2 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 23c51d62f902..cf0179d84bc5 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -36,28 +36,41 @@ static ssize_t efivarfs_file_write(struct file *file,
 	if (IS_ERR(data))
 		return PTR_ERR(data);
 
+	inode_lock(inode);
+	if (d_unhashed(file->f_path.dentry)) {
+		/*
+		 * file got removed; don't allow a set.  Caused by an
+		 * unsuccessful create or successful delete write
+		 * racing with us.
+		 */
+		bytes = -EIO;
+		goto out;
+	}
+
 	bytes = efivar_entry_set_get_size(var, attributes, &datasize,
 					  data, &set);
-	if (!set && bytes) {
+	if (!set) {
 		if (bytes == -ENOENT)
 			bytes = -EIO;
 		goto out;
 	}
 
 	if (bytes == -ENOENT) {
-		drop_nlink(inode);
-		d_delete(file->f_path.dentry);
-		dput(file->f_path.dentry);
+		/*
+		 * zero size signals to release that the write deleted
+		 * the variable
+		 */
+		i_size_write(inode, 0);
 	} else {
-		inode_lock(inode);
 		i_size_write(inode, datasize + sizeof(attributes));
 		inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
-		inode_unlock(inode);
 	}
 
 	bytes = count;
 
 out:
+	inode_unlock(inode);
+
 	kfree(data);
 
 	return bytes;
@@ -106,8 +119,37 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 	return size;
 }
 
+static int efivarfs_file_release(struct inode *inode, struct file *file)
+{
+	bool release;
+	struct efivar_entry *var = inode->i_private;
+
+	inode_lock(inode);
+	release = (--var->open_count == 0 && i_size_read(inode) == 0);
+	inode_unlock(inode);
+
+	if (release)
+		simple_recursive_removal(file->f_path.dentry, NULL);
+
+	return 0;
+}
+
+static int efivarfs_file_open(struct inode *inode, struct file *file)
+{
+	struct efivar_entry *entry = inode->i_private;
+
+	file->private_data = entry;
+
+	inode_lock(inode);
+	entry->open_count++;
+	inode_unlock(inode);
+
+	return 0;
+}
+
 const struct file_operations efivarfs_file_operations = {
-	.open	= simple_open,
-	.read	= efivarfs_file_read,
-	.write	= efivarfs_file_write,
+	.open		= efivarfs_file_open,
+	.read		= efivarfs_file_read,
+	.write		= efivarfs_file_write,
+	.release	= efivarfs_file_release,
 };
diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
index 18a600f80992..32b83f644798 100644
--- a/fs/efivarfs/internal.h
+++ b/fs/efivarfs/internal.h
@@ -26,6 +26,7 @@ struct efi_variable {
 
 struct efivar_entry {
 	struct efi_variable var;
+	unsigned long open_count;
 };
 
 int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
-- 
2.35.3

Re: [PATCH v2 0/6] convert efivarfs to manage object data correctly

[Ard Biesheuvel]

On Tue, 7 Jan 2025 at 03:36, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> I've added fsdevel because I'm hopping some kind vfs person will check
> the shift from efivarfs managing its own data to its data being
> managed as part of the vfs object lifetimes.  The following paragraph
> should describe all you need to know about the unusual features of the
> filesystem.
>
> efivarfs is a filesystem projecting the current state of the UEFI
> variable store and allowing updates via write.  Because EFI variables
> contain both contents and a set of attributes, which can't be mapped
> to filesystem data, the u32 attribute is prepended to the output of
> the file and, since UEFI variables can't be empty, this makes every
> file at least 5 characters long.  EFI variables can be removed either
> by doing an unlink (easy) or by doing a conventional write update that
> reduces the content to zero size, which means any write update can
> potentially remove the file.
>
> Currently efivarfs has two bugs: it leaks memory and if a create is
> attempted that results in an error in the write, it creates a zero
> length file remnant that doesn't represent an EFI variable (i.e. the
> state reflection of the EFI variable store goes out of sync).
>
> The code uses inode->i_private to point to additionaly allocated
> information but tries to maintain a global list of the shadowed
> varibles for internal tracking.  Forgetting to kfree() entries in this
> list when they are deleted is the source of the memory leak.
>
> I've tried to make the patches as easily reviewable by non-EFI people
> as possible, so some possible cleanups (like consolidating or removing
> the efi lock handling and possibly removing the additional entry
> allocation entirely in favour of simply converting the dentry name to
> the variable name and guid) are left for later.
>
> The first patch removes some unused fields in the entry; patches 2-3
> eliminate the list search for duplication (some EFI variable stores
> have buggy iterators) and replaces it with a dcache lookup.  Patch 4
> move responsibility for freeing the entry data to inode eviction which
> both fixes the memory leak and also means we no longer need to iterate
> over the variable list and free its entries in kill_sb.  Since the
> variable list is now unused, patch 5 removes it and its helper
> functions.
>
> Patch 6 fixes the second bug by introducing a file_operations->release
> method that checks to see if the inode size is zero when the file is
> closed and removes it if it is.  Since all files must be at least 5 in
> length we use a zero i_size as an indicator that either the variable
> was removed on write or that it wasn't correctly created in the first
> place.
>
> v2: folded in feedback from Al Viro: check errors on lookup and delete
>     zero length file on last close
>
> James
>
> ---
>
> James Bottomley (6):
>   efivarfs: remove unused efi_varaible.Attributes and .kobj
>   efivarfs: add helper to convert from UC16 name and GUID to utf8 name
>   efivarfs: make variable_is_present use dcache lookup
>   efivarfs: move freeing of variable entry into evict_inode
>   efivarfs: remove unused efivarfs_list
>   efivarfs: fix error on write to new variable leaving remnants
>

Thanks James,

I've tentatively queued up this series, as well as the hibernate one,
to get some coverage from the robots while I run some tests myself.

Are there any existing test suites that cover efivarfs that you could recommend?

Re: [PATCH v2 0/6] convert efivarfs to manage object data correctly

[Ard Biesheuvel]

On Thu, 9 Jan 2025 at 10:50, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 7 Jan 2025 at 03:36, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> >
> > I've added fsdevel because I'm hopping some kind vfs person will check
> > the shift from efivarfs managing its own data to its data being
> > managed as part of the vfs object lifetimes.  The following paragraph
> > should describe all you need to know about the unusual features of the
> > filesystem.
> >
> > efivarfs is a filesystem projecting the current state of the UEFI
> > variable store and allowing updates via write.  Because EFI variables
> > contain both contents and a set of attributes, which can't be mapped
> > to filesystem data, the u32 attribute is prepended to the output of
> > the file and, since UEFI variables can't be empty, this makes every
> > file at least 5 characters long.  EFI variables can be removed either
> > by doing an unlink (easy) or by doing a conventional write update that
> > reduces the content to zero size, which means any write update can
> > potentially remove the file.
> >
> > Currently efivarfs has two bugs: it leaks memory and if a create is
> > attempted that results in an error in the write, it creates a zero
> > length file remnant that doesn't represent an EFI variable (i.e. the
> > state reflection of the EFI variable store goes out of sync).
> >
> > The code uses inode->i_private to point to additionaly allocated
> > information but tries to maintain a global list of the shadowed
> > varibles for internal tracking.  Forgetting to kfree() entries in this
> > list when they are deleted is the source of the memory leak.
> >
> > I've tried to make the patches as easily reviewable by non-EFI people
> > as possible, so some possible cleanups (like consolidating or removing
> > the efi lock handling and possibly removing the additional entry
> > allocation entirely in favour of simply converting the dentry name to
> > the variable name and guid) are left for later.
> >
> > The first patch removes some unused fields in the entry; patches 2-3
> > eliminate the list search for duplication (some EFI variable stores
> > have buggy iterators) and replaces it with a dcache lookup.  Patch 4
> > move responsibility for freeing the entry data to inode eviction which
> > both fixes the memory leak and also means we no longer need to iterate
> > over the variable list and free its entries in kill_sb.  Since the
> > variable list is now unused, patch 5 removes it and its helper
> > functions.
> >
> > Patch 6 fixes the second bug by introducing a file_operations->release
> > method that checks to see if the inode size is zero when the file is
> > closed and removes it if it is.  Since all files must be at least 5 in
> > length we use a zero i_size as an indicator that either the variable
> > was removed on write or that it wasn't correctly created in the first
> > place.
> >
> > v2: folded in feedback from Al Viro: check errors on lookup and delete
> >     zero length file on last close
> >
> > James
> >
> > ---
> >
> > James Bottomley (6):
> >   efivarfs: remove unused efi_varaible.Attributes and .kobj
> >   efivarfs: add helper to convert from UC16 name and GUID to utf8 name
> >   efivarfs: make variable_is_present use dcache lookup
> >   efivarfs: move freeing of variable entry into evict_inode
> >   efivarfs: remove unused efivarfs_list
> >   efivarfs: fix error on write to new variable leaving remnants
> >
>
> Thanks James,
>
> I've tentatively queued up this series, as well as the hibernate one,
> to get some coverage from the robots while I run some tests myself.
>

For the record,

Tested-by: Ard Biesheuvel <ardb@kernel.org>

including the hibernation pieces. It looks pretty to me solid to me.

I'd add a Reviewed-by: as well if I wasn't so clueless about VFS
stuff, so I'll gladly take one from the audience.

Thanks again, James - this is a really nice cleanup.

Re: [PATCH v2 0/6] convert efivarfs to manage object data correctly

[James Bottomley]

On Thu, 2025-01-09 at 10:50 +0100, Ard Biesheuvel wrote:
> On Tue, 7 Jan 2025 at 03:36, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
[...]
> > James Bottomley (6):
> >   efivarfs: remove unused efi_varaible.Attributes and .kobj
> >   efivarfs: add helper to convert from UC16 name and GUID to utf8
> > name
> >   efivarfs: make variable_is_present use dcache lookup
> >   efivarfs: move freeing of variable entry into evict_inode
> >   efivarfs: remove unused efivarfs_list
> >   efivarfs: fix error on write to new variable leaving remnants
> > 
> 
> Thanks James,
> 
> I've tentatively queued up this series, as well as the hibernate one,
> to get some coverage from the robots while I run some tests myself.
> 
> Are there any existing test suites that cover efivarfs that you could
> recommend?

I'm afraid I couldn't find any.  I finally wrote a few shell scripts to
try out multiple threads updating the same variable.  I think I can
probably work out how to add these to the kselftest infrastructure. 
Hibernation was a real pain because it doesn't work with secure boot,
but I finally wrote a UEFI shell script to modify variables and reset.
Unfortunately I don't think we have a testing framework I can add these
to.

Regards,

James

Re: [PATCH v2 0/6] convert efivarfs to manage object data correctly

[Ard Biesheuvel]

On Thu, 9 Jan 2025 at 16:50, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Thu, 2025-01-09 at 10:50 +0100, Ard Biesheuvel wrote:
> > On Tue, 7 Jan 2025 at 03:36, James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> [...]
> > > James Bottomley (6):
> > >   efivarfs: remove unused efi_varaible.Attributes and .kobj
> > >   efivarfs: add helper to convert from UC16 name and GUID to utf8
> > > name
> > >   efivarfs: make variable_is_present use dcache lookup
> > >   efivarfs: move freeing of variable entry into evict_inode
> > >   efivarfs: remove unused efivarfs_list
> > >   efivarfs: fix error on write to new variable leaving remnants
> > >
> >
> > Thanks James,
> >
> > I've tentatively queued up this series, as well as the hibernate one,
> > to get some coverage from the robots while I run some tests myself.
> >
> > Are there any existing test suites that cover efivarfs that you could
> > recommend?
>
> I'm afraid I couldn't find any.  I finally wrote a few shell scripts to
> try out multiple threads updating the same variable.  I think I can
> probably work out how to add these to the kselftest infrastructure.
> Hibernation was a real pain because it doesn't work with secure boot,
> but I finally wrote a UEFI shell script to modify variables and reset.
> Unfortunately I don't think we have a testing framework I can add these
> to.
>

I tested the hibernation changes using QEMU/arm64, using a
non-persistent varstore image, and checked whether adding and/or
deleting variables via efivarfs resulted in the expected behavior
after a resume from hibernate.

I also checked the behavior regarding zero sized files, and that looks
sound to me. But I didn't go as far as torture test it with concurrent
updates as you have.

All in all, I think it is reasonable to queue this up so it gets some
wider exposure.

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

[Al Viro]

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.

Re: [PATCH v2 5/6] efivarfs: remove unused efivarfs_list

[Al Viro]

On Mon, Jan 06, 2025 at 06:35:24PM -0800, James Bottomley wrote:
> Remove all function helpers and mentions of the efivarfs_list now that
> all consumers of the list have been removed and entry management goes
> exclusively through the inode.

BTW, do we need efivarfs_callback() separation from efivar_init()?
As minimum, what's the point of callback argument?

Re: [PATCH v2 6/6] efivarfs: fix error on write to new variable leaving remnants

[Al Viro]

On Mon, Jan 06, 2025 at 06:35:25PM -0800, James Bottomley wrote:

> +	inode_lock(inode);
> +	if (d_unhashed(file->f_path.dentry)) {
> +		/*
> +		 * file got removed; don't allow a set.  Caused by an
> +		 * unsuccessful create or successful delete write
> +		 * racing with us.
> +		 */
> +		bytes = -EIO;
> +		goto out;
> +	}

Wouldn't the check for zero ->i_size work here?  Would be easier to
follow...

Re: [PATCH v2 6/6] efivarfs: fix error on write to new variable leaving remnants

[James Bottomley]

On Thu, 2025-01-16 at 18:45 +0000, Al Viro wrote:
> On Mon, Jan 06, 2025 at 06:35:25PM -0800, James Bottomley wrote:
> 
> > +       inode_lock(inode);
> > +       if (d_unhashed(file->f_path.dentry)) {
> > +               /*
> > +                * file got removed; don't allow a set.  Caused by
> > an
> > +                * unsuccessful create or successful delete write
> > +                * racing with us.
> > +                */
> > +               bytes = -EIO;
> > +               goto out;
> > +       }
> 
> Wouldn't the check for zero ->i_size work here?  Would be easier to
> follow...

Unfortunately not.  The pathway for creating a variable involves a call
to efivarfs_create() (create inode op) first, which would in itself
create a zero length file, then a call to efivarfs_file_write(), so if
we key here on zero length we'd never be able to create new variables.

The idea behind the check is that delete could race with write and if
so, we can't resurrect the variable once it's been unhashed from the
directory, so we need to error out at that point.

Regards,

James

Re: [PATCH v2 5/6] efivarfs: remove unused efivarfs_list

[James Bottomley]

On Thu, 2025-01-16 at 18:42 +0000, Al Viro wrote:
> On Mon, Jan 06, 2025 at 06:35:24PM -0800, James Bottomley wrote:
> > Remove all function helpers and mentions of the efivarfs_list now
> > that all consumers of the list have been removed and entry
> > management goes exclusively through the inode.
> 
> BTW, do we need efivarfs_callback() separation from efivar_init()?
> As minimum, what's the point of callback argument?

This one's simply historical reasons.  The original code had the
callback so for ease of reviewing it seemed easier to keep it.

Regards,

James

Re: [PATCH v2 6/6] efivarfs: fix error on write to new variable leaving remnants

[Al Viro]

On Thu, Jan 16, 2025 at 01:54:44PM -0500, James Bottomley wrote:
> On Thu, 2025-01-16 at 18:45 +0000, Al Viro wrote:
> > On Mon, Jan 06, 2025 at 06:35:25PM -0800, James Bottomley wrote:
> > 
> > > +       inode_lock(inode);
> > > +       if (d_unhashed(file->f_path.dentry)) {
> > > +               /*
> > > +                * file got removed; don't allow a set.  Caused by
> > > an
> > > +                * unsuccessful create or successful delete write
> > > +                * racing with us.
> > > +                */
> > > +               bytes = -EIO;
> > > +               goto out;
> > > +       }
> > 
> > Wouldn't the check for zero ->i_size work here?  Would be easier to
> > follow...
> 
> Unfortunately not.  The pathway for creating a variable involves a call
> to efivarfs_create() (create inode op) first, which would in itself
> create a zero length file, then a call to efivarfs_file_write(), so if
> we key here on zero length we'd never be able to create new variables.
> 
> The idea behind the check is that delete could race with write and if
> so, we can't resurrect the variable once it's been unhashed from the
> directory, so we need to error out at that point.

D'oh...  Point, but it still feels as if you are misplacing the object
state here ;-/

OK, so we have
	* created, open but yet to be written into
	* live
	* removed

Might be better off with explicit state in efivar_entry...

Re: [PATCH v2 6/6] efivarfs: fix error on write to new variable leaving remnants

[James Bottomley]

On Thu, 2025-01-16 at 18:59 +0000, Al Viro wrote:
> On Thu, Jan 16, 2025 at 01:54:44PM -0500, James Bottomley wrote:
> > On Thu, 2025-01-16 at 18:45 +0000, Al Viro wrote:
> > > On Mon, Jan 06, 2025 at 06:35:25PM -0800, James Bottomley wrote:
> > > 
> > > > +       inode_lock(inode);
> > > > +       if (d_unhashed(file->f_path.dentry)) {
> > > > +               /*
> > > > +                * file got removed; don't allow a set.  Caused by an
> > > > +                * unsuccessful create or successful delete write
> > > > +                * racing with us.
> > > > +                */
> > > > +               bytes = -EIO;
> > > > +               goto out;
> > > > +       }
> > > 
> > > Wouldn't the check for zero ->i_size work here?  Would be easier
> > > to follow...
> > 
> > Unfortunately not.  The pathway for creating a variable involves a
> > call to efivarfs_create() (create inode op) first, which would in
> > itself create a zero length file, then a call to
> > efivarfs_file_write(), so if we key here on zero length we'd never
> > be able to create new variables.
> > 
> > The idea behind the check is that delete could race with write and
> > if so, we can't resurrect the variable once it's been unhashed from
> > the directory, so we need to error out at that point.
> 
> D'oh...  Point, but it still feels as if you are misplacing the
> object state here ;-/
> 
> OK, so we have
>         * created, open but yet to be written into
>         * live
>         * removed
> 
> Might be better off with explicit state in efivar_entry...

OK, that would get rid of the race in efivarfs_file_release I'd been
worrying about where we can decide to remove the file under the inode
lock but have to make it unhashed after dropping the inode lock.

Regards,

James

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

[James Bottomley]

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

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

[James Bottomley]

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

Re: [PATCH v2 0/6] convert efivarfs to manage object data correctly

[James Bottomley]

On Thu, 2025-01-09 at 10:50 +0100, Ard Biesheuvel wrote:
> Are there any existing test suites that cover efivarfs that you could
> recommend?

The good news is there is actually an existing test suite.  I was
writing some for selftests/filesystems/efivarfs, but it turns out they
exist in selftests/efivarfs.  You can run them from the kernel source
tree (in a VM with your changes) as:

make -C tools/testing/selftests TARGETS=efivarfs run_tests

So I've merged all the testing I had here and started writing new ones.

The bad news is that writing new tests I've run across another corner
case in the efivarfs code: you can set the inode size to anything you
want (as root) which means you can take a real variable and get it to
mimic an uncommitted one (at least to stat):

# ls -l /sys/firmware/efi/efivars/PK-8be4df61-93ca-11d2-aa0d-00e098032b8c 
-rw-r--r-- 1 root root 841 Jan 18 13:40 /sys/firmware/efi/efivars/PK-8be4df61-93ca-11d2-aa0d-00e098032b8c
# chattr -i /sys/firmware/efi/efivars/PK-8be4df61-93ca-11d2-aa0d-00e098032b8c
# > /sys/firmware/efi/efivars/PK-8be4df61-93ca-11d2-aa0d-00e098032b8c
# ls -l /sys/firmware/efi/efivars/PK-8be4df61-93ca-11d2-aa0d-00e098032b8c
-rw-r--r-- 1 root root 0 Jan 18 13:40 /sys/firmware/efi/efivars/PK-8be4df61-93ca-11d2-aa0d-00e098032b8c

I'm not sure how much of a bug this is for the old code (only systemd
seems to check for zero size files), and it's only in the cache inode,
so if you cat the file you get the fully 841 bytes.  However, obviously
it becomes a huge problem with my new code because you can use the
truncate inode to actually delete the variable file (even thought the
variable is still there) so I need to add a fix for it to my series. 
I'll post it separately when I have it to see what you think.

Regards,

James

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

[Ard Biesheuvel]

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

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

[James Bottomley]

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

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

[Ard Biesheuvel]

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?

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

[James Bottomley]

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