[RFT PATCH 03/15] drm/ingenic: Call drm_atomic_helper_shutdown() at shutdown time

[Doug Anderson]

Hi,

On Thu, Sep 14, 2023 at 1:14 AM Maxime Ripard <mripard at kernel.org> wrote:
>
> > > So it doesn't have any relationship with the unbind/remove timing, and
> > > for all we know it can be there indefinitely, while the application
> > > continues to interact with the driver.
> >
> > I spent some time thinking about similar issues recently and, assuming
> > my understanding is correct, I'd at least partially disagree.
> >
> > Specifically, I _think_ the only thing that's truly required to remain
> > valid until userspace closes the last open "fd" is the memory for the
> > "struct drm_device" itself, right? My understanding is that this is
> > similar to how "struct device" works. The memory backing a "struct
> > device" has to live until the last client releases a reference to it
> > even if everything else about a device has gone away. So if it was all
> > working perfectly then if the Linux driver backing the "struct
> > drm_device" goes away then we'd release resources and NULL out a bunch
> > of stuff in the "struct drm_device" but still keep the actual "struct
> > drm_device" around since userspace still has a reference. Pretty much
> > all userspace calls would fail, but at least they wouldn't crash. Is
> > that roughly the gist?
>
> Yes, but also, no.
>
> In the spirit, you're right. However, there's three things interfering
> here:
>
>   - You don't always have a match between device and KMS entity. Display
>     pipelines are usually multiple devices working together, and while
>     you probably have a 1:1 relationship with bridges and panels (and to
>     some extent encoders/connectors), the planes and framebuffers for
>     example are a mess :) So, if the device backing the planes is to be
>     removed, what are you removing exactly? All of the planes and
>     framebuffers? Do you free the buffers allocated by the userspace
>     (that it might still use?)?
>
>   - In addition to that, KMS doesn't deal with individual entities being
>     hotplugged so neither the subsystem nor the application expect to
>     have a connector being removed.
>
>   - ioctl's aren't filtered once the device is starting to get removed
>     on most drivers.
>
> So due to 1 and 2, we can't really partially remove components unless
> the application is aware of it, and it doesn't expect to. And most
> drivers still allow (probably unwillingly though) the application to
> call ioctls once the DRM device has lost at least one of its backing
> devices.

We "can't", but we "can", right? Userspace can freely unbind a driver.
Unless you want to dig into if the community would allow a driver to
block "unbind" then we have to, at the very least, not crash the
kernel when userspace does this. Ideally we'd have something more
elegant than just "don't crash the kernel", but at least we shouldn't
crash.


> > Assuming that's correct, then _most_ of the resource acquiring /
> > memory allocation can still happen in the device probe() routine and
> > can still use devm as long as we do something to ensure that any
> > resources released are no longer pointed to by anything in the "struct
> > drm_device".
> >
> > To make it concrete, I think we want this (feel free to correct). For
> > simplicity, I'm assuming a driver that _doesn't_ use the component
> > framework:
> >
> > a) Linux driver probe() happens. The "struct drm_device" is allocated
> > in probe() by devm_drm_dev_alloc(). This takes a reference to the
> > "struct drm_device". The device also acquires resources / allocates
> > memory.
>
> You need to differentiate resources and allocations there. Resources can
> be expected to go away at the same time than the device, so using devm
> is fine. Allocations are largely disconnected from the device lifetime,
> and using devm leads to UAF.

Right. I think my original point was looking at "ingenic-drm-drv.c".
Much of the "devm" stuff there is resources and those specific things
could be moved to probe() instead of bind(), right?

For allocations, I think you'd have to look at each allocation. If the
allocation needed to live as long as the "struct drm_device" then devm
is clearly the wrong choice. ...but not every allocation needs to live
that long. Also, even if in the "simple" case allocations need to live
as long as a "struct drm_device", it's possible that there are some
cases where there's only an indirect reference to the memory. In that
case, you could NULL out the indirect reference and then free it.
Obviously someone would need to take care here.


> > b) Userspace acquires a reference to the "struct drm_device". Refcount
> > is now 2 (one from userspace, one from the Linux driver).
> >
> > c) The Linux driver unbinds, presumably because userspace requested
> > it. From earlier I think we decided that we can't (by design) block
> > unbind. Once unbind happens then we shouldn't try to keep operating
> > the device
>
> That part is correct, because the resources aren't there anymore.
>
> > the driver should stop running.
>
> But for the reasons above, the driver needs to still operate (in a
> degraded mode).

So I think here is where the disconnect is from our viewpoints. IMO
when a Linux driver is unbound then it makes no sense to try to
operate the device in "a degraded mode". When a Linux driver is
unbound then it should be releasing all of the resources from the
device (iomaps, IRQs, regulators, GPIOs, etc). That's just what
unbinding a driver is supposed to do.

I understand what you're saying above about display pipelines being
multiple Linux drivers working together and that it doesn't make lots
of sense to just unbind a random Linux device driver in the middle of
things. ...and I don't really have a simple/great answer for how to do
something super elegant if userspace tries to just randomly unbind one
of the many drivers in an active display pipeline.


> > As part of the unbind, the remove() is called and also "devm"
> > resources are deallocated. If any of the things freed are pointed to
> > by the "struct drm_device" then the code needs to NULL them out at
> > this time.
>
> Right, we also need to make sure we don't access any of the resources
> that got freed. This is typically done by protecting all the accesses
> with drm_dev_enter/drm_dev_exit.
>
> > Also we should make sure that any callback functions that userspace
> > could cause to be invoked return errors.
>
> That would prevent any new ioctl from occuring after the device has been
> removed, but that doesn't fix the race condition if it's removed while
> there's a commit happening. This is further complicated by the fact that
> commits can be queued (so you would have multiple submitted already) or
> made asynchronous.

I guess I would have expected that the remove() callback in the device
would prevent new commits from starting and then block waiting until
any in-progress commits were finished? ...kinda like how drivers call
del_timer_sync() in their remove functions...


> > Our code could go away at any point here since userspace could "rmmod"
> > our module.
>
> Yeah, we probably have a bug there. Boris also reported something like
> that recently where if you add an action with drmm_add_action, and then
> remove the module, the function would have been free'd by the time it
> executes.

I'm fairly certain that you can prevent a module from being unloaded
by just grabbing a refcount to it. However, I'm not sure that's the
right solution. If we're trying to run driver code after a driver has
been unbound then, IMO, that's the bug.


> > However, it's not a panacea for everything. Specifically once
> > the Linux driver unbind finishes then the device isn't functional
> > anymore.
>
> What's wrong with it then?

I'm mostly just saying don't just search-and-replace "devm" with
"drmm" in your driver and call it done. You need to think carefully
about which things are which lifetime.

---

Ironically, while digging into this I'm tempted to take back my
original request. Despite the kernel docs I pointed at [1], it
actually looks like it might be fine to use "devm" within a
component's bind() function. In try_to_bring_up_aggregate_device() it
seems like the code is opening up a nested "devres" group specifically
to allow this to work. A little bit of testing that I did with this
shows that, indeed, the nesting seems to be working. Am I missing
something here?

[1] https://docs.kernel.org/driver-api/component.html