2017-12-21 06:54:39

by Keith Packard

Subject: [PATCH] drm: move lease init after validation in drm_lease_create

Patch bd36d3bab2e3d08f80766c86487090dbceed4651 fixed a deadlock in the
failure path of drm_lease_create. This made the partially initialized
lease object visible for a short window of time.

To avoid having the lessee state appear transiently, I've rearranged
the code so that the lessor fields are not filled in until the
parameters are all validated and the function will succeed.

Signed-off-by: Keith Packard <[email protected]>
---
drivers/gpu/drm/drm_lease.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index 59849f02e2ad..1402c0e71b03 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -220,17 +220,6 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr

mutex_lock(&dev->mode_config.idr_mutex);

- /* Insert the new lessee into the tree */
- id = idr_alloc(&(drm_lease_owner(lessor)->lessee_idr), lessee, 1, 0, GFP_KERNEL);
- if (id < 0) {
- error = id;
- goto out_lessee;
- }
-
- lessee->lessee_id = id;
- lessee->lessor = drm_master_get(lessor);
- list_add_tail(&lessee->lessee_list, &lessor->lessees);
-
idr_for_each_entry(leases, entry, object) {
error = 0;
if (!idr_find(&dev->mode_config.crtc_idr, object))
@@ -246,6 +235,17 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr
}
}

+ /* Insert the new lessee into the tree */
+ id = idr_alloc(&(drm_lease_owner(lessor)->lessee_idr), lessee, 1, 0, GFP_KERNEL);
+ if (id < 0) {
+ error = id;
+ goto out_lessee;
+ }
+
+ lessee->lessee_id = id;
+ lessee->lessor = drm_master_get(lessor);
+ list_add_tail(&lessee->lessee_list, &lessor->lessees);
+
/* Move the leases over */
lessee->leases = *leases;
DRM_DEBUG_LEASE("new lessee %d %p, lessor %d %p\n", lessee->lessee_id, lessee, lessor->lessee_id, lessor);
--
2.15.1

2017-12-21 08:50:22

by Daniel Vetter

Subject: Re: [PATCH] drm: move lease init after validation in drm_lease_create

On Wed, Dec 20, 2017 at 10:54:24PM -0800, Keith Packard wrote:
> Patch bd36d3bab2e3d08f80766c86487090dbceed4651 fixed a deadlock in the
> failure path of drm_lease_create. This made the partially initialized
> lease object visible for a short window of time.
>
> To avoid having the lessee state appear transiently, I've rearranged
> the code so that the lessor fields are not filled in until the
> parameters are all validated and the function will succeed.
>
> Signed-off-by: Keith Packard <[email protected]>

Good catch, sorry I missed that. And registering the object as the very
last thing is definitely the safe pattern.

Applied, thanks for your patch.
-Daniel

> ---
> drivers/gpu/drm/drm_lease.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index 59849f02e2ad..1402c0e71b03 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -220,17 +220,6 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr
>
> mutex_lock(&dev->mode_config.idr_mutex);
>
> - /* Insert the new lessee into the tree */
> - id = idr_alloc(&(drm_lease_owner(lessor)->lessee_idr), lessee, 1, 0, GFP_KERNEL);
> - if (id < 0) {
> - error = id;
> - goto out_lessee;
> - }
> -
> - lessee->lessee_id = id;
> - lessee->lessor = drm_master_get(lessor);
> - list_add_tail(&lessee->lessee_list, &lessor->lessees);
> -
> idr_for_each_entry(leases, entry, object) {
> error = 0;
> if (!idr_find(&dev->mode_config.crtc_idr, object))
> @@ -246,6 +235,17 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr
> }
> }
>
> + /* Insert the new lessee into the tree */
> + id = idr_alloc(&(drm_lease_owner(lessor)->lessee_idr), lessee, 1, 0, GFP_KERNEL);
> + if (id < 0) {
> + error = id;
> + goto out_lessee;
> + }
> +
> + lessee->lessee_id = id;
> + lessee->lessor = drm_master_get(lessor);
> + list_add_tail(&lessee->lessee_list, &lessor->lessees);
> +
> /* Move the leases over */
> lessee->leases = *leases;
> DRM_DEBUG_LEASE("new lessee %d %p, lessor %d %p\n", lessee->lessee_id, lessee, lessor->lessee_id, lessor);
> --
> 2.15.1
>

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