connector/edid probing races

[Daniel Vetter]

On Fri, Mar 10, 2017 at 01:13:12PM -0500, Sean Paul wrote:
> On Fri, Mar 10, 2017 at 05:52:34PM +0100, Daniel Vetter wrote:
> > On Fri, Mar 10, 2017 at 11:01:54AM -0500, Sean Paul wrote:
> > > On Fri, Mar 10, 2017 at 02:12:14PM +1000, Dave Airlie wrote:
> > > > Talk to Jonas (jadahl) on irc, he had 3 mutters running and on hotplug
> > > > all 3 of them were diving into the connector getting code. Now I think
> > > > we can avoid this in userspace by not probing when not owning the VT,
> > > > but it's still messy behaviour.
> > > > 
> > > > It looks like one thread does a getconnector, this fills out the EDID
> > > > blob with a specific blob id,
> > > > then second thread does getconnector, which replaces the EDID blob
> > > > with another one
> > > > the first thread calls get property blob, but the blob id it is
> > > > looking for is destroyed.
> > > > 
> > > > Ideas?
> > > 
> > > I think you could probably fix this by introducing a new
> > > drm_property_overwrite_global_blob() function. If the size of the current blob is
> > > the same as the new blob, you could just overwrite the data and preserve the id
> > > and references. You could then replace the drm_property_replace_global_blob() call in
> > > drm_mode_connector_update_edid_property() to call the new function. Of course,
> > > you would need to fall back on *replace_global_blob() if size differed, but it
> > > would probably solve the issue above.
> > 
> > blobs are imutable (not that userspace can use that much since we can
> > reuse ids), 
> 
> Pedantic distinction: blobs themselves aren't inherantly immutable. However, the
> EDID property is defined as immutable. I suppose there could conceivably be some
> userspace somewhere that relies on this. Bummer.

It's more tricky: The property is labelled as IMMUTABLE, but that's a lie,
it essentially only means that userspace isn't allowed to change it. The
kernel is.

But blobs themselves are immutable, this is the same semantics as with
framebuffer objects, and we carried that over when we started to allow
userspace to create blobs (for atomic). The only benefit is for drivers,
as long as you hold a life reference you can just compare pointers and if
they're equal, you know nothing changed. Due to id reuse, userspace
unfortunately can't do this (and yes we've had clever userspace who got
this wrong).

> > I think if we do this we should compare the contents of the
> > blob.
> > 
> > > That said, I'm not convinced it should be kernel's responsibility to really
> > > solve this problem. How much effort should we put into mitigating the effects of
> > > a racey and wasteful userspace?
> > 
> > It's a tradeoff between "why should the kernel care" and "every compositor
> > will get this wrong". Maybe something like libweston will fix this
> > eventually ....
> 
> So the question is whether it's worth it to penalize well-behaved compositors
> with superfluous memcmp on EDID fetches to make up for the ill-behaved ones. I
> don't think it is, but perhaps there are other benefits to preserving the single
> blob for the lifetime of the connection.

If you can profile that memcmp in a 25ms edid read, I'll buy this argument
:-) I think the better argument against is that there's still cases where
userspace must recover from similar issues (getconnector racing with a hpd
which can also update the edid), but those are even rarer. It might be
good to keep the easier-to-hit races open, to force better error handling
in userspace.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch