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

[PATCH 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.

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     |  31 ++++---
 fs/efivarfs/inode.c    |   5 --
 fs/efivarfs/internal.h |  20 ++---
 fs/efivarfs/super.c    |  59 +++++++-------
 fs/efivarfs/vars.c     | 179 ++++++++++-------------------------------
 5 files changed, 99 insertions(+), 195 deletions(-)

-- 
2.35.3

[PATCH 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 d71d2e08422f..107aad8a3443 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 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 107aad8a3443..4b7330b90958 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 3cc89bb624f0..7a07b767e2cc 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 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>
---
 fs/efivarfs/internal.h |  4 ++++
 fs/efivarfs/super.c    | 20 ++++++++++++++++++++
 fs/efivarfs/vars.c     | 26 ++------------------------
 3 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
index 4b7330b90958..84a36e6fb653 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;
@@ -64,4 +66,6 @@ extern struct inode *efivarfs_get_inode(struct super_block *sb,
 			const struct inode *dir, int mode, dev_t dev,
 			bool is_removable);
 
+
+
 #endif /* EFIVAR_FS_INTERNAL_H */
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index b22441f7f7c6..dc3870ae784b 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -181,6 +181,26 @@ 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)
+		return true;
+
+	qstr.name = name;
+	qstr.len = strlen(name);
+	dentry = d_hash_and_lookup(sb->s_root, &qstr);
+	kfree(name);
+	if (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 7a07b767e2cc..f6380fdbe173 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 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 84a36e6fb653..d768bfa7f12b 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 dc3870ae784b..70b99f58c906 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);
 }
 
@@ -269,13 +275,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,
 };
@@ -398,7 +397,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 f6380fdbe173..bda8e8b869e8 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 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 586446e02ef7..ad4f66e0d09d 100644
--- a/fs/efivarfs/inode.c
+++ b/fs/efivarfs/inode.c
@@ -77,7 +77,6 @@ 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 d768bfa7f12b..e3816ec0e9d8 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 70b99f58c906..c9425a546691 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);
 }
 
@@ -208,8 +205,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;
@@ -253,7 +249,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);
@@ -344,7 +339,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)
@@ -379,8 +374,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;
 
@@ -396,8 +389,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 bda8e8b869e8..4cac01a0e483 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 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.

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 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 variable that doesn't exist in the EFI table.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 fs/efivarfs/file.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 23c51d62f902..edf363f395f5 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -38,22 +38,24 @@ static ssize_t efivarfs_file_write(struct file *file,
 
 	bytes = efivar_entry_set_get_size(var, attributes, &datasize,
 					  data, &set);
-	if (!set && bytes) {
+	if (!set) {
 		if (bytes == -ENOENT)
 			bytes = -EIO;
 		goto out;
 	}
 
+	inode_lock(inode);
 	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);
 	}
+	inode_unlock(inode);
 
 	bytes = count;
 
@@ -106,8 +108,19 @@ 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)
+{
+	if (i_size_read(inode) == 0) {
+		drop_nlink(inode);
+		d_delete(file->f_path.dentry);
+		dput(file->f_path.dentry);
+	}
+	return 0;
+}
+
 const struct file_operations efivarfs_file_operations = {
-	.open	= simple_open,
-	.read	= efivarfs_file_read,
-	.write	= efivarfs_file_write,
+	.open		= simple_open,
+	.read		= efivarfs_file_read,
+	.write		= efivarfs_file_write,
+	.release	= efivarfs_file_release,
 };
-- 
2.35.3

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

[Dionna Amalie Glaze]

On Tue, Dec 10, 2024 at 9:03 AM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:

>  extern const struct file_operations efivarfs_file_operations;
>  extern const struct inode_operations efivarfs_dir_inode_operations;
> @@ -64,4 +66,6 @@ extern struct inode *efivarfs_get_inode(struct super_block *sb,
>                         const struct inode *dir, int mode, dev_t dev,
>                         bool is_removable);
>
> +
> +

Unnecessary
>  #endif /* EFIVAR_FS_INTERNAL_H */
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index b22441f7f7c6..dc3870ae784b 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -181,6 +181,26 @@ 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)
> +               return true;

Why is this true? I understand the previous implementation would have
hit a null dereference trying to calculate strsize1 on null, so this
isn't worse, but if we considered its length to be 0, it would not be
found.

> +
> +       qstr.name = name;
> +       qstr.len = strlen(name);
> +       dentry = d_hash_and_lookup(sb->s_root, &qstr);
> +       kfree(name);
> +       if (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 7a07b767e2cc..f6380fdbe173 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
>
>


-- 
-Dionna Glaze, PhD, CISSP, CCSP (she/her)

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

[James Bottomley]

On Tue, 2024-12-10 at 09:14 -0800, Dionna Amalie Glaze wrote:
> On Tue, Dec 10, 2024 at 9:03 AM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> 
> >  extern const struct file_operations efivarfs_file_operations;
> >  extern const struct inode_operations
> > efivarfs_dir_inode_operations;
> > @@ -64,4 +66,6 @@ extern struct inode *efivarfs_get_inode(struct
> > super_block *sb,
> >                         const struct inode *dir, int mode, dev_t
> > dev,
> >                         bool is_removable);
> > 
> > +
> > +
> 
> Unnecessary

I can remove the extra line.

> >  #endif /* EFIVAR_FS_INTERNAL_H */
> > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> > index b22441f7f7c6..dc3870ae784b 100644
> > --- a/fs/efivarfs/super.c
> > +++ b/fs/efivarfs/super.c
> > @@ -181,6 +181,26 @@ 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)
> > +               return true;
> 
> Why is this true? I understand the previous implementation would have
> hit a null dereference trying to calculate strsize1 on null, so this
> isn't worse, but if we considered its length to be 0, it would not be
> found.

Because for safety on failure we need to assume a collision.  kmalloc
failing will already have dropped an error message so adding another
here (particularly when the log will likely be filling up with these
because we're in a critical memory shortage situation) would seem to be
overkill.  The memory allocation will never fail ordinarily and if it
does the system will be degrading fast, so EFI filesystem variable
collision will be the least of the problem.

Regards,

James

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

[Christian Brauner]

On Tue, Dec 10, 2024 at 12:02:24PM -0500, James Bottomley wrote:
> 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.
> 
> 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 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 variable that doesn't exist in the EFI table.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  fs/efivarfs/file.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
> index 23c51d62f902..edf363f395f5 100644
> --- a/fs/efivarfs/file.c
> +++ b/fs/efivarfs/file.c
> @@ -38,22 +38,24 @@ static ssize_t efivarfs_file_write(struct file *file,
>  
>  	bytes = efivar_entry_set_get_size(var, attributes, &datasize,
>  					  data, &set);
> -	if (!set && bytes) {
> +	if (!set) {
>  		if (bytes == -ENOENT)
>  			bytes = -EIO;
>  		goto out;
>  	}
>  
> +	inode_lock(inode);
>  	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);
>  	}
> +	inode_unlock(inode);
>  
>  	bytes = count;
>  
> @@ -106,8 +108,19 @@ 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)
> +{
> +	if (i_size_read(inode) == 0) {
> +		drop_nlink(inode);
> +		d_delete(file->f_path.dentry);
> +		dput(file->f_path.dentry);
> +	}

Without wider context the dput() looks UAF-y as __fput() will do:

struct dentry *dentry = file->f_path.dentry;
if (file->f_op->release)
        file->f_op->release(inode, file);
dput(dentry);

Is there an extra reference on file->f_path.dentry taken somewhere?

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

[Christian Brauner]

On Tue, Dec 10, 2024 at 12:02:22PM -0500, 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.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---

This looks like a good idea to me.

>  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 84a36e6fb653..d768bfa7f12b 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 dc3870ae784b..70b99f58c906 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);
>  }
>  
> @@ -269,13 +275,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,
>  };
> @@ -398,7 +397,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 f6380fdbe173..bda8e8b869e8 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
> 

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

