Re: [PATCH] mm: do_shared_fault: fix potential NULL pointer dereference

Re: [PATCH] mm: do_shared_fault: fix potential NULL pointer dereference

[Bob Liu]

On Sat, Mar 1, 2014 at 5:59 AM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 28 Feb 2014 10:07:45 +0100 Peter Zijlstra <peterz@infradead.org> wrote:
>
>> On Fri, Feb 28, 2014 at 01:59:59AM +0200, Kirill A. Shutemov wrote:
>> > On Thu, Feb 27, 2014 at 03:48:08PM -0800, Andrew Morton wrote:
>> > > On Thu, 27 Feb 2014 21:26:40 +0800 Bob Liu <lliubbo@gmail.com> wrote:
>> > >
>> > > > --- a/mm/memory.c
>> > > > +++ b/mm/memory.c
>> > > > @@ -3419,6 +3419,7 @@ static int do_shared_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>> > > >                 pgoff_t pgoff, unsigned int flags, pte_t orig_pte)
>> > > >  {
>> > > >         struct page *fault_page;
>> > > > +       struct address_space *mapping;
>> > > >         spinlock_t *ptl;
>> > > >         pte_t *pte;
>> > > >         int dirtied = 0;
>> > > > @@ -3454,13 +3455,14 @@ static int do_shared_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>> > > >
>> > > >         if (set_page_dirty(fault_page))
>> > > >                 dirtied = 1;
>> > > > +       mapping = fault_page->mapping;
>> > > >         unlock_page(fault_page);
>> > > > -       if ((dirtied || vma->vm_ops->page_mkwrite) && fault_page->mapping) {
>> > > > +       if ((dirtied || vma->vm_ops->page_mkwrite) && mapping) {
>> > > >                 /*
>> > > >                  * Some device drivers do not set page.mapping but still
>> > > >                  * dirty their pages
>> > > >                  */
>> > > > -               balance_dirty_pages_ratelimited(fault_page->mapping);
>> > > > +               balance_dirty_pages_ratelimited(mapping);
>> > > >         }
>> > > >
>> > > >         /* file_update_time outside page_lock */
>> > >
>> > > So from my reading of the email thread, this patch has issues: the
>> > > compiler can just undo what you did.
>> >
>> > If I read PeterZ correctly, we are fine with the fix since unlock_page()
>> > has release semantics.
>>
>> Yeah, the compiler should not re-read mapping after unlock_page(). That
>> said; it might be good to put a comment near there saying we actually
>> rely on this, and _why_ we rely on this.
>
> Like this?
>
> --- a/mm/memory.c~mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix
> +++ a/mm/memory.c
> @@ -3476,6 +3476,12 @@ set_pte:
>
>         if (set_page_dirty(fault_page))
>                 dirtied = 1;
> +       /*
> +        * Take a local copy of the address_space - page.mapping may be zeroed
> +        * by truncate after unlock_page().   The address_space itself remains
> +        * pinned by vma->vm_file's reference.  We rely on unlock_page()'s
> +        * release semantics to prevent the compiler from undoing this copying.
> +        */
>         mapping = fault_page->mapping;
>         unlock_page(fault_page);
>         if ((dirtied || vma->vm_ops->page_mkwrite) && mapping) {
>
> I don't actually know if that's true.  What *does* protect ->mapping
> from reclaim, drop_caches, etc?
>

I also puzzled what can protect ->mapping. This patch just change the
handling of shared-write-pagefault back to the same way as it used to
be. Perhaps we should add if(mapping==NULL) into
balance_dirty_pages_ratelimited() and balance_dirty_pages()?

BTW: Sasha, could you please have a test with this patch?

-- 
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: do_shared_fault: fix potential NULL pointer dereference

[Sasha Levin]

On 02/28/2014 10:14 PM, Bob Liu wrote:
> BTW: Sasha, could you please have a test with this patch?

Yup, it's running ever since you sent the patch.

I've seen the issue occur only once, so I'd like to leave it for at least over the weekend before 
confirming it's gone.


Thanks,
Sasha

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: do_shared_fault: fix potential NULL pointer dereference

[Andrew Morton]

On Sat, 1 Mar 2014 11:14:17 +0800 Bob Liu <lliubbo@gmail.com> wrote:

> >
> > --- a/mm/memory.c~mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix
> > +++ a/mm/memory.c
> > @@ -3476,6 +3476,12 @@ set_pte:
> >
> >         if (set_page_dirty(fault_page))
> >                 dirtied = 1;
> > +       /*
> > +        * Take a local copy of the address_space - page.mapping may be zeroed
> > +        * by truncate after unlock_page().   The address_space itself remains
> > +        * pinned by vma->vm_file's reference.  We rely on unlock_page()'s
> > +        * release semantics to prevent the compiler from undoing this copying.
> > +        */
> >         mapping = fault_page->mapping;
> >         unlock_page(fault_page);
> >         if ((dirtied || vma->vm_ops->page_mkwrite) && mapping) {
> >
> > I don't actually know if that's true.  What *does* protect ->mapping
> > from reclaim, drop_caches, etc?
> >
> 
> I also puzzled what can protect ->mapping.

Yes, ->mapping is pinned by the reference from vma->vm_file. 
vma->vm_file (and the vma itself) are protected by mmap_sem (held for
rear or for write).

I'll stick this in there, see what happens..

--- a/mm/memory.c~a
+++ a/mm/memory.c
@@ -3422,6 +3422,8 @@ static int do_shared_fault(struct mm_str
 	struct vm_fault vmf;
 	int ret, tmp;
 
+	WARN_ON_ONCE(!rwsem_is_locked(&mm->mmap_sem));
+
 	ret = __do_fault(vma, address, pgoff, flags, &fault_page);
 	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY)))
 		return ret;
_

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>