On Thu, 2015-09-03 at 01:21 +0900, sf...@users.sourceforge.net wrote: > sf...@users.sourceforge.net: > > > > Xavier Chantry: > > > I can reproduce the bug on all my machines, so it shouldn't be too > > > difficult to reproduce. Applying the following hunk is enough to trigger > > > the bug. > > > > Unfortunately still I don't have enough time. > > I understand this bug has a big impact for you and any other user who > > have applied aufs3-mmap.patch. As soon as I get the time, I'm sure I > > will dive into this problem. > > I've thought about the problem a little (still I'm busy). > It looks like a race condition around mm->mmap_sem. > > - get_file(file); > + vma_get_file(vma); > up_read(&mm->mmap_sem); > error = vfs_fsync(file, 0); > - fput(file); > + vma_fput(vma); > if (error || start >= end) > goto out; > down_read(&mm->mmap_sem); > > Since this fput/vma_fput is between up(mmap_sem) and down(mmap_sem), > vma_fput() is not protected, and got vm_file NULL unexpectedly. It > means vma->vm_file is changed (or being changed) by someone else (other > thread in your case) after up_read(&mm->mmap_sem). This is surely a bug > in aufs[34]-mmap.patch. It shoule be done such like this essentially. > get_file(file); > pr = vma->vm_prfile; > ;;; > fput(file); > if (pr) > fput(pr); > > The fix will look a little smarter than above. > I hope I can post the fix in a few weeks.
I don't see any need for holding a reference to vma->vm_prfile here. There is also a similar bug in madvise_remove() which I can trigger by calling madvise(..., MADV_REMOVE) in parallel with another thread that does mmap() and munmap() of the same address range. This patch against 3.16 fixes both for me: From: Ben Hutchings <b...@decadent.org.uk> Date: Thu, 10 Sep 2015 02:19:59 +0100 Subject: aufs3: mmap: Fix races in madvise_remove() and sys_msync() Bug-Debian: https://bugs.debian.org/796036 In madvise_remove() and sys_msync() we drop the mmap_sem before dropping references to the mapped file(s). As soon as we drop the mmap_sem, the vma we got them from might be destroyed by another thread, so calling vma_do_fput() is a possible use-after-free. In these cases we don't actually need a reference to the aufs file, so revert to using get_file() and fput() directly. Signed-off-by: Ben Hutchings <b...@decadent.org.uk> --- --- a/mm/madvise.c +++ b/mm/madvise.c @@ -327,12 +327,12 @@ static long madvise_remove(struct vm_are * vma's reference to the file) can go away as soon as we drop * mmap_sem. */ - vma_get_file(vma); + get_file(f); up_read(¤t->mm->mmap_sem); error = do_fallocate(f, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, offset, end - start); - vma_fput(vma); + fput(f); down_read(¤t->mm->mmap_sem); return error; } --- a/mm/msync.c +++ b/mm/msync.c @@ -84,13 +84,13 @@ SYSCALL_DEFINE3(msync, unsigned long, st start = vma->vm_end; if ((flags & MS_SYNC) && file && (vma->vm_flags & VM_SHARED)) { - vma_get_file(vma); + get_file(file); up_read(&mm->mmap_sem); if (vma->vm_flags & VM_NONLINEAR) error = vfs_fsync(file, 1); else error = vfs_fsync_range(file, fstart, fend, 1); - vma_fput(vma); + fput(file); if (error || start >= end) goto out; down_read(&mm->mmap_sem); --- END --- This bug has some security impact (at least a minor DoS, but possible privilege escalation) so I'm going to request a CVE ID for it. Ben. ------------------------------------------------------------------------------ Monitor Your Dynamic Infrastructure at Any Scale With Datadog! Get real-time metrics from all of your servers, apps and tools in one place. SourceForge users - Click here to start your Free Trial of Datadog now! http://pubads.g.doubleclick.net/gampad/clk?id=241902991&iu=/4140
