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

Laurent Pinchart
2021-07-08T08:24:18Z

Hi Greg,

On Thu, Jul 08, 2021 at 08:49:39AM +0200, Greg KH wrote:
> 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 :)

My too long todo list contains an entry to address this in the V4L2
subsystem to start with (see
https://lore.kernel.org/linux-media/20200816003315.GA13826@roeck-us.net/).
It may be a good prototype to then extend it to cdev. If I get a chance
to work on it, I'll CC you.

> > > > > > 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