Page MenuHomeFreeBSD

Prevent detaching driver if the attach is not finished
ClosedPublic

Authored by rk_semihalf.com on Feb 27 2019, 7:55 AM.

Details

Summary

When device is in attaching state, detach should return EBUSY instead of success. In other case, there could be race between attach and detach during driver unloading. If driver goes sleep and releases GIANT lock during attaching, unloading module could start. In such case when attach continues after module unload, page fault "supervisor read instruction, page not present" occurred.

Test Plan

Running multiple (~100) kldload; kldunload on driver that sleeps in attach (for example ENA on AWS instance).

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

imp accepted this revision.Feb 27 2019, 2:29 PM

It's an OK workaround, but the real problem is that we're not properly interlocked elsewhere.

sys/kern/subr_bus.c
3005

Please add a comment that this is a horrific hack designed to cope with busses that aren't properly locked that allow entry into both attach and detach.

Sadly, most busses are like this in the tree, and it's one of the big issues with my work trying to properly lock newbus.

This revision is now accepted and ready to land.Feb 27 2019, 2:29 PM
emaste added inline comments.Feb 27 2019, 3:15 PM
sys/kern/subr_bus.c
3005

Warner is it worth going a little further than a comment, and emitting a warning upon DS_ATTACHING?

mw added inline comments.Feb 28 2019, 2:35 PM
sys/kern/subr_bus.c
3005

How about following then:

if (dev->state == DS_BUSY)
    return (EBUSY);
if (dev->state == DS_ATTACHING) {
    device_printf(dev, "device in attaching state! Deferring detach.\n");
    return (EBUSY);
}
emaste added inline comments.Feb 28 2019, 2:50 PM
sys/kern/subr_bus.c
3005

That LGTM but I'll defer to @imp

imp added inline comments.Feb 28 2019, 10:29 PM
sys/kern/subr_bus.c
3005

This makes me happier since we warn about the problem, even if the root cause is well beyond the scope of this review to fix.

This revision was automatically updated to reflect the committed changes.
mw added inline comments.Mar 1 2019, 1:19 AM
sys/kern/subr_bus.c
3005

I also added mentioning the work-around character of the patch in the commit log (r344676.)

Thanks,
Marcin