[resend with the right mailing lists]
Hi all,
Nothing too major, only things I did find in all my igt test extending for drm lease is some corner cases around implicit planes and atomic target crtcs. Review and comments very much appreciated.
Cheers, Daniel
Test-with: 20190228141918.26043-1-daniel.vetter@ffwll.ch
Daniel Vetter (7): drm/leases: Drop object_id validation for negative ids drm/lease: Drop recursive leads checks drm/leases: Don't init to 0 in drm_master_create drm/lease: Check for lessor outside of locks drm/lease: Make sure implicit planes are leased drm/atomic: Wire file_priv through for property changes drm/atomic: -EACCESS for lease-denied crtc lookup
drivers/gpu/drm/drm_atomic_uapi.c | 36 +++++++++++++++++++++++------------- drivers/gpu/drm/drm_auth.c | 2 -- drivers/gpu/drm/drm_crtc.c | 4 ++++ drivers/gpu/drm/drm_crtc_internal.h | 1 + drivers/gpu/drm/drm_lease.c | 13 +++---------- drivers/gpu/drm/drm_mode_object.c | 5 +++-- drivers/gpu/drm/drm_plane.c | 8 ++++++++ 7 files changed, 42 insertions(+), 27 deletions(-)
Not exactly sure what's the aim here, but the canonical nil object has id == 0, we don't use negative object ids for anything. Plus all object_id are valided by the object_idr, there's nothing we need to do on top of that ENOENT check a bit further down.
Spotted while typing exhaustive igt coverage for all these corner-cases.
Cc: Keith Packard keithp@keithp.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_lease.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index 603b0bd9c5ce..1176d814cf7f 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -403,11 +403,6 @@ static int fill_object_idr(struct drm_device *dev, /* step one - get references to all the mode objects and check for validity. */ for (o = 0; o < object_count; o++) { - if ((int) object_ids[o] < 0) { - ret = -EINVAL; - goto out_free_objects; - } - objects[o] = drm_mode_object_find(dev, lessor_priv, object_ids[o], DRM_MODE_OBJECT_ANY);
We disallow subleasing, so no point checking whether the master holds all the leases - it will.
Spotted while typing exhaustive igt coverage for all these corner cases.
Cc: Keith Packard keithp@keithp.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_lease.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index 1176d814cf7f..cce5d9dd52ff 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -220,8 +220,6 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr error = 0; if (!idr_find(&dev->mode_config.object_idr, object)) error = -ENOENT; - else if (!_drm_lease_held_master(lessor, object)) - error = -EACCES; else if (_drm_has_leased(lessor, object)) error = -EBUSY;
We kzalloc.
Cc: Keith Packard keithp@keithp.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_auth.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index 1669c42c40ed..bcf0a5a1018f 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -109,8 +109,6 @@ struct drm_master *drm_master_create(struct drm_device *dev) master->dev = dev;
/* initialize the tree of output resource lessees */ - master->lessor = NULL; - master->lessee_id = 0; INIT_LIST_HEAD(&master->lessees); INIT_LIST_HEAD(&master->lessee_list); idr_init(&master->leases);
Daniel Vetter daniel.vetter@ffwll.ch writes:
We kzalloc.
Cc: Keith Packard keithp@keithp.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
Reviewed-by: Mika Kuoppala mika.kuoppala@linux.intel.com
drivers/gpu/drm/drm_auth.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index 1669c42c40ed..bcf0a5a1018f 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -109,8 +109,6 @@ struct drm_master *drm_master_create(struct drm_device *dev) master->dev = dev;
/* initialize the tree of output resource lessees */
- master->lessor = NULL;
- master->lessee_id = 0; INIT_LIST_HEAD(&master->lessees); INIT_LIST_HEAD(&master->lessee_list); idr_init(&master->leases);
-- 2.14.4
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
The lessor is invariant over a lifetime of a lease, we don't have to grab any locks for that. Speeds up the common case of not being a lease.
Cc: Keith Packard keithp@keithp.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_lease.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index cce5d9dd52ff..694ff363a90b 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -111,7 +111,7 @@ static bool _drm_has_leased(struct drm_master *master, int id) */ bool _drm_lease_held(struct drm_file *file_priv, int id) { - if (file_priv == NULL || file_priv->master == NULL) + if (!file_priv || !file_priv->master) return true;
return _drm_lease_held_master(file_priv->master, id); @@ -133,7 +133,7 @@ bool drm_lease_held(struct drm_file *file_priv, int id) struct drm_master *master; bool ret;
- if (file_priv == NULL || file_priv->master == NULL) + if (!file_priv || !file_priv->master || !file_priv->master->lessor) return true;
master = file_priv->master; @@ -159,7 +159,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) int count_in, count_out; uint32_t crtcs_out = 0;
- if (file_priv == NULL || file_priv->master == NULL) + if (!file_priv || !file_priv->master || !file_priv->master->lessor) return crtcs_in;
master = file_priv->master;
If userspace doesn't enable universal planes, then we automatically add the primary and cursor planes. But for universal userspace there's no such check (and maybe we only want to give the lessee one plane, maybe not even the primary one), hence we need to check for the implied plane.
v2: don't forget setcrtc ioctl.
v3: Still allow disabling of the crtc in SETCRTC.
Cc: stable@vger.kernel.org Cc: Keith Packard keithp@keithp.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_crtc.c | 4 ++++ drivers/gpu/drm/drm_plane.c | 8 ++++++++ 2 files changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 7dabbaf033a1..790ba5941954 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -559,6 +559,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
plane = crtc->primary;
+ /* allow disabling with the primary plane leased */ + if (crtc_req->mode_valid && !drm_lease_held(file_priv, plane->base.id)) + return -EACCES; + mutex_lock(&crtc->dev->mode_config.mutex); DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret); diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 4cfb56893b7f..d6ad60ab0d38 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -960,6 +960,11 @@ static int drm_mode_cursor_common(struct drm_device *dev, if (ret) goto out;
+ if (!drm_lease_held(file_priv, crtc->cursor->base.id)) { + ret = -EACCES; + goto out; + } + ret = drm_mode_cursor_universal(crtc, req, file_priv, &ctx); goto out; } @@ -1062,6 +1067,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
plane = crtc->primary;
+ if (!drm_lease_held(file_priv, plane->base.id)) + return -EACCES; + if (crtc->funcs->page_flip_target) { u32 current_vblank; int r;
We need this to make sure lessees can only connect their plane/connectors to crtc objects they own. And note that this is irrespective of whether the lessor is atomic or not, lessor cannot prevent lessees from enabling atomic.
Cc: stable@vger.kernel.org Cc: Keith Packard keithp@keithp.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic_uapi.c | 32 +++++++++++++++++++------------- drivers/gpu/drm/drm_crtc_internal.h | 1 + drivers/gpu/drm/drm_mode_object.c | 5 +++-- 3 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 4eb81f10bc54..f0dbfeb00926 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -512,8 +512,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, }
static int drm_atomic_plane_set_property(struct drm_plane *plane, - struct drm_plane_state *state, struct drm_property *property, - uint64_t val) + struct drm_plane_state *state, struct drm_file *file_priv, + struct drm_property *property, uint64_t val) { struct drm_device *dev = plane->dev; struct drm_mode_config *config = &dev->mode_config; @@ -521,7 +521,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, int ret;
if (property == config->prop_fb_id) { - struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val); + struct drm_framebuffer *fb; + fb = drm_framebuffer_lookup(dev, file_priv, val); drm_atomic_set_fb_for_plane(state, fb); if (fb) drm_framebuffer_put(fb); @@ -537,7 +538,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, return -EINVAL;
} else if (property == config->prop_crtc_id) { - struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val); + struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val); return drm_atomic_set_crtc_for_plane(state, crtc); } else if (property == config->prop_crtc_x) { state->crtc_x = U642I64(val); @@ -681,14 +682,14 @@ static int drm_atomic_set_writeback_fb_for_connector( }
static int drm_atomic_connector_set_property(struct drm_connector *connector, - struct drm_connector_state *state, struct drm_property *property, - uint64_t val) + struct drm_connector_state *state, struct drm_file *file_priv, + struct drm_property *property, uint64_t val) { struct drm_device *dev = connector->dev; struct drm_mode_config *config = &dev->mode_config;
if (property == config->prop_crtc_id) { - struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val); + struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val); return drm_atomic_set_crtc_for_connector(state, crtc); } else if (property == config->dpms_property) { /* setting DPMS property requires special handling, which @@ -749,8 +750,10 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, } else if (property == connector->colorspace_property) { state->colorspace = val; } else if (property == config->writeback_fb_id_property) { - struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val); - int ret = drm_atomic_set_writeback_fb_for_connector(state, fb); + struct drm_framebuffer *fb; + int ret; + fb = drm_framebuffer_lookup(dev, file_priv, val); + ret = drm_atomic_set_writeback_fb_for_connector(state, fb); if (fb) drm_framebuffer_put(fb); return ret; @@ -947,6 +950,7 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state, }
int drm_atomic_set_property(struct drm_atomic_state *state, + struct drm_file *file_priv, struct drm_mode_object *obj, struct drm_property *prop, uint64_t prop_value) @@ -969,7 +973,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state, }
ret = drm_atomic_connector_set_property(connector, - connector_state, prop, prop_value); + connector_state, file_priv, + prop, prop_value); break; } case DRM_MODE_OBJECT_CRTC: { @@ -997,7 +1002,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state, }
ret = drm_atomic_plane_set_property(plane, - plane_state, prop, prop_value); + plane_state, file_priv, + prop, prop_value); break; } default: @@ -1369,8 +1375,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, goto out; }
- ret = drm_atomic_set_property(state, obj, prop, - prop_value); + ret = drm_atomic_set_property(state, file_priv, + obj, prop, prop_value); if (ret) { drm_mode_object_put(obj); goto out; diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index 216f2a9ee3d4..0719a235d6cc 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -214,6 +214,7 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state, struct drm_connector *connector, int mode); int drm_atomic_set_property(struct drm_atomic_state *state, + struct drm_file *file_priv, struct drm_mode_object *obj, struct drm_property *prop, uint64_t prop_value); diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index a9005c1c2384..f32507e65b79 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -451,6 +451,7 @@ static int set_property_legacy(struct drm_mode_object *obj, }
static int set_property_atomic(struct drm_mode_object *obj, + struct drm_file *file_priv, struct drm_property *prop, uint64_t prop_value) { @@ -477,7 +478,7 @@ static int set_property_atomic(struct drm_mode_object *obj, obj_to_connector(obj), prop_value); } else { - ret = drm_atomic_set_property(state, obj, prop, prop_value); + ret = drm_atomic_set_property(state, file_priv, obj, prop, prop_value); if (ret) goto out; ret = drm_atomic_commit(state); @@ -520,7 +521,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data, goto out_unref;
if (drm_drv_uses_atomic_modeset(property->dev)) - ret = set_property_atomic(arg_obj, property, arg->value); + ret = set_property_atomic(arg_obj, file_priv, property, arg->value); else ret = set_property_legacy(arg_obj, property, arg->value);
With the previous patch drm_crtc_find will return NULL when the crtc isn't in our lease, which will then disable the plane/connector. No longer an issue since the lessor can't escape their lease terms anymore, but not quite great semantics yet either.
Catch this and return -EACCES, so that at least evil test cases have a better chance of making sure the kernel works correctly.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index f0dbfeb00926..6687215cc188 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -539,6 +539,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
} else if (property == config->prop_crtc_id) { struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val); + if (val && !crtc) + return -EACCES; return drm_atomic_set_crtc_for_plane(state, crtc); } else if (property == config->prop_crtc_x) { state->crtc_x = U642I64(val); @@ -690,6 +692,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
if (property == config->prop_crtc_id) { struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val); + if (val && !crtc) + return -EACCES; return drm_atomic_set_crtc_for_connector(state, crtc); } else if (property == config->dpms_property) { /* setting DPMS property requires special handling, which
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Mika Kuoppala