sysctl(9): add CTLFLAG_NEEDGIANT flag

[kaktus]

Summary

Add CTLFLAG_NEEDGIANT flag (modelled after D_NEEDGIANT) that will be used to mark sysctls that still require Giant.

Rewrite sysctl_handle_string() to use internal locking instead of grabbing Giant.

[kib]

share/man/man9/sysctl.9

887

It is very confusing and probably wrong wording, but I am not sure about your intent.
If the handler locks Giant internally, then it is mp-safe from the PoV of the sysctl subsystem. I suspect that you mean that sysctl code needs to lock Giant around the call to the handler, at least this is how it is coded now.

888

s/strongly discouraged/not allowed/ ? Say something that the flag is a transitional measure and will be removed shortly, if you care about documenting it at all.

sys/kern/kern_sysctl.c

1687

Doesn't this unlock allos outlen to become invalid, if a parallel modification is performed ? I believe that the use of SYSCTL_OUT below would imply that.

[kaktus]

Address @kib comments.
Drop the man page, add a longer comment in sys/sysctl.h describing transient nature of the flag.
Call SYSCTL_OUT under lock.

[kib]

sys/kern/kern_sysctl.c

1698

I still do not like this. In particular, I do not like that for non-wired case, an error from SYSCTL_IN leaves the in-kernel string undefined, for instance if part of it was copied in, but part is on faulting page.

Might be, malloc the buffer, do copyin, then memcpy from the buffer to the final place under the lock, similar to the read case. It would also remove ordering between sysctlstringlock and all VFS/VM/net locks.

sys/sys/sysctl.h

49

So why not use _Static_assert as is, same as done in other parts of the kernel ?

[kaktus]

Address comments.

sys/sys/sysctl.h

49

I asked on IRC and was told that static_assert is the preferred way.

[kib]

sys/kern/kern_sysctl.c

1674

What is the point of using strnlen ?

Also, why do you need to call it under the lock ?

1690

Again, why is SYSCTL_OUT is called under the lock ?

sys/sys/sysctl.h

49

Who said you that ? Was it specifically static assert vs. _Static_assert, or somebody was lazy to type it correctly ?

Definitely _Static_assert is preferred over CTASSERT().

[kaktus]

Address comments.

[kib]

sys/kern/kern_sysctl.c

1685–1687

Don't this strlen call requires the same locking for !ro_string ? Otherwise a parallel update would cause UB.

1694

Drop excessive () in both lines

[kaktus]

Address comments.