On Fri, Oct 17, 2025 at 11:41:56PM +0200, Danilo Krummrich wrote:
On Fri Oct 17, 2025 at 8:44 PM CEST, Jason Gunthorpe wrote:
On Fri, Oct 17, 2025 at 08:19:06PM +0200, Danilo Krummrich wrote:
On 10/17/25 6:37 PM, Jason Gunthorpe wrote:
On Fri, Oct 17, 2025 at 06:29:10PM +0200, Danilo Krummrich wrote:
I'm not sure about MISC device though. Unless there's a good reason, I think MISC device should be "fenced" instead.
misc is a very small wrapper around raw fops, and raw fops are optimized for performance. Adding locking that many important things like normal files don't need to all fops would not be agreed.
The sketch in this series where we have a core helper to provide a shim fops that adds on the lock is smart and I think could be an agreeable way to make a synchronous misc and cdev unregister for everyone to trivially use.
Sure, for MISC devices without a parent for instance there are no device resources to access anyways.
There are many situations with misc that can get people into trouble without parent:
misc_deregister(x); timer_shutdown_sync(y); kfree(z);
For example. It is is buggy if the fops touch y or z.
This is why a _sync version is such a nice clean idea because with 5 letters the above can just be fixed.
Wrapping everything in a revocable would be a huge PITA.
That's a bit of a different problem though. Revocable clearly isn't the solution. _sync() works, but doesn't account for the actual problem, which is that the file private has at least shared ownership of y and z.
So, it's more of an ownership / lifetime problem. The file private data should either own y and z entirely or a corresponding reference count that is dropped in fops release().
I think both versions are popular in the kernel.. You can legimately treat y and z the same as device resources without creating a correctness problem and it is less code.
You can also do refcounts.
For instance at least in C you'd never argue that people should use refcount private data when they use a timer or irq subsystem. You'd use a simple sync cleanup and be done with it.
These bugs are coming because of mixing models, we have a bunch of sync APIs in the kernel that are easy to use and then you get these weird non-sync ones without any clear documentation :)
Jason