| Message ID | 20211110113334.1.I1687e716adb2df746da58b508db3f25423c40b27@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: Fix mmap to include VM_IO and VM_DONTDUMP | expand |
Quoting Douglas Anderson (2021-11-10 11:33:42) > In commit 510410bfc034 ("drm/msm: Implement mmap as GEM object > function") we switched to a new/cleaner method of doing things. That's > good, but we missed a little bit. > > Before that commit, we used to _first_ run through the > drm_gem_mmap_obj() case where `obj->funcs->mmap()` was NULL. That meant > that we ran: > > vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; > vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); > vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); > > ...and _then_ we modified those mappings with our own. Now that > `obj->funcs->mmap()` is no longer NULL we don't run the default > code. It looks like the fact that the vm_flags got VM_IO / VM_DONTDUMP > was important because we're now getting crashes on Chromebooks that > use ARC++ while logging out. Specifically a crash that looks like this > (this is on a 5.10 kernel w/ relevant backports but also seen on a > 5.15 kernel): > > Unable to handle kernel paging request at virtual address ffffffc008000000 > Mem abort info: > ESR = 0x96000006 > EC = 0x25: DABT (current EL), IL = 32 bits > SET = 0, FnV = 0 > EA = 0, S1PTW = 0 > Data abort info: > ISV = 0, ISS = 0x00000006 > CM = 0, WnR = 0 > swapper pgtable: 4k pages, 39-bit VAs, pgdp=000000008293d000 > [ffffffc008000000] pgd=00000001002b3003, p4d=00000001002b3003, > pud=00000001002b3003, pmd=0000000000000000 > Internal error: Oops: 96000006 [#1] PREEMPT SMP > [...] > CPU: 7 PID: 15734 Comm: crash_dump64 Tainted: G W 5.10.67 #1 [...] > Hardware name: Qualcomm Technologies, Inc. sc7280 IDP SKU2 platform (DT) > pstate: 80400009 (Nzcv daif +PAN -UAO -TCO BTYPE=--) > pc : __arch_copy_to_user+0xc0/0x30c > lr : copyout+0xac/0x14c > [...] > Call trace: > __arch_copy_to_user+0xc0/0x30c > copy_page_to_iter+0x1a0/0x294 > process_vm_rw_core+0x240/0x408 > process_vm_rw+0x110/0x16c > __arm64_sys_process_vm_readv+0x30/0x3c > el0_svc_common+0xf8/0x250 > do_el0_svc+0x30/0x80 > el0_svc+0x10/0x1c > el0_sync_handler+0x78/0x108 > el0_sync+0x184/0x1c0 > Code: f8408423 f80008c3 910020c6 36100082 (b8404423) > > Let's add the two flags back in. > > While we're at it, the fact that we aren't running the default means > that we _don't_ need to clear out VM_PFNMAP, so remove that and save > an instruction. > > NOTE: it was confirmed that VM_IO was the important flag to fix the > problem I was seeing, but adding back VM_DONTDUMP seems like a sane > thing to do so I'm doing that too. > > Fixes: 510410bfc034 ("drm/msm: Implement mmap as GEM object function") > Reported-by: Stephen Boyd <swboyd@chromium.org> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org> Tested-by: Stephen Boyd <swboyd@chromium.org>
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index 6b03e00cc5f2..ae18bfb22502 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -1056,8 +1056,7 @@ static int msm_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struct { struct msm_gem_object *msm_obj = to_msm_bo(obj); - vma->vm_flags &= ~VM_PFNMAP; - vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND; + vma->vm_flags |= VM_IO | VM_MIXEDMAP | VM_DONTEXPAND | VM_DONTDUMP; vma->vm_page_prot = msm_gem_pgprot(msm_obj, vm_get_page_prot(vma->vm_flags)); return 0;
In commit 510410bfc034 ("drm/msm: Implement mmap as GEM object function") we switched to a new/cleaner method of doing things. That's good, but we missed a little bit. Before that commit, we used to _first_ run through the drm_gem_mmap_obj() case where `obj->funcs->mmap()` was NULL. That meant that we ran: vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); ...and _then_ we modified those mappings with our own. Now that `obj->funcs->mmap()` is no longer NULL we don't run the default code. It looks like the fact that the vm_flags got VM_IO / VM_DONTDUMP was important because we're now getting crashes on Chromebooks that use ARC++ while logging out. Specifically a crash that looks like this (this is on a 5.10 kernel w/ relevant backports but also seen on a 5.15 kernel): Unable to handle kernel paging request at virtual address ffffffc008000000 Mem abort info: ESR = 0x96000006 EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0 EA = 0, S1PTW = 0 Data abort info: ISV = 0, ISS = 0x00000006 CM = 0, WnR = 0 swapper pgtable: 4k pages, 39-bit VAs, pgdp=000000008293d000 [ffffffc008000000] pgd=00000001002b3003, p4d=00000001002b3003, pud=00000001002b3003, pmd=0000000000000000 Internal error: Oops: 96000006 [#1] PREEMPT SMP [...] CPU: 7 PID: 15734 Comm: crash_dump64 Tainted: G W 5.10.67 #1 [...] Hardware name: Qualcomm Technologies, Inc. sc7280 IDP SKU2 platform (DT) pstate: 80400009 (Nzcv daif +PAN -UAO -TCO BTYPE=--) pc : __arch_copy_to_user+0xc0/0x30c lr : copyout+0xac/0x14c [...] Call trace: __arch_copy_to_user+0xc0/0x30c copy_page_to_iter+0x1a0/0x294 process_vm_rw_core+0x240/0x408 process_vm_rw+0x110/0x16c __arm64_sys_process_vm_readv+0x30/0x3c el0_svc_common+0xf8/0x250 do_el0_svc+0x30/0x80 el0_svc+0x10/0x1c el0_sync_handler+0x78/0x108 el0_sync+0x184/0x1c0 Code: f8408423 f80008c3 910020c6 36100082 (b8404423) Let's add the two flags back in. While we're at it, the fact that we aren't running the default means that we _don't_ need to clear out VM_PFNMAP, so remove that and save an instruction. NOTE: it was confirmed that VM_IO was the important flag to fix the problem I was seeing, but adding back VM_DONTDUMP seems like a sane thing to do so I'm doing that too. Fixes: 510410bfc034 ("drm/msm: Implement mmap as GEM object function") Reported-by: Stephen Boyd <swboyd@chromium.org> Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/gpu/drm/msm/msm_gem.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)