Re: [PATCH v2 6/6] efi/runtime-wrappers: retire the worker if a wedged call ever returns

[Breno Leitao <leitao@debian.org>]

Hello Ard,

On Fri, Jun 12, 2026 at 01:11:11PM +0200, Ard Biesheuvel wrote:
> On Fri, 12 Jun 2026, at 13:01, Breno Leitao wrote:
> > +	/*
> > +	 * If __efi_queue_work() timed out and disabled runtime services, the
> > +	 * caller is gone and efi_rts_work is no longer ours: park the worker
> > +	 * so it never signals the stale completion or runs again.
> > +	 */
> > +	if (!efi_enabled(EFI_RUNTIME_SERVICES))
> > +		efi_rts_park_worker();
> > +
> 
> So one thing to note here is that the arm64 version of the exception recovery
> (in efi_runtime_fixup_exception()) will also clear EFI_RUNTIME_SERVICES, and
> that will occur before this check. This means we will park the worker not only
> on a time out, but also on an sync exception in the firmware.

Correct. Just to clarify the code flow for a faulty EFI:

	efi_call_rts  (EFI faults)
	-> __do_kernel_fault()
	->  efi_runtime_fixup_exception() == true   (clears EFI_RUNTIME_SERVICES, rewrites regs)
	-> __efi_rt_asm_recover
	-> caller of __efi_rt_asm_wrapper ->
	-> back into efi_call_rts(), with status = EFI_ABORTED (and EFI_RUNTIME_SERVICES unset).

Previous it was completing (complete(&efi_rts_work.efi_rts_comp);) and
disabling the EFI_RUNTIME_SERVICES

With this change it is parking the workthread with falut as well (with
EFI_RUNTIME_SERVICES disabled).

> I suspect this is actually what we want, but it deserves to be called out.

I think so.

It would not be hard to park on timeout, probably adding a new variable to
track "tiemout operations":


  if (!wait_for_completion_timeout(&efi_rts_work.efi_rts_comp, EFI_RTS_TIMEOUT)) {
      pr_err("EFI runtime service %d wedged in firmware; disabling EFI runtime services\n", id);
+     WRITE_ONCE(efi_rts_work.timeout, true); 
      clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
      return EFI_ABORT
  }

Then Worker tail tests the timeout field, not the bit:

  if (READ_ONCE(efi_rts_work.field))
      efi_rts_park_worker();

But, from my newbie view, you want to park in both cases.

Thanks for the heads-up,
--breno