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
