* [PATCH] drm/amdkfd: Prevent user space using both svm and kfd api to register same user buffer
@ 2023-02-07 20:35 Xiaogang.Chen
2023-02-07 20:48 ` Felix Kuehling
0 siblings, 1 reply; 4+ messages in thread
From: Xiaogang.Chen @ 2023-02-07 20:35 UTC (permalink / raw)
To: amd-gfx; +Cc: Xiaogang Chen, felix.kuehling
From: Xiaogang Chen <xiaogang.chen@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@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)) {
+
+ 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);
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/amdkfd: Prevent user space using both svm and kfd api to register same user buffer
2023-02-07 20:35 [PATCH] drm/amdkfd: Prevent user space using both svm and kfd api to register same user buffer Xiaogang.Chen
@ 2023-02-07 20:48 ` Felix Kuehling
2023-02-07 23:36 ` Chen, Xiaogang
0 siblings, 1 reply; 4+ messages in thread
From: Felix Kuehling @ 2023-02-07 20:48 UTC (permalink / raw)
To: Xiaogang.Chen, amd-gfx
Am 2023-02-07 um 15:35 schrieb Xiaogang.Chen:
> From: Xiaogang Chen <xiaogang.chen@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@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);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/amdkfd: Prevent user space using both svm and kfd api to register same user buffer
2023-02-07 20:48 ` Felix Kuehling
@ 2023-02-07 23:36 ` Chen, Xiaogang
2023-02-08 4:29 ` Felix Kuehling
0 siblings, 1 reply; 4+ messages in thread
From: Chen, Xiaogang @ 2023-02-07 23:36 UTC (permalink / raw)
To: Felix Kuehling, amd-gfx
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@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@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);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/amdkfd: Prevent user space using both svm and kfd api to register same user buffer
2023-02-07 23:36 ` Chen, Xiaogang
@ 2023-02-08 4:29 ` Felix Kuehling
0 siblings, 0 replies; 4+ messages in thread
From: Felix Kuehling @ 2023-02-08 4:29 UTC (permalink / raw)
To: Chen, Xiaogang, amd-gfx
Am 2023-02-07 um 18:36 schrieb 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@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@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.
Fair enough. But it has to be consistent. If the SVM code fills the
interval tree with tagged addresses, we can't look for untagged
addresses here. Given that the SVM code currently uses (potentially)
tagged addresses, we should not use untagged addresses here. A proper
fix for untagging addresses in the SVM code would be needed as a
separate commit.
Regards,
Felix
>>
>> 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);
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-02-08 4:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-07 20:35 [PATCH] drm/amdkfd: Prevent user space using both svm and kfd api to register same user buffer Xiaogang.Chen
2023-02-07 20:48 ` Felix Kuehling
2023-02-07 23:36 ` Chen, Xiaogang
2023-02-08 4:29 ` Felix Kuehling
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.