Re: [PATCH 1/4] mm: vm_page_prot: update with WRITE_ONCE/READ_ONCE

[Andrea Arcangeli <aarcange@redhat.com>]

Hello Hillf,

On Thu, Sep 22, 2016 at 03:17:52PM +0800, Hillf Danton wrote:
> Hey Andrea
> > 
> > @@ -111,13 +111,16 @@ static pgprot_t vm_pgprot_modify(pgprot_t oldprot, unsigned long vm_flags)
> >  void vma_set_page_prot(struct vm_area_struct *vma)
> >  {
> >  	unsigned long vm_flags = vma->vm_flags;
> > +	pgprot_t vm_page_prot;
> > 
> > -	vma->vm_page_prot = vm_pgprot_modify(vma->vm_page_prot, vm_flags);
> > +	vm_page_prot = vm_pgprot_modify(vma->vm_page_prot, vm_flags);
> >  	if (vma_wants_writenotify(vma)) {
> 
> Since vma->vm_page_prot is currently used in vma_wants_writenotify(), is 
> it possible that semantic change is introduced here with local variable? 

>From a short review I think you're right.

Writing an intermediate value with WRITE_ONCE before clearing
VM_SHARED wouldn't be correct either if the "vma" was returned by
vma_merge, so to fix this, the intermediate vm_page_prot needs to be
passed as parameter to vma_wants_writenotify(vma, vm_page_prot).

For now it's safer to drop this patch 1/4. The atomic setting of
vm_page_prot in mprotect is an orthogonal problem to the vma_merge
case8 issues in the other patches. The side effect would be the same
("next" vma ptes going out of sync with the write bit set, because
vm_page_prot was the intermediate value created with VM_SHARED still
set in vm_flags) but it's not a bug in vma_merge/vma_adjust here.

I can correct and resend this one later.

While at it, I've to say the handling of VM_SOFTDIRTY across vma_merge
also seems dubious when it's not mmap_region calling vma_merge but
that would be yet another third orthogonal problem, so especially that
one should be handled separately as it'd be specific to soft dirty
only, the atomicity issue above is somewhat more generic.

On a side note, the fix for vma_merge in -mm changes nothing in regard
of the above or soft dirty, they're orthogonal issues.

Thanks,
Andrea

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