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@xxxxxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx>
---
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
