Re: [RFC PATCH 0/3] gpu: nova-core: add basic timer subdevice implementation

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Mar 04, 2025 at 05:10:45PM +0100, Simona Vetter wrote:
> On Fri, Feb 28, 2025 at 02:40:13PM -0400, Jason Gunthorpe wrote:
> > On Fri, Feb 28, 2025 at 11:52:57AM +0100, Simona Vetter wrote:
> > 
> > > - Nuke the driver binding manually through sysfs with the unbind files.
> > > - Nuke all userspace that might beholding files and other resources open.
> > > - At this point the module refcount should be zero and you can unload it.
> > > 
> > > Except developers really don't like the manual unbind step, and so we're
> > > missing try_module_get() in a bunch of places where it really should be.
> > 
> > IMHO they are not missing, we just have a general rule that if a
> > cleanup function, required to be called prior to module exit, revokes
> > any .text pointers then you don't need to hold the module refcount.
> > 
> > file_operations doesn't have such a cleanup function which is why it
> > takes the refcount.
> > 
> > hrtimer does have such a function which is why it doesn't take the
> > refcount.
> 
> I was talking about a bunch of other places, where it works like
> file_operations, except we don't bother with the module reference count.
> I've seen patches fly by where people "fix" these things because module
> unload is "broken".

Sure, but there are only two correct API approaches, either you
require the user to make a cancel call that sanitizes the module
references, or you manage them internally.

Hope and pray isn't an option :)

> gpu drivers can hog console_lock (yes we're trying to get away from that
> as much as possible), at that point a cavalier attitude of "you can just
> wait" isn't very appreciated.

What are you trying to solve here? If the system is already stuck
infinitely on the console lock why is module remove even being
considered?

module remove shouldn't be a remedy for a crashed driver...

> > But so is half removing the driver while it is doing *anything* and
> > trying to mitigate that with a different kind of hard to do locking
> > fix. *shrug*
> 
> The thing is that rust helps you enormously with implementing revocable
> resources and making sure you're not cheating with all the bail-out paths.

Assuming a half alive driver with MMIO and interrupts ripped away
doesn't lock up.

Assuming all your interrupt triggered sleeps have gained a shootdown
mechanism.

Assuming all the new extra error paths this creates don't corrupt the
internal state of the driver and cause it to lockup.

Meh. It doesn't seem like such an obvious win to me. Personally I'm
terrified of the idea of a zombie driver half sitting around in a
totally untestable configuration working properly..

> It cannot help you with making sure you have interruptible/abortable
> sleeps in all the right places. 

:(

> > Like, I see a THIS_MODULE in driver->fops == amdgpu_driver_kms_fops ?
> 
> Yeah it's there, except only for the userspace references and not for the
> kernel internal ones. Because developers get a bit prickle about adding
> those unfortunately due to "it breaks module unload". Maybe we just should
> add them, at least for rust.

Yeah, I think such obviously wrong things should be pushed back
against. We don't want EAF bugs in the kernel, we want security...

> You've missed the "it will upset developers part". I've seen people remove
> module references that are needed, to "fix" driver unloading.

When done properly the module can be unloaded. Most rdma driver
modules are unloadable, live, while FDs are open.

> The third part is that I'm not aware of anything in rust that would
> guarantee that the function pointer and the module reference actually
> belong to each another. Which means another runtime check most likely, and
> hence another thing that shouldn't fail which kinda can now.

I suspect it has to come from the C code API contracts, which leak
into the binding design.

If the C API handles module refcounting internally then rust is fine
so long as it enforces THIS_MODULE.

If the C API requires cancel then rust is fine so long as the binding
guarantees cancel before module unload.

Jason