Differential D12920
Avoid a double-panic caused by KASSERTs Authored by jtl on Nov 3 2017, 12:15 AM. Tags None Referenced Files
Details When running with INVARIANTS, the kernel contains extra checks. However, these assumptions may not hold true once we've panic'd. Therefore, the checks hold less value after a panic. Additionally, if one of the checks fails while we are already panic'd, this creates a double-panic which can interfere with debugging the original panic. Therefore, this change suppresses a response to KASSERT checks once we've panic'd.
Diff Detail
Event TimelineComment Actions I have doubts that this is useful. Generally, code is not prepared to continue execution after the panic call. I agree with the opinion that a panic inside the panic could be some secondary violation caused by the first issue, but still it means that code is too broken to continue. Did you considered either looping unconditionally, or under some tunable/sysctl control ? Another quirk that would be useful there, I suspect, is to (re-)enter the debugger if present, instead of plain return. Comment Actions I agree that the code is not prepared to continue execution after the panic call. That is one of the reasons for skipping KASSERT checks in these circumstances. Once we have panic'd, the operation of the system has changed so much from the normal operation that it probably doesn't make sense to continue checking assertions.
It isn't just secondary violations (where one failed assertion leads to another failed assertion). The entire operation of the system has changed, and not all assertions are prepared to correctly check conditions both in normal operations and in panics. (And, in fairness, I don't think they should need to be prepared to operate correctly in both modes.) I also don't think a failed KASSERT after the system has already panic'd necessarily means the system is too broken to continue taking a core dump. A second panic() may indicate the system is too broken to continue, and I don't propose changing that. But, KASSERTs are different. We sometimes add KASSERTS for things that "shouldn't happen", but will be fine for the limited operation during a panic. I don't think it makes sense to panic the system a second time because another assertion failed.
The whole reason for this change is that it is useful to us. We regularly see cases where two assertions fail. Once the second assertion fails, the system aborts taking a core dump and we lose our debugging information.
I did consider conditioning this on a tunable/sysctl (which defaulted to ignoring KASSERTs after a panic), just in case someone was truly trying to debug something related to core dumps or panics. I'm willing to add that, if you think it is important/useful. (If the second panic is allowed, the logic would be identical to that used for any other second panic. I believe re-entry to DDB is allowed, if configured through a sysctl; core dumps are not automatically taken.) Comment Actions At least I saw the re-entry several times. I do believe that the control would be not a waste of time to add it. |