Page MenuHomeFreeBSD

new syscall: __sysctlbyname
ClosedPublic

Authored by kaktus on Sep 22 2018, 3:43 AM.

Details

Summary

Create a new syscall to avoid two calls to __sysctl: first to resolve the name, and second to actually execute the action. This call pattern is quite common so call kernel just once.

Tested with older and newer binaries on amd64 and compat32.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

kaktus created this revision.Sep 22 2018, 3:43 AM
kaktus updated this revision to Diff 48340.Sep 22 2018, 3:52 AM
mjg requested changes to this revision.Sep 22 2018, 4:06 AM
mjg added inline comments.
lib/libc/gen/sysctlbyname.c
58

this should be namelen to avoid namespace collision with newlen

the strlen call should be done in a separate line

sys/compat/freebsd32/freebsd32_misc.c
2269

namelen is unsigned, so this should just check for == 0 instead of < 1

2272

vast majority of names are short, afair < 16. this should have a buffer of similar size to avoid having to do malloc in the common case.

2278

why the magic numbers

2293

the entire point of a __sysctlbyname syscall is to do all the sysctl work *once*. that is, the code should be reorganized so that the handler can be looked up by the name and then executed. this patch, just like the current approach, first finds the oid number and then performs yet another lookup to translate it the handler. this basically finds the handler twice. it does provide an upshot of only one syscall trip, but I don't think that's good enough of an improvement to integrate.

2298

this ignored the error

This revision now requires changes to proceed.Sep 22 2018, 4:06 AM
kaktus marked 2 inline comments as done.Sep 25 2018, 1:41 PM
kaktus added inline comments.
sys/compat/freebsd32/freebsd32_misc.c
2293

OK, i'll rewrite this.

2298

So did every other usage of this function in this file, and in most cases in the whole kernel. I don't know if assigning EFAULT now, when the action was potentially made if this was write/set and not read only sysctl makes any sense here.

kaktus updated this revision to Diff 48428.Sep 25 2018, 1:43 PM

Update diff with minor fixes.

kaktus marked 2 inline comments as done.Sep 25 2018, 1:43 PM
kib added inline comments.Sep 25 2018, 2:48 PM
lib/libc/gen/sysctlbyname.c
47–48

What is the point of creating this single-use variable ? Esp. of type u_int which theoretically causes truncation ?

lib/libc/sys/Symbol.map
402

This is the wrong version.

469

And I do not understand this line at all.

sys/compat/freebsd32/freebsd32_misc.c
2286

if (error != 0)

2289

if (oldlenp != NULL)

sys/compat/freebsd32/freebsd32_proto.h
713 ↗(On Diff #48428)

Remove generated parts from the review.

sys/compat/freebsd32/syscalls.master
1143

Why namelen is u_int while newlen uint32_t ?

mjg added a comment.Aug 21 2019, 5:46 AM

I think the kernel part is operational well enough. With this cleaned up I think it may be fine for inclusion - after we get userspace to use the syscall we can improve internals much later.

Do you still have interest in working on this?

brooks requested changes to this revision.Aug 21 2019, 9:15 AM

It would be helpful to generate this diff against HEAD. That would exclude the generated files and the freebsd32 capabilities.conf entry.

sys/compat/freebsd32/syscalls.master
1143

both should be size_t.

The implementation should be freebsd32___sysctlbyname().

sys/kern/syscalls.master
2856

Please don't add more instances of this last line. Just live with the extra underscores in the code.

This revision now requires changes to proceed.Aug 21 2019, 9:15 AM
kaktus updated this revision to Diff 61088.Aug 21 2019, 8:42 PM

Update diff to r351302 and address some comments.

mjg added a comment.EditedAug 21 2019, 8:57 PM

Kernel code has to get deduplicated. You can pass relevant sizes down and just have one variant do the trick.

Drop the NAMEBUFLEN macro. You can just sizeof(buf) instead for comparison.

This needs a TODO comment explaining the lookup should be done once.

Apart from this I have no further comments about kernel part.

One may consdier a temporary fallbackin libc in case of ENOSYS where the code redoes work the old way. Should save some headache during upgrades. To be removed in few months time.

kaktus updated this revision to Diff 61275.Aug 25 2019, 11:08 PM

Split the function for one that do the actual work and teach sys___sysctlbyname and freebsd32___sysctlbyname to call it properly.

Provide a compat layer in libc by ignoring the SIGSYS that can be generated by the kernel when the new libc is used with older kernel. As calling sigaction(2) twice every time is rather expensive and possibly intrusive I'd suggest to keep it around for a rather short period of time, say a few weeks and announce the change on current@ to remind folks to reboot with the new kernel before installworld. I provided an #ifndef HAVE___SYSCTLBYNAME for someone who wants to force the build without libc compat shims, but I'm not entirely happy with this solution either.

kaktus updated this revision to Diff 61277.Aug 25 2019, 11:19 PM

Check for ENOSYS.

kib added inline comments.Aug 26 2019, 7:25 AM
lib/libc/gen/sysctlbyname.c
63

This is no-go outright. Library must not manipulate global process state. It is both non-thread-safe and non-async-signal-safe.

If you want to handle older kernels in libc(why ?) use __FreeBSD_version. Look for examples.

kaktus added inline comments.Aug 26 2019, 9:06 AM
lib/libc/gen/sysctlbyname.c
63

I agree, it was just an idea to ease the upgrade process for those installing new libc while running older kernel, because even reboot would be killed by SIGSYS as it use sysctlbyname(3).
Guarding on __FreeBSD_version wouldn't help here anyway, it'd need a runtime check on osreldate or so to find if the syscall is present.
So my idea is to drop this broken-anyway-compat and add an UPDATING note, maybe post heads up on current@, or should I implement runtime check on kern.osreldate for some period of time?

kib added inline comments.Aug 26 2019, 9:30 AM
lib/libc/gen/sysctlbyname.c
63

My opinion is that newer libc requires newer kernel, but some people are nicer to users.

Look at lib/libc/sys/stat.c for what I mean by checks for __FreeBSD_version.

kaktus updated this revision to Diff 61285.Aug 26 2019, 11:17 AM

Add a lightweight compat layer in libc.

imp accepted this revision.Aug 26 2019, 4:23 PM

Thanks for doing the compat shim. I think this would have been one area where it would have been hard to recover from a make installworld before a new kernel was in place...

mjg accepted this revision.Aug 26 2019, 11:36 PM

Kernel part looks legit to me modulo some nits which I'm not going to bother with.

If others accept the userspace part I'll commit this later.

kaktus updated this revision to Diff 61343.Aug 27 2019, 9:04 AM

Update to r351542 and bump __FreeBSD_version again.

kib accepted this revision.Aug 27 2019, 2:02 PM

Two minor nits.

sys/compat/freebsd32/freebsd32_misc.c
2270

Set error to zero in this branch, avoid initialization at declaration.

2279

error from suword32 should be preserved and returned there, I think.

kaktus updated this revision to Diff 61368.Aug 27 2019, 6:38 PM

Address @kib's comments.

sys/compat/freebsd32/freebsd32_misc.c
2279

All other invocations of this function in this file don't do that either, and I don't know if returning EFAULT here when the action was already performed if the request was to change setting makes any sense.

mjg added a comment.Sep 2 2019, 1:33 PM

@brooks do you have further comments?

brooks added a comment.Sep 2 2019, 1:43 PM

sys/compat/freebsd32/syscalls.master seems to be missing now...

kaktus updated this revision to Diff 61549.Sep 2 2019, 1:52 PM

add a missing file

brooks accepted this revision.Sep 2 2019, 2:04 PM

LGTM

This revision is now accepted and ready to land.Sep 2 2019, 2:04 PM