Re: [PATCH v2] drm/drm_lease: Prevent deadlock in case drm_lease_create() fails

From: Daniel Vetter <daniel@ffwll.ch>
To: Marius Vlad <marius-cristian.vlad@nxp.com>
Cc: daniel.vetter@ffwll.ch, keithp@keithp.com,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/drm_lease: Prevent deadlock in case drm_lease_create() fails
Date: Thu, 14 Dec 2017 08:25:29 +0100	[thread overview]
Message-ID: <20171214072529.GG26573@phenom.ffwll.local> (raw)
In-Reply-To: <20171213181048.32719-1-marius-cristian.vlad@nxp.com>

On Wed, Dec 13, 2017 at 08:10:48PM +0200, Marius Vlad wrote:
> This case can been seen when creating the lease with the same objects passed.
> 
> [  605.515097] 2 locks held by testapp/3337:
> [  605.519027]  #0:  (&dev->mode_config.idr_mutex){......}, at: [<ffff0000085f1664>] drm_mode_create_lease_ioctl+0x384/0x858
> [  605.530045]  #1:  (&dev->mode_config.idr_mutex){......}, at: [<ffff0000085f11bc>] drm_lease_destroy+0x2c/0x110
> 
> Which was causing the process to hang:
> 
> [  605.398827] [<ffff0000080856cc>] __switch_to+0x94/0xa8
> [  605.404030] [<ffff000008c05d00>] __schedule+0x1b0/0x698
> [  605.409322] [<ffff000008c06224>] schedule+0x3c/0xa8
> [  605.414260] [<ffff000008c06628>] schedule_preempt_disabled+0x20/0x38
> [  605.420677] [<ffff000008c07370>] mutex_lock_nested+0x158/0x340
> [  605.426572] [<ffff0000085f11bc>] drm_lease_destroy+0x2c/0x110
> [  605.432389] [<ffff0000085cecf0>] drm_master_put+0xc0/0xc8
> [  605.437845] [<ffff0000085f175c>] drm_mode_create_lease_ioctl+0x47c/0x858
> [  605.444612] [<ffff0000085d4460>] drm_ioctl+0x198/0x448
> [  605.449811] [<ffff000008201134>] do_vfs_ioctl+0xa4/0x748
> [  605.455192] [<ffff000008201864>] SyS_ioctl+0x8c/0xa0
> [  605.460216] [<ffff000008082f4c>] __sys_trace_return+0x0/0x4
> 
> drm_mode_create_lease_ioctl() calls drm_lease_create() which acquires a lock
> on dev->mode_config.idr_mutex. In case of failure, drm_lease_create() calls 
> drm_master_put() which in turn tries to acquire the same lock when calling
> drm_lease_destroy(). 
> 
> v2: - Reverse the order at exit in case of fail, so that unlocking takes place
> before dropping the reference.
>     - Include detail information about deadlock (Daniel Vetter)
> 
> 
> Signed-off-by: Marius Vlad <marius-cristian.vlad@nxp.com>

Yeah that makes a lot more sense. Applied to -fixes, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_lease.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index d1eb56a..59849f0 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -254,10 +254,10 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr
>  	return lessee;
>  
>  out_lessee:
> -	drm_master_put(&lessee);
> -
>  	mutex_unlock(&dev->mode_config.idr_mutex);
>  
> +	drm_master_put(&lessee);
> +
>  	return ERR_PTR(error);
>  }
>  
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel