[Intel-gfx] [PATCH] drm/i915/fbdev: Serialise early hotplug events with async fbdev config

[Chris Wilson]

Quoting Lukas Wunner (2017-11-25 21:03:35)
> On Sat, Nov 25, 2017 at 07:41:55PM +0000, Chris Wilson wrote:
> > As both the hotplug event and fbdev configuration run asynchronously, it
> > is possible for them to run concurrently. If configuration fails, we were
> > freeing the fbdev causing a use-after-free in the hotplug event.
> 
> That'll teach me to muck around in this complicated driver. :-)
> 
> IIUC, the issue is that ifbdev is briefly non-NULL and the if clause
> happens to be executed when it's non-NULL and it becomes NULL upon
> or during execution of intel_fbdev_output_poll_changed(), is that
> correct?

Yes.
 
> Wouldn't the proper solution be to set ifbdev only after configuration
> was successful, i.e. somewhere at the end of intelfb_create()?
> With a memory barrier in case intel_fbdev_output_poll_changed is running
> on a different CPU?

I was looking at if we could easily do that; trying to look at various
methods we could defer the calls back to fbdev until after it was
registered (because that is the crux of the issue imo). I didn't see
anything as easy as using the existing one-off synchronisation.

> > In order to keep the dev_priv->ifbdev alive after failure, we have to
> > avoid the free and leave it empty until we unload the module.
> 
> Well, that seems to defeat the goal stated in the commit message of
> 366e39b4d2c5 to free up the memory if fbdev initialization failed.
> Not that it's a big deal for me personally, just noting. :-)

I know, but it should at least be a small bit of unused allocation, and
we already expect to pay the cost of that allocation for normal use
cases. It may be the straw that breaks the camel's back, hopefully no
one notices in the calamity. What we might do then is just pull the
struct into dev_priv under ifdef FBDEV.
-Chris