Slipped through the cracks in my review. The one issue I spotted is that drm_mode_object_find now acquires references and can be used on FB objects, which caused follow-on bugs in get/set_prop ioctls. Follow-up patches will fix that.
Cc: Dave Airlie airlied@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d078a5c34d48..52e6001a40e4 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -380,10 +380,9 @@ static struct drm_mode_object *_object_find(struct drm_device *dev, * @id: id of the mode object * @type: type of the mode object * - * Note that framebuffers cannot be looked up with this functions - since those - * are reference counted, they need special treatment. Even with - * DRM_MODE_OBJECT_ANY (although that will simply return NULL - * rather than WARN_ON()). + * This function is used to look up a modeset object. It will acquire a + * reference for reference counted objects. This reference must be dropped again + * by callind drm_mode_object_unreference(). */ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev, uint32_t id, uint32_t type) @@ -398,6 +397,14 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev, } EXPORT_SYMBOL(drm_mode_object_find);
+/** + * drm_mode_object_unreference - incr the fb refcnt + * @obj: mode_object + * + * This functions decrements the object's refcount if it is a refcounted modeset + * object. It is a no-op on any other object. This is used to drop references + * acquired with drm_mode_object_reference(). + */ void drm_mode_object_unreference(struct drm_mode_object *obj) { if (obj->free_cb) { @@ -411,8 +418,9 @@ EXPORT_SYMBOL(drm_mode_object_unreference); * drm_mode_object_reference - incr the fb refcnt * @obj: mode_object * - * This function operates only on refcounted objects. - * This functions increments the object's refcount. + * This functions increments the object's refcount if it is a refcounted modeset + * object. It is a no-op on any other object. References should be dropped again + * by calling drm_mode_object_unreference(). */ void drm_mode_object_reference(struct drm_mode_object *obj) {
Dave Airlie had at least the refcount leak fixed in a later patch (but that patch does other things which need a bit more work). But we still have the trouble that silly userspace could hit the WARN_ON in drm_mode_object_find.
Fix this all up to make sure we don't leak objects, and don't spew into demsg.
Fixes: d0f37cf62979 ("drm/mode: move framebuffer reference into object.") Testcase: igt/kms_addfb_basic/invalid-*-prop* Cc: Dave Airlie airlied@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_crtc.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 52e6001a40e4..4089c81fe710 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -389,9 +389,7 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev, { struct drm_mode_object *obj = NULL;
- /* Framebuffers are reference counted and need their own lookup - * function.*/ - WARN_ON(type == DRM_MODE_OBJECT_FB || type == DRM_MODE_OBJECT_BLOB); + WARN_ON(type == DRM_MODE_OBJECT_BLOB); obj = _object_find(dev, id, type); return obj; } @@ -5005,7 +5003,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, } if (!obj->properties) { ret = -EINVAL; - goto out; + goto out_unref; }
ret = get_properties(obj, file_priv->atomic, @@ -5013,6 +5011,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, (uint64_t __user *)(unsigned long)(arg->prop_values_ptr), &arg->count_props);
+out_unref: + drm_mode_object_unreference(obj); out: drm_modeset_unlock_all(dev); return ret; @@ -5055,20 +5055,20 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data, goto out; } if (!arg_obj->properties) - goto out; + goto out_unref;
for (i = 0; i < arg_obj->properties->count; i++) if (arg_obj->properties->properties[i]->base.id == arg->prop_id) break;
if (i == arg_obj->properties->count) - goto out; + goto out_unref;
prop_obj = drm_mode_object_find(dev, arg->prop_id, DRM_MODE_OBJECT_PROPERTY); if (!prop_obj) { ret = -ENOENT; - goto out; + goto out_unref; } property = obj_to_property(prop_obj);
@@ -5091,6 +5091,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
drm_property_change_valid_put(property, ref);
+out_unref: + drm_mode_object_unreference(arg_obj); out: drm_modeset_unlock_all(dev); return ret;
Random drive-by refactoring I spotted.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 40c7b268a9bc..d25abce0450f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2510,12 +2510,9 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); */ void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc) { - if (crtc->state) { - drm_property_unreference_blob(crtc->state->mode_blob); - drm_property_unreference_blob(crtc->state->degamma_lut); - drm_property_unreference_blob(crtc->state->ctm); - drm_property_unreference_blob(crtc->state->gamma_lut); - } + if (crtc->state) + __drm_atomic_helper_crtc_destroy_state(crtc, crtc->state); + kfree(crtc->state); crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);
@@ -2621,8 +2618,8 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state); */ void drm_atomic_helper_plane_reset(struct drm_plane *plane) { - if (plane->state && plane->state->fb) - drm_framebuffer_unreference(plane->state->fb); + if (plane->state) + __drm_atomic_helper_plane_destroy_state(plane, plane->state);
kfree(plane->state); plane->state = kzalloc(sizeof(*plane->state), GFP_KERNEL); @@ -2743,6 +2740,10 @@ void drm_atomic_helper_connector_reset(struct drm_connector *connector) struct drm_connector_state *conn_state = kzalloc(sizeof(*conn_state), GFP_KERNEL);
+ if (connector->state) + __drm_atomic_helper_connector_destroy_state(connector, + connector->state); + kfree(connector->state); __drm_atomic_helper_connector_reset(connector, conn_state); }
Need to move the free function around a bit, but otherwise mostly just removing code.
Specifically we can nuke all the _locked variants since the weak idr reference is now protected by the idr_mutex, which we never hold anywhere expect in the lookup/reg/unreg functions. And those never call anything else.
Another benefit of this is that this patch switches the weak reference logic from kref_put_mutex to kref_get_unless_zero. And the later is in general more flexible wrt accomodating multiple weak references protected by different locks, which might or might not come handy eventually.
But one consequence of that switch is that we need to acquire the blob_lock from the free function for the list_del calls. That's a bit tricky to pull off, but works well if we pick the exact same scheme as is already used for framebuffers. Most important changes:
- filp list is maintainer by create/destroy_blob ioctls directly (already the case, so we can just remove the redundant list_del from the free function).
- filp close handler walks the filp-private list lockless - works because we know no one else can access it. I copied the same comment from the fb code over to explain this.
- Otherwise we need to sufficiently restrict blob_lock critical sections to avoid all the unreference calls. Easy to do once the blob_lock only protects the list, and no longer the weak reference.
Cc: Dave Airlie airlied@gmail.com Cc: Daniel Stone daniels@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_crtc.c | 164 +++++++++++++-------------------------------- include/drm/drm_crtc.h | 1 - 2 files changed, 45 insertions(+), 120 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 4089c81fe710..8ea3adf6459d 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -360,10 +360,6 @@ static struct drm_mode_object *_object_find(struct drm_device *dev, obj = NULL; if (obj && obj->id != id) obj = NULL; - /* don't leak out unref'd fb's */ - if (obj && - obj->type == DRM_MODE_OBJECT_BLOB) - obj = NULL;
if (obj && obj->free_cb) { if (!kref_get_unless_zero(&obj->refcount)) @@ -389,7 +385,6 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev, { struct drm_mode_object *obj = NULL;
- WARN_ON(type == DRM_MODE_OBJECT_BLOB); obj = _object_find(dev, id, type); return obj; } @@ -4259,6 +4254,20 @@ done: return ret; }
+static void drm_property_free_blob(struct kref *kref) +{ + struct drm_property_blob *blob = + container_of(kref, struct drm_property_blob, base.refcount); + + mutex_lock(&blob->dev->mode_config.blob_lock); + list_del(&blob->head_global); + mutex_unlock(&blob->dev->mode_config.blob_lock); + + drm_mode_object_unregister(blob->dev, &blob->base); + + kfree(blob); +} + /** * drm_property_create_blob - Create new blob property * @@ -4296,20 +4305,16 @@ drm_property_create_blob(struct drm_device *dev, size_t length, if (data) memcpy(blob->data, data, length);
- mutex_lock(&dev->mode_config.blob_lock); - - ret = drm_mode_object_get(dev, &blob->base, DRM_MODE_OBJECT_BLOB); + ret = drm_mode_object_get_reg(dev, &blob->base, DRM_MODE_OBJECT_BLOB, + true, drm_property_free_blob); if (ret) { kfree(blob); - mutex_unlock(&dev->mode_config.blob_lock); return ERR_PTR(-EINVAL); }
- kref_init(&blob->refcount); - + mutex_lock(&dev->mode_config.blob_lock); list_add_tail(&blob->head_global, &dev->mode_config.property_blob_list); - mutex_unlock(&dev->mode_config.blob_lock);
return blob; @@ -4317,27 +4322,6 @@ drm_property_create_blob(struct drm_device *dev, size_t length, EXPORT_SYMBOL(drm_property_create_blob);
/** - * drm_property_free_blob - Blob property destructor - * - * Internal free function for blob properties; must not be used directly. - * - * @kref: Reference - */ -static void drm_property_free_blob(struct kref *kref) -{ - struct drm_property_blob *blob = - container_of(kref, struct drm_property_blob, refcount); - - WARN_ON(!mutex_is_locked(&blob->dev->mode_config.blob_lock)); - - list_del(&blob->head_global); - list_del(&blob->head_file); - drm_mode_object_unregister(blob->dev, &blob->base); - - kfree(blob); -} - -/** * drm_property_unreference_blob - Unreference a blob property * * Drop a reference on a blob property. May free the object. @@ -4346,42 +4330,14 @@ static void drm_property_free_blob(struct kref *kref) */ void drm_property_unreference_blob(struct drm_property_blob *blob) { - struct drm_device *dev; - if (!blob) return;
- dev = blob->dev; - - DRM_DEBUG("%p: blob ID: %d (%d)\n", blob, blob->base.id, atomic_read(&blob->refcount.refcount)); - - if (kref_put_mutex(&blob->refcount, drm_property_free_blob, - &dev->mode_config.blob_lock)) - mutex_unlock(&dev->mode_config.blob_lock); - else - might_lock(&dev->mode_config.blob_lock); + drm_mode_object_unreference(&blob->base); } EXPORT_SYMBOL(drm_property_unreference_blob);
/** - * drm_property_unreference_blob_locked - Unreference a blob property with blob_lock held - * - * Drop a reference on a blob property. May free the object. This must be - * called with blob_lock held. - * - * @blob: Pointer to blob property - */ -static void drm_property_unreference_blob_locked(struct drm_property_blob *blob) -{ - if (!blob) - return; - - DRM_DEBUG("%p: blob ID: %d (%d)\n", blob, blob->base.id, atomic_read(&blob->refcount.refcount)); - - kref_put(&blob->refcount, drm_property_free_blob); -} - -/** * drm_property_destroy_user_blobs - destroy all blobs created by this client * @dev: DRM device * @file_priv: destroy all blobs owned by this file handle @@ -4391,14 +4347,14 @@ void drm_property_destroy_user_blobs(struct drm_device *dev, { struct drm_property_blob *blob, *bt;
- mutex_lock(&dev->mode_config.blob_lock); - + /* + * When the file gets released that means no one else can access the + * blob list any more, so no need to grab dev->blob_lock. + */ list_for_each_entry_safe(blob, bt, &file_priv->blobs, head_file) { list_del_init(&blob->head_file); - drm_property_unreference_blob_locked(blob); + drm_property_unreference_blob(blob); } - - mutex_unlock(&dev->mode_config.blob_lock); }
/** @@ -4410,35 +4366,11 @@ void drm_property_destroy_user_blobs(struct drm_device *dev, */ struct drm_property_blob *drm_property_reference_blob(struct drm_property_blob *blob) { - DRM_DEBUG("%p: blob ID: %d (%d)\n", blob, blob->base.id, atomic_read(&blob->refcount.refcount)); - kref_get(&blob->refcount); + drm_mode_object_reference(&blob->base); return blob; } EXPORT_SYMBOL(drm_property_reference_blob);
-/* - * Like drm_property_lookup_blob, but does not return an additional reference. - * Must be called with blob_lock held. - */ -static struct drm_property_blob *__drm_property_lookup_blob(struct drm_device *dev, - uint32_t id) -{ - struct drm_mode_object *obj = NULL; - struct drm_property_blob *blob; - - WARN_ON(!mutex_is_locked(&dev->mode_config.blob_lock)); - - mutex_lock(&dev->mode_config.idr_mutex); - obj = idr_find(&dev->mode_config.crtc_idr, id); - if (!obj || (obj->type != DRM_MODE_OBJECT_BLOB) || (obj->id != id)) - blob = NULL; - else - blob = obj_to_blob(obj); - mutex_unlock(&dev->mode_config.idr_mutex); - - return blob; -} - /** * drm_property_lookup_blob - look up a blob property and take a reference * @dev: drm device @@ -4451,16 +4383,12 @@ static struct drm_property_blob *__drm_property_lookup_blob(struct drm_device *d struct drm_property_blob *drm_property_lookup_blob(struct drm_device *dev, uint32_t id) { - struct drm_property_blob *blob; - - mutex_lock(&dev->mode_config.blob_lock); - blob = __drm_property_lookup_blob(dev, id); - if (blob) { - if (!kref_get_unless_zero(&blob->refcount)) - blob = NULL; - } - mutex_unlock(&dev->mode_config.blob_lock); + struct drm_mode_object *obj; + struct drm_property_blob *blob = NULL;
+ obj = _object_find(dev, id, DRM_MODE_OBJECT_BLOB); + if (obj) + blob = obj_to_blob(obj); return blob; } EXPORT_SYMBOL(drm_property_lookup_blob); @@ -4565,26 +4493,21 @@ int drm_mode_getblob_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;
- drm_modeset_lock_all(dev); - mutex_lock(&dev->mode_config.blob_lock); - blob = __drm_property_lookup_blob(dev, out_resp->blob_id); - if (!blob) { - ret = -ENOENT; - goto done; - } + blob = drm_property_lookup_blob(dev, out_resp->blob_id); + if (!blob) + return -ENOENT;
if (out_resp->length == blob->length) { blob_ptr = (void __user *)(unsigned long)out_resp->data; if (copy_to_user(blob_ptr, blob->data, blob->length)) { ret = -EFAULT; - goto done; + goto unref; } } out_resp->length = blob->length; +unref: + drm_property_unreference_blob(blob);
-done: - mutex_unlock(&dev->mode_config.blob_lock); - drm_modeset_unlock_all(dev); return ret; }
@@ -4663,13 +4586,11 @@ int drm_mode_destroyblob_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;
- mutex_lock(&dev->mode_config.blob_lock); - blob = __drm_property_lookup_blob(dev, out_resp->blob_id); - if (!blob) { - ret = -ENOENT; - goto err; - } + blob = drm_property_lookup_blob(dev, out_resp->blob_id); + if (!blob) + return -ENOENT;
+ mutex_lock(&dev->mode_config.blob_lock); /* Ensure the property was actually created by this user. */ list_for_each_entry(bt, &file_priv->blobs, head_file) { if (bt == blob) { @@ -4686,13 +4607,18 @@ int drm_mode_destroyblob_ioctl(struct drm_device *dev, /* We must drop head_file here, because we may not be the last * reference on the blob. */ list_del_init(&blob->head_file); - drm_property_unreference_blob_locked(blob); mutex_unlock(&dev->mode_config.blob_lock);
+ /* One reference from lookup, and one from the filp. */ + drm_property_unreference_blob(blob); + drm_property_unreference_blob(blob); + return 0;
err: mutex_unlock(&dev->mode_config.blob_lock); + drm_property_unreference_blob(blob); + return ret; }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 43c31496a5a7..297e527f1cac 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -250,7 +250,6 @@ struct drm_framebuffer { struct drm_property_blob { struct drm_mode_object base; struct drm_device *dev; - struct kref refcount; struct list_head head_global; struct list_head head_file; size_t length;
dri-devel@lists.freedesktop.org
-
Daniel Vetter