Use syscall_helper_register() rather than syscall_register().

[brooks]

Summary

The usage is simpler and more common.

Obtained from: CheriBSD
Sponsored by: DARPA, AFRL

[cem]

I looked for a manual page for syscall_helper_register but it didn't exist :-(.

The conversion looks good to me.

[brooks]

• Per @jhb's comment on D14232 let MOD_UNLOAD handle the failure case.

[cem]

Huh, it surprises me that MOD_UNLOAD gets invoked if MOD_LOAD returns an error. But I can see it in module_register_init().

[rmacklem]

Since MOD_UNLOAD is not allowed and just fails, I don't see a reason
to call kgss_unload() and get rid of the syscalls.

The old code just left the module loaded and usable.
(Most of the NFS related modules cannot be safely unloaded,
since there is no way to know if no process/thread is executing
within them. The nfsd.ko is the main exception and can be
safely unloaded, since it only uses its own threads and it knows
if these still exist.)

Again, since I know nothing about the interface, I'll take myself off
the review.

[jhb]

In D14227#299184, @cem wrote:

Huh, it surprises me that MOD_UNLOAD gets invoked if MOD_LOAD returns an error. But I can see it in module_register_init().

I also found this surprising FWIW.

[jhb]

In D14227#299261, @rmacklem wrote:

Since MOD_UNLOAD is not allowed and just fails, I don't see a reason
to call kgss_unload() and get rid of the syscalls.

The old code just left the module loaded and usable.
(Most of the NFS related modules cannot be safely unloaded,
since there is no way to know if no process/thread is executing
within them. The nfsd.ko is the main exception and can be
safely unloaded, since it only uses its own threads and it knows
if these still exist.)

Again, since I know nothing about the interface, I'll take myself off
the review.

FWIW, the syscall_register/unregister stuff does its own thread counter now to handle this race for you. Once syscall_unregister() returns, no thread is currently in that syscall and no new thread can enter the syscall. Thus, if system calls are the only places threads can enter the system taht you are worried about, then syscall_helper_unregister() should be sufficient to permit you to unload. (You will still need to ensure you cleanup global variables such as locks, etc.)

[jhb]

sys/kgssapi/gss_impl.c

290 ↗

(On Diff #39061)

I think it's best to register syscalls last after data structures are initialized, not first.

320 ↗

(On Diff #39061)

That means you could also safely mtx_destroy(&kgss_gssd_lock) here since it would always be initialized even if MOD_LOAD failed due to kgss_load() failing.

[rmacklem]

I'm sure there's a way to add this to the above comment box,
but I don't know how to do it.

• W.r.t. the syscall_helper() maintaining a counter of caller threads...

---> For the NFS/Krpc modules it isn't normally syscall threads.

 Usually they are called through the VFS or via a set of function
 pointers or...
- There really isn't any reason to unload any of these as far as I know.

[brooks]

• Register syscalls last in the load process.

[cem]

sys/kgssapi/gss_impl.c

84 ↗

(On Diff #39123)

theoretically this returns an error value that should be propagated up.

practically, that value is always 0 (success).