[PATCH] drm/amdkfd: Prevent user space using both svm and kfd api to register same user buffer

[Felix Kuehling]

Am 2023-02-07 um 15:35 schrieb Xiaogang.Chen:
> From: Xiaogang Chen <xiaogang.chen at amd.com>
>
> When xnack is on user space can use svm page restore to set a vm range without
> setup it first, then use regular api to register. Currently kfd api and svm are
> not interoperable. We already have check on that, but for user buffer the mapping
> address is not same as buffer cpu virtual address. Add checking on that to
> avoid error propagate to hmm.
>
> Signed-off-by: Xiaogang Chen <Xiaogang.Chen at amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index f79b8e964140..cb7acb0b9b52 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1065,6 +1065,23 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
>   		mutex_unlock(&p->svms.lock);
>   		return -EADDRINUSE;
>   	}
> +
> +	/* When register user buffer check if it has been registered by svm by
> +	 * buffer cpu virtual address.
> +	 */
> +	if (flags & KFD_IOC_ALLOC_MEM_FLAGS_USERPTR) {
> +
> +		if (interval_tree_iter_first(&p->svms.objects,
> +				untagged_addr(args->mmap_offset) >> PAGE_SHIFT,
> +				(untagged_addr(args->mmap_offset) + args->size - 1) >> PAGE_SHIFT)) {

Instead of nesting two if-blocks, you can write this as a single 
if-block like

	if ((flags & KFD_IOC_ALLOC_MEM_FLAGS_USERPTR) &&
	    interval_tree_iter_first(&p->svms.objects,
				     untagged_addr(args->mmap_offset) >> PAGE_SHIFT,
				     (untagged_addr(args->mmap_offset)  + args->size - 1) >> PAGE_SHIFT) {

I'm also not sure untagged_addr is needed here. If it is, we're missing 
it in a bunch of other places too. Most notably, we don't untag pointers 
anywhere in the SVM API.

Regards,
   Felix


> +
> +			pr_err("User Buffer Address: 0x%llx already allocated by SVM\n",
> +				untagged_addr(args->mmap_offset));
> +			mutex_unlock(&p->svms.lock);
> +			return -EADDRINUSE;
> +		}
> +
> +	}
>   	mutex_unlock(&p->svms.lock);
>   #endif
>   	mutex_lock(&p->mutex);