[added correct mailing list for bug report] On Tue, 2025-02-25 at 12:10 +0000, Richard Hughes wrote: > Hi, > > I'm not sure what I'm supposed to do about: > > commit 908af31f4896f2c0645031f8b74a89d3a8beb5b9 > Author: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> > Date: Sun Jan 19 10:12:12 2025 -0500 > > 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. > 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@xxxxxxxxxxxxxxxxxxxxx> > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx> > > It causes a regression in fwupd -- seen in > https://github.com/fwupd/fwupd/issues/8495 and > https://bugzilla.redhat.com/show_bug.cgi?id=2346831 so far -- and it > seems broken for anyone (including me) updating to 6.14. OK, so the problem with this as a bug report is that it doesn't explain what you're doing. However > I can work around the behavior in > https://github.com/fwupd/fwupd/pull/8500 ;(which is also the arguably > correct thing to do) but it's going to cause a panic as I have to get > an updated fwupd out on all distros so we'll need releases for > multiple branches. Reading the code in the fix, it looks like you were creating a file in EFI (which is naturally zero length), then closing it (because glib gio specifically has an API for this), then clearing the immutable bit and then writing to it to actually create a variable? However, none of that dance is at all required. A newly created file naturally allows writing on the file descriptor you used to create it. It's only if you open it again that the entry becomes immutable. So your update has the correct logic: if file exists clear immutable and write otherwise add O_CREAT. > I don't mind fixing fwupd (as it's doing a dumb thing) but could we > revert the kernel change for 6.14 and give everyone a few weeks to > update userspace? Thanks. The change is rather embedded in a set of other fixes now. If we wanted a temporary and quickly removable work around for the current kernel, I think using i_size to signal whether the file is newly created and not written (0) or failed a write (1) and only removing the file if it failed a write might be a simple two line fix. That way we still keep the benefit of cleanup on a failed write while not impacting your pattern. Can you confirm this has that effect? Regards, James --- diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c index cb1b6d0c3454..c294a8fc566d 100644 --- a/fs/efivarfs/file.c +++ b/fs/efivarfs/file.c @@ -57,10 +57,11 @@ static ssize_t efivarfs_file_write(struct file *file, if (bytes == -ENOENT) { /* - * zero size signals to release that the write deleted - * the variable + * FIXME: temporary workaround for fwupdate, signal + * failed write with a 1 to keep created but not + * written files */ - i_size_write(inode, 0); + i_size_write(inode, 1); } else { i_size_write(inode, datasize + sizeof(attributes)); inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode)); @@ -124,7 +125,8 @@ static int efivarfs_file_release(struct inode *inode, struct file *file) struct efivar_entry *var = inode->i_private; inode_lock(inode); - var->removed = (--var->open_count == 0 && i_size_read(inode) == 0); + /* FIXME: temporary work around for fwupdate */ + var->removed = (--var->open_count == 0 && i_size_read(inode) == 1); inode_unlock(inode); if (var->removed)
