[PATCH 01/19] drm/amdkfd: Fix double Mutex lock order

[Oded Gabbay]

On Sat, Aug 12, 2017 at 12:56 AM, Felix Kuehling <Felix.Kuehling at amd.com> wrote:
>
> From: Yair Shachar <yair.shachar at amd.com>
>
> Signed-off-by: Yair Shachar <yair.shachar at amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 6316aad..2a45718e 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -451,8 +451,8 @@ static int kfd_ioctl_dbg_register(struct file *filep,
>                 return -EINVAL;
>         }
>
> -       mutex_lock(kfd_get_dbgmgr_mutex());
>         mutex_lock(&p->mutex);
> +       mutex_lock(kfd_get_dbgmgr_mutex());
>
>         /*
>          * make sure that we have pdd, if this the first queue created for
> @@ -460,8 +460,8 @@ static int kfd_ioctl_dbg_register(struct file *filep,
>          */
>         pdd = kfd_bind_process_to_device(dev, p);
>         if (IS_ERR(pdd)) {
> -               mutex_unlock(&p->mutex);
>                 mutex_unlock(kfd_get_dbgmgr_mutex());
> +               mutex_unlock(&p->mutex);
>                 return PTR_ERR(pdd);
>         }
>
> @@ -480,8 +480,8 @@ static int kfd_ioctl_dbg_register(struct file *filep,
>                 status = -EINVAL;
>         }
>
> -       mutex_unlock(&p->mutex);
>         mutex_unlock(kfd_get_dbgmgr_mutex());
> +       mutex_unlock(&p->mutex);
>
>         return status;
>  }
> --
> 2.7.4
>

Hi Felix,
Could you please explain why this change is necessary ?

It seems to me this actually makes things a bit worse in a
multi-process environment, because the p->mutex is per process but the
dbgmgr mutex is global. Therefore, if process A first takes the
process mutex, and process B takes the dbgmgr mutex (in this function
or some other function, such as kfd_ioctl_dbg_address_watch) *before*
process A managed to take dbgmgr mutex, then process A will be locked
from doing other, totally unrelated functions, such as
kfd_ioctl_create_queue.

While, if we keep things as they are now, process A will first take
the dbgmgr mutex, making process B stuck on it, but allowing it to do
other unrelated ioctls because he hadn't taken the process mutex.

Thanks,
Oded