Message-ID: <20170402131926.gejo4wnicwwekgqk@phenom.ffwll.local>
Date:   Sun, 2 Apr 2017 15:19:26 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Keith Packard <keithp@...thp.com>
Cc:     linux-kernel@...r.kernel.org, Dave Airlie <airlied@...hat.com>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 3/4] drm: Check mode object lease status in all master
 ioctl paths

On Sat, Apr 01, 2017 at 10:08:40AM -0700, Keith Packard wrote:
> Attempts to modify un-leased objects are rejected with an error.
> Information returned about unleased objects is modified to make them
> appear unusable and/or disconnected.
> 
> Signed-off-by: Keith Packard <keithp@...thp.com>

Just a merge-technical idea to make this easier to change and less painful
to rebase until we've locked down the semantics properly. And also because
I expect we'll need v2 of this uapi soon or later anyway:

I think it'd be good if we could consolidate all the lease checking into
drm_mode_object_find (respectively __drm_mode_object_find). We'd need to
wire up the fpriv to be able to do that, but we could upstream that patch
right away before anything else. That should take care of most of the
checks in this patch here.

There's a few things on top:
- filtering the various bitmasks. I think you have most, but we could
  perhaps upstream the helpers for these.
- filtering object lists (essentially getresources and getplanes ioctls).
- filtering implicit objects in the legacy ioctl. E.g. page_flip done
  through atomic doesn't just need the CRTC id, but also the id of the
  primary plane plus of the FB_ID atomic property. Similarly for all the
  other legacy ioctls. I think we want to make sure there's no difference
  here in behaviour.

Especially for the last one it might be simplest to outright disallow all
legacy ioctl and require that sub-drm_master nodes only get access to the
read-only GET* ioctl (they get that anyway, even when they're not the
current master), plus atomic. Makes it a _lot_ easier to implement.
Downside is that amdgpu _really_ needs to land atomic asap :-)

