Don't invoke semunload() if seminit() fails during MOD_LOAD.
ClosedPublic
Actions

Authored by jhb on Oct 6 2020, 4:58 PM.

Details

Reviewers

kib
markj

Commits

rS366584: Don't invoke semunload() if seminit() fails during MOD_LOAD.

Summary

The module handler code invokes a MOD_UNLOAD event immediately if
MOD_LOAD fails. The result was that if seminit() failed, semunload()
was invoked twice. semunload() is not idempotent however and would
try to remove it's process_exit eventhandler twice resulting in a
panic.

Test Plan

in a branch of CheriBSD we had a bug that caused seminit to fail which then exposed this as a boot-time panic
we don't appear to have sysvsem tests in kyua directly, but I ran the sys/audit/interprocess tests which test audit record generation for sysvsem and those all passed

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 34021
Build 31208: arc lint + arc unit

Event Timeline

jhb requested review of this revision.Oct 6 2020, 4:58 PM

jhb created this revision.

Harbormaster completed remote builds in B34021: Diff 77945.Oct 6 2020, 4:58 PM

The behavior that MOD_UNLOAD gets invoked for a failed MOD_LOAD is perhaps surprising. Most of our other APIs aren't quite like that (e.g. we don't call device_detach() if device_attach() fails but instead require device_attach() to clean up if an error occurs).

Hm, seminit() fails if either sem_syscalls or sem32_syscalls registration fails. In this case, there are a lot of resources allocated that needs to be freed.
On the other hand it seems that syscall_helper_register/unregister is not safe to do if register failed. I believe this is what should be fixed somehow.

This revision is now accepted and ready to land.Oct 6 2020, 5:25 PM

In D26696#594852, @jhb wrote:

The behavior that MOD_UNLOAD gets invoked for a failed MOD_LOAD is perhaps surprising. Most of our other APIs aren't quite like that (e.g. we don't call device_detach() if device_attach() fails but instead require device_attach() to clean up if an error occurs).

Yeah, I think this has tripped me up before. It might be nice to mention it in MODULE.9.

markj accepted this revision.Oct 6 2020, 5:38 PM

In D26696#594853, @kib wrote:

Hm, seminit() fails if either sem_syscalls or sem32_syscalls registration fails. In this case, there are a lot of resources allocated that needs to be freed.
On the other hand it seems that syscall_helper_register/unregister is not safe to do if register failed. I believe this is what should be fixed somehow.

This does get cleaned up because of this code in module_register_init():

	MOD_SUNLOCK;
	error = MOD_EVENT(mod, MOD_LOAD);
	if (error) {
		MOD_EVENT(mod, MOD_UNLOAD);

Also, syscall_helper_deregister() does DTRT since there is a per-syscall "is registered" flag.

The bug here is that it gets cleaned up twice, and semunload() can't be safely called twice.

Closed by commit rS366584: Don't invoke semunload() if seminit() fails during MOD_LOAD. (authored by jhb). · Explain WhyOct 9 2020, 8:20 PM

This revision was automatically updated to reflect the committed changes.

jhb added a commit: rS366584: Don't invoke semunload() if seminit() fails during MOD_LOAD..

Herald added a subscriber: imp. · View Herald TranscriptOct 9 2020, 8:20 PM

Revision Contents
Changeset List

Path

Size

sys/

kern/

sysv_sem.c

2 lines

Diff 77945

View Options

Don't invoke semunload() if seminit() fails during MOD_LOAD.ClosedPublicActions