It's been a while.

I will try to check if the patch can handle the case described by Michael below.

   OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);

   ...

   gBS->RestoreTPL (OldTpl);
   gBS->RestoreTPL (OldTpl);

or

   OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL - 1);
   gBS->RaiseTPL (TPL_HIGH_LEVEL);

   ..

   gBS->RestoreTPL (OldTpl);


Michael, more cases in your mind?

Thanks,
Ray

________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, March 5, 2024 12:19
To: Michael Brown <mcb30@ipxe.org>; Paolo Bonzini <pbonzini@redhat.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Laszlo Ersek <lersek@redhat.com>
Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

Michael,
do you have any updated patch?

Thanks,
Ray
________________________________
From: Michael Brown <mcb30@ipxe.org>
Sent: Friday, March 1, 2024 19:10
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io <devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Laszlo Ersek <lersek@redhat.com>
Subject: Re: [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

On 01/03/2024 09:33, Paolo Bonzini wrote:
> On Fri, Mar 1, 2024 at 10:27 AM Michael Brown <mcb30@ipxe.org> wrote:
>> It's possible that it doesn't matter.  The new logic will effectively
>> mean that RestoreTPL() will restore not only the TPL but also the
>> interrupts-enabled state to whatever existed at the time of the
>> corresponding RaiseTPL().
>
> Right: that's what my comment says
>
> +  // However, when the handler calls RestoreTPL
> +  // before returning, we want to keep interrupts disabled.  This
> +  // restores the exact state at the beginning of the handler,
> +  // before the call to RaiseTPL(): low TPL and interrupts disabled.
>
> but indeed it applies beyond interrupt handlers. It might even be a bugfix.

Right.  I'm leaning towards treating this as a bugfix: essentially
tightening up the semantics of RestoreTPL() to mean:

- any callbacks in the range OldTpl < Tpl < gEfiCurrentTpl will be
dispatched with interrupts unconditionally enabled

- the TPL will be restored to OldTpl

- the interrupt state will be restored to the value it had when the TPL
was last raised from OldTpl

It feels as though this should be able to be cleanly modelled with a
single global state array

   BOOLEAN mSavedInterruptState[TPL_HIGH_LEVEL]

(or possibly a bitmask, though using the array avoids having to disable
interrupts just to write a value).

I still need to think through the subtleties, to make sure it could cope
with pathological edge cases such as

   OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);

   ...

   gBS->RestoreTPL (OldTpl);
   gBS->RestoreTPL (OldTpl);

or

   OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL - 1);
   gBS->RaiseTPL (TPL_HIGH_LEVEL);

   ..

   gBS->RestoreTPL (OldTpl);

I think that at least one of the above pathological usage patterns would
break the existing mInterruptedTplMask patches, since they currently
clear state in RestoreTPL() and so will not correctly handle a duplicate
call to RestoreTPL().

I'll try to get a patch put together over the weekend.

Thanks,

Michael




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119602): https://edk2.groups.io/g/devel/message/119602
Mute This Topic: https://groups.io/mt/104642317/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-