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

[Daniel Vetter <daniel@ffwll.ch>]

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.

I guess we could save this by doing normal id allocations for fbs and
monotonically increasing allocations for all other objects.
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 97eba56..ab0fe6d 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -286,7 +286,7 @@ static int drm_mode_object_get_reg(struct drm_device *dev,
>  	idr_preload(GFP_KERNEL);
>  	spin_lock_irq(&dev->mode_config.idr_lock);
>  
> -	ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_NOWAIT);
> +	ret = idr_alloc_cyclic(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_NOWAIT);
>  	if (ret >= 0) {
>  		/*
>  		 * Set up the object linking under the protection of the idr
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch