On Sun, Aug 24, 2014 at 02:00:11AM +0300, Kirill A. Shutemov wrote:
> On Sat, Aug 23, 2014 at 06:11:59PM -0400, Peter Feiner wrote:
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index dfc791c..f1a5382 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -851,8 +851,23 @@ static ssize_t clear_refs_write(struct file *file,
> > const char __user *buf,
> > if (type == CLEAR_REFS_MAPPED && !vma->vm_file)
> > continue;
> > if (type == CLEAR_REFS_SOFT_DIRTY) {
> > - if (vma->vm_flags & VM_SOFTDIRTY)
> > + if (vma->vm_flags & VM_SOFTDIRTY) {
>
> Why do we need the branch here. Does it save us anything?
> Looks like we can update vm_flags and enable writenotify unconditionally.
> Indentation level is high enough already.
You're right, we don't need the branch here. I'll change for v3.
> > vma->vm_flags &= ~VM_SOFTDIRTY;
> > + /*
> > + * We don't have a write lock on
> > + * mm->mmap_sem, so we race with the
> > + * fault handler reading vm_page_prot.
> > + * Therefore writable PTEs (that won't
> > + * have soft-dirty set) can be created
> > + * for read faults. However, since the
> > + * PTE lock is held while vm_page_prot
> > + * is read and while we write protect
> > + * PTEs during our walk, any writable
> > + * PTEs that slipped through will be
> > + * write protected.
> > + */
>
> Hm.. Isn't this yet another bug?
> Updating vma->vm_flags without down_write(&mm->mmap_sem) looks troublesome
> to me. Am I wrong?
As I said in the comment, it looks fishy but we're still fixing the bug. That
is, no writable PTEs will sneak by that don't have soft-dirty set.
I was originally going to submit something that dropped the mmap_sem and
re-took it in write mode before manipulating vm_page_prot. The control flow was
slightly hairy, so I convinced myself that the race is benign :-)
If I'm right and the race is benign, it still might be worth having the more
straightforward & obviously correct implementation since this isn't performance
critical code.
> > +/* Enable write notifications without blowing away special flags. */
> > +static inline void vma_enable_writenotify(struct vm_area_struct *vma)
> > +{
> > + vma->vm_page_prot = pgprot_modify(vma->vm_page_prot,
> > + vm_get_page_prot(vma->vm_flags &
> > + ~VM_SHARED));
>
> I think this way is more readable:
>
> pgprot_t newprot;
> newprot = vm_get_page_prot(vma->vm_flags & ~VM_SHARED);
> vma->vm_page_prot = pgprot_modify(vma->vm_page_prot, newprot);
>
Looks good. I'll update.
> > +}
> > +
> > +/* Disable write notifications without blowing away special flags. */
> > +static inline void vma_disable_writenotify(struct vm_area_struct *vma)
> > +{
> > + vma->vm_page_prot = pgprot_modify(vma->vm_page_prot,
> > + vm_get_page_prot(vma->vm_flags));
>
> ditto.
I'll change this too.
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
