linux-kernel - Re: [RFC PATCH 1/8] mm/madvise: propagate vma->vm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210927115507.6xfpugeg3swookbh@box>
Date:   Mon, 27 Sep 2021 14:55:07 +0300
From:   "Kirill A. Shutemov" <kirill@...temov.name>
To:     Nadav Amit <nadav.amit@...il.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Linux-MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Peter Xu <peterx@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Minchan Kim <minchan@...nel.org>,
        Colin Cross <ccross@...gle.com>,
        Suren Baghdasarya <surenb@...gle.com>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>
Subject: Re: [RFC PATCH 1/8] mm/madvise: propagate vma->vm_end changes

On Mon, Sep 27, 2021 at 03:11:20AM -0700, Nadav Amit wrote:
> 
> > On Sep 27, 2021, at 2:08 AM, Kirill A. Shutemov <kirill@...temov.name> wrote:
> > 
> > On Sun, Sep 26, 2021 at 09:12:52AM -0700, Nadav Amit wrote:
> >> From: Nadav Amit <namit@...are.com>
> >> 
> >> The comment in madvise_dontneed_free() says that vma splits that occur
> >> while the mmap-lock is dropped, during userfaultfd_remove(), should be
> >> handled correctly, but nothing in the code indicates that it is so: prev
> >> is invalidated, and do_madvise() will therefore continue to update VMAs
> >> from the "obsolete" end (i.e., the one before the split).
> >> 
> >> Propagate the changes to end from madvise_dontneed_free() back to
> >> do_madvise() and continue the updates from the new end accordingly.
> > 
> > Could you describe in details a race that would lead to wrong behaviour?
> 
> Thanks for the quick response.
> 
> For instance, madvise(MADV_DONTNEED) can race with mprotect() and cause
> the VMA to split.
> 
> Something like:
> 
>   CPU0				CPU1
>   ----				----
>   madvise(0x10000, 0x2000, MADV_DONTNEED)
>   -> userfaultfd_remove()
>    [ mmap-lock dropped ]
> 				mprotect(0x11000, 0x1000, PROT_READ)
> 				[splitting the VMA]
> 
> 				read(uffd)
> 				[unblocking userfaultfd_remove()]
> 
>    [ resuming ]
>    end = vma->vm_end
>    [end == 0x11000]
> 
>    madvise_dontneed_single_vma(vma, 0x10000, 0x11000)
> 
>   Following this operation, 0x11000-0x12000 would not be zapped.

Okay, fair enough.

Wouldn't something like this work too:

diff --git a/mm/madvise.c b/mm/madvise.c
index 0734db8d53a7..0898120c5c04 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -796,6 +796,7 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
 			 */
 			return -ENOMEM;
 		}
+		*prev = vma;
 		if (!can_madv_lru_vma(vma))
 			return -EINVAL;
 		if (end > vma->vm_end) {
-- 
 Kirill A. Shutemov