Don't acquire sysctlmemlock unnecessarily

[pkelsey]

Summary

If req.oldptr is NULL, there is no need to acquire
sysctlmemlock, as there is no user buffer that can be wired down.
Calling sysctl(3) or sysctlbyname(3) with a NULL oldp and a non-NULL
oldlenp is a common idiom, used to obtain the length of a
variable-length sysctl entry before retrieving the value.

Note that currently when the above idiom is used, *oldlenp is
typically an uninitialized value in the caller's context and thus
sysctlmemlock is acquired essentially at random.

[kib]

This is probably fine.

But, from the description of the sysctlmemlock, it seems that the lock must be owned whenever sysctl_wire_old_buffer() is called, am I right ? If yes, I think an assertion that the lock is owned, should be added to the wire_old_buffer().

[pkelsey]

In D2987#59000, @kib wrote:

This is probably fine.

But, from the description of the sysctlmemlock, it seems that the lock must be owned whenever sysctl_wire_old_buffer() is called, am I right ? If yes, I think an assertion that the lock is owned, should be added to the wire_old_buffer().

sysctl_wire_old_buffer() is called unconditionally by the handlers that use it, but sysctlmemlock is only acquired if servicing a userland request, and the length of the old value buffer is > PAGE_SIZE (and with this patch, also only if the old value buffer pointer is not NULL), so such an assert in sysctl_wire_old_buffer() would fail incorrectly when those conditions were not present, something that occurs in normal operation. The sysctlmemlock ownership assert would have to be conditioned on the same criteria as the acquisition of sysctlmemlock.

I'm not sure it's appropriate to add this assert, even if properly conditioned. sysctlmemlock is not required for correctness - it exists to limit the number of pages that can be concurrently wired down for userland sysctl requests, but this limit is not a hard one, as there is already the compromise for small requests (< PAGE_SIZE) for the sake of better concurrency. So this assert would fail if there was another call path for sysctl_wire_old_buffer() that otherwise met the sysctlmemlock acquisition criteria but that did not acquire sysctlmemlock, the net result of which would be extra wired pages, but otherwise correct operation. Converting that situation into a kernel panic doesn't seem right.

[kib]

Convincing.