Revert r243980 and r244105

[cem]

Summary

This is needed to teach Coverity about KASSERT; panic() is __dead2,
which should let Coverity learn about KASSERT assumptions. (Obviously,
kassert_warn was a huge violation of these assumptions.)

A follow-up patch may add a KWARN framework that accomplishes something
like the old kassert_warn, log, ddb, knobs did. Callsites must
explicitly opt in to KWARN behavior rather than KASSERT.

[jhb]

The name is a misnomer now.

I would perhaps go ahead and add KWARN() instead renaming the kassert optional bits to be kwarn tweaks instead (and default to not panic'ing for warnings).

I think it would be best if that patch became a strawman to post to arch@ get some consensus before pushing it into the tree.

One thing we could do is change the various WITNESS "warnings" like LOR reports and WITNESS_WARN to use KWARN and obsolete the witness knobs to control entering ddb on a warning by instead having the KWARN knobs for that take their place.

kern/subr_witness.c

188 ↗

(On Diff #15641)

Best to just revert these all back to panic() I think. IIRC, I didn't think that any of these panic's before were WARN worthy. Things like WITNESS_WARN and LORs are warnings, but not the explicit panic()'s in here.

[cem]

In D6117#130281, @jhb wrote:

I would perhaps go ahead and add KWARN() instead renaming the kassert optional bits to be kwarn tweaks instead (and default to not panic'ing for warnings).

I think it would be best if that patch became a strawman to post to arch@ get some consensus before pushing it into the tree.

I was afraid you'd say that :-).

One thing we could do is change the various WITNESS "warnings" like LOR reports and WITNESS_WARN to use KWARN and obsolete the witness knobs to control entering ddb on a warning by instead having the KWARN knobs for that take their place.

Do you have any other ideas for KWARN sites? Should I keep the old "kassert" sysctls for controlling KWARNs or do you think anybody aside from iX ever used them?

I'm having trouble imagining how the KWARN stuff could even be used meaningfully without being able to specify *which* warns get a particular behavior. But it would be pretty easy to make something as functional as the kassert warn stuff that exists today (i.e., not very).

[cem]

Revert kassert_panics in subr_witness to ordinary panic (revert r244105 too).

[jhb]

Perhaps for simplicity preserve all of the existing kassert knobs (renamed to kwarn). It probably makes it less work in the patch to just do a rename instead of trying to pick a subset. With witness having a global "drop into ddb or not" and "panic or not" knob was sufficient for me previously. Global knobs like that for kwarn are probably fine for now.

[cem]

Follow-up adding KWARN facility: https://reviews.freebsd.org/D6134

[kan]

Glad to see this stuff getting kicked out of the 'normal' KASSERT path.

[alfred]

During my time at iXsystems we used this facility several times to get a log from a customer site with a number of "kasserts".

The reason we did this was multiple reasons:

1. We needed to ship a kernel with asserts enabled.
2. When we did, the first assert hit was: a) In an unrelated module and not relevant. b) Not enough information came back from just the first assert.
3. We found it more useful to get multiple errors back from a customer in one trip rather than one fix at a time. Unfortunately one fix at a time would have had us lose the customer.

The KASSERT/assert system is very, very, very useful. However if you are at a last resort sending a debug kernel (with Kassert enabled) and do not get enough information back then you will lose that customer.

I understand that a few vocal folks are upset, like seriously, seriously upset, however at the time this was the only way we could effectively debug a customer problem and my hope was that others could make use of it as well.

Linux has had a similar functionality for many years. In Linux there is the kernel "oops()" which does nearly the same thing.

Initially I mocked Linux's "oops" for being silly and "wrong", using the exact same reasons that many have used to dislike "kassert_warn". However once I was responsible for an extremely pissed off customer who was paying us quite a sum of money AND I was not getting enough information back, I changed my mind.

https://en.wikipedia.org/wiki/Linux_kernel_oops

-Alfred

[cem]

Ok. Here's another proposal:

Add a mode between INVARIANTS off and INVARIANTS on. Call it INVARIANTS_OPTIONAL (or _NETFLIX, I don't care what color it is).

• In !INVARIANTS mode, you don't have KASSERTs at all (like today).

• In INVARIANTS && INVARIANTS_OPTIONAL mode, you get the all the print-and-continue KASSERT knobs you have today. (So, same default of panicing, but optionally they can be disabled and turned into logs.)

• In INVARIANTS && !INVARIANTS_OPTIONAL mode, KASSERT always panics.

I would suggest GENERIC move from the current mode, effectively INVARIANTS_OPTIONAL, to !INVARIANTS_OPTIONAL. But if you want to ship a kernel with pass-through assertions enabled, you can still do that by enabling INVARIANTS_OPTIONAL.

Does that meet your needs well enough?

P.S., if you're reliably hitting a bogus assertion, next time please remove that assert for everyone else that uses FreeBSD. Thanks.

[alfred]

In D6117#134261, @cem wrote:

Ok. Here's another proposal:

Add a mode between INVARIANTS off and INVARIANTS on. Call it INVARIANTS_OPTIONAL (or _NETFLIX, I don't care what color it is).

• In !INVARIANTS mode, you don't have KASSERTs at all (like today).

• In INVARIANTS && INVARIANTS_OPTIONAL mode, you get the all the print-and-continue KASSERT knobs you have today. (So, same default of panicing, but optionally they can be disabled and turned into logs.)

• In INVARIANTS && !INVARIANTS_OPTIONAL mode, KASSERT always panics.

I would suggest GENERIC move from the current mode, effectively INVARIANTS_OPTIONAL, to !INVARIANTS_OPTIONAL. But if you want to ship a kernel with pass-through assertions enabled, you can still do that by enabling INVARIANTS_OPTIONAL.

Does that meet your needs well enough?

That sounds reasonable. Thank you.

P.S., if you're reliably hitting a bogus assertion, next time please remove that assert for everyone else that uses FreeBSD. Thanks.

Well about that... one time it was in the cdrom driver and we had no one with deep enough knowledge of cdrom driver on the team to fix it or to know it should be removed. Basically we had no clue and not enough time to grab enough clue. Another few times we weren't hitting a false positive, but rather we were hitting multiple asserts and it was advantageous to get all of them as opposed to only the first.

thank you.

[emaste]

Add a mode between INVARIANTS off and INVARIANTS on. Call it INVARIANTS_OPTIONAL (or _NETFLIX, I don't care what color it is).

I like this proposal.

At the risk of feeding into a bikeshed, I think INVARIANTS_OPTIONAL is unclear: it's not the invariant that's optional, but the panic. INVARIANTS_WARN?

I would suggest GENERIC move from the current mode, effectively INVARIANTS_OPTIONAL, to !INVARIANTS_OPTIONAL.

I agree with this too.

[cem]

In D6117#134395, @emaste wrote:

Add a mode between INVARIANTS off and INVARIANTS on. Call it INVARIANTS_OPTIONAL (or _NETFLIX, I don't care what color it is).

At the risk of feeding into a bikeshed, I think INVARIANTS_OPTIONAL is unclear: it's not the invariant that's optional, but the panic. INVARIANTS_WARN?

The way I saw it, by choosing to proceed when the invariant is violated, regardless of what gets logged/printed, you're making the invariant "optional."

Also, WARN doesn't describe the full behavior of making the panic optional. I'm not proposing that this new mode *only* warn. It would keep the existing behavior of defaulting KASSERT to panic, with an off switch.

Maybe INVARIANTS_PANIC_OPTIONAL?

[emaste]

In D6117#134400, @cem wrote:

Maybe INVARIANTS_PANIC_OPTIONAL?

That sounds good to me, although I'd rather something shorter (but don't have a good suggestion).