From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 458CAAC11A6 for ; Fri, 1 Mar 2024 12:09:43 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=eqAmdpdT2WRpYfWVKC+8HBeq0ANVMFKLCbCnrGJ4fe0=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1709294981; v=1; b=ax/41dkMaZpIYD9aIkEgldTbq+s4JrV88ipwUEB6j/yZNxWBpCv4ih2jIAEKG7LkOQ0T9Uf1 RJmm3or0FEztFIsrkTiGQqMMMUGQmC3o3HEZHM1dEJRnCjxLOuMcerB9gQbqNQMMxClpyqJ+ij1 0BMYlWYovN6+52QiyV7WHgWU= X-Received: by 127.0.0.2 with SMTP id NP4hYY7687511xhiCyGRLey2; Fri, 01 Mar 2024 04:09:41 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.21065.1709294981305372900 for ; Fri, 01 Mar 2024 04:09:41 -0800 X-Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-199-bcjJFlkJMYSpfOfB0SEC1A-1; Fri, 01 Mar 2024 07:09:36 -0500 X-MC-Unique: bcjJFlkJMYSpfOfB0SEC1A-1 X-Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-4128defb707so8581245e9.0 for ; Fri, 01 Mar 2024 04:09:36 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCUaQmj584qsmFF6Rvsp8KZksG08HaGL68w3uGea/9I3Qg4fUcJSiRFM1VJuMOG3HOgXZyTvO29KnsPt1bjlbvwvexKgcA== X-Gm-Message-State: Mu1rW6Eh9O9RcWWSOLE926QTx7686176AA= X-Received: by 2002:adf:eb09:0:b0:33e:1c91:2346 with SMTP id s9-20020adfeb09000000b0033e1c912346mr1385815wrn.31.1709294975677; Fri, 01 Mar 2024 04:09:35 -0800 (PST) X-Google-Smtp-Source: AGHT+IEbp7PzzGrmKiQWl/k7olmE3zEg1cSZo/tqjQkBLpjbHNFUWBqUQCjx9vFGe6mmqpWFYJ6f6oZoflEnqYMeBpY= X-Received: by 2002:adf:eb09:0:b0:33e:1c91:2346 with SMTP id s9-20020adfeb09000000b0033e1c912346mr1385801wrn.31.1709294975384; Fri, 01 Mar 2024 04:09:35 -0800 (PST) MIME-Version: 1.0 References: <20240229130246.3-1-ray.ni@intel.com> <20240229130246.3-3-ray.ni@intel.com> <0102018df956cc94-e49702ac-a077-4b44-923c-3f33a7142f10-000000@eu-west-1.amazonses.com> <0102018df9b5541d-c855d474-9fcf-47af-bd0f-8ea913c984e3-000000@eu-west-1.amazonses.com> In-Reply-To: <0102018df9b5541d-c855d474-9fcf-47af-bd0f-8ea913c984e3-000000@eu-west-1.amazonses.com> From: "Paolo Bonzini" Date: Fri, 1 Mar 2024 13:09:21 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts To: Michael Brown Cc: "Ni, Ray" , edk2-devel-groups-io , "Kinney, Michael D" , Liming Gao , Laszlo Ersek X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,pbonzini@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="000000000000d3143c0612983f3d" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="ax/41dkM"; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --000000000000d3143c0612983f3d Content-Type: text/plain; charset="UTF-8" Il ven 1 mar 2024, 12:10 Michael Brown ha scritto: > It feels as though this should be able to be cleanly modelled with a > single global state array > > BOOLEAN mSavedInterruptState[TPL_HIGH_LEVEL] > Pretty much, yes. But you only have to write it when the interrupts are already disabled, so the bitmask works and makes it easier to clear "all values at NewTpl and above" with just an AND. > > (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 think my patch works (modulo the 1 vs. 1U issue) for the second. Declaring the first to be invalid is a good idea IMO. Also it would only break in interrupt handlers and would revert to just causing a stack overflow in the interrupt storm scenario, so it wouldn't be too bad... Paolo > 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 (#116238): https://edk2.groups.io/g/devel/message/116238 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] -=-=-=-=-=-=-=-=-=-=-=- --000000000000d3143c0612983f3d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


Il ven 1 mar 2024, 12:10 Michael Brown <mcb30@ipxe.org> ha scritto:
It feels as though this should be able to be cleanly= modelled with a
single global state array

=C2=A0 =C2=A0BOOLEAN mSavedInterruptState[TPL_HIGH_LEVEL]
<= /div>

Pretty much, yes. = But you only have to write it when the interrupts are already disabled, so = the bitmask works and makes it easier to clear "all values at NewTpl a= nd above" with just an AND.

(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

=C2=A0 =C2=A0OldTpl =3D gBS->RaiseTPL (TPL_HIGH_LEVEL);

=C2=A0 =C2=A0...

=C2=A0 =C2=A0gBS->RestoreTPL (OldTpl);
=C2=A0 =C2=A0gBS->RestoreTPL (OldTpl);

or

=C2=A0 =C2=A0OldTpl =3D gBS->RaiseTPL (TPL_HIGH_LEVEL - 1);
=C2=A0 =C2=A0gBS->RaiseTPL (TPL_HIGH_LEVEL);

=C2=A0 =C2=A0..

=C2=A0 =C2=A0gBS->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 think my patch works (modulo the 1 vs. 1U issue) for= the second. Declaring the first to be invalid is a good idea IMO. Also it = would only break in interrupt handlers and would revert to just causing a s= tack overflow in the interrupt storm scenario, so it wouldn't be too ba= d...

Paolo


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

Thanks,

Michael

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#116238) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--000000000000d3143c0612983f3d--