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 ↗

(On Diff #67339)

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 ↗

(On Diff #67339)

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 ↗

(On Diff #67339)

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

1695 ↗

(On Diff #67631)

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 ↗

(On Diff #67631)

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 ↗

(On Diff #67631)

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

[kib]

sys/kern/kern_sysctl.c

1670 ↗

(On Diff #67800)

What is the point of using strnlen ?

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

1684 ↗

(On Diff #67800)

Again, why is SYSCTL_OUT is called under the lock ?

sys/sys/sysctl.h

49 ↗

(On Diff #67631)

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

1679 ↗

(On Diff #67814)

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

1686 ↗

(On Diff #67814)

Drop excessive () in both lines

[kaktus]

Address comments.

[avg]

head/sys/kern/kern_sysctl.c

1671

This is a very belated observation / comment, but what was the reason to change strnlen(tmparg, arg2 - 1) + 1 to strlen(tmparg) ?

The old code was able to handle unterminated strings as long as arg2 was equal to string length.
Perhaps, that was unconventional but it used to work.

The reason I am asking is because this change in behavior was not spelled out in the commit message
and the commit is about locking.
So, I wonder if the change was intentional at all.

[kib]

head/sys/kern/kern_sysctl.c

1671

I asked about strnlen() use above, and it seems that the case you describe was missed during rewrite. Feel free to fix.