Fix KSTACK_PAGES issue

[wma_semihalf.com]

Summary

If KSTACK_PAGES was changed to anything alse than the default,
the value from param.h was taken instead in some places and
the value from KENRCONF in some others. This resulted in
inconsistency which caused corruption in SMP envorinment.

Ensure all places where KSTACK_PAGES are used the opt_kstack_pages.h
is included.
The issue with wrong stack size is very hard to debug, so author
decided to remove default to unhide any potential code errors.
From now, each time one use KSTACK_PAGES the proper file must
be included as well or the compile will generate an error.
The default was moved to GENERIC config instead.

The file opt_kstack_pages.h could not be included in param.h
because was breaking the toolchain compilation.

[andrew]

The parts of this change to add the missing includes are good, I'm not convinced about the change to GENERIC and params.h.

sys/arm64/include/param.h

98 ↗

(On Diff #6960)

You will need a stronger argument as to why we should do this differently to other architectures.

[wma_semihalf.com]

sys/arm64/include/param.h

98 ↗

(On Diff #6960)

Hm... isn't the argument about error vulnerability good enough? I'm pretty sure that sooner or later, when someone creates new file which uses KSTACK_PAGES he looks where is defined and includes <machine/param.h> forgetting about prior including of "opt_kstack_pages.h" (*). In that case the default value will be used regardless of the one set up in KERNCONF. The compiler won't show even slightest indication of an error and the bug will appear in the most unpredictable place.
I remember how much time took to find this bug, because errors it caused were at the first look completely unrelated to anything. If we really want to have armv8 the tier1 architecture, I'd like to remove this piece of code because is dangerous.

(*) All armv8 machdep files are the perfect example of that situation. The code was copied from armv7 (where I think that KSTACK_PAGES doesn't work either, but no one ever tried to change the default) and the bug propagated.

[andrew]

sys/arm64/include/param.h

98 ↗

(On Diff #6960)

The argument is that your changing it just in arm64 to be different than amd64, arm i386, mips, powerpc, and sparc64, i.e. all the other architectures. You will need to explain why you are changing it in this one place, but not in the 6 other places we define this.

[wma_semihalf.com]

Andrew, I would love to change it in other places as well, but the interaction with so many maintainers will slow down this change and I don't have any mips/powerpc hardware to run tests on.
I have a proposal. Today I will prepare another patch with separate review removing KSTACK_PAGES from param.h in other archs and ask on the fbsd list for some help with testing on more exotic hardware, This allow submitting this to arm+arm64 while waiting for approvals from mips/sparc/x86/etc Will that work for you?

[kib]

sys/arm64/include/param.h

98 ↗

(On Diff #6960)

IMO moving KSTACK_PAGES to GENERIC is not right, since you require now that any kernel config has the statement for KSTACK_PAGES. The knob is not considered something that average user should tune.

The more proper solution is to use kstack_pages variable instead of the preprocessing symbol. This might even allow to set the default kernel stack size using tunable. The tricky place is the td0 stack allocation in locore. E.g., on i386 it looks easy to use kstack_pages for the stack size, but it is not possible to fetch the value from the kernel environment so early.

[kib]

I am fine with the last patch, which adds missing include.

Note that it uncovers more serious issues. E.g., the quick look at the amd64/amd64/stack_machdep.c clearly indicates that the use of KSTACK_PAGES there is bogus, probably curthread->td_kstack_pages is the right thing to use, but it is still not quite correct as well. In fact, the if() should check whether the f_frame is inside the [curthread->td_kstack; curthread->td_kstack + PAGE_SIZE * curthread->td_kstack_pages).

[kib]

In D3094#61493, @kib wrote:

Note that it uncovers more serious issues. E.g., the quick look at the amd64/amd64/stack_machdep.c clearly indicates that the use of KSTACK_PAGES there is bogus, probably curthread->td_kstack_pages is the right thing to use, but it is still not quite correct as well. In fact, the if() should check whether the f_frame is inside the [curthread->td_kstack; curthread->td_kstack + PAGE_SIZE * curthread->td_kstack_pages).

https://reviews.freebsd.org/D3108
should fix the x86 issue, and allow you to drop these files from your change.