Page MenuHomeFreeBSD

Prevent detaching driver if the attach is not finished
ClosedPublic

Authored by rk_semihalf.com on Feb 27 2019, 7:55 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 4, 12:22 PM
Unknown Object (File)
Oct 15 2024, 6:08 PM
Unknown Object (File)
Oct 14 2024, 5:05 PM
Unknown Object (File)
Oct 14 2024, 1:18 PM
Unknown Object (File)
Sep 30 2024, 2:44 PM
Unknown Object (File)
Sep 30 2024, 2:25 PM
Unknown Object (File)
Sep 21 2024, 7:20 PM
Unknown Object (File)
Sep 19 2024, 4:44 PM
Subscribers

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

sys/kern/subr_bus.c
3005 ↗(On Diff #54453)

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
sys/kern/subr_bus.c
3005 ↗(On Diff #54453)

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

sys/kern/subr_bus.c
3005 ↗(On Diff #54453)

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);
}
sys/kern/subr_bus.c
3005 ↗(On Diff #54453)

That LGTM but I'll defer to @imp

sys/kern/subr_bus.c
3005 ↗(On Diff #54453)

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.
sys/kern/subr_bus.c
3005 ↗(On Diff #54453)

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

Thanks,
Marcin