efivars: deal with uncommitted efi variables #35497
Conversation
|
Sorry, but let's not tape over the problem in userspace. This is caused by kernel not flushing the efivar entry list/dentries after resuming from hibernation, and hence should really be something for kernel people to fix. |
|
But it's not only related to hibernate. Previous |
No. Again, the existence of "uncommitted vars" already deems the inconsistency in efivarfs. Note that in kernel, |
|
Thanks for your explanation. Is there any work going on in linux kernel? I would like to track it. |
|
cc @poettering Could you bring this up to the kernel efivarfs people? |
FYI @ardbiesheuvel |
|
I think we should handle this case gracefully. I'll reopen the pull request for additional discussion. |
|
I will update the PR in a few days. |
|
can someone explain to me what an "uncommited efi var" is? |
Excellent question, I'd like to know as well. |
As explained above, I think it's utterly just inconsistency in efivarfs, where the stored metadata diverges from what's actually in firmware, in this specific case reading from non-existent var. |
1c000ac to
53c3859
Compare
53c3859 to
cd1a8cb
Compare
src/basic/efivars.c
Outdated
| @@ -96,6 +96,23 @@ int efi_get_variable( | |||
| (void) usleep_safe(EFI_RETRY_DELAY); | |||
| } | |||
|
|
|||
| /* According to comment of linux source code, efivarfs represents uncommitted | |||
There was a problem hiding this comment.
we should not reference the concept of "uncommited" variables in a comment here, if noone knows what that precisely is supposed to be.
Maybe somebody can ping the person who added the comment about this to the kernel and ask for clarification what that is supposed to be?
There was a problem hiding this comment.
I'll wait for the clarification to update the comment.
| */ | ||
| if (n == 0) | ||
| return log_debug_errno(SYNTHETIC_ERRNO(ENOENT), | ||
| "EFI variable %s is uncommitted", p); |
There was a problem hiding this comment.
here too, that's not a useful debug message if noone knows what "uncommitted" is supposed to mean in this context
There was a problem hiding this comment.
I'll wait for the clarification to update the debug message.
|
was this issue every reported to evifvarfs maintainers (whoever that might be?). if that's public on some ML I'd love it to have linked here. if not, can someone report this please? |
I'm one of the co-maintainers but Linux VFS stuff is not my forte. I'm not aware of any reports, although James Bottomley started looking into efivarfs issues very recently. https://lore.kernel.org/linux-efi/[email protected]/ |
Unfortunately kernel reports EOF if there's an inconsistency between efivarfs var list and what's actually stored in firmware, c.f. systemd#34304. Hence we translate EOF back to ENOENT here, as with kernel behavior before torvalds/linux@3fab70c If the kernel changes behaviour (to flush dentries on resume), we can drop this at some point in the future. But note that the commit is 11 years old at this point so we'll need to deal with the current behaviour for a long time. Fix systemd#34304.
cd1a8cb to
1b8cf8b
Compare
|
Hey @jejb, you as @ardbiesheuvel pointed out you have been fixing bugs in efivarfs code recently. Do you have any idea what "uncommitted" efivars are supposed to be, as per torvalds/linux@3fab70c? noone appears to know. it appears efivarfs is simply broken in hibernation cases, i.e. appears the list is not updated after resume, and contains remains of efi vars that existed on the old boot, without being refreshed to show the ones from the current boot. |
Hi! This is specified in https://uefi.org/sites/default/files/resources/UEFI_Spec_2_7.pdf section 8.2 Variable Services. The parameter But efivarfs did allow zero-length files to be created (I haven't verified if this is still the case), e.g. with The old behavior that produced |
|
@xlz ah, interesting! thanks so much for reporting back, much appreciated! |
|
@poettering Do I need to remove the comments related to "uncommited efi variables"? |
|
@wrvsrx please update the comment along the lines of @xlz just wrote, i.e. that a zero size env var is not allowed in efi and hence the variable doesn#t really exist in the backing store as long as it is zero sized, and the kernel calls this "uncommited". Oh, and I don't think we should return ENOENT for this, given that the env var does exist in efivarfs, even though it doesn't exist in the efi backing store. maybe return ENOMEDIUM or so in this case? |
|
On Wed, 2024-12-18 at 00:57 -0800, Lennart Poettering wrote:
Hey @jejb, you as @ardbiesheuvel pointed out you have been fixing
bugs in efivarfs code recently. Do you have any idea what
"uncommitted" efivars are supposed to be, as per
torvalds/linux@3fab70c
?
noone appears to know.
It may be to do with some mechanism that got removed.
What I do know is that efivarfs does the commit to RT services in the
write routine, so it doesn't cache variable changes. Now it is true
that UEFI itself does to avoid NVRAM wear, but that shouldn't produce
any artifact visible to the kernel. The spec actually says a read
cannot produce a zero length variable.
it appears efivarfs is simply broken in hibernation cases, i.e.
appears the list is not updated after resume, and contains remains of
efi vars that existed on the old boot, without being refreshed to
show the ones from the current boot.
There was a bug in efivarfs that I'm proposing to fix here:
***@***.***/
The semantic gap between how efi variables work and how a filesystem
works (in a filesystem, creation and write are two separate operations,
but they're only a single one in UEFI variables) meant that if you
created a variable but failed to set it in the write (i.e. the UEFI
layer rejected the variable creation with an error) we'd keep a zero
length file around. This state doesn't represent "uncommitted"
variables, though, it represents non-existent ones because the attempt
to create them already failed and there won't be another from efivarfs.
The way efivarfs represents files (4 byte attribute preceding contents)
means there can never be zero length files; they always have to have a
length of at least 5 since the UEFI spec also forbids zero length
variables. In the not impossible case that a buggy UEFI implementation
were to allow zero length variables, we'd still represent them with a
length of 4 because they'd still have attributes.
|
|
so handling this case here cleanly certainly (as proposed with the ENOMEDIUM thing) makes sense, but I am still wondering the relationship here to #34304. Do I get this right: because the kernel when coming back from resume doesn't rescan efi vars we end up in this situation where efivarfs exposes inodes for which no backing variables actually exist? The kernel code now ends up handling this the same way as "empty" efi vars, which also exist as inodes but not in the backing store? so what is the strategy now in this case? should we try to remove the hibernation efi var in this case? will it work and delete the inode without being annoyed about the fact that efi doesn't actually know about the thing? |
|
Then again, we fstat() the file above and check the size to ensure it is indeed an existing variable, yet when actually trying to read it a bogus 0-sized file is returned. So, can someone report the original issue about re-enumerating efivar list after resume? |
|
On Wed, 2024-12-18 at 05:02 -0800, Lennart Poettering wrote:
Do I get this right: because the kernel when coming back from resume
doesn't rescan efi vars we end up in this situation where efivarfs
exposes indoes for which no backing variables actually exist? The
kernel code now ends up handling this the same way as "empty" efi
vars, which also exist as inodes but not in the backing store?
There's no pm hooks in the efivarfs code that would catch this, so it
would definitely come back to the state on suspend without checking to
see if any variable updates have happened in the interim, yes.
Probably what it should be doing is deleting all the dentries on
suspend and re-running the populate code on resume.
|
Not really. As mentioend, the explicit fstat() and st_size check should have caught the "normal" uncommited vars. This is something else - inconsistency between efivarfs and actual firmware state. Effectively the var doesn't exist, and the caller needs to handle this in the exact same way as ENOENT (the kernel before torvalds/linux@3fab70c even returned ENOENT!). Any other errno would render the PR useless, as the caller gets unexpected errno and bails. |
Fix #34304.
According to linux source code, "efivarfs represents uncommitted variables with zero-length files. Reading them should return EOF.". However,
st_sizeof the efi fd can be larger than 0. This commit deal with such case.