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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
