linux-kernel - Re: [PATCH 1/7] rust: pci: pass driver data by value to `unbind`

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DEZMS6Y4A7XE.XE7EUBT5SJFJ@kernel.org>
Date: Tue, 16 Dec 2025 13:14:00 +0100
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Alexandre Courbot" <acourbot@...dia.com>
Cc: "Alice Ryhl" <aliceryhl@...gle.com>, "David Airlie" <airlied@...il.com>,
 "Simona Vetter" <simona@...ll.ch>, "Bjorn Helgaas" <bhelgaas@...gle.com>,
 Krzysztof Wilczyński <kwilczynski@...nel.org>, "Miguel
 Ojeda" <ojeda@...nel.org>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Trevor Gross" <tmgross@...ch.edu>,
 "John Hubbard" <jhubbard@...dia.com>, "Alistair Popple"
 <apopple@...dia.com>, "Joel Fernandes" <joelagnelf@...dia.com>, "Timur
 Tabi" <ttabi@...dia.com>, "Edwin Peer" <epeer@...dia.com>, "Eliot Courtney"
 <ecourtney@...dia.com>, <nouveau@...ts.freedesktop.org>,
 <dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
 <linux-pci@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH 1/7] rust: pci: pass driver data by value to `unbind`

On Tue Dec 16, 2025 at 6:13 AM CET, Alexandre Courbot wrote:
> When unbinding a PCI driver, the `T::unbind` callback is invoked by the
> driver framework, passing the driver data as a `Pin<&T>`.
>
> This artificially restricts what the driver can do, as it cannot mutate
> any state on the data. This becomes a problem in e.g. Nova, which needs
> to invoke mutable methods when unbinding.
>
> `remove_callback` retrieves the driver data by value, and drops it right
> after the call to `T::unbind`, meaning it is the only reference to the
> driver data by the time `T::unbind` is called.
>
> There is thus no reason for not granting full ownership of the data to
> `T::unbind`, so do it.

There are multiple reasons I did avoid this for:

(1) Race conditions

A driver can call Device::drvdata() and obtain a reference to the driver's
device private data as long as it has a &Device<Bound> and asserts the correct
type of the driver's device private data [1].

Assume you have an IRQ registration, for instance, that lives within this device
private data.  Within the IRQ handler, nothing prevents us from calling
Device::drvdata() given that the IRQ handler has a Device<Bound> reference.

Consequently, with passing the device private data by value to unbind() it can
happen that we have both a mutable and immutable reference at of the device
private data at the same time.

The same is true for a lot of other cases, such as work items or workqueues that
are scoped to the Device being bound living within the device private data.

More generally, you can't take full ownership of the device private data as long
as the device is not yet fully unbound (which is not yet the case in unbind()).

(2) Design

It is intentional that the device private data has a defined lifetime that ends
with the device being unbound from its driver. This way we get the guarantee
that any object stored in the device private data won't survive device / driver
unbind. If we give back the ownership to the driver, this guarantee is lost.

Conclusion:

Having that said, if you need mutable access to the fields of the device private
data within unbind() the corresponding field(s) should be protected by a lock.

Alternatively, you have mutable access within the destructor as well, but there
you don't have a bound device anymore. Which is only consequent, since we can't
call the destructor of the device private data before the device is fully
unbound.

(In fact, as by now, there is a bug with this, which I noticed a few days ago
when I still was on vacation: The bus implementations call the destructor of the
device private data too early, i.e. when the device is not fully unbound yet.

I'm working on a fix for this. Luckily, as by now, this is not a real bug in
practice, yet it has to be fixed.)

>From your end you don't have to worry about this though. nova-core should just
employ a lock for this, as we will need it in the future anyways, since we will
have concurrent access to the GSP.

[1] https://rust.docs.kernel.org/kernel/device/struct.Device.html#method.drvdata