From: Felix Kuehling Felix.Kuehling@amd.com
The thunk needs to mmap all BOs for CPU access to allow the debugger to access them. Invisible ones are mapped with PROT_NONE.
Fixes: 71df0368e9b6 ("drm/amdgpu: Implement mmap as GEM object function") Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com Signed-off-by: Alex Deucher alexander.deucher@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 3e487e199771..2d6b2d77b738 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -1404,8 +1404,7 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu( domain = alloc_domain = AMDGPU_GEM_DOMAIN_VRAM; alloc_flags = AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE; alloc_flags |= (flags & KFD_IOC_ALLOC_MEM_FLAGS_PUBLIC) ? - AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED : - AMDGPU_GEM_CREATE_NO_CPU_ACCESS; + AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED : 0; } else if (flags & KFD_IOC_ALLOC_MEM_FLAGS_GTT) { domain = alloc_domain = AMDGPU_GEM_DOMAIN_GTT; alloc_flags = 0;
From: Felix Kuehling Felix.Kuehling@amd.com
KFD Thunk maps invisible VRAM BOs with PROT_NONE, MAP_PRIVATE. is_cow_mapping returns true for these mappings. Add a check for vm_flags & VM_WRITE to avoid mmap failures on private read-only or PROT_NONE mappings.
Fixes: f91142c62161 ("drm/ttm: nuke VM_MIXEDMAP on BO mappings v3") Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com Signed-off-by: Alex Deucher alexander.deucher@amd.com --- drivers/gpu/drm/ttm/ttm_bo_vm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index f56be5bc0861..a75e90c7d4aa 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -552,7 +552,7 @@ static const struct vm_operations_struct ttm_bo_vm_ops = { int ttm_bo_mmap_obj(struct vm_area_struct *vma, struct ttm_buffer_object *bo) { /* Enforce no COW since would have really strange behavior with it. */ - if (is_cow_mapping(vma->vm_flags)) + if (is_cow_mapping(vma->vm_flags) && (vma->vm_flags & VM_WRITE)) return -EINVAL;
ttm_bo_get(bo);
Am 08.07.21 um 21:36 schrieb Alex Deucher:
From: Felix Kuehling Felix.Kuehling@amd.com
KFD Thunk maps invisible VRAM BOs with PROT_NONE, MAP_PRIVATE. is_cow_mapping returns true for these mappings. Add a check for vm_flags & VM_WRITE to avoid mmap failures on private read-only or PROT_NONE mappings.
I'm pretty sure that this is not working as expected.
Fixes: f91142c62161 ("drm/ttm: nuke VM_MIXEDMAP on BO mappings v3") Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com Signed-off-by: Alex Deucher alexander.deucher@amd.com
drivers/gpu/drm/ttm/ttm_bo_vm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index f56be5bc0861..a75e90c7d4aa 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -552,7 +552,7 @@ static const struct vm_operations_struct ttm_bo_vm_ops = { int ttm_bo_mmap_obj(struct vm_area_struct *vma, struct ttm_buffer_object *bo) { /* Enforce no COW since would have really strange behavior with it. */
- if (is_cow_mapping(vma->vm_flags))
- if (is_cow_mapping(vma->vm_flags) && (vma->vm_flags & VM_WRITE))
is_cow_mapping() already checks for VM_MAYWRITE, so this here shouldn't be necessary.
Christian.
return -EINVAL;ttm_bo_get(bo);
On Fri, Jul 9, 2021 at 8:38 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 08.07.21 um 21:36 schrieb Alex Deucher:
From: Felix Kuehling Felix.Kuehling@amd.com
KFD Thunk maps invisible VRAM BOs with PROT_NONE, MAP_PRIVATE. is_cow_mapping returns true for these mappings. Add a check for vm_flags & VM_WRITE to avoid mmap failures on private read-only or PROT_NONE mappings.
I'm pretty sure that this is not working as expected.
Fixes: f91142c62161 ("drm/ttm: nuke VM_MIXEDMAP on BO mappings v3") Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com Signed-off-by: Alex Deucher alexander.deucher@amd.com
drivers/gpu/drm/ttm/ttm_bo_vm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index f56be5bc0861..a75e90c7d4aa 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -552,7 +552,7 @@ static const struct vm_operations_struct ttm_bo_vm_ops = { int ttm_bo_mmap_obj(struct vm_area_struct *vma, struct ttm_buffer_object *bo) { /* Enforce no COW since would have really strange behavior with it. */
if (is_cow_mapping(vma->vm_flags))
if (is_cow_mapping(vma->vm_flags) && (vma->vm_flags & VM_WRITE))is_cow_mapping() already checks for VM_MAYWRITE, so this here shouldn't be necessary.
MAYWRITE != WRITE
But then you need to make sure you do catch mprotect() calls to catch the cow, and I'm not sure that's even possible. Otherwise it'll go boom again on the page refcount. -Daniel
Christian.
return -EINVAL; ttm_bo_get(bo);
On 2021-07-09 2:38 a.m., Christian König wrote:
Am 08.07.21 um 21:36 schrieb Alex Deucher:
From: Felix Kuehling Felix.Kuehling@amd.com
KFD Thunk maps invisible VRAM BOs with PROT_NONE, MAP_PRIVATE. is_cow_mapping returns true for these mappings. Add a check for vm_flags & VM_WRITE to avoid mmap failures on private read-only or PROT_NONE mappings.
I'm pretty sure that this is not working as expected.
Not sure what you mean. Debugger access to the memory through the PROT_NONE VMAs is definitely working, with both ptrace and /proc/<pid>/mem.
Fixes: f91142c62161 ("drm/ttm: nuke VM_MIXEDMAP on BO mappings v3") Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com Signed-off-by: Alex Deucher alexander.deucher@amd.com
drivers/gpu/drm/ttm/ttm_bo_vm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index f56be5bc0861..a75e90c7d4aa 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -552,7 +552,7 @@ static const struct vm_operations_struct ttm_bo_vm_ops = { int ttm_bo_mmap_obj(struct vm_area_struct *vma, struct ttm_buffer_object *bo) { /* Enforce no COW since would have really strange behavior with it. */ - if (is_cow_mapping(vma->vm_flags)) + if (is_cow_mapping(vma->vm_flags) && (vma->vm_flags & VM_WRITE))
is_cow_mapping() already checks for VM_MAYWRITE, so this here shouldn't be necessary.
AFAICT, VM_MAYWRITE is not based on the PROT_... bits used to create the VMA, but based on the permissions of the file. So as long as the render node is writable, VM_MAYWRITE is set for all VMAs that map it.
I would agree that it's probably a bad idea for the Thunk to map these VMAs with MAP_PRIVATE. We can try changing the Thunk to use MAP_SHARED for these PROT_NONE mappings. But that doesn't change the fact that this kernel patch broke existing usermode.
Regards, Felix
Christian.
return -EINVAL; ttm_bo_get(bo);
Am 09.07.21 um 21:31 schrieb Felix Kuehling:
On 2021-07-09 2:38 a.m., Christian König wrote:
Am 08.07.21 um 21:36 schrieb Alex Deucher:
From: Felix Kuehling Felix.Kuehling@amd.com
KFD Thunk maps invisible VRAM BOs with PROT_NONE, MAP_PRIVATE. is_cow_mapping returns true for these mappings. Add a check for vm_flags & VM_WRITE to avoid mmap failures on private read-only or PROT_NONE mappings.
I'm pretty sure that this is not working as expected.
Not sure what you mean. Debugger access to the memory through the PROT_NONE VMAs is definitely working, with both ptrace and /proc/<pid>/mem.
Fixes: f91142c62161 ("drm/ttm: nuke VM_MIXEDMAP on BO mappings v3") Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com Signed-off-by: Alex Deucher alexander.deucher@amd.com
drivers/gpu/drm/ttm/ttm_bo_vm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index f56be5bc0861..a75e90c7d4aa 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -552,7 +552,7 @@ static const struct vm_operations_struct ttm_bo_vm_ops = { int ttm_bo_mmap_obj(struct vm_area_struct *vma, struct ttm_buffer_object *bo) { /* Enforce no COW since would have really strange behavior with it. */ - if (is_cow_mapping(vma->vm_flags)) + if (is_cow_mapping(vma->vm_flags) && (vma->vm_flags & VM_WRITE))
is_cow_mapping() already checks for VM_MAYWRITE, so this here shouldn't be necessary.
AFAICT, VM_MAYWRITE is not based on the PROT_... bits used to create the VMA, but based on the permissions of the file. So as long as the render node is writable, VM_MAYWRITE is set for all VMAs that map it.
I would agree that it's probably a bad idea for the Thunk to map these VMAs with MAP_PRIVATE. We can try changing the Thunk to use MAP_SHARED for these PROT_NONE mappings. But that doesn't change the fact that this kernel patch broke existing usermode.
Yeah, but see the discussion around MAP_PRIVATE and COW regarding this. What Thunk did here was a really bad idea.
I think we have only two options here: 1. Accept the breakage of thunk. 2. Add a workaround in amdgpu/amdkfd to force MAP_PRIVATE into MAP_SHARED in the kernel.
Regards, Christian.
Regards, Felix
Christian.
return -EINVAL; ttm_bo_get(bo);
On 2021-07-09 3:37 p.m., Christian König wrote:
Am 09.07.21 um 21:31 schrieb Felix Kuehling:
On 2021-07-09 2:38 a.m., Christian König wrote:
Am 08.07.21 um 21:36 schrieb Alex Deucher:
From: Felix Kuehling Felix.Kuehling@amd.com
KFD Thunk maps invisible VRAM BOs with PROT_NONE, MAP_PRIVATE. is_cow_mapping returns true for these mappings. Add a check for vm_flags & VM_WRITE to avoid mmap failures on private read-only or PROT_NONE mappings.
I'm pretty sure that this is not working as expected.
Not sure what you mean. Debugger access to the memory through the PROT_NONE VMAs is definitely working, with both ptrace and /proc/<pid>/mem.
Fixes: f91142c62161 ("drm/ttm: nuke VM_MIXEDMAP on BO mappings v3") Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com Signed-off-by: Alex Deucher alexander.deucher@amd.com
drivers/gpu/drm/ttm/ttm_bo_vm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index f56be5bc0861..a75e90c7d4aa 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -552,7 +552,7 @@ static const struct vm_operations_struct ttm_bo_vm_ops = { int ttm_bo_mmap_obj(struct vm_area_struct *vma, struct ttm_buffer_object *bo) { /* Enforce no COW since would have really strange behavior with it. */ - if (is_cow_mapping(vma->vm_flags)) + if (is_cow_mapping(vma->vm_flags) && (vma->vm_flags & VM_WRITE))
is_cow_mapping() already checks for VM_MAYWRITE, so this here shouldn't be necessary.
AFAICT, VM_MAYWRITE is not based on the PROT_... bits used to create the VMA, but based on the permissions of the file. So as long as the render node is writable, VM_MAYWRITE is set for all VMAs that map it.
I would agree that it's probably a bad idea for the Thunk to map these VMAs with MAP_PRIVATE. We can try changing the Thunk to use MAP_SHARED for these PROT_NONE mappings. But that doesn't change the fact that this kernel patch broke existing usermode.
For the record, changing the Thunk to use MAP_SHARED works.
Yeah, but see the discussion around MAP_PRIVATE and COW regarding this. What Thunk did here was a really bad idea.
Hindsight ... Which part was a bad idea?
* Creating a PROT_NONE VMA? That's necessary to let ptrace or /proc/<pid>/mem access the memory. It's a brilliant idea, IMHO * Making that VMA MAP_PRIVATE? Probably a bad idea in hindsight. The process itself can't access this memory, let alone write to it. So I didn't think too much about whether to make it shared or private. Either way, we are not causing any actual COW on these mappings because they are not writable, and we never make them writable with mprotect either * Introducing a COW check after letting it slide for 15 years? It's a reasonable check. In this case it catches a false positive. Had this check been in place 4 or 5 years ago, it would have easily prevented this mess
I think we have only two options here:
- Accept the breakage of thunk.
Really?
- Add a workaround in amdgpu/amdkfd to force MAP_PRIVATE into
MAP_SHARED in the kernel.
I tried to do this in amdgpu_gem_object_mmap and spent 4 hours debugging why it doesn't work. It breaks because the mapping->i_mmap_writable gets unbalanced and causes subsequent mappings to fail when that atomic counter becomes negative (indicating DENYWRITE). I could fix it by calling mapping_map_writable. But I don't think this is intended as driver API. I see no precedent of any driver calling this. For the record this works, but it feels fragile and ugly:
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -255,6 +255,20 @@ static int amdgpu_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_str if (bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) return -EPERM;
+ /* Workaround for Thunk bug creating PROT_NONE,MAP_PRIVATE mappings + * for debugger access to invisible VRAM. Should have used MAP_SHARED + * instead. + */ + if (is_cow_mapping(vma->vm_flags) && + !(vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC))) { + int ret; + + ret = mapping_map_writable(vma->vm_file->f_mapping); + if (ret) + return ret; + vma->vm_flags |= VM_SHARED | VM_MAYSHARE; + } + return drm_gem_ttm_mmap(obj, vma); }
3. Improve my previous workaround by adding a similar check for COW in ttm_bo_vm_ops.mprotect. I'll send an updated patch.
Regards, Felix
Regards, Christian.
Regards, Felix
Christian.
return -EINVAL; ttm_bo_get(bo);
Am 10.07.21 um 02:29 schrieb Felix Kuehling:
On 2021-07-09 3:37 p.m., Christian König wrote:
Am 09.07.21 um 21:31 schrieb Felix Kuehling:
On 2021-07-09 2:38 a.m., Christian König wrote:
Am 08.07.21 um 21:36 schrieb Alex Deucher:
From: Felix Kuehling Felix.Kuehling@amd.com
KFD Thunk maps invisible VRAM BOs with PROT_NONE, MAP_PRIVATE. is_cow_mapping returns true for these mappings. Add a check for vm_flags & VM_WRITE to avoid mmap failures on private read-only or PROT_NONE mappings.
I'm pretty sure that this is not working as expected.
Not sure what you mean. Debugger access to the memory through the PROT_NONE VMAs is definitely working, with both ptrace and /proc/<pid>/mem.
Fixes: f91142c62161 ("drm/ttm: nuke VM_MIXEDMAP on BO mappings v3") Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com Signed-off-by: Alex Deucher alexander.deucher@amd.com
drivers/gpu/drm/ttm/ttm_bo_vm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index f56be5bc0861..a75e90c7d4aa 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -552,7 +552,7 @@ static const struct vm_operations_struct ttm_bo_vm_ops = { int ttm_bo_mmap_obj(struct vm_area_struct *vma, struct ttm_buffer_object *bo) { /* Enforce no COW since would have really strange behavior with it. */ - if (is_cow_mapping(vma->vm_flags)) + if (is_cow_mapping(vma->vm_flags) && (vma->vm_flags & VM_WRITE))
is_cow_mapping() already checks for VM_MAYWRITE, so this here shouldn't be necessary.
AFAICT, VM_MAYWRITE is not based on the PROT_... bits used to create the VMA, but based on the permissions of the file. So as long as the render node is writable, VM_MAYWRITE is set for all VMAs that map it.
I would agree that it's probably a bad idea for the Thunk to map these VMAs with MAP_PRIVATE. We can try changing the Thunk to use MAP_SHARED for these PROT_NONE mappings. But that doesn't change the fact that this kernel patch broke existing usermode.
For the record, changing the Thunk to use MAP_SHARED works.
Yeah, but see the discussion around MAP_PRIVATE and COW regarding this. What Thunk did here was a really bad idea.
Hindsight ... Which part was a bad idea?
* Creating a PROT_NONE VMA? That's necessary to let ptrace or /proc/<pid>/mem access the memory. It's a brilliant idea, IMHO * Making that VMA MAP_PRIVATE? Probably a bad idea in hindsight. The process itself can't access this memory, let alone write to it. So I didn't think too much about whether to make it shared or private. Either way, we are not causing any actual COW on these mappings because they are not writable, and we never make them writable with mprotect either * Introducing a COW check after letting it slide for 15 years? It's a reasonable check. In this case it catches a false positive. Had this check been in place 4 or 5 years ago, it would have easily prevented this mess
A combination of everything.
MAP_PRIVATE would have previously caused very odd behavior and/or a BUG_ON()/WARN_ON() in the VMA code (depending on platform).
MAP_PRIVATE + PROT_NONE kind of worked because it never faulted a page so we never actually triggered a COW.
I think we have only two options here:
- Accept the breakage of thunk.
Really?
Only as last resort, I'm running out of ideas how to fix this cleanly.
- Add a workaround in amdgpu/amdkfd to force MAP_PRIVATE into
MAP_SHARED in the kernel.
I tried to do this in amdgpu_gem_object_mmap and spent 4 hours debugging why it doesn't work. It breaks because the mapping->i_mmap_writable gets unbalanced and causes subsequent mappings to fail when that atomic counter becomes negative (indicating DENYWRITE). I could fix it by calling mapping_map_writable. But I don't think this is intended as driver API. I see no precedent of any driver calling this. For the record this works, but it feels fragile and ugly:
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -255,6 +255,20 @@ static int amdgpu_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_str if (bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) return -EPERM;
+ /* Workaround for Thunk bug creating PROT_NONE,MAP_PRIVATE mappings + * for debugger access to invisible VRAM. Should have used MAP_SHARED + * instead. + */ + if (is_cow_mapping(vma->vm_flags) && + !(vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC))) { + int ret;
+ ret = mapping_map_writable(vma->vm_file->f_mapping); + if (ret) + return ret; + vma->vm_flags |= VM_SHARED | VM_MAYSHARE; + }
Yeah, I'm on another thread where we are discussing mapping_map_writable() for vma_set_file() as well.
return drm_gem_ttm_mmap(obj, vma); }
- Improve my previous workaround by adding a similar check for COW in
ttm_bo_vm_ops.mprotect. I'll send an updated patch.
That has it's own issues, but going to reply there in detail.
Regards, Christian.
Regards, Felix
Regards, Christian.
Regards, Felix
Christian.
return -EINVAL; ttm_bo_get(bo);
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Christian König -
Daniel Vetter -
Felix Kuehling