Re: [PATCH 1/2] efi/runtime-wrappers: bound the wait for EFI runtime service calls

[Ard Biesheuvel]

On Thu, 11 Jun 2026, at 12:21, Ard Biesheuvel wrote:
> 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 <leitao@xxxxxxxxxx>
>> ---
>>  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.

Also, could we prevent the kthread that runs the workqueue from being scheduled
again if we decide that the runtime services are wedged? x86 has some logic for
this when a page fault occurs, and I wonder if there is a generic way to do
something similar.