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

[Rob Clark]

On Fri, Aug 29, 2014 at 9:10 AM, David Herrmann <dh.herrmann at gmail.com> wrote:
> Hi
>
> On Fri, Aug 29, 2014 at 2:57 PM, David Herrmann <dh.herrmann at gmail.com> wrote:
>> Hi
>>
>> On Fri, Aug 29, 2014 at 2:51 PM, Daniel Vetter <daniel at 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 at vger.kernel.org> # 3.16+
>>>> Signed-off-by: David Herrmann <dh.herrmann at 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.
>
> On a second thought: maybe your idea isn't as bad as I thought. I
> mean, everyone must do type-checking when looking up mode-objects, so
> it seems safe to rely on that.

note that for atomic there are cases where we do lookup w/ ANY_TYPE..
*but* the idea might still work since FB's are still handled
specially(ish) do to refcnting..

BR,
-R


> Thanks
> David
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel