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 = {