Message-ID: <20250227150709.GF39591@nvidia.com>
Date: Thu, 27 Feb 2025 11:07:09 -0400
From: Jason Gunthorpe <jgg@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
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 Thu, Feb 27, 2025 at 12:32:45PM +0100, Danilo Krummrich wrote:
> On Wed, Feb 26, 2025 at 07:47:30PM -0400, Jason Gunthorpe wrote:
> > On Wed, Feb 26, 2025 at 10:31:10PM +0100, Danilo Krummrich wrote:
> > > 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.
> > 
> > Right, exactly
> > 
> > But it is not just PCI bar. There are a *huge* number of kernel APIs
> > that have built in to them the same sort of requirement - teardown
> > MUST run with remove, and once done the resource cannot be used by
> > another thread.
> > 
> > Basically most things involving function pointers has this sort of
> > lifecycle requirement because it is a common process that prevents a
> > EAF of module unload.
> 
> You're still mixing topics, the whole Devres<pci::Bar> thing as about limiting
> object lifetime to the point where the driver is unbound.
> 
> Shutting down asynchronous execution of things, i.e. workqueues, timers, IOCTLs
> to prevent unexpected access to the module .text section is a whole different
> topic.

Again, the standard kernel design pattern is to put these things
together so that shutdown isolates concurrency which permits free
without UAF.

> In other words, assuming that we properly enforce that there are no async
> execution paths after remove() or module_exit() (not necessarily the same),
> we still need to ensure that a pci::Bar object does not outlive remove().

Yes, you just have to somehow use rust to ensure a call pci_iounmap()
happens during remove, after the isolation.

You are already doing it with devm.  It seems to me the only problem
you have is nobody has invented a way in rust to contract that the devm
won't run until the threads are isolated.

I don't see this as insolvable, you can have some input argument to
any API that creates concurrency that also pushes an ordered
destructor to the struct device lifecycle that ensures it cancels that
concurrency.

> Device resources are a bit special, since their lifetime must be cap'd at device
> unbind, *independent* of the object lifetime they reside in. Hence the Devres
> container.

I'd argue many resources should be limited to device unbind. Memory is
perhaps the only exception.

> > My fear, that is intensifying as we go through this discussion, is
> > that rust binding authors have not fully comprehended what the kernel
> > life cycle model and common design pattern actually is, and have not
> > fully thought through issues like module unload creating a lifetime
> > cycle for *function pointers*.
> 
> I do *not* see where you take the evidence from to make such a generic
> statement.

Well, I take the basic insistance that is OK to leak stuff from driver
scope to module scope is not well designed.

> Especially because there aren't a lot of abstractions upstream yet that fall
> under this category.

And I am thinking forward to other APIs you will need and how they
will interact and not feeling good about this direction.

> > The thing is once you have a mechanism to shutdown all the stuff you
> > don't need the overhead of this revocable checking on the normal
> > paths. What you need is a way to bring your pci::Bar into a safety
> > contract that remove will shootdown concurrency and that directly
> > denies references to pci::Bar, and the same contract will guarentee it
> > frees pci::Bar memory.
> 
> This contract needs to be technically enforced, not by convention as
> we do in C.

People do amazing things with contracts in rust, why is this case so
hard?

> Data that is accessed from a work item can't be freed under the
> workqueue by design in Rust.

What? That's madness, alot of work functions are freeing
something. They are often the terminal point of an object's lifecycle
because you often have to allocate memory to launch the work in the
first place.

Certainly if you restrict workqueues to be very limited then alot of
their challenging problems disappear :\

Jason

Powered by blists - more mailing lists