Page MenuHomeFreeBSD

sysctl(9): add CTLFLAG_NEEDGIANT flag
ClosedPublic

Authored by kaktus on Jan 27 2020, 12:00 AM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kaktus created this revision.Jan 27 2020, 12:00 AM
kib added inline comments.Jan 27 2020, 3:27 PM
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 updated this revision to Diff 67631.Feb 1 2020, 7:02 PM
kaktus removed a reviewer: manpages.

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 added inline comments.Feb 3 2020, 8:36 PM
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 updated this revision to Diff 67800.Feb 4 2020, 11:48 PM

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 added inline comments.Feb 5 2020, 10:41 AM
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 updated this revision to Diff 67814.Feb 5 2020, 11:47 AM

Address comments.

kib added inline comments.Feb 5 2020, 8:45 PM
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 updated this revision to Diff 67833.Feb 5 2020, 9:29 PM

Address comments.

kib accepted this revision.Feb 5 2020, 9:45 PM
This revision is now accepted and ready to land.Feb 5 2020, 9:45 PM
This revision was automatically updated to reflect the committed changes.