Implement random modulus with kern.randompid=1

[marieheleneka_gmail.com]

Summary

Background

kern.randompid introduces some randomization in which PID is chosen for the next process, instead of just bumping by 1.
Values:
kern.randompid=(value less than 2) is currently no-op and sets kern.randompid to 0.
If set to a value of ( 2 <= kern.randompid < 100), it's set to 100.
If set to a value of (100 <= kern.randompid <= (PID_MAX - 100)), it will be used as modulus when kern_fork.c tries to allocate new PID. Formula is lastpid += arc4random() % kern.randompid;
If set to a value of (kern.randompid > PID_MAX-100), it's set to PID_MAX-100.

(Should this be documented somewhere else? I'll happily do so, just tell me where ;) )

This patch changes

When kern.randompid is set to 1, it will no longer be auto-corrected to 0 but to a random value between 100 and 999 (inclusive). This should help make this feature slightly more randomized, as admin doesn't have to pick a (hopefully) random number on their own.. which also tends to be a set-and-forget value.

SUGGESTED COMMIT MESSAGE

Setting the sysctl kern.randompid to 1 is no longer a no-op, but rather sets it to a random value between 100 and 1123 inclusive.

Submitted by: Marie Helene Kvello-Aune
relnotes: yes

Test Plan

1: Apply patch
2: Compile, install and boot kernel
3: run "sysctl kern.randompid=1" as root

Expected result: Value should be set to a random number between 100 and 1123 inclusive

Hopefully won't observe the implosion of the universe.

[marieheleneka_gmail.com]

Want feedback on: Is the number range 100-999 sane?

[imp]

It would be better to use a less predictable method of getting the next PID if you want random PIDs...

[marieheleneka_gmail.com]

This patch doesn't implement PID randomization; that's already in. This patch just makes the sysctl value "1" generate a random modulus, instead of leaving the feature disabled.

[des]

sys/kern/kern_fork.c

217–218

This is redundant.

[des]

BTW, 100 is a reasonable lower bound, but perhaps pid_max - 100 would be a better upper bound than 999, since it is already the maximum for an explicitly selected value.

[marieheleneka_gmail.com]

In D12238#254056, @des wrote:

BTW, 100 is a reasonable lower bound, but perhaps pid_max - 100 would be a better upper bound than 999, since it is already the maximum for an explicitly selected value.

I'm a bit concerned about the existing comment about the value:

Using a modulus that is too big causes a LOT more process table scans and slows down fork processing as the pidchecked caching is defeated.

Although 999 might be on the low end, I don't think we should risk triggering this by making the upper bound too high.

[marieheleneka_gmail.com]

sys/kern/kern_fork.c

217–218

Good catch. Fixed, will update patch in a few.

[des]

sys/kern/kern_fork.c

215

I'm not sure I understand this; arc4random() returns a uint32_t, so both the abs() and the cast are unnecessary.

BTW, it is generally better to use a cross-product than modulo, since the latter gives a small bias, but then you have to cast to a wider type so the multiplication won't overflow:

pid = (uint64_t)arc4random() * (999 - 100 + 1) / UINT32_MAX + 100;

[des]

I think I was unclear. The abs() in your code is only required because the cast to int can result in a negative value. If you drop the cast, you can also drop the abs() and all will be fine. The stuff I wrote about a cross-product is optional :)

[marieheleneka_gmail.com]

Implement random modulus with kern.randompid=1
Fixed some incorrect assumptions (casting+abs()) and using cross-cast instead of modulus for more randomized results.

[marieheleneka_gmail.com]

Fixed bug introduced in previous revision: setting kern.randompid=0 sets it to 100
Restructured if-then-else tree to make slightly more sense while here. (feedback from des@)

[marieheleneka_gmail.com]

Since the upper bound of modulus was picked somewhat randomly, change it so the math is simpler. (suggested by des@)

[des]

Committed in rS323390 which was tagged with the wrong differential number.

[marieheleneka_gmail.com]

Was committed in rS323390. (This comment is only to close the revision)