Re: 2.6.23 regression: accessing invalid mmap'ed memory from gdb causes unkillable spinning

[Nick Piggin]

On Wed, Oct 31, 2007 at 08:11:10AM -0700, Linus Torvalds wrote:
> 
> 
> On Wed, 31 Oct 2007, Nick Piggin wrote:
> > 
> > However I actually don't really like how this all works. I don't like that
> > filemap.c should have to know about ptrace, or exactly what ptrace wants here.
> 
> It shouldn't. It should just fail when it fails. Then, handle_mm_fault() 
> should return an error code, which should cause get_user_pages() to return 
> an error code. Which should make ptrace just stop.
> 
> So I think your patch is wrong. mm/filemap.c should *not* care about who 
> does the fault.  I think the proper patch is something untested like the 
> appended...

Well the patch is right, in the context of the regression I introduced
(and so it should probably go into 2.6.23).

I'd love to get rid of that outside data content crap if possible in
2.6.24. I think you're the one who has the best feeling for the ptrace
cases we have in the VM, so I trust you ;)


> > It's a bit hairy to force insert page into pagecache and pte into pagetables
> > here, given the races.
> 
> It's also wrong. They shouldn't be in the page cache, since that can cause 
> problems with truncate etc. Maybe it doesn't any more, but it's reasonable 
> to believe that a page outside of i_size should not exist.

No I believe it could still be a problem (and at least, it is quite
fragile).

 
> > In access_process_vm, can't we just zero fill in the case of a sigbus? Linus?
> > That will also avoid changing applicatoin behaviour due to a gdb read...
> 
> access_process_vm() should just return how many bytes it could fill (which 
> means a truncated copy - very including zero bytes - for an error), and 
> the caller should decide what the right thing to do is.
> 
> But who knows, maybe I missed something.

I hope it works.
 

> Duane? Does this fix things for you?
> 
> 			Linus
> 
> ---
>  mm/filemap.c |   13 ++-----------
>  1 files changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 9940895..188cf5f 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1300,7 +1300,7 @@ int filemap_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>  
>  	size = (i_size_read(inode) + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
>  	if (vmf->pgoff >= size)
> -		goto outside_data_content;
> +		return VM_FAULT_SIGBUS;
>  
>  	/* If we don't want any read-ahead, don't bother */
>  	if (VM_RandomReadHint(vma))
> @@ -1377,7 +1377,7 @@ retry_find:
>  	if (unlikely(vmf->pgoff >= size)) {
>  		unlock_page(page);
>  		page_cache_release(page);
> -		goto outside_data_content;
> +		return VM_FAULT_SIGBUS;
>  	}
>  
>  	/*
> @@ -1388,15 +1388,6 @@ retry_find:
>  	vmf->page = page;
>  	return ret | VM_FAULT_LOCKED;
>  
> -outside_data_content:
> -	/*
> -	 * An external ptracer can access pages that normally aren't
> -	 * accessible..
> -	 */
> -	if (vma->vm_mm == current->mm)
> -		return VM_FAULT_SIGBUS;
> -
> -	/* Fall through to the non-read-ahead case */
>  no_cached_page:
>  	/*
>  	 * We're only likely to ever get here if MADV_RANDOM is in
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/