Message-ID: <Z7-IHgcVVS8XBurW@cassiopeiae>
Date: Wed, 26 Feb 2025 22:31:10 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: Joel Fernandes <joelagnelf@...dia.com>,
	Alexandre Courbot <acourbot@...dia.com>,
	Dave Airlie <airlied@...il.com>, Gary Guo <gary@...yguo.net>,
	Joel Fernandes <joel@...lfernandes.org>,
	Boqun Feng <boqun.feng@...il.com>,
	John Hubbard <jhubbard@...dia.com>, Ben Skeggs <bskeggs@...dia.com>,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	nouveau@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
	paulmck@...nel.org
Subject: Re: [RFC PATCH 0/3] gpu: nova-core: add basic timer subdevice
 implementation

On Wed, Feb 26, 2025 at 01:21:20PM -0400, Jason Gunthorpe wrote:
> On Wed, Feb 26, 2025 at 02:16:58AM +0100, Danilo Krummrich wrote:
> > Again, the reason a pci::Bar needs to be revocable in Rust is that we can't have
> > the driver potentially keep the pci::Bar alive (or even access it) after the
> > device is unbound.
> 
> My impression is that nobody has yet come up with a Rust way to
> implement the normal kernel design pattern of revoke threads then free
> objects in safe rust.

I get where you're coming from (and I agree), but that is a different issue.

Let's take a step back and look again why we have Devres (and Revocable) for
e.g. pci::Bar.

The device / driver model requires that device resources are only held by a
driver, as long as the driver is bound to the device.

For instance, in C we achieve this by calling

	pci_iounmap()
	pci_release_region()

from remove().

We rely on this, we trust drivers to actually do this.

We also trust drivers that they don't access the pointer originally returned by
pci_iomap() after remove(). Typically, drivers do this by shutting down all
asynchronous execution paths, e.g. workqueues. Some other drivers might still
run code after remove() and hence needs some synchronization, like DRM.

In Rust pci_iounmap() and pci_release_region() are called when the pci::Bar
object is dropped. But we don't want to trust the driver to actually do this.
Instead, we want to ensure that the driver can *not* do something that is not
allowed by the device / driver model.

Therefore, we never hand out a raw pci::Bar to driver, but a Devres<pci::Bar>.
With this a driver can't prevent the pci::Bar being dropped once the device is
unbound.

So, the main objective here is to ensure that a driver can't keep the pci::Bar
(and hence the memory mapping) alive arbitrarily.

Now, let's get back to concurrent code that might still attempt to use the
pci::Bar. Surely, we need mechanisms to shut down all asynchronous execution
paths (e.g. workqueues) once the device is unbound. But that's not the job of
Devres<pci::Bar>. The job of Devres<pci::Bar> is to be robust against misuse.

Again, that the revocable characteristic comes in handy for drivers that still
run code after remove() intentionally, is a nice coincidence.

> 
> Yes, this is a peculiar lifetime model, but it is pretty important in
> the kernel. I'm not convinced you can just fully ignore it in Rust as
> a design pattern. We use it pretty much everywhere a function pointer
> is involved.
> 
> For instance, I'm looking at workqueue.rs and wondering why is it safe
> against Execute After Free races. I see none of the C functions I
> would expect to be used to prevent those races in the code.
> 
> Even the simple example:
> 
> //! fn print_later(val: Arc<MyStruct>) {
> //!     let _ = workqueue::system().enqueue(val);
> //! }
> 
> Seems to be missing the EAF prevention ie WorkItem::run() is in .text
> of THIS_MODULE and I see nothing is preventing THIS_MODULE from being
> unloaded.
> 
> The expectation of work queues is to follow the above revoke threads
> then free pattern. A module should do that sequence in the driver
> remove() or module __exit function.

Fully agree with that.

I guess you're referring to cancel_work_sync() and friends as well as
destroy_workqueue(), etc.

They're indeed missing, this is because the workqueue work originates from the
Rust binder efforts and binder is only used builtin, so there was no need so
far.

But yes, once people start using workqueues for other modules, we surely need to
extend the abstraction accordingly.

Other abstractions do consider this though, e.g. the upcoming hrtimer work. [1]

In terms of IOCTLs it depends on the particular subsystem, but this is (or will
be) also reflected by the corresponding abstraction. Dropping a
MiscDeviceRegistration [2] on module_exit() for instance will ensure that there
are no concurrent IOCTLs, just like the corresponding C code.

[1] https://lore.kernel.org/rust-for-linux/20250224-hrtimer-v3-v6-12-rc2-v9-0-5bd3bf0ce6cc@kernel.org/
[2] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/miscdevice.rs#n50

Powered by blists - more mailing lists