Am 16.05.2021 um 03:17 schrieb Laszlo Ersek:
On 05/14/21 17:44, Marvin Häuser wrote:
On 14.05.21 17:23, Lendacky, Thomas wrote:
On 5/14/21 10:04 AM, Marvin Häuser wrote:
+ // Check to be sure that the "allocate below" behavior hasn't
changed.
+ // This will also catch a failed allocation, as "-1" is
returned on
+ // failure.
+ //
+ if (CpuMpData->SevEsAPResetStackStart >=
CpuMpData->WakeupBuffer) {
+ DEBUG ((DEBUG_ERROR,
+ "SEV-ES AP reset stack is not below wakeup buffer\n"));
+
+ ASSERT (FALSE);
Should the ASSERT not only catch the broken "allocate below" behaviour,
i.e. not trigger on failed allocation?
I think it's best to trigger on a failed allocation as well rather than
continuing and allowing a page fault or some other problem to occur.
Well, it should handle the error in a safe way, i.e. the deadloop below.
To not ASSERT on plausible conditions is a common design guideline in
most low-level projects including Linux kernel.
Best regards,
Marvin
Thanks,
Tom
+ CpuDeadLoop ();
"DEBUG + ASSERT(FALSE) + CpuDeadLoop()" is a pattern in edk2.
In RELEASE builds, it will lead to a CpuDeadLoop(). That's the main goal
-- don't continue execution if the condition controlling the whole block
fired.
In DEBUG and NOOPT builds, the pattern will lead to a debug message
(usually at the "error" level), followed by an assertion failure. The
error message of the assertion failure is irrelevant ("FALSE"). The
point of adding ASSERT ahead of CpuDeadLoop() is that the way ASSERT
hangs execution is customizable, via "PcdDebugPropertyMask", unlike
CpuDeadLoop(). In many cases, ASSERT() uses CpuDeadLoop() itself, so the
effect is the same -- the explicit CpuDeadLoop is not reached. In other
configs, ASSERT() can raise a debug exception (CpuBreakpoint()).
I absolutely do not *expect* Tom to change this, it was just a slight
remark (as many places have this anyway). I'll still try to explain why
I made that remark, but for whom it is of no interest, I do not expect
it to be read. I'm fine with the patch as-is myself. Thank you a lot, Tom!
I know it, unfortunately, is a pattern in EDK II - taking this pattern
too far is what caused the 8-revision patch regarding untrusted inputs
we submitted previously. :)
There are many concerns about unconventional ASSERTs, though I must
admit none but one (and that one barely) really apply here, which is why
I have trouble explaining why I believe it should be changed. Here are
some reasons outside the context of this patch:
1) Consistency between DEBUG and RELEASE builds: I think one can justify
to have a breakpoint on a condition that may realistically occur. But a
deadloop can give a wrong impression about how production code works.
E.g. it also is a common pattern in EDK II to ASSERT on memory
allocation failure but *not* have a proper check after, so DEBUG builds
will nicely error or deadloop, while RELEASE goes ahead and causes a CPU
exception or memory corruption depending on the context. Thus,
real-world error handling cannot really be tested. This does not apply
because there *is* a RELEASE deadloop.
2) Static analysis: Some static analysers use ASSERT information for
their own analysis, and try to give hints about unsafe or unreachable
code based on own annotations. This kind of applies, but only when
substituting EDK II ASSERT with properly recognisable ASSERTs (e.g.
__builtin_unreachable).
2) Dynamic analysis: ASSERTs can be useful when fuzzing for example.
Enabled Sanitizers will only catch unsafe behaviour, but maybe you have
some extra code in place to sanity-check the results further. An ASSERT
yields an error dump (usually followed by the worker dying). However, as
allocation failures are perfectly expected, this can cause a dramatic
about of False Positives and testing interruption. This does not apply
because deadloop'd code cannot really be fuzz-tested anyway.
ASSERTs really are designed as unbreakable conditions, i.e. 1)
preconditions 2) invariants 3) postconditions. No allocator in early
kernel-space or lower can really guarantee allocation success, thus it
cannot be a postcondition of any such function. And while it might make
debugging look a bit easier (but you will see from the backtrace anyway
where you halted), it messes with all tools that assume proper usage.
Also, I just realised, you can of course see it from the address value
when debugging, but you cannot see it from the ASSERT or DEBUG message
*which* of the two logical error conditions failed (i.e. broken
allocator or OOM). Changing the ASSERT would fix that. :)
Best regards,
Marvin
The required part of the pattern is CpuDeadLoop(); the DEBUG message
makes it more debugging-friendly, and the ASSERT(), with the tweakable
"hang method", makes it even more debugging-friendly.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75157): https://edk2.groups.io/g/devel/message/75157
Mute This Topic: https://groups.io/mt/82757192/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
