Re: [PATCH?] Crash in 2.4.17/ptrace

Re: [PATCH?] Crash in 2.4.17/ptrace

[Manfred Spraul]

> >
> > 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?

Please don't do it at all.
The test is there to ensure that there is a 'struct page' for address
found in the pages tables.
For framebuffers addresses, there is no page structure, and then the
page reference count updates read/write to random memory.

--
    Manfred

Re: [PATCH?] Crash in 2.4.17/ptrace

[Alan Cox]

> For framebuffers addresses, there is no page structure, and then the
> page reference count updates read/write to random memory.

If it is a physical pci bus object why do we need to refcount it, surely
"no page" is ok. Its just up to the driver not to do anything stupid and
the core code to honour the pci/pci transfer quirks (or when faced with
a hard one just say "no")

Re: [PATCH?] Crash in 2.4.17/ptrace

[Manfred Spraul]

From: "Alan Cox" <alan@lxorguk.ukuu.org.uk>
> > For framebuffers addresses, there is no page structure, and then the
> > page reference count updates read/write to random memory.
>
> If it is a physical pci bus object why do we need to refcount it,
> surely "no page" is ok. Its just up to the driver not to do anything
> stupid and the core code to honour the pci/pci transfer quirks (or
> when faced with a hard one just say "no")
>
With multi-threaded apps, munmap() during O_DIRECT is possible. vma
structures don't contain a reference count.

For ptrace, taking the physical address from the PTE and ioremapping it
temporarily would work, but ...

--
    Manfred

Re: [PATCH?] Crash in 2.4.17/ptrace

[Daniel Jacobowitz]

On Mon, Jan 28, 2002 at 10:05:31PM +0000, Alan Cox wrote:
> > For framebuffers addresses, there is no page structure, and then the
> > page reference count updates read/write to random memory.
> 
> If it is a physical pci bus object why do we need to refcount it, surely
> "no page" is ok. Its just up to the driver not to do anything stupid and
> the core code to honour the pci/pci transfer quirks (or when faced with
> a hard one just say "no")

So, is there a clear interface by which access_process_vm ought to be
able to get at the mapped framebuffer, or should get_user_pages just
punt off it?

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

[PATCH?] Crash in 2.4.17/ptrace

[Daniel Jacobowitz]

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:

--- 2.4.18-pre7/2.4.18-pre7/kernel/ptrace.c	Fri Dec 21 12:42:04 2001
+++ 2.4.17/kernel-source-2.4.17/kernel/ptrace.c	Mon Jan 28 15:30:39 2002
@@ -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);
+			len -= bytes;
+			buf += bytes;
+			continue;
+		}
+#endif
+
+
 		maddr = kmap(page);
 		if (write) {
 			memcpy(maddr + offset, buf, bytes);


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

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

Re: [PATCH?] Crash in 2.4.17/ptrace

[Andrew Morton]

Daniel Jacobowitz wrote:
> 
> 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:
> 
> --- 2.4.18-pre7/2.4.18-pre7/kernel/ptrace.c     Fri Dec 21 12:42:04 2001
> +++ 2.4.17/kernel-source-2.4.17/kernel/ptrace.c Mon Jan 28 15:30:39 2002
> @@ -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);
> +                       len -= bytes;
> +                       buf += bytes;
> +                       continue;
> +               }
> +#endif
> +
> +
>                 maddr = kmap(page);
>                 if (write) {
>                         memcpy(maddr + offset, buf, bytes);

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.

--- linux-2.4.18-pre7/mm/memory.c	Fri Dec 21 11:19:23 2001
+++ linux-akpm/mm/memory.c	Mon Jan 28 12:54:40 2002
@@ -453,6 +453,7 @@ int get_user_pages(struct task_struct *t
 		vma = find_extend_vma(mm, start);
 
 		if ( !vma ||
+		     (vma->vm_flags & VM_IO) ||
 		    (!force &&
 		     	((write && (!(vma->vm_flags & VM_WRITE))) ||
 		    	 (!write && (!(vma->vm_flags & VM_READ))) ) )) {

> 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.

-

Re: [PATCH?] Crash in 2.4.17/ptrace

[Daniel Jacobowitz]

On Mon, Jan 28, 2002 at 01:03:05PM -0800, Andrew Morton wrote:
> 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.
> 
> --- linux-2.4.18-pre7/mm/memory.c	Fri Dec 21 11:19:23 2001
> +++ linux-akpm/mm/memory.c	Mon Jan 28 12:54:40 2002
> @@ -453,6 +453,7 @@ int get_user_pages(struct task_struct *t
>  		vma = find_extend_vma(mm, start);
>  
>  		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.

> > 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.

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

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

Re: [PATCH?] Crash in 2.4.17/ptrace

[Andrew Morton]

Daniel Jacobowitz wrote:
> 
> 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'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.
> 
> > > 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.
> 
> 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 ;)

-

Re: [PATCH?] Crash in 2.4.17/ptrace

[Alan Cox]

> 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.

Actually we already know if this will fail or not. Check the pci quirks file
We set a bunch of flags for safety issues on pci<->pci transfers. I did that
anticipating one day more than just a few capture cards would care

Alan

Re: [PATCH?] Crash in 2.4.17/ptrace

[Andrew Morton]

Andrew Morton wrote:
> 
> Daniel Jacobowitz wrote:
> >
> > 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.

I lied.  Linus applied it, but not, it seems, Marcelo.

So.  Here's a patch.

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.

It also allows get_user_pages() to detect and skip these
IO mappings, so ptrace, O_DIRECT, etc will not permit
I/O against these mappings.

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
+++ linux-akpm/drivers/video/acornfb.c	Mon Jan 28 14:00:21 2002
@@ -1139,9 +1139,6 @@ acornfb_mmap(struct fb_info *info, struc
 	off += start;
 	vma->vm_pgoff = off >> PAGE_SHIFT;
 
-	/* This is an IO map - tell maydump to skip this VMA */
-	vma->vm_flags |= VM_IO;
-
 #ifdef CONFIG_CPU_32
 	pgprot_val(vma->vm_page_prot) &= ~L_PTE_CACHEABLE;
 #endif
--- linux-2.4.18-pre7/drivers/video/igafb.c	Thu Nov 22 23:02:58 2001
+++ linux-akpm/drivers/video/igafb.c	Mon Jan 28 14:02:10 2002
@@ -293,8 +293,6 @@ static int igafb_mmap(struct fb_info *in
 	if (!map_size)
 		return -EINVAL;
 
-	vma->vm_flags |= VM_IO;
-
 	if (!fb->mmaped) {
 		int lastconsole = 0;
 
--- linux-2.4.18-pre7/drivers/video/sgivwfb.c	Thu Nov 22 23:02:58 2001
+++ linux-akpm/drivers/video/sgivwfb.c	Mon Jan 28 14:02:49 2002
@@ -846,7 +846,6 @@ static int sgivwfb_mmap(struct fb_info *
     return -EINVAL;
   offset += sgivwfb_mem_phys;
   pgprot_val(vma->vm_page_prot) = pgprot_val(vma->vm_page_prot) | _PAGE_PCD;
-  vma->vm_flags |= VM_IO;
   if (remap_page_range(vma->vm_start, offset, size, vma->vm_page_prot))
     return -EAGAIN;
   vma->vm_file = file;
--- linux-2.4.18-pre7/drivers/video/fbmem.c	Fri Dec 21 11:19:14 2001
+++ linux-akpm/drivers/video/fbmem.c	Mon Jan 28 14:07:08 2002
@@ -543,6 +543,8 @@ fb_mmap(struct file *file, struct vm_are
 		lock_kernel();
 		res = fb->fb_mmap(info, file, vma);
 		unlock_kernel();
+		/* This is an IO map - tell maydump to skip this VMA */
+		vma->vm_flags |= VM_IO;
 		return res;
 	}
 
@@ -576,12 +578,13 @@ fb_mmap(struct file *file, struct vm_are
 		return -EINVAL;
 	off += start;
 	vma->vm_pgoff = off >> PAGE_SHIFT;
+	/* This is an IO map - tell maydump to skip this VMA */
+	vma->vm_flags |= VM_IO;
 #if defined(__sparc_v9__)
 	vma->vm_flags |= (VM_SHM | VM_LOCKED);
 	if (io_remap_page_range(vma->vm_start, off,
 				vma->vm_end - vma->vm_start, vma->vm_page_prot, 0))
 		return -EAGAIN;
-	vma->vm_flags |= VM_IO;
 #else
 #if defined(__mc68000__)
 #if defined(CONFIG_SUN3)
@@ -607,8 +610,6 @@ fb_mmap(struct file *file, struct vm_are
 	pgprot_val(vma->vm_page_prot) |= _CACHE_UNCACHED;
 #elif defined(__arm__)
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
-	/* This is an IO map - tell maydump to skip this VMA */
-	vma->vm_flags |= VM_IO;
 #elif defined(__sh__)
 	pgprot_val(vma->vm_page_prot) &= ~_PAGE_CACHABLE;
 #else

Re: [PATCH?] Crash in 2.4.17/ptrace

[Benjamin Herrenschmidt]

>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
the scatter gather list to "sort" lines ;)

If the HW cause a fault, the disk controller should stop and
report an error, and the process should be signaled instead
of getting an oops, but I don't know these code path in linux
at all so...

>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.

Well. My point of view here is fix copy_to_user, but well...

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

Ben.

Re: [PATCH?] Crash in 2.4.17/ptrace

[Andrea Arcangeli]

On Mon, Jan 28, 2002 at 11:15:28PM +0100, Benjamin Herrenschmidt wrote:
> >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

you can do direct DMA to framebuffer on most sane hardware, yes.  But
the kiobuf structure on 2.4 doesn't allow that, it only works with valid
'page structures' not physical addresses. This is valid for both rawio
and O_DIRECT. The MM part is the same for both of course.

Andrea

Re: [PATCH?] Crash in 2.4.17/ptrace

[Andrea Arcangeli]

On Mon, Jan 28, 2002 at 01:29:15PM -0800, Andrew Morton wrote:
> Daniel Jacobowitz wrote:
> > 
> > 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'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.
> > 
> > > > 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.
> > 
> > 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.

another basic problem about allowing map_user_kiobuf to succeed on
invalid pages, is that calling PageReserved/SetPageDirty on a null
pointer will crash, etc...

> 
> 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).

Andrea

Re: [PATCH?] Crash in 2.4.17/ptrace

[Andrew Morton]

Andrea Arcangeli wrote:
> 
> ...
> 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).
> 

I find it rather hard to decide about this.  get_user_pages()
leaves null page pointers in the page[] array for invalid
pages, and that's a reasonable API, as long as all callers
are actually aware of it....

In the O_DIRECT case, the kernel does not crash, because
brw_kiovec() does:

                        map  = iobuf->maplist[pageind];
                        if (!map) {
                                err = -EFAULT;
                                goto finished;
                        }

However, I think it _would_ crash if the first entry in the maplist[]
was non-null, and the second is null, because that would cause
generic_file_direct_IO() to call mark_dirty_kiobuf(), and
mark_dirty_kiobuf() forgets to check for NULL page *'s in the maplist[].

Given the difficulty of testing all this, and the dubious benefit
in allowing a holey maplist[], I'm inclined to just disallow it
in 2.4.   What do you think?



--- linux-2.4.18-pre7/mm/memory.c	Fri Dec 21 11:19:23 2001
+++ linux-akpm/mm/memory.c	Mon Jan 28 16:26:47 2002
@@ -453,6 +453,7 @@ int get_user_pages(struct task_struct *t
 		vma = find_extend_vma(mm, start);
 
 		if ( !vma ||
+		     (vma->vm_flags & VM_IO) ||
 		    (!force &&
 		     	((write && (!(vma->vm_flags & VM_WRITE))) ||
 		    	 (!write && (!(vma->vm_flags & VM_READ))) ) )) {
@@ -486,8 +487,9 @@ int get_user_pages(struct task_struct *t
 				/* FIXME: call the correct function,
 				 * depending on the type of the found page
 				 */
-				if (pages[i])
-					page_cache_get(pages[i]);
+				if (!pages[i])
+					goto bad_page;
+				page_cache_get(pages[i]);
 			}
 			if (vmas)
 				vmas[i] = vma;
@@ -497,7 +499,19 @@ int get_user_pages(struct task_struct *t
 		} while(len && start < vma->vm_end);
 		spin_unlock(&mm->page_table_lock);
 	} while(len);
+out:
 	return i;
+
+	/*
+	 * We found an invalid page in the VMA.  Release all we have
+	 * so far and fail.
+	 */
+bad_page:
+	spin_unlock(&mm->page_table_lock);
+	while (i--)
+		page_cache_release(pages[i]);
+	i = -EFAULT;
+	goto out;
 }
 
 /*

Re: [PATCH?] Crash in 2.4.17/ptrace

[Andrea Arcangeli]

On Mon, Jan 28, 2002 at 04:19:00PM -0500, Daniel Jacobowitz wrote:
> On Mon, Jan 28, 2002 at 01:03:05PM -0800, Andrew Morton wrote:
> > 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.
> > 
> > --- linux-2.4.18-pre7/mm/memory.c	Fri Dec 21 11:19:23 2001
> > +++ linux-akpm/mm/memory.c	Mon Jan 28 12:54:40 2002
> > @@ -453,6 +453,7 @@ int get_user_pages(struct task_struct *t
> >  		vma = find_extend_vma(mm, start);
> >  
> >  		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

For this reason (and also because there aren't only framebuffers mmapped
out there) I guess it's better to just add (yet another) flag to
get_user_pages, so that it fails with an error when it encounters a
page out of the mem_map array.

> 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.
> 
> > > 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.
> 
> I'm sure there's a good reason to not do that in general.  Mind
> enlightening me?
> 
> -- 
> Daniel Jacobowitz                           Carnegie Mellon University
> MontaVista Software                         Debian GNU/Linux Developer


Andrea

Re: [PATCH?] Crash in 2.4.17/ptrace

[Andrew Morton]

Andrew Morton wrote:
> 
>  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.
> 

Sorry, this won't work.  We'll just get a random page struct outside
mem_map[] and the kernel will die.