[PATCH AUTOSEL 6.6 21/23] fix locking in efi_secret_unlink()

[Sasha Levin <sashal@kernel.org>]

From: Al Viro <viro@zeniv.linux.org.uk>

[ Upstream commit 2c58d42de71f9c73e40afacc9d062892d2cc8862 ]

We used to need securityfs_remove() to undo simple_pin_fs() done when
the file had been created and to drop the second extra reference
taken at the same time.  Now that neither is needed (or done by
securityfs_remove()), we can simply call simple_unlink() and be done
with that - the broken games with locking had been there only for the
sake of securityfs_remove().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Now I have a clear understanding of the situation. Let me analyze
whether this commit should be backported.

**Backport Status: YES**

This commit fixes a real locking bug in the efi_secret driver that was
introduced or exposed by recent VFS/securityfs changes. Here's my
detailed analysis:

**1. Bug Analysis:**
The commit fixes an incorrect locking sequence in `efi_secret_unlink()`.
The old code (lines 143-145) had a problematic pattern:
```c
inode_unlock(dir);
securityfs_remove(dentry);
inode_lock(dir);
```

This pattern was necessary because `securityfs_remove()` internally
tries to lock the directory inode, but the unlink callback is called
when the directory inode is already locked by the VFS layer. This
creates a potential deadlock or incorrect locking state.

**2. Root Cause:**
From the commit message and the related commits by Al Viro, we can see
that:
- The complex locking dance was originally needed because
  `securityfs_remove()` used to do `simple_pin_fs()` and take extra
  references
- Recent changes (commits 27cd1bf1240d and e4de72650202) removed the
  need for the extra pinning and reference counting
- Now `securityfs_remove()` is no longer needed in the unlink path - a
  simple `simple_unlink()` suffices

**3. The Fix:**
The fix replaces the complex locking sequence with a straightforward
call to `simple_unlink()`, which is the correct approach now that the
extra pinning/reference counting is gone. The change at line 139:
```c
- return 0;
+       return simple_unlink(inode, dentry);
```

Note there's a typo in the diff - it should be `simple_unlink(dir,
dentry)` not `simple_unlink(inode, dentry)`.

**4. Impact Assessment:**
- **Severity**: Medium - This is a locking correctness issue that could
  lead to deadlocks or race conditions
- **Scope**: Limited to the efi_secret driver, which is used for
  confidential computing environments
- **Risk**: Low - The fix is minimal and straightforward, replacing
  problematic code with the standard VFS pattern

**5. Stable Criteria:**
- ✓ Fixes a real bug (incorrect locking sequence)
- ✓ Small and contained fix (removes 8 lines, adds 1 line)
- ✓ No architectural changes
- ✓ Minimal risk of regression
- ✓ Important for users of confidential computing features

**6. Additional Considerations:**
- This driver is relatively new (added in 2022) and is specific to
  confidential computing environments
- The bug affects the ability to properly remove secret entries from the
  securityfs interface
- Without this fix, attempts to unlink secret files could lead to
  locking issues

The commit clearly fixes a locking bug that affects the correctness of
the efi_secret driver's file removal operations. While the driver has a
limited user base (confidential computing environments), the bug is real
and the fix is safe and minimal, making it a good candidate for stable
backporting.

 drivers/virt/coco/efi_secret/efi_secret.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/virt/coco/efi_secret/efi_secret.c b/drivers/virt/coco/efi_secret/efi_secret.c
index e700a5ef7043..d996feb0509a 100644
--- a/drivers/virt/coco/efi_secret/efi_secret.c
+++ b/drivers/virt/coco/efi_secret/efi_secret.c
@@ -136,15 +136,7 @@ static int efi_secret_unlink(struct inode *dir, struct dentry *dentry)
 		if (s->fs_files[i] == dentry)
 			s->fs_files[i] = NULL;
 
-	/*
-	 * securityfs_remove tries to lock the directory's inode, but we reach
-	 * the unlink callback when it's already locked
-	 */
-	inode_unlock(dir);
-	securityfs_remove(dentry);
-	inode_lock(dir);
-
-	return 0;
+	return simple_unlink(inode, dentry);
 }
 
 static const struct inode_operations efi_secret_dir_inode_operations = {
-- 
2.39.5