> ---
>  drivers/gpu/drm/drm_atomic.c      |  3 +++
>  drivers/gpu/drm/drm_auth.c        |  2 +-
>  drivers/gpu/drm/drm_color_mgmt.c  |  3 +++
>  drivers/gpu/drm/drm_connector.c   | 52 ++++++++++++++++++++++++++-------------
>  drivers/gpu/drm/drm_crtc.c        | 15 ++++++++---
>  drivers/gpu/drm/drm_encoder.c     | 18 +++++++++++---
>  drivers/gpu/drm/drm_mode_object.c |  3 +++
>  drivers/gpu/drm/drm_plane.c       | 36 +++++++++++++++++++++++----
>  include/drm/drm_lease.h           |  4 +++
>  include/drm/drm_mode_object.h     |  1 +
>  10 files changed, 108 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index fdfb1ec17e66..a3cb95f7f1c1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -2134,6 +2134,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			goto out;
>  		}
>  
> +		if ((ret = drm_lease_check(file_priv->master, obj->id)) < 0)
> +			goto out;
> +
>  		if (!obj->properties) {
>  			drm_mode_object_unreference(obj);
>  			ret = -ENOENT;
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 1db4f63860d1..44c99d12f4c1 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -303,7 +303,7 @@ void drm_master_release(struct drm_file *file_priv)
>   */
>  bool drm_is_current_master(struct drm_file *fpriv)
>  {
> -	return fpriv->is_master && fpriv->master == fpriv->minor->dev->master;
> +	return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;

This feels like it should be part of the earlier patch to make
sub-drm_master objects for leasing. In that patch we should also make sure
that all the current docs about drm_master are updated correctly (but
maybe only later on, when we get towards merging this). This change here
is pretty important wrt set/drop_master ioctl behaviour and leases.

Cheers, Daniel

>  }
>  EXPORT_SYMBOL(drm_is_current_master);
>  
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 6543ebde501a..f8d7a499cf95 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -206,6 +206,9 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
>  		goto out;
>  	}
>  
> +	if ((ret = drm_lease_check(file_priv->master, crtc->base.id)) < 0)
> +		goto out;
> +
>  	if (crtc->funcs->gamma_set == NULL) {
>  		ret = -ENOSYS;
>  		goto out;
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 7a7019ac9388..a95db57dd966 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1079,6 +1079,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	struct drm_mode_modeinfo u_mode;
>  	struct drm_mode_modeinfo __user *mode_ptr;
>  	uint32_t __user *encoder_ptr;
> +	bool leased;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -1093,9 +1094,16 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  		goto out_unlock;
>  	}
>  
> -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
> -		if (connector->encoder_ids[i] != 0)
> -			encoders_count++;
> +	leased = drm_lease_check(file_priv->master, connector->base.id) == 0;
> +
> +	DRM_DEBUG_LEASE("connector %d leased %s\n", connector->base.id, leased ? "true" : "false");
> +
> +	if (leased) {
> +		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
> +			if (connector->encoder_ids[i] != 0 &&
> +			    drm_lease_check(file_priv->master, connector->encoder_ids[i]) == 0)
> +				encoders_count++;
> +	}
>  
>  	if (out_resp->count_modes == 0) {
>  		connector->funcs->fill_modes(connector,
> @@ -1104,21 +1112,29 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	}
>  
>  	/* delayed so we get modes regardless of pre-fill_modes state */
> -	list_for_each_entry(mode, &connector->modes, head)
> -		if (drm_mode_expose_to_userspace(mode, file_priv))
> -			mode_count++;
> +	if (leased)
> +		list_for_each_entry(mode, &connector->modes, head)
> +			if (drm_mode_expose_to_userspace(mode, file_priv))
> +				mode_count++;
>  
>  	out_resp->connector_id = connector->base.id;
>  	out_resp->connector_type = connector->connector_type;
>  	out_resp->connector_type_id = connector->connector_type_id;
> -	out_resp->mm_width = connector->display_info.width_mm;
> -	out_resp->mm_height = connector->display_info.height_mm;
> -	out_resp->subpixel = connector->display_info.subpixel_order;
> -	out_resp->connection = connector->status;
> +	if (leased) {
> +		out_resp->mm_width = connector->display_info.width_mm;
> +		out_resp->mm_height = connector->display_info.height_mm;
> +		out_resp->subpixel = connector->display_info.subpixel_order;
> +		out_resp->connection = connector->status;
> +	} else {
> +		out_resp->mm_width = 0;
> +		out_resp->mm_height = 0;
> +		out_resp->subpixel = 0;
> +		out_resp->connection = connector_status_disconnected;
> +	}
>  
>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  	encoder = drm_connector_get_encoder(connector);
> -	if (encoder)
> +	if (encoder && leased)
>  		out_resp->encoder_id = encoder->base.id;
>  	else
>  		out_resp->encoder_id = 0;
> @@ -1145,12 +1161,14 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	}
>  	out_resp->count_modes = mode_count;
>  
> -	ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic,
> -			(uint32_t __user *)(unsigned long)(out_resp->props_ptr),
> -			(uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr),
> -			&out_resp->count_props);
> -	if (ret)
> -		goto out;
> +	if (leased) {
> +		ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic,
> +						     (uint32_t __user *)(unsigned long)(out_resp->props_ptr),
> +						     (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr),
> +						     &out_resp->count_props);
> +		if (ret)
> +			goto out;
> +	}
>  
>  	if ((out_resp->count_encoders >= encoders_count) && encoders_count) {
>  		copied = 0;
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e75f62cd8a65..95026ca74568 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -347,6 +347,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  {
>  	struct drm_mode_crtc *crtc_resp = data;
>  	struct drm_crtc *crtc;
> +	bool leased;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -355,9 +356,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  	if (!crtc)
>  		return -ENOENT;
>  
> +	leased = drm_lease_check(file_priv->master, crtc->base.id) == 0;
> +
> +	DRM_DEBUG_LEASE("crtc %d leased %s\n", crtc->base.id, leased ? "true" : "false");
> +
>  	drm_modeset_lock_crtc(crtc, crtc->primary);
>  	crtc_resp->gamma_size = crtc->gamma_size;
> -	if (crtc->primary->fb)
> +	if (crtc->primary->fb && leased)
>  		crtc_resp->fb_id = crtc->primary->fb->base.id;
>  	else
>  		crtc_resp->fb_id = 0;
> @@ -365,7 +370,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  	if (crtc->state) {
>  		crtc_resp->x = crtc->primary->state->src_x >> 16;
>  		crtc_resp->y = crtc->primary->state->src_y >> 16;
> -		if (crtc->state->enable) {
> +		if (crtc->state->enable && leased) {
>  			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
>  			crtc_resp->mode_valid = 1;
>  
> @@ -375,7 +380,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  	} else {
>  		crtc_resp->x = crtc->x;
>  		crtc_resp->y = crtc->y;
> -		if (crtc->enabled) {
> +		if (crtc->enabled && leased) {
>  			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
>  			crtc_resp->mode_valid = 1;
>  
> @@ -529,6 +534,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	}
>  	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
>  
> +	if ((ret = drm_lease_check(file_priv->master, crtc->base.id)) < 0) {
> +		DRM_DEBUG_KMS("CRTC lease not held\n");
> +		goto out;
> +	}
>  	if (crtc_req->mode_valid) {
>  		/* If we have a mode we need a framebuffer. */
>  		/* If we pass -1, set the mode with the currently bound fb */
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index 992879f15f23..24d03e13f522 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -201,6 +201,7 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
>  	struct drm_mode_get_encoder *enc_resp = data;
>  	struct drm_encoder *encoder;
>  	struct drm_crtc *crtc;
> +	bool leased;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -209,9 +210,13 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
>  	if (!encoder)
>  		return -ENOENT;
>  
> +	leased = drm_lease_check(file_priv->master, encoder->base.id) == 0;
> +
> +	DRM_DEBUG_LEASE("encoder %d leased %s\n", encoder->base.id, leased ? "true" : "false");
> +
>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  	crtc = drm_encoder_get_crtc(encoder);
> -	if (crtc)
> +	if (crtc && leased)
>  		enc_resp->crtc_id = crtc->base.id;
>  	else
>  		enc_resp->crtc_id = 0;
> @@ -219,8 +224,15 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
>  
>  	enc_resp->encoder_type = encoder->encoder_type;
>  	enc_resp->encoder_id = encoder->base.id;
> -	enc_resp->possible_crtcs = encoder->possible_crtcs;
> -	enc_resp->possible_clones = encoder->possible_clones;
> +	if (leased) {
> +		enc_resp->possible_crtcs = drm_lease_filter_crtcs(file_priv->master,
> +								  encoder->possible_crtcs);
> +		enc_resp->possible_clones = drm_lease_filter_encoders(file_priv->master,
> +								      encoder->possible_clones);
> +	} else {
> +		enc_resp->possible_crtcs = 0;
> +		enc_resp->possible_clones = 0;
> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index 9f17085b1fdd..9f8559d82a17 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -404,6 +404,9 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> +	if ((ret = drm_lease_check(file_priv->master, arg->obj_id)) != 0)
> +		goto out;
> +
>  	if (!arg_obj->properties)
>  		goto out_unref;
>  
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 62b98f386fd1..df811869c1dd 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -383,6 +383,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
>  	struct drm_mode_get_plane *plane_resp = data;
>  	struct drm_plane *plane;
>  	uint32_t __user *format_ptr;
> +	bool leased;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -391,27 +392,34 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
>  	if (!plane)
>  		return -ENOENT;
>  
> +	leased = drm_lease_check(file_priv->master, plane->base.id) == 0;
> +
>  	drm_modeset_lock(&plane->mutex, NULL);
> -	if (plane->crtc)
> +	if (plane->crtc && leased)
>  		plane_resp->crtc_id = plane->crtc->base.id;
>  	else
>  		plane_resp->crtc_id = 0;
>  
> -	if (plane->fb)
> +	if (plane->fb && leased)
>  		plane_resp->fb_id = plane->fb->base.id;
>  	else
>  		plane_resp->fb_id = 0;
>  	drm_modeset_unlock(&plane->mutex);
>  
>  	plane_resp->plane_id = plane->base.id;
> -	plane_resp->possible_crtcs = plane->possible_crtcs;
> +	if (leased)
> +		plane_resp->possible_crtcs = drm_lease_filter_crtcs(file_priv->master,
> +								    plane->possible_crtcs);
> +	else
> +		plane_resp->possible_crtcs = 0;
> +
>  	plane_resp->gamma_size = 0;
>  
>  	/*
>  	 * This ioctl is called twice, once to determine how much space is
>  	 * needed, and the 2nd time to fill it.
>  	 */
> -	if (plane->format_count &&
> +	if (plane->format_count && leased &&
>  	    (plane_resp->count_format_types >= plane->format_count)) {
>  		format_ptr = (uint32_t __user *)(unsigned long)plane_resp->format_type_ptr;
>  		if (copy_to_user(format_ptr,
> @@ -420,7 +428,10 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
>  			return -EFAULT;
>  		}
>  	}
> -	plane_resp->count_format_types = plane->format_count;
> +	if (leased)
> +		plane_resp->count_format_types = plane->format_count;
> +	else
> +		plane_resp->count_format_types = 0;
>  
>  	return 0;
>  }
> @@ -551,6 +562,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	struct drm_plane *plane;
>  	struct drm_crtc *crtc = NULL;
>  	struct drm_framebuffer *fb = NULL;
> +	int ret;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -566,6 +578,12 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  		return -ENOENT;
>  	}
>  
> +	if ((ret = drm_lease_check(file_priv->master, plane->base.id)) < 0) {
> +		DRM_DEBUG_KMS("Plane lease not held: %d error %d\n",
> +			      plane->base.id, ret);
> +		return ret;
> +	}
> +
>  	if (plane_req->fb_id) {
>  		fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
>  		if (!fb) {
> @@ -687,6 +705,11 @@ static int drm_mode_cursor_common(struct drm_device *dev,
>  		DRM_DEBUG_KMS("Unknown CRTC ID %d\n", req->crtc_id);
>  		return -ENOENT;
>  	}
> +	if ((ret = drm_lease_check(file_priv->master, crtc->base.id)) < 0) {
> +		DRM_DEBUG_KMS("CRTC lease not held %d error %d\n",
> +			      crtc->base.id, ret);
> +		goto out;
> +	}
>  
>  	/*
>  	 * If this crtc has a universal cursor plane, call that plane's update
> @@ -785,6 +808,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (!crtc)
>  		return -ENOENT;
>  
> +	if ((ret = drm_lease_check(file_priv->master, crtc->base.id)) < 0)
> +		return ret;
> +
>  	if (crtc->funcs->page_flip_target) {
>  		u32 current_vblank;
>  		int r;
> diff --git a/include/drm/drm_lease.h b/include/drm/drm_lease.h
> index e02adf3e42fd..8f91fc4226e3 100644
> --- a/include/drm/drm_lease.h
> +++ b/include/drm/drm_lease.h
> @@ -48,4 +48,8 @@ static inline int drm_lease_check(struct drm_master *master, int id) {
>  	return 0;
>  }
>  
> +uint32_t drm_lease_filter_crtcs(struct drm_master *master, uint32_t crtcs);
> +
> +uint32_t drm_lease_filter_encoders(struct drm_master *master, uint32_t encoders);
> +
>  #endif /* _DRM_LEASE_H_ */
> diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
> index 43460b21d112..07830182598b 100644
> --- a/include/drm/drm_mode_object.h
> +++ b/include/drm/drm_mode_object.h
> @@ -24,6 +24,7 @@
>  #define __DRM_MODESET_H__
>  
>  #include <linux/kref.h>
> +#include <drm/drm_lease.h>
>  struct drm_object_properties;
>  struct drm_property;
>  struct drm_device;
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists