Re: [PATCH 2/2] drm: don't recycle used modeset IDs

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Aug 29, 2014 at 02:57:12PM +0200, David Herrmann wrote:
> Hi
> 
> On Fri, Aug 29, 2014 at 2:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Aug 29, 2014 at 02:01:01PM +0200, David Herrmann wrote:
> >> With MST, we now have connector hotplugging, yey! Pretty easy to use in
> >> user-space, but introduces some nasty races:
> >>  * If a connector is removed and added again while a compositor is in
> >>    background, it will get the same ID again. If the compositor wakes up,
> >>    it cannot know whether its the same connector or a new one, thus they
> >>    must re-read EDID information, etc.
> >>  * possible_clones, possible_crtcs, etc. depend on indices, not IDs. So if
> >>    an object is removed and a new one is added, those bitmasks are invalid
> >>    and must be refreshed. This currently does not affect connectors, but
> >>    only crtcs and encoders, but it's only a matter of time when this will
> >>    change.
> >>
> >> The easiest way to protect against this, is to not recylce modeset IDs.
> >> Instead, always allocate a new, higher ID. All ioctls that affect modeset
> >> objects, take IDs. Thus, during hotplug races, those ioctls will simply
> >> fail if invalid IDs are passed. They will no longer silently run on a
> >> newly hotplugged object.
> >>
> >> Furthermore, hotplug-races during state sync can now be easily detected. A
> >> call to GET_RESOURCES returns a list of available IDs atomically.
> >> User-space can now start fetching all those objects via GET_* ioctls. If
> >> any of those fails, they know that the given object was unplugged. Thus,
> >> the "possible_*" bit-fields are invalidated. User-space can now decide
> >> whether to restart the sync all over or wait for the 'change' uevent that
> >> is sent on modeset object modifications (or, well, is supposed to be sent
> >> and probably will be at some point).
> >>
> >> With this in place, modeset object hotplugging should work fine for all
> >> modeset objects in the KMS API.
> >>
> >> CC'ing stable so we can rely on all kernels with MST to not recycle IDs.
> >>
> >> Cc: <stable@vger.kernel.org> # 3.16+
> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> >
> > So userspace just needs to cycle through piles of framebuffer objects to
> > make bad things happen? Doesn't sound like a terribly solid plan.
> 
> IDs will still get recycled, but only once all IDs got used. So this is "safe".
> 
> Sure, user-space can create 2 billion framebuffers and destroy them
> again, thus causing the ID range to overflow and recycle old IDs. Not
> sure how fast you can run 2 billion syscalls.. If that's a real issue,
> I'd vote for using the high-range for user-space managed objects,
> low-range for kernel-space managed objects ([1...INT_MAX] and
> [INT_MAX+1...UINT_MAX] or so).
> 
> > I guess we could save this by doing normal id allocations for fbs and
> > monotonically increasing allocations for all other objects.
> 
> This doesn't work. A connector with ID #n might get unplugged and
> another process created a new FB, which will then get ID #n. Sure, I
> doubt there's a real conflict as ioctls check the type, but it still
> sounds really ugly to me.

Oh, ugly for sure, but it should work since userspace doesn't really have
any reason to mix up ids and the kernel does need to check the type
anyway. And with a simple

	if (type == OBJ_FB)
		idr_alloc
	else
		idr_alloc_cyclic

I think it should work out with minimal fuss. At least the full separation
of id spaces wouldn't be any more or less complexity.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch