Re: [PATCH v2 22/50] convert efivarfs

[Al Viro <viro@zeniv.linux.org.uk>]

On Tue, Oct 28, 2025 at 05:45:40PM +0000, Al Viro wrote:

> FWIW, having a special path for "we are in foofs_fill_super(), fuck
> the locking - nobody's going to access it anyway" is not a great
> idea, simply because the helpers tend to get reused on codepaths
> where we can't cut corners that way.

	BTW, looking through efivarfs codebase now... *both* callers
of efivarfs_create_dentry() end up doing dcache lookups, with variously
convoluted call chains.  Look: efivarfs_check_missing() has an explicit
try_lookup_noperm() before the call of efivarfs_create_dentry().
efivarfs_callback() doesn't, but it's called via
	efivar_init(efivarfs_callback, sb, true)
and with the last argument being true efivar_init() will precede the call
of the callback with efivarfs_variable_is_present().  Guess what does that
thing (never used anywhere else) do?  Right, the call of try_lookup_noperm().

Why do we bother with that?  What's wrong with having efivarfs_create_dentry()
returning -EEXIST in case of dentry already being there and turning the
chunk in efivar_init() into
			err = func(variable_name, vendor_guid,
				   variable_name_size, data);
			if (err == -EEXIST) {
				if (duplicate_check)
					dup_variable_bug(variable_name,
							 &vendor_guid,
							 variable_name_size);
				else
					err = 0;
			}
			if (err)
				status = EFI_NOT_FOUND;
Note that both possible callbacks become almost identical and I wouldn't
be surprised if that "almost" is actually "completely"...  <checks> yep.

So I'm not sure we want that callback to be an argument, but that's
a separate followup.  For now, do you see any problems with the following
patch?  [Completely untested, on top of the posted series]

diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
index f913b6824289..045d53fd0f3c 100644
--- a/fs/efivarfs/internal.h
+++ b/fs/efivarfs/internal.h
@@ -55,8 +55,6 @@ 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 298ab3c929eb..80ed81bbd4a5 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -189,52 +189,6 @@ static const struct dentry_operations efivarfs_d_ops = {
 	.d_hash = efivarfs_d_hash,
 };
 
-static struct dentry *efivarfs_alloc_dentry(struct dentry *parent, char *name)
-{
-	struct dentry *d;
-	struct qstr q;
-	int err;
-
-	q.name = name;
-	q.len = strlen(name);
-
-	err = efivarfs_d_hash(parent, &q);
-	if (err)
-		return ERR_PTR(err);
-
-	d = d_alloc(parent, &q);
-	if (d)
-		return d;
-
-	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;
-
-	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;
-
-	dentry = try_lookup_noperm(&QSTR(name), sb->s_root);
-	kfree(name);
-	if (!IS_ERR_OR_NULL(dentry))
-		dput(dentry);
-
-	return dentry != NULL;
-}
-
 static int efivarfs_create_dentry(struct super_block *sb, efi_char16_t *name16,
 				  unsigned long name_size, efi_guid_t vendor,
 				  char *name)
@@ -244,7 +198,7 @@ static int efivarfs_create_dentry(struct super_block *sb, efi_char16_t *name16,
 	struct dentry *dentry, *root = sb->s_root;
 	unsigned long size = 0;
 	int len;
-	int err = -ENOMEM;
+	int err = 0;
 	bool is_removable = false;
 
 	/* length of the variable name itself: remove GUID and separator */
@@ -253,41 +207,36 @@ static int efivarfs_create_dentry(struct super_block *sb, efi_char16_t *name16,
 	if (efivar_variable_is_removable(vendor, name, len))
 		is_removable = true;
 
+	dentry = simple_start_creating(root, name);
+	if (IS_ERR(dentry)) {
+		err = PTR_ERR(dentry);
+		goto out_name;
+	}
+
 	inode = efivarfs_get_inode(sb, d_inode(root), S_IFREG | 0644, 0,
 				   is_removable);
-	if (!inode)
-		goto fail_name;
+	if (unlikely(!inode)) {
+		err = -ENOMEM;
+		goto out_dentry;
+	}
 
 	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);
-		goto fail_inode;
-	}
-
 	__efivar_entry_get(entry, NULL, &size, NULL);
 
-	/* copied by the above to local storage in the dentry. */
-	kfree(name);
-
 	inode_lock(inode);
 	inode->i_private = entry;
 	i_size_write(inode, size + sizeof(__u32)); /* attributes + data */
 	inode_unlock(inode);
 	d_make_persistent(dentry, inode);
-	dput(dentry);
-
-	return 0;
 
-fail_inode:
-	iput(inode);
-fail_name:
+out_dentry:
+	simple_done_creating(dentry);
+out_name:
 	kfree(name);
-
 	return err;
 }
 
@@ -407,42 +356,6 @@ static const struct fs_context_operations efivarfs_context_ops = {
 	.free		= efivarfs_free,
 };
 
-static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t vendor,
-				  unsigned long name_size, void *data)
-{
-	char *name;
-	struct super_block *sb = data;
-	struct dentry *dentry;
-	int err;
-
-	if (guid_equal(&vendor, &LINUX_EFI_RANDOM_SEED_TABLE_GUID))
-		return 0;
-
-	name = efivar_get_utf8name(name16, &vendor);
-	if (!name)
-		return -ENOMEM;
-
-	dentry = try_lookup_noperm(&QSTR(name), sb->s_root);
-	if (IS_ERR(dentry)) {
-		err = PTR_ERR(dentry);
-		goto out;
-	}
-
-	if (!dentry) {
-		/* found missing entry */
-		pr_info("efivarfs: creating variable %s\n", name);
-		return efivarfs_create_dentry(sb, name16, name_size, vendor, name);
-	}
-
-	dput(dentry);
-	err = 0;
-
- out:
-	kfree(name);
-
-	return err;
-}
-
 static struct file_system_type efivarfs_type;
 
 static int efivarfs_freeze_fs(struct super_block *sb)
@@ -493,7 +406,7 @@ static int efivarfs_unfreeze_fs(struct super_block *sb)
 		}
 	}
 
-	efivar_init(efivarfs_check_missing, sb, false);
+	efivar_init(efivarfs_callback, sb, false);
 	pr_info("efivarfs: finished resyncing variable state\n");
 	return 0;
 }
diff --git a/fs/efivarfs/vars.c b/fs/efivarfs/vars.c
index 6edc10958ecf..d893e928891a 100644
--- a/fs/efivarfs/vars.c
+++ b/fs/efivarfs/vars.c
@@ -407,6 +407,8 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		case EFI_SUCCESS:
 			variable_name_size = var_name_strnsize(variable_name,
 							       variable_name_size);
+			err = func(variable_name, vendor_guid,
+				   variable_name_size, data);
 
 			/*
 			 * Some firmware implementations return the
@@ -416,18 +418,16 @@ 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 (duplicate_check &&
-			    efivarfs_variable_is_present(variable_name,
-							 &vendor_guid, data)) {
-				dup_variable_bug(variable_name, &vendor_guid,
-						 variable_name_size);
-				status = EFI_NOT_FOUND;
-			} else {
-				err = func(variable_name, vendor_guid,
-					   variable_name_size, data);
-				if (err)
-					status = EFI_NOT_FOUND;
+			if (err == -EEXIST) {
+				if (duplicate_check)
+					dup_variable_bug(variable_name,
+							 &vendor_guid,
+							 variable_name_size);
+				else
+					err = 0;
 			}
+			if (err)
+				status = EFI_NOT_FOUND;
 			break;
 		case EFI_UNSUPPORTED:
 			err = -EOPNOTSUPP;