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

[Chen, Xiaogang]

On 2/7/2023 2:48 PM, Felix Kuehling wrote:
>
> 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.
memory virtual address tagging is architecture dependent. Ex: if virtual 
address is 48bits and use 64bits pointer, people can use additional bits 
for different purpose. From kernel source tree seems only arm64 and 
sparc define untagged_addr that are not noop. For other architectures it 
is defined as noop. I think we want have it if run on different 
architecture cpu.
>
> 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);