Hi Liviu, On 10/12/22 17:07, Liviu Dudau wrote:
Hi Danilo, Appologies again for the delay in reviewing this as I was at XDC last week.
No worries, thanks for following up.
Looking at the documentation for drm_dev_unplug, you can get a hint about what is going on: /* * [....] There is one * shortcoming however, drm_dev_unplug() marks the drm_device as unplugged before * drm_atomic_helper_shutdown() is called. This means that if the disable code * paths are protected, they will not run on regular driver module unload, * possibly leaving the hardware enabled. */
Yes, that's the issue we have and pretty unfortunate. What we'd want for platform device drivers is to still be able to enter the sections locked with drm_dev_{enter,exit} on driver unbind, which we can't for at the moment.
I discussed this with Daniel Vetter on #dri-devel and for now he suggests to just not lock access to platform device bound resources and respin the patchset removing those parts.
Besides that I'll also work on a solution for drm_dev_unplug() / drm_dev_{enter,exit} to overcome this issue in the future.
I'm finally getting to a conclusion: I'm still not sure what problem you were trying to solve when you have started this series and if you have found a scenario where you've got use after free then I would like to be able to reproduce it on my setup. Otherwise, I think this whole series introduces a regression on the behaviour of the driver and I would be inclined to reject it.
The problem is that DRM modeset objects should never be allocated with devm_*, since this can result in use-after free issues on driver unload. They should be freed when the last reference to the object is dropped, which DRM managed APIs take care of. As a consequence, DRM managed objects can potentially out-live platform device bound resources, which then should be protected from access. The first at least we can and should do.
It's not an issue that's unique to hdlcd, it's just a known issue to be addressed by all drivers. There's still the shortcoming concerning protecting platform bound resources as discussed above, but "the drmm parts should be a good idea no matter what" to cite Daniel.
I'll send a v4 without the drm_dev_{enter,exit} parts removed if that's fine for you.
- Danilo
Best regards, Liviu
