cdev/devm_* issues (was Re: [TECH TOPIC] Rust for Linux)

[Greg KH <greg@kroah.com>]

On Wed, Jul 07, 2021 at 08:17:08PM +0300, Laurent Pinchart wrote:
> On Wed, Jul 07, 2021 at 02:45:04PM +0200, Greg KH wrote:
> > On Wed, Jul 07, 2021 at 01:38:44PM +0100, James Bottomley wrote:
> > > On Wed, 2021-07-07 at 14:20 +0200, Greg KH wrote:
> > > > On Wed, Jul 07, 2021 at 12:34:31PM +0100, James Bottomley wrote:
> > > > > On Wed, 2021-07-07 at 11:50 +0100, Mark Brown wrote:
> > > > > > On Tue, Jul 06, 2021 at 10:36:14PM +0200, Linus Walleij wrote:
> > > > > > > On Tue, Jul 6, 2021 at 10:00 PM Roland Dreier wrote:
> > > > > > > > "devres" / devm_xxx was an attempt to deal with this in C,
> > > > > > > > but it only solves some cases of this and has not received a
> > > > > > > > lot of adoption (we can argue about the reasons).
> > > > > > >  
> > > > > > > Really? From my point of view that is adopted all over the map.
> > > > > > > I add new users all the time and use it as much as I can when
> > > > > > > writing new drivers.
> > > > > > 
> > > > > > Yes, it's *super* widely used in most of the kernel.  Perhaps
> > > > > > there's some subsystems that reject it for some reason.
> > > > > > 
> > > > > > > I think it's a formidable success, people just need to learn to
> > > > > > > do it more.
> > > > > > 
> > > > > > There *are* issues with people adopting it too enthusiastically -
> > > > > > as well as the memory lifetime issues that Laurent mentioned it's
> > > > > > easy for it to cause problems with interrupt handlers that are
> > > > > > left live longer than they should be and try to use things that
> > > > > > were already deallocated.
> 
> (CC'ing Daniel Vetter as the author of the DRM devres-based resource
> management)
> 
> I've given this lots of thoughts lately, in the context of V4L2, but it
> should be roughly the same for character devices in general. Here's what
> I think should be done for drivers that expose character devices.
> 
> - Drivers must stop allocating their internal data structure (the one
>   they set as device data with dev_set_drvdata()) with devm_kzalloc().
>   The data structure must instead be allocated with a plain kzalloc()
>   and reference-counted.
> 
>   Most drivers will register a single character device using a
>   subsystem-specific API (e.g. video_register_device() in V4L2). The
>   subsystem needs to provide a .release() callback, called when the
>   last reference to the character device is released. Drivers must
>   implement this, and can simply free their internal data structure at
>   this point.
> 
>   For drivers that register multiple character devices, or in general
>   expose multiple interfaces to userspace or other parts of the kernel,
>   the internal data structure must be properly reference-counted, with a
>   reference released in each .release() callback. There may be ways to
>   simplify this.
> 
>   This can be seen as going back to the pre-devm_kzalloc() era, but it's
>   only about undoing a mistake that was done way too often (to be fair,
>   many drivers used to just kfree() the data in the driver's .remove()
>   operation, so it wasn't always a regression, only enshrining a
>   preexisting bad practice).
> 
>   This is only part of the puzzle though. There's a remove/use race that
>   still needs to be solved.
> 
> - In .remove(), drivers must inform the character device that new access
>   from userspace are not allowed anymore. This would set a flag in
>   struct cdev that would result in all new calls from userspace through
>   file operations to be rejected (with -ENXIO for instance). This should
>   be wrapped in subsystem-specific functions (e.g.
>   video_device_disconnect() wrapping cdev_disconnect()). From now on, no
>   new calls are possible, but existing calls may be in progress.
> 
> - Drivers then need to cancel all pending I/O and wait for completion.
>   I/O (either direct memory-mapped I/O or through bus APIs, such as I2C
>   or USB transactions) are not allowed anymore after .remove() returns.
>   This will have the side effect of waking up userspace contexts that
>   are waiting for I/O completion (through a blocking file I/O operation
>   for instance). Existing calls from userspace will start completing.
> 
>   This is also a good place to start shutting down the device, for
>   instance disabling interrupts.
> 
> - The next step is for drivers to wait until all calls from userspace
>   complete. This should be done with a call count in struct cdev that is
>   updated upon entry and exit of calls from userspace, and a wait queue
>   to wait for that count to go to 0. This should be wrapped in
>   subsystem-specific APIs. As the flag that indicates device removal is
>   set, no new calls are allowed so the counter can only decrease, and as
>   all pending I/O have terminated or have been cancelled, no pending
>   calls should be blocked.
> 
> - Finally, drivers should unregister the character device, through the
>   appropriate subsystem API.
> 
>   At this point, memory mappings and file handles referencing the device
>   may still exist, but no file operation is in progress. The device is
>   quiescent.
> 
>   Care needs to be taken in drivers and subsystems to not start any I/O
>   operation when handling the file .release() operation or the
>   destruction of memory mappings. Overall I don't expect much issues,
>   but I'm sure some drivers do strange things in those code paths.
> 
> - When the least memory mapping is gone and the last file handle is
>   closed, the subsystem will call the driver's .release() callback. At
>   this point, the driver will perform the operations listed in the first
>   item of this list.
> 
> 
> The challenge here will be to make this as easy as possible for drivers,
> to avoid risk of drivers getting it wrong. The DRM/KMS subsystem has
> created a devres-based system to handle this, with the devres associated
> with the drm_device (the abstraction of the cdevs exposed by DRM to
> userspace), *not* the physical device. It has a drawback though, it
> assumes that a DRM driver will only ever want to register a drm_device
> and nothing else, and hardcodes that assumption in the way it releases
> resources. That's fine for most DRM drivers, but if a driver was to
> register a drm_device and something else (such as a V4L2 video_device,
> an input device, ...), the DRM subsystem will get in the way.
> 
> I have two questions:
> 
> - Does the above make sense ?

Yes, totally, thank you for taking the time to write this all out.  It's
been in the back of my mind for over a decade that we need to work on
these issues, but have not had any time to sit down and write it all
down like you have.

> - Assuming it does, how do we get from the current mess to a situation
>   where writing a driver will be a pleasure, not a punishment ? :-)

That's the real question.  Thanks to a lot of cleanups that Christoph
has recently done to the lower-level cdev code, the lower levels are now
in a shape where we can work on them better.  I'm going to try to carve
out some time in the next few months to start to work on these things.
I think that once we get the ideas of what needs to be done, and a
working core change, I can unleash some interns on doing tree-wide
cleanups/changes to help bring everything into alignment.

I'm going to save this email off for reference for later.  But of
course, if others want to start to attack this earlier, all the better :)

thanks,

greg k-h



> 
> > > > > Fixing this should not be beyond the wit of humankind, though.  If
> > > > > we delayed deallocation to module release, that would fix the
> > > > > interrupt issue, wouldn't it?  Perhaps all devres memory for
> > > > > devices should live until then anyway and thus be automatically
> > > > > deallocated instead of needing an explicit free ... the problem
> > > > > with that being compiled in devices currently optimize away the
> > > > > module refcounting, but that should be fixable.
> > > > 
> > > > module code lifespans are different than device structure lifespans,
> > > > it's when people get them confused, as here, that we have problems.
> > > 
> > > I'm not claiming they are.  What I am claiming is that module lifetime
> > > must always encompass the device lifetimes.  Therefore, you can never
> > > be incorrect by using a module lifetime for anything attached to a
> > > device, just inefficient for using memory longer than potentially
> > > needed.  However, in a lot of use cases, the device is created on
> > > module init and destroyed on module exit, so the inefficiency is barely
> > > noticeable.
> > 
> > In almost no use case is the device created on module init of a driver.
> > devices are created by busses, look at the USB code as an example.  The
> > usb bus creates the devices and then individual modules bind to that
> > device as needed.  If the device is removed from the system, wonderful,
> > the device is unbound, but the module is still loaded.  So if you really
> > wanted to, with your change, just do a insert/remove of a USB device a
> > few zillion times and then memory is all gone :(
> > 
> > > The question I'm asking is shouldn't we optimize for this?
> > 
> > No.
> > 
> > > so let people allocate devm memory safe in the knowledge it will be
> > > freed on module release?
> > 
> > No, see above.  Modules are never removed in a normal system.  devices
> > are.
> > 
> > And the drm developers have done great work in unwinding some of these
> > types of mistakes in their drivers, let's not go backwards please.
> 
> -- 
> Regards,
> 
> Laurent Pinchart