| Message ID | 20250107023525.11466-7-James.Bottomley@HansenPartnership.com |
|---|---|
| State | New |
| Headers | show |
| Series | convert efivarfs to manage object data correctly | expand |
On Mon, Jan 06, 2025 at 06:35:25PM -0800, James Bottomley wrote: > + 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; > + } Wouldn't the check for zero ->i_size work here? Would be easier to follow...
On Thu, 2025-01-16 at 18:59 +0000, Al Viro wrote: > On Thu, Jan 16, 2025 at 01:54:44PM -0500, James Bottomley wrote: > > On Thu, 2025-01-16 at 18:45 +0000, Al Viro wrote: > > > On Mon, Jan 06, 2025 at 06:35:25PM -0800, James Bottomley wrote: > > > > > > > + 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; > > > > + } > > > > > > Wouldn't the check for zero ->i_size work here? Would be easier > > > to follow... > > > > Unfortunately not. The pathway for creating a variable involves a > > call to efivarfs_create() (create inode op) first, which would in > > itself create a zero length file, then a call to > > efivarfs_file_write(), so if we key here on zero length we'd never > > be able to create new variables. > > > > The idea behind the check is that delete could race with write and > > if so, we can't resurrect the variable once it's been unhashed from > > the directory, so we need to error out at that point. > > D'oh... Point, but it still feels as if you are misplacing the > object state here ;-/ > > OK, so we have > * created, open but yet to be written into > * live > * removed > > Might be better off with explicit state in efivar_entry... OK, that would get rid of the race in efivarfs_file_release I'd been worrying about where we can decide to remove the file under the inode lock but have to make it unhashed after dropping the inode lock. Regards, James
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c index 23c51d62f902..cf0179d84bc5 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,37 @@ 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) +{ + 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, - .read = efivarfs_file_read, - .write = efivarfs_file_write, + .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 *),
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. In the case of multiple racing opens and closes, the open is counted to ensure that the zero size check is done on the last close. 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 last 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 zero size variable that doesn't exist in the EFI table. Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> --- v2: implement counter for last close --- fs/efivarfs/file.c | 60 +++++++++++++++++++++++++++++++++++------- fs/efivarfs/internal.h | 1 + 2 files changed, 52 insertions(+), 9 deletions(-)