[PATCH v1] driver core: unbind only the driver whose unbind file was written

[PATCH v1] driver core: unbind only the driver whose unbind file was written

[Nickolai Zeldovich]

unbind_store() checks dev->driver == drv without the device lock and
then calls device_driver_detach(), which is
device_release_driver_internal(dev, NULL, dev->parent): with a NULL
driver that function releases whichever driver is bound once it holds
the lock.  Between the check and the lock another thread can unbind
drv and bind a different driver to the same device (a second write to
the unbind and bind files, a driver_override write followed by
drivers_probe, or the bus re-probing the device after the first
unbind), and the write to drv's unbind file then detaches the other
driver, one the writer never named, and reports success.

The pattern is as old as the unbind file: the check was never under the
lock and the release never named the driver.  The locked variant has
existed since commit 4bdb35506b89 ("driver core: Add a wrapper around
__device_release_driver()") added the drv argument to
device_release_driver_internal() for driver_detach(), which has the
same requirement.

Pass drv to device_release_driver_internal(): it re-checks dev->driver
under the lock and releases the driver only when it is still the one
the file belongs to.  When the driver changed in between, the write
detaches nothing; drv is no longer bound to the device either way, so
the write still returns the count.

Found by formal verification of drivers/base/bus.c.

Fixes: 151ef38f7c0e ("[PATCH] driver core: Add the ability to unbind drivers to devices from userspace")
Assisted-by: LLM
Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
---

Notes:
    The unlocked dev->driver == drv test stays as the fast path (a device
    bound to another driver still gets -ENODEV there); making the write
    return -ENODEV when only the locked re-check fails would need a return
    value from device_release_driver_internal(), which three other callers
    ignore, and the writer's aim, drv no longer bound to the device, is met
    in that case. The race window is the time
    between the test and __device_driver_lock(); it was not hit in testing
    (no in-tree pair of drivers with a slow probe was raced), the
    interleaving is described in the message and a userspace racer is in
    the review material. bind_store() has no such hole: __driver_probe_device()
    checks dev->driver under the lock.

 drivers/base/bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index d17bd91490ee..8553183fd67f 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -241,7 +241,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
 	if (dev && dev->driver == drv) {
-		device_driver_detach(dev);
+		device_release_driver_internal(dev, drv, dev->parent);
 		err = count;
 	}
 	put_device(dev);

base-commit: 70dd4ee72eb2d5793c62e71c14e3aba78e5a4e56
-- 
2.53.0

Re: [PATCH v1] driver core: unbind only the driver whose unbind file was written

[Greg Kroah-Hartman]

On Fri, Oct 16, 2026 at 10:09:35PM -0400, Nickolai Zeldovich wrote:
> unbind_store() checks dev->driver == drv without the device lock and
> then calls device_driver_detach(), which is
> device_release_driver_internal(dev, NULL, dev->parent): with a NULL
> driver that function releases whichever driver is bound once it holds
> the lock.  Between the check and the lock another thread can unbind
> drv and bind a different driver to the same device (a second write to
> the unbind and bind files, a driver_override write followed by
> drivers_probe, or the bus re-probing the device after the first
> unbind), and the write to drv's unbind file then detaches the other
> driver, one the writer never named, and reports success.

bind/unbind now causes a taint, so what is the real problem here?  A
root user is poking around and causing problems, so any error is their
fault for doing this?

> The pattern is as old as the unbind file: the check was never under the
> lock and the release never named the driver.  The locked variant has
> existed since commit 4bdb35506b89 ("driver core: Add a wrapper around
> __device_release_driver()") added the drv argument to
> device_release_driver_internal() for driver_detach(), which has the
> same requirement.

Because this has always been present, and no one has actually reported
this, perhaps it is not really a problem at all?  In other words, what
real-life-use requires this?  Again, bind/unbind is for developers only.

> Pass drv to device_release_driver_internal(): it re-checks dev->driver
> under the lock and releases the driver only when it is still the one
> the file belongs to.  When the driver changed in between, the write
> detaches nothing; drv is no longer bound to the device either way, so
> the write still returns the count.
> 
> Found by formal verification of drivers/base/bus.c.

Where is the formal verification proof?  Please don't have your LLM
claim this without that as it's pointless.

> Fixes: 151ef38f7c0e ("[PATCH] driver core: Add the ability to unbind drivers to devices from userspace")
> Assisted-by: LLM
> Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
> ---
> 
> Notes:
>     The unlocked dev->driver == drv test stays as the fast path (a device
>     bound to another driver still gets -ENODEV there); making the write
>     return -ENODEV when only the locked re-check fails would need a return
>     value from device_release_driver_internal(), which three other callers
>     ignore, and the writer's aim, drv no longer bound to the device, is met
>     in that case. The race window is the time
>     between the test and __device_driver_lock(); it was not hit in testing
>     (no in-tree pair of drivers with a slow probe was raced), the
>     interleaving is described in the message and a userspace racer is in
>     the review material. bind_store() has no such hole: __driver_probe_device()
>     checks dev->driver under the lock.

I don't understand, this is not a place where a "race" can happen.

thanks,

greg k-h

Re: [PATCH v1] driver core: unbind only the driver whose unbind file was written

[Nickolai Zeldovich]

On Sat, Oct 17, 2026 at 5:49 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> > The pattern is as old as the unbind file: the check was never under the
> > lock and the release never named the driver.  The locked variant has
> > existed since commit 4bdb35506b89 ("driver core: Add a wrapper around
> > __device_release_driver()") added the drv argument to
> > device_release_driver_internal() for driver_detach(), which has the
> > same requirement.
>
> Because this has always been present, and no one has actually reported
> this, perhaps it is not really a problem at all?  In other words, what
> real-life-use requires this?  Again, bind/unbind is for developers only.

The goal was to pin down a precise spec of what should happen when
writing a device name A to /sys/bus/B/drivers/D/unbind.  I think the
intended effect is that it should unbind driver D from device A.  The
code already has a check that tries to ensure this.  However, the
check isn't technically enough to achieve this precise effect, in the
presence of concurrent operations on other cores (like
binding/unbinding other drivers to the same device).

It sounds like this level of precise specification under weird
concurrent operations isn't useful for developer-facing interfaces.
I'll stick to more clear-cut bugs.

> ...
> > Found by formal verification of drivers/base/bus.c.
>
> Where is the formal verification proof?  Please don't have your LLM
> claim this without that as it's pointless.

The intended point was to give some indication of where this patch is
coming from: it aims to formally prove something about the behavior of
the kernel, rather than fix any specific issue encountered by a Linux
user.  There isn't a formal spec for the Linux user-kernel boundary,
though (as in the above example: what exactly should happen in this
weird concurrent scenario?), so for many cases I'm trying to infer
what the expected spec is.

Happy to describe it in more detail once I have a coherent checkpoint.
The current proofs aren't complete for Linux yet; we'll see if it's
possible to get there.  We verified a smaller OS kernel
(https://arxiv.org/abs/2609.04043) and now I'm trying to do the same
for Linux, roughly speaking.

Nickolai.

Re: [PATCH v1] driver core: unbind only the driver whose unbind file was written

[Greg Kroah-Hartman]

On Sat, Oct 17, 2026 at 08:24:03AM -0400, Nickolai Zeldovich wrote:
> On Sat, Oct 17, 2026 at 5:49 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > > The pattern is as old as the unbind file: the check was never under the
> > > lock and the release never named the driver.  The locked variant has
> > > existed since commit 4bdb35506b89 ("driver core: Add a wrapper around
> > > __device_release_driver()") added the drv argument to
> > > device_release_driver_internal() for driver_detach(), which has the
> > > same requirement.
> >
> > Because this has always been present, and no one has actually reported
> > this, perhaps it is not really a problem at all?  In other words, what
> > real-life-use requires this?  Again, bind/unbind is for developers only.
> 
> The goal was to pin down a precise spec of what should happen when
> writing a device name A to /sys/bus/B/drivers/D/unbind.  I think the
> intended effect is that it should unbind driver D from device A.  The
> code already has a check that tries to ensure this.  However, the
> check isn't technically enough to achieve this precise effect, in the
> presence of concurrent operations on other cores (like
> binding/unbinding other drivers to the same device).

You can not bind/unbind drivers to the same device at the same time, the
driver core should protect yourself from this, BUT most importantly,
bind/unbind is a "best effort, you keep the pieces of the broken kernel
when something goes wrong".

> It sounds like this level of precise specification under weird
> concurrent operations isn't useful for developer-facing interfaces.

Again no one should be doing this, and if they are, they are root and
can do much worse things :)

> I'll stick to more clear-cut bugs.

Or just prove that this is a real bug please.

> > ...
> > > Found by formal verification of drivers/base/bus.c.
> >
> > Where is the formal verification proof?  Please don't have your LLM
> > claim this without that as it's pointless.
> 
> The intended point was to give some indication of where this patch is
> coming from: it aims to formally prove something about the behavior of
> the kernel, rather than fix any specific issue encountered by a Linux
> user.  There isn't a formal spec for the Linux user-kernel boundary,
> though (as in the above example: what exactly should happen in this
> weird concurrent scenario?), so for many cases I'm trying to infer
> what the expected spec is.

Then why did your LLM say that this was found in this way if that's not
what you really did?

People read what you wrote, you should also do so :)

thanks,

greg k-h