Re: [PATCH 1/2] efi/runtime-wrappers: bound the wait for EFI runtime service calls
"Ard Biesheuvel" <[email protected]> Thu, 11 Jun 2026 12:21:42 +0200
| Newsgroups | org.kernel.vger.linux-efi,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
Hi Breno, On Tue, 9 Jun 2026, at 13:55, Breno Leitao wrote: > When an EFI runtime service call hangs in firmware, the kworker on > efi_rts_wq is stuck inside the firmware call and cannot be cancelled. > The kernel currently waits indefinitely on the completion, and the > caller holds efi_runtime_lock for the duration, so every subsequent > EFI runtime caller (efivarfs, NVRAM writes, set_wakeup_time, ACPI PRM > handlers, ...) is wedged until reboot. The only externally visible > symptom is a "workqueue lockup" message and userspace processes piling > up uninterruptibly on the semaphore. > > Replace the indefinite wait_for_completion() in __efi_queue_work() > with wait_for_completion_timeout() bounded by EFI_RTS_TIMEOUT > (120 seconds). On timeout, log the wedged runtime service id and > return EFI_TIMEOUT to the caller. > > The wedged worker is intentionally leaked - it is still inside > firmware and cannot be cancelled - and the shared efi_rts_work is > abandoned to it. EFI runtime services become unavailable until reboot; > the alternative is permanent userspace hang. > > This patch should land together with the follow-up that introduces > the efi_rts_dead fast-fail flag: between the two, a subsequent > __efi_queue_work() caller will re-INIT_WORK() and re-init_completion() > on the work_struct and completion that the leaked worker still owns, > which can corrupt workqueue state and let the next caller observe > the leaked call's status as if it were its own. The split exists for > review clarity; reviewers may prefer to squash. > > Known limitation: the union efi_rts_args that __efi_queue_work() hands > to the worker contains pointers into the caller's stack frame (the > compound literal in efi_queue_work() and the in/out buffers it points > to, e.g. *tm in GetTime). Once the caller returns -EIO and unwinds, > those slots are reusable. If firmware eventually unblocks and writes > to the output buffers after the timeout has fired, the writes land in > whatever now occupies that memory. In practice firmware that hangs for > more than 120 seconds tends to stay hung. A follow-up bouncing args > and output buffers through kmalloc would close this gap. > > At fleet scale this turns a generic "workqueue lockup" or stalled-task > mystery into an unambiguous "EFI firmware is at fault" signal in > dmesg. > > Signed-off-by: Breno Leitao <[email protected]> > --- > drivers/firmware/efi/runtime-wrappers.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/firmware/efi/runtime-wrappers.c > b/drivers/firmware/efi/runtime-wrappers.c > index da8d29621644..6ce6d094066e 100644 > --- a/drivers/firmware/efi/runtime-wrappers.c > +++ b/drivers/firmware/efi/runtime-wrappers.c > @@ -118,6 +118,14 @@ union efi_rts_args { > > struct efi_runtime_work efi_rts_work; > > +/* > + * Upper bound on how long we wait for a single EFI runtime service > + * call to finish before declaring firmware wedged. Chosen to be longer > + * than any plausible legitimate call (including UpdateCapsule on slow > + * SPI-NOR) while still bounding userspace wait time. > + */ > +#define EFI_RTS_TIMEOUT (120 * HZ) > + > /* > * efi_queue_work: Queue EFI runtime service call and wait for completion > * @_rts: EFI runtime service function identifier > @@ -338,10 +346,16 @@ static efi_status_t __efi_queue_work(enum efi_rts_ids id, > * queue_work() returns 0 if work was already on queue, > * _ideally_ this should never happen. > */ > - if (queue_work(efi_rts_wq, &efi_rts_work.work)) > - wait_for_completion(&efi_rts_work.efi_rts_comp); > - else > + if (!queue_work(efi_rts_wq, &efi_rts_work.work)) { > pr_err("Failed to queue work to efi_rts_wq.\n"); > + goto exit; > + } > + > + if (!wait_for_completion_timeout(&efi_rts_work.efi_rts_comp, > + EFI_RTS_TIMEOUT)) { > + pr_err("EFI runtime service %d wedged in firmware\n", id); > + return EFI_TIMEOUT; Could we just clear the EFI_RUNTIME_SERVICES bit here right away? That way, we probably won't need the second patch (unless I'm mistaken). It /should/ also block the calls that are not routed via the workqueue, e.g., any EFI pstore calls to the non-blocking SetVariable() variant, but I just noticed that we never check EFI_RUNTIME_SERVICES on those code paths, which is probably a bug. And please return EFI_ABORTED rather than EFI_TIMEOUT - probably doesn't matter in practice but I'd like to avoid introducing more EFI return codes in the runtime context that the spec mentions only for boot services stuff.