Crash in 2.4.17/ptrace

I've been debugging frame buffer graphics lately, and encountering a very annoying problem. If the debugee has /dev/fb/0 mapped, and I try to print out the contents of a pointer into that buffer, GDB crashes in kernel/ptrace.c:access_process_vm. The problem seems to be that get_user_pages returns a NULL page. Something as simple as this prevents the crash:

kernel-source-2.4.17/kernel/ptrace.c Mon Jan 28 15:30:39 2002 renamed from 2.4.18-pre7/kernel/ptrace.c Fri Dec 21 12:42:04 2001 +5 -0

@@ -160,6 +160,18 @@ int access_process_vm(struct task_struct
 
                flush_cache_page(vma, addr);
 
+#if 1
+               if (!page)
+               {
+                       /* FIXME: Writes? */
+                       if (!write) memset (buf, 0, bytes);

Of course, I would much rather be able to see the contents of the framebuffer. Any suggestions?

Oh nice. And it seems that, say, an O_DIRECT write of, say, a mmaped framebuffer will also oops the kernel.

Most callers of get_user_pages() aren't prepared for a null page* in the returned array.

This patch *may* be sufficient, but perhaps get_user_pages() should just bale out as soon as it finds an invalid page, rather than sticking a null page * into the returned array and continuing.

Quoted 2 lines

Of course, I would much rather be able to see the contents of the framebuffer. Any suggestions?

Not with this patch, I'm afraid. For your testing purposes you could just remove the VALID_PAGE() test in mm/memory.c:get_page_map(), and then gdb should be able to get at the framebuffer.

Frame buffers aren't reliable marked VM_IO when mapped, currently. Ben H. said he was going to push a fix for this at least to the PPC trees today or tomorrow.

It's cute - fbmem.c goes out of its way to set the flag on some architectures and not others. I can't imagine why.

But with that, yes, that should fix it.

Quoted 3 lines

Quoted 2 lines

Of course, I would much rather be able to see the contents of the framebuffer. Any suggestions?

Quoted 3 lines

Not with this patch, I'm afraid. For your testing purposes you could just remove the VALID_PAGE() test in mm/memory.c:get_page_map(), and then gdb should be able to get at the framebuffer.

I'm sure there's a good reason to not do that in general. Mind enlightening me?

In the O_DIRECT/map_user_kiobuf case, we could end up asking the disk controller to perform busmastering against the video PCI device, which will probably explode somewhere down the chain.

Well, get_user_pages is used by several parts of the kernel. In the O_DIRECT/map_user_kiobuf case, we could end up asking the disk controller to perform busmastering against the video PCI device, which will probably explode somewhere down the chain.

Also, just because the hardware is mapped into the process virtual address space, it's not necessarily all accessible. It is possible to get a bus fault against part of the mapping. And the kernel doesn't expect to get bus faults on the source of copy_to_user, I think.

I'm sure Andrea will have a better notion than I. Sometimes I just fling out random patches to get people thinking about things ;)

Quoted 4 lines

Well, get_user_pages is used by several parts of the kernel. In the O_DIRECT/map_user_kiobuf case, we could end up asking the disk controller to perform busmastering against the video PCI device, which will probably explode somewhere down the chain.

Well... not sure. I'd like this to be doable. I have worked on some high-end broadcast video stuffs in the past, and we did intensive use of direct bus master from the disk controller to the framebuffer linear aperture. Actually, we even controlled

For your testing purposes you
could just remove the VALID_PAGE() test in mm/memory.c:get_page_map(), and then gdb should be able to get at the framebuffer.

Quoted 4 lines · Daniel Jacobowitz

Frame buffers aren't reliable marked VM_IO when mapped, currently. Ben H. said he was going to push a fix for this at least to the PPC trees today or tomorrow.

They are now, I hope. I fixed that in 2.4.18-pre2.
drivers/video/fbmem.c:fb_mmap() marks the vma as
VM_IO for all architectures. But perhaps I missed some; an audit is needed in there, which I'll do.

It marks all framebuffer mappings as VM_IO. This prevents kernel deadlocks which can occur when a program which has a framebuffer mapping attempts to dump core.

I've Cc'ed linux-fbdev-devel. Could someone please review?

--- linux-2.4.18-pre7/drivers/video/acornfb.c Thu Nov 22 23:02:58 2001

Quoted 4 lines

It marks all framebuffer mappings as VM_IO. This prevents kernel deadlocks which can occur when a program which has a framebuffer mapping attempts to dump core.

Good this is needed.

Quoted 4 lines

I've Cc'ed linux-fbdev-devel. Could someone please review?

--- linux-2.4.18-pre7/drivers/video/acornfb.c Thu Nov 22 23:02:58 2001

You missed the atyfb driver. Other then that it looks fine. Geert what do you think?

Ah, thanks. Also I missed sbusfb.c (not that it was fatal - the driver got it right anyway).

Here's the current patch. After some discussion with Ben Herrenschmidt, it now sets VM_IO against the vma *before* calling the subdriver's mmap method, just in case the driver really does want to clear that flag (presumably, a shadow buffer in main memory).

drivers/video/fbmem.c +10 -0

@@ -541,6 +541,16 @@ fb_mmap(struct file *file, struct vm_are
 	if (fb->fb_mmap) {
 		int res;
 		lock_kernel();
+		/*
+		 * This is an IO map - tell maydump to skip this VMA.
+		 * If, for some reason, the sub-driver wishes to allow the
+		 * mapping to be dumpable and accessible via ptrace then
+		 * it may clear VM_IO later.
+		 * (This only makes sense if the buffer is actually
+		 * in main memory, and is described by a page struct in
+		 * mem_map[])
+		 */
+		vma->vm_flags |= VM_IO;
 		res = fb->fb_mmap(info, file, vma);
 		unlock_kernel();
 		return res;

Quoted 15 lines · Andrew Morton

Oh nice. And it seems that, say, an O_DIRECT write of, say, a mmaped framebuffer will also oops the kernel.

Most callers of get_user_pages() aren't prepared for a null page* in the returned array.

This patch *may* be sufficient, but perhaps get_user_pages() should just bale out as soon as it finds an invalid page, rather than sticking a null page * into the returned array and continuing.

mm/memory.c +0 -0

@@ -453,6 +453,7 @@ int get_user_pages(struct task_struct *t
 		vma = find_extend_vma(mm, start);
 

mm/memory.c int get_user_pages(struct task_struct *t · line 459

Quoted 5 lines

mm/memory.c +1 -0

 		if ( !vma ||
+		     (vma->vm_flags & VM_IO) ||
 		    (!force &&
 		     	((write && (!(vma->vm_flags & VM_WRITE))) ||
 		    	 (!write && (!(vma->vm_flags & VM_READ))) ) )) {

Frame buffers aren't reliable marked VM_IO when mapped, currently. Ben

H. said he was going to push a fix for this at least to the PPC trees today or tomorrow.

It's cute - fbmem.c goes out of its way to set the flag on some architectures and not others. I can't imagine why.

But with that, yes, that should fix it.

Quoted 3 lines

Quoted 2 lines

Of course, I would much rather be able to see the contents of the framebuffer. Any suggestions?

Quoted 3 lines

Not with this patch, I'm afraid. For your testing purposes you could just remove the VALID_PAGE() test in mm/memory.c:get_page_map(), and then gdb should be able to get at the framebuffer.

I'm sure there's a good reason to not do that in general. Mind enlightening me?

Signature · 2 lines

Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Quoted 4 lines · Daniel Jacobowitz

Frame buffers aren't reliable marked VM_IO when mapped, currently. Ben H. said he was going to push a fix for this at least to the PPC trees today or tomorrow.

They are now, I hope. I fixed that in 2.4.18-pre2.
drivers/video/fbmem.c:fb_mmap() marks the vma as
VM_IO for all architectures. But perhaps I missed some; an audit is needed in there, which I'll do.

Quoted 8 lines

It's cute - fbmem.c goes out of its way to set the flag on some architectures and not others. I can't imagine why.

But with that, yes, that should fix it.

Quoted 3 lines

Quoted 2 lines

Of course, I would much rather be able to see the contents of the framebuffer. Any suggestions?

Quoted 6 lines

Quoted 3 lines

Not with this patch, I'm afraid. For your testing purposes you could just remove the VALID_PAGE() test in mm/memory.c:get_page_map(), and then gdb should be able to get at the framebuffer.

I'm sure there's a good reason to not do that in general. Mind enlightening me?

Well, get_user_pages is used by several parts of the kernel. In the O_DIRECT/map_user_kiobuf case, we could end up asking the disk controller to perform busmastering against the video PCI device, which will probably explode somewhere down the chain.

Also, just because the hardware is mapped into the process virtual address space, it's not necessarily all accessible. It is possible to get a bus fault against part of the mapping. And the kernel doesn't expect to get bus faults on the source of copy_to_user, I think.

I'm sure Andrea will have a better notion than I. Sometimes I just fling out random patches to get people thinking about things ;)

...
Well, I think your earlier suggestion to bale out with an error if an invalid page is found sounds like the cleaner fix (possibly in function of yet another bitflag, so if somebody wants to get the nearby pages regardless of an invalid pages somewhere, it can).