Why noinline?

How does this disable witness?

You might add some comment explaining why we don't take the whole segment.

It was an attempt to optimize for size instead of speed in run-once startup code.

However, now looking at the actual generated amd64 code, the size savings is only 16 bytes. Both clang and gcc eliminate the third inline call entirely, but both generate the third call for the __noinline case.

I'll remove the __noinline to simplify logic.

It might be nice if generally we had a convention to tag startup/init code. Just for fun I also tried tagging witness_startup_count with __attribute__(__cold__). Results below for size of witness_startup_count + witness_startup_calc.

It doesn't... Setting witness_watch = 0 (or -1) would also disable checks that don't rely on these allocations, and might also be confusing as that's a separate user-controlled tunable. I was only attempting to provide a way to survive the allocation failure.

How about I replace "disabling" with "minimizing startup allocations"?

Sure.

Yes, with caveat. The compiler puts them in a separate section, .text.unlikely, and then our kernel linker script collects them all in the .text section. I'm unsure if this has the effect of grouping all the .text.unlikely code in the end result.

I'm okay either way, will follow your preference between "disabling" and "startup allocations minimized". I agree it's functionally disabled even if certain sleep-ok checks work, hence the initial verbiage. Did you mean to add a code comment about the pedantic part but leave the user message as "disabled"?

I'd somewhat expect so.

Right, I think a code comment would make this a bit clearer, but IMO it's ok as-is too.

Pedantically, all values are provided by user and all calculations could overflow.

In bootverbose mode, I would print the witness parameters somewhere so they appear in the dmesg. Perhaps it should be done right after the witness_startup_count().

Yes, that's true (and what I meant by the second sentence of the commit log). I saw the scope here as just addressing too-big configurations like the one asomers reported, but otherwise the current patch doesn't protect against all values that would cause overflows. I thought about providing more robust protection, but wasn't sure it was worth the complexity. I'm guessing there are a lot of tunables that have similar weaknesses.

Would you all like to see the tunables be fully robust? I don't really object to going in that direction. I might suggest we put in an intermediate step and provide some infrastructure like TUNABLE_*_FETCH_LIMIT macros to clamp or reject values beyond specified limits.

To address the robustness in a more focused way, what I thought of doing was defining

#define WITNESS_TUNE_MAX (1ul << (ULONG_WIDTH / 2 - 1)

And enforcing that on the tunables, and that limit should prevent overflow with the current math (divide bits by two because of the square).

Like at the top of witness_startup()? I can add that.

That was my reaction to the claim of 'prevent overflow'. Also I do not think that we need to go that far, which is why I said that my comment is pedantic. Might be, a comment somewhere that we do not care about user causing the arithmetic to fail is enough, only about user taking too much memory.

Perhaps yes. I am thinking of debugging cases when user get early panic and only can provide dmesg, since userspace cannot be reached.