[James Bottomley]

On Wed, 2024-12-11 at 12:16 +0100, Christian Brauner wrote:
> On Tue, Dec 10, 2024 at 12:02:24PM -0500, James Bottomley wrote:
[...]
> > +static int efivarfs_file_release(struct inode *inode, struct file
> > *file)
> > +{
> > +       if (i_size_read(inode) == 0) {
> > +               drop_nlink(inode);
> > +               d_delete(file->f_path.dentry);
> > +               dput(file->f_path.dentry);
> > +       }
> 
> Without wider context the dput() looks UAF-y as __fput() will do:
> 
> struct dentry *dentry = file->f_path.dentry;
> if (file->f_op->release)
>         file->f_op->release(inode, file);
> dput(dentry);
> 
> Is there an extra reference on file->f_path.dentry taken somewhere?

Heh, well, this is why I cc'd fsdevel to make sure I got all the fs
bits I used to be familiar with, but knowledge of which has atrophied,
correct.

I think it's paired with the extra dget() just after d_instantiate() in
fs/efivarfs/inode.c:efivarfs_create().  The reason being this is a
pseudo-filesystem so all the dentries representing objects have to be
born with a positive reference count to prevent them being reclaimed
under memory pressure.

Regards,

James

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

[James Bottomley]

On Tue, 2024-12-10 at 12:02 -0500, James Bottomley wrote:
> 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 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 variable that doesn't exist in the EFI table.

Systemd doesn't like 0 length files appearing in efivarfs, even if only
momentarily, so I think this needs updating to prevent even momentary
instances of zero length files:

https://github.com/systemd/systemd/issues/34304

These occur for two reasons

   1. The system has hibernated and resumed and the dcache entries are
      now out of sync with the original variables
   2. between the create and a successful write of a variable being
      created in efivarfs

1. can only really be fixed by adding a hibernation hook to the
filesystem code, which would be a separate patch set (which I'll work
on after we get this upstream); but 2. can be fixed by ensuring that
all variables returned from .create aren't visible in the directory
listing until a successful write.

Since we need the file to be visible to lookups but not the directory,
the only two ways of doing this are either to mark the directory in a
way that libfs.c:dcache_readdir() won't see it ... I think this would
have to be marking it as a cursor (we'd remove the cursor mark on
successful write); or to implement our own .iterate_shared function and
hijack the actor to skip newly created files (this is similar to what
overlayfs does to merge directories) which would be identified as
having zero size.

Do the fs people have a preference? The cursor mark is simpler to
implement but depends on internal libfs.c magic. The actor hijack is at
least something that already exists, so would be less prone to breaking
due to internal changes.

Regards,

James

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

[Christian Brauner]

> Do the fs people have a preference? The cursor mark is simpler to
> implement but depends on internal libfs.c magic. The actor hijack is at
> least something that already exists, so would be less prone to breaking
> due to internal changes.

I think making this filesystem specific is better than plumbing this
into dcache_readdir().

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

[James Bottomley]

On Sun, 2024-12-22 at 11:12 +0100, Christian Brauner wrote:
> > Do the fs people have a preference? The cursor mark is simpler to
> > implement but depends on internal libfs.c magic. The actor hijack
> > is at least something that already exists, so would be less prone
> > to breaking due to internal changes.
> 
> I think making this filesystem specific is better than plumbing this
> into dcache_readdir().

Neither would require changes to libfs.c: the dcache_readdir already
does the ignore cursor behaviour; the code in efivarfs would just set
the cursor flag to take advantage of it.

However, on further consideration I think making the zero size entries
not show up in the directory listing doesn't really help anything: they
still have to be found on lookup (otherwise nothing mediates a same
variable create race) which means an open by name from userspace will
still get hold of one.  The good news is that after this change they
should only show up fleetingly instead of hanging around until the next
reboot, so the chances of anyone hitting a problem is much smaller.

Regards,

James

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

[James Bottomley]

On Wed, 2024-12-11 at 07:39 -0500, James Bottomley wrote:
> On Wed, 2024-12-11 at 12:16 +0100, Christian Brauner wrote:
> > On Tue, Dec 10, 2024 at 12:02:24PM -0500, James Bottomley wrote:
> [...]
> > > +static int efivarfs_file_release(struct inode *inode, struct
> > > file
> > > *file)
> > > +{
> > > +       if (i_size_read(inode) == 0) {
> > > +               drop_nlink(inode);
> > > +               d_delete(file->f_path.dentry);
> > > +               dput(file->f_path.dentry);
> > > +       }
> > 
> > Without wider context the dput() looks UAF-y as __fput() will do:
> > 
> > struct dentry *dentry = file->f_path.dentry;
> > if (file->f_op->release)
> >         file->f_op->release(inode, file);
> > dput(dentry);
> > 
> > Is there an extra reference on file->f_path.dentry taken somewhere?
> 
> Heh, well, this is why I cc'd fsdevel to make sure I got all the fs
> bits I used to be familiar with, but knowledge of which has
> atrophied, correct.
> 
> I think it's paired with the extra dget() just after d_instantiate()
> in fs/efivarfs/inode.c:efivarfs_create().  The reason being this is a
> pseudo-filesystem so all the dentries representing objects have to be
> born with a positive reference count to prevent them being reclaimed
> under memory pressure.

Actually on further testing, this did turn out to be a use after free.
Not because of the dput, but because file->release is called for every
closed filehandle, so if you open the file for creation more than once,
both closes will try to delete it and ... oops.

The way I thought of mediating this is to check d_hashed in the file-
>release to see if the file has already been deleted.  That also means
we need a d_hashed() check in write because we can't resurrect the now
deleted file.  And finally something needs to mediate the check and
remove or check and add, so I used the inode semaphore for that.  The
updated patch is below and now passes the concurrency tests.

Regards,

James

------8>8>8><8<8<8-------------
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Subject: [PATCH 6/6] efivarfs: fix error on write to new variable leaving remnants

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.

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 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 variable that doesn't exist in the EFI table.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 fs/efivarfs/file.c | 44 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 23c51d62f902..70a673e7fda3 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,21 @@ 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)
+{
+	inode_lock(inode);
+	if (i_size_read(inode) == 0 && !d_unhashed(file->f_path.dentry)) {
+		drop_nlink(inode);
+		d_delete(file->f_path.dentry);
+		dput(file->f_path.dentry);
+	}
+	inode_unlock(inode);
+	return 0;
+}
+
 const struct file_operations efivarfs_file_operations = {
-	.open	= simple_open,
-	.read	= efivarfs_file_read,
-	.write	= efivarfs_file_write,
+	.open		= simple_open,
+	.read		= efivarfs_file_read,
+	.write		= efivarfs_file_write,
+	.release	= efivarfs_file_release,
 };
-- 
2.35.3

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

[Al Viro]

On Mon, Dec 23, 2024 at 02:52:12PM -0500, James Bottomley wrote:
>  
> +static int efivarfs_file_release(struct inode *inode, struct file *file)
> +{
> +	inode_lock(inode);
> +	if (i_size_read(inode) == 0 && !d_unhashed(file->f_path.dentry)) {
> +		drop_nlink(inode);
> +		d_delete(file->f_path.dentry);
> +		dput(file->f_path.dentry);
> +	}
> +	inode_unlock(inode);
> +	return 0;
> +}

This is wrong; so's existing logics for removal from write().  Think
what happens if you open the sucker, have something bound on top of
it and do that deleting write().

Let me look into that area...

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

[Al Viro]

On Tue, Dec 10, 2024 at 12:02:21PM -0500, James Bottomley wrote:
> 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.

> +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)
> +		return true;
> +
> +	qstr.name = name;
> +	qstr.len = strlen(name);
> +	dentry = d_hash_and_lookup(sb->s_root, &qstr);
> +	kfree(name);
> +	if (dentry)
> +		dput(dentry);

If that ever gets called with efivarfs_valid_name(name, strlen(name))
being false, that's going to oops...

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

[James Bottomley]

On Mon, 2024-12-23 at 20:05 +0000, Al Viro wrote:
> On Mon, Dec 23, 2024 at 02:52:12PM -0500, James Bottomley wrote:
> >  
> > +static int efivarfs_file_release(struct inode *inode, struct file
> > *file)
> > +{
> > +       inode_lock(inode);
> > +       if (i_size_read(inode) == 0 && !d_unhashed(file-
> > >f_path.dentry)) {
> > +               drop_nlink(inode);
> > +               d_delete(file->f_path.dentry);
> > +               dput(file->f_path.dentry);
> > +       }
> > +       inode_unlock(inode);
> > +       return 0;
> > +}
> 
> This is wrong; so's existing logics for removal from write().  Think
> what happens if you open the sucker, have something bound on top of
> it and do that deleting write().

Shouldn't the bind have taken a dentry reference? in which case we'll
just drop the dentry but it won't be the final put, so it will still
hang around.

> Let me look into that area...

Thanks; as you say, delete from write has been around for over a decade
in this filesystem.  We can defer the delete, but it has to happen
somewhere if a write causes an EFI variable to be removed.

Regards,

James

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

[James Bottomley]

On Mon, 2024-12-23 at 20:20 +0000, Al Viro wrote:
> On Tue, Dec 10, 2024 at 12:02:21PM -0500, James Bottomley wrote:
> > 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.
> 
> > +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)
> > +               return true;
> > +
> > +       qstr.name = name;
> > +       qstr.len = strlen(name);
> > +       dentry = d_hash_and_lookup(sb->s_root, &qstr);
> > +       kfree(name);
> > +       if (dentry)
> > +               dput(dentry);
> 
> If that ever gets called with efivarfs_valid_name(name, strlen(name))
> being false, that's going to oops...

Well for the current use case that can't happen because a) that check
is gone from efivarfs_d_hash and b) the name is constructed to pass
this check anyway.  But I'm guessing you mean I should be doing

if (dentry && !IS_ERR(dentry))
   dput(dentry);

?

Regards,

James

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

[James Bottomley]

On Mon, 2024-12-23 at 20:05 +0000, Al Viro wrote:
> On Mon, Dec 23, 2024 at 02:52:12PM -0500, James Bottomley wrote:
> >  
> > +static int efivarfs_file_release(struct inode *inode, struct file
> > *file)
> > +{
> > +       inode_lock(inode);
> > +       if (i_size_read(inode) == 0 && !d_unhashed(file-
> > >f_path.dentry)) {
> > +               drop_nlink(inode);
> > +               d_delete(file->f_path.dentry);
> > +               dput(file->f_path.dentry);
> > +       }
> > +       inode_unlock(inode);
> > +       return 0;
> > +}
> 
> This is wrong; so's existing logics for removal from write().  Think
> what happens if you open the sucker, have something bound on top of
> it and do that deleting write().
> 
> Let me look into that area...

I thought about this some more.  I could see a twisted container use
case where something like this might happen (expose some but not all
efi variables to the container).

So, help me understand the subtleties here.  If it's the target of a
bind mount, that's all OK, because you are allowed to delete the
target.  If something is bind mounted on to an efivarfs object, the
is_local_mountpoint() check in vfs_unlink would usually trip and
prevent deletion (so the subtree doesn't become unreachable).  If I
were to duplicate that, I think the best way would be simply to do a
d_put() in the file->release function and implement drop_nlink() in
d_prune (since last put will always call __dentry_kill)?

Regards,

James

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

[Al Viro]

On Mon, Dec 23, 2024 at 05:56:04PM -0500, James Bottomley wrote:
> > Let me look into that area...
> 
> I thought about this some more.  I could see a twisted container use
> case where something like this might happen (expose some but not all
> efi variables to the container).
> 
> So, help me understand the subtleties here.  If it's the target of a
> bind mount, that's all OK, because you are allowed to delete the
> target.  If something is bind mounted on to an efivarfs object, the
> is_local_mountpoint() check in vfs_unlink would usually trip and
> prevent deletion (so the subtree doesn't become unreachable).  If I
> were to duplicate that, I think the best way would be simply to do a
> d_put() in the file->release function and implement drop_nlink() in
> d_prune (since last put will always call __dentry_kill)?

Refcounting is not an issue.  At all.

Inability to find and evict the mount, OTOH, very much is.  And after your
blind d_delete() that's precisely what will happen.

You are steadily moving towards more and more convoluted crap, in places
where it really does not belong.

If anything, simple_recursive_removal() should be used for that, instead
of trying to open-code bizarre subsets of its functionality...

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

[James Bottomley]

On Mon, 2024-12-23 at 23:12 +0000, Al Viro wrote:
> On Mon, Dec 23, 2024 at 05:56:04PM -0500, James Bottomley wrote:
> > > Let me look into that area...
> > 
> > I thought about this some more.  I could see a twisted container
> > use case where something like this might happen (expose some but
> > not all efi variables to the container).
> > 
> > So, help me understand the subtleties here.  If it's the target of
> > a bind mount, that's all OK, because you are allowed to delete the
> > target.  If something is bind mounted on to an efivarfs object, the
> > is_local_mountpoint() check in vfs_unlink would usually trip and
> > prevent deletion (so the subtree doesn't become unreachable).  If I
> > were to duplicate that, I think the best way would be simply to do
> > a d_put() in the file->release function and implement drop_nlink()
> > in d_prune (since last put will always call __dentry_kill)?
> 
> Refcounting is not an issue.  At all.
> 
> Inability to find and evict the mount, OTOH, very much is.  And after
> your blind d_delete() that's precisely what will happen.
> 
> You are steadily moving towards more and more convoluted crap, in
> places where it really does not belong.
> 
> If anything, simple_recursive_removal() should be used for that,
> instead of trying to open-code bizarre subsets of its
> functionality...

OK, so like the below?

In my defence, simple_recursive_removal() isn't mentioned in
Documentation/filesystems and the function itself also has no
documentation, so even if I had stumbled across it in libfs.c the
recursive in the name would have lead me to believe it wasn't for
single dentry removal.

Regards,

James

---8>8>8><8<8<8---

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Subject: [PATCH] efivarfs: fix error on write to new variable leaving remnants

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.

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 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 variable that doesn't exist in the EFI table.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 fs/efivarfs/file.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 23c51d62f902..0e545c8be173 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,17 @@ 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)
+{
+	if (i_size_read(inode) == 0)
+		simple_recursive_removal(file->f_path.dentry, NULL);
+
+	return 0;
+}
+
 const struct file_operations efivarfs_file_operations = {
-	.open	= simple_open,
-	.read	= efivarfs_file_read,
-	.write	= efivarfs_file_write,
+	.open		= simple_open,
+	.read		= efivarfs_file_read,
+	.write		= efivarfs_file_write,
+	.release	= efivarfs_file_release,
 };
-- 
2.35.3

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

[Al Viro]

On Mon, Dec 23, 2024 at 11:04:58PM -0500, James Bottomley wrote:

> +static int efivarfs_file_release(struct inode *inode, struct file *file)
> +{
> +	if (i_size_read(inode) == 0)
> +		simple_recursive_removal(file->f_path.dentry, NULL);
> +
> +	return 0;
> +}

What happens if you have

	fd = creat(name, 0700);
	fd2 = open(name, O_RDONLY);
	close(fd2);
	write(fd, "barf", 4);

or, better yet, if open()/close() pair happens in an unrelated thread
poking around?

I mean, having that logics in ->release() feels very awkward...

For that matter, what about
	fd = creat(name, 0700);
	fd2 = open(name, O_RDWR);
	close(fd);
	write(fd2, "barf", 4);

I'm not asking about the implementation; what behaviour do you want
to see in userland?

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

[James Bottomley]

On Tue, 2024-12-24 at 04:44 +0000, Al Viro wrote:
> On Mon, Dec 23, 2024 at 11:04:58PM -0500, James Bottomley wrote:
> 
> > +static int efivarfs_file_release(struct inode *inode, struct file
> > *file)
> > +{
> > +       if (i_size_read(inode) == 0)
> > +               simple_recursive_removal(file->f_path.dentry,
> > NULL);
> > +
> > +       return 0;
> > +}
> 
> What happens if you have
> 
>         fd = creat(name, 0700);
>         fd2 = open(name, O_RDONLY);
>         close(fd2);
>         write(fd, "barf", 4);
> 
> or, better yet, if open()/close() pair happens in an unrelated thread
> poking around?

According to my tests you get -EIO to the last write.  I could be glib
and point out that a write of "barf" would return EINVAL anyway, but
assuming it's correctly formatted, you'll get -EIO from the d_unhashed
check before the variable gets created.  If you want to check this
yourself, useful writes that will create a variable are:

echo 0700000054|xxd -r -p > name

And to delete it:

echo 07000000|xxd -r -p > name

You can check your above scenario from a shell with

{ sleep 10; echo 0700000054|xxd -r -p; } > name &
cat name  

> I mean, having that logics in ->release() feels very awkward...
> 
> For that matter, what about
>         fd = creat(name, 0700);
>         fd2 = open(name, O_RDWR);
>         close(fd);
>         write(fd2, "barf", 4);

Same thing: -EIO to last write.

> I'm not asking about the implementation; what behaviour do you want
> to see in userland?

Given the fact that very few things actually manipulate efi variables
and when they do they perform open/write/close in quick succession to
set or remove variables, I think the above behaviour is consistent with
the use and gets rid of the ghost files problem and won't cause any
additional issues.  On the other hand the most intuitive thing would be
to remove zero length files on last close, not first, so if you have a
thought on how to do that easily, I'm all ears.

Regards,

James

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

[James Bottomley]

On Tue, 2024-12-24 at 08:07 -0500, James Bottomley wrote:
[...]

> On the other hand the most intuitive thing would be to remove zero
> length files on last close, not first, so if you have a thought on
> how to do that easily, I'm all ears.

I could do this by adding an open_count to the i_private data struct
efivar_entry and reimplementing simple_open as an efivarfs specific
open that increments this count and decrementing it in ->release(). 
That's still somewhat adding "more convoluted crap", though ...

Regards,

James

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

[James Bottomley]

On Tue, 2024-12-24 at 10:09 -0500, James Bottomley wrote:
> On Tue, 2024-12-24 at 08:07 -0500, James Bottomley wrote:
> [...]
> 
> > On the other hand the most intuitive thing would be to remove zero
> > length files on last close, not first, so if you have a thought on
> > how to do that easily, I'm all ears.
> 
> I could do this by adding an open_count to the i_private data struct
> efivar_entry and reimplementing simple_open as an efivarfs specific
> open that increments this count and decrementing it in ->release(). 
> That's still somewhat adding "more convoluted crap", though ...

There being no other suggestions as to how the vfs might do this; this
is a sketch of the additional code needed to do it within efivarfs.  As
you can see, it's not actually that much.  If this is OK with everyone
I'll fold it in and post a v2.  Since all simple_open really does is
copy i_private to file->private_data, there's really not a lot of
duplication in the attached.

Regards,

James

---

diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 0e545c8be173..cf0179d84bc5 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -121,14 +121,34 @@ static ssize_t efivarfs_file_read(struct file
*file, char __user *userbuf,
 
 static int efivarfs_file_release(struct inode *inode, struct file
*file)
 {
-	if (i_size_read(inode) == 0)
+	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,
+	.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 *),