linux-kernel - Re: [PATCH v6] hugetlb: don't delete vma_lock in hugetlb MADV

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] [day] [month] [year] [list]

Message-ID: <Y2B69ViNMYHvPXYk@monkey>
Date:   Mon, 31 Oct 2022 18:48:37 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Nadav Amit <nadav.amit@...il.com>
Cc:     Linux-MM <linux-mm@...ck.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        Naoya Horiguchi <naoya.horiguchi@...ux.dev>,
        David Hildenbrand <david@...hat.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Mina Almasry <almasrymina@...gle.com>,
        Peter Xu <peterx@...hat.com>, Rik van Riel <riel@...riel.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Matthew Wilcox <willy@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Wei Chen <harperchen1110@...il.com>,
        "# 5 . 10+" <stable@...r.kernel.org>
Subject: Re: [PATCH v6] hugetlb: don't delete vma_lock in hugetlb
 MADV_DONTNEED processing

On 10/31/22 17:54, Nadav Amit wrote:
> On Oct 31, 2022, at 3:34 PM, Mike Kravetz <mike.kravetz@...cle.com> wrote:
> 
> > madvise(MADV_DONTNEED) ends up calling zap_page_range() to clear the page
> > tables associated with the address range.  For hugetlb vmas,
> > zap_page_range will call __unmap_hugepage_range_final.  However,
> > __unmap_hugepage_range_final assumes the passed vma is about to be removed
> > and deletes the vma_lock to prevent pmd sharing as the vma is on the way
> > out.  In the case of madvise(MADV_DONTNEED) the vma remains, but the
> > missing vma_lock prevents pmd sharing and could potentially lead to issues
> > with truncation/fault races.
> > 
> 
> [snip]
> 
> > index 978c17df053e..517c8cc8ccb9 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -3464,4 +3464,7 @@ madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> >  */
> > #define  ZAP_FLAG_DROP_MARKER        ((__force zap_flags_t) BIT(0))
> > 
> > +/* Set in unmap_vmas() to indicate an unmap call.  Only used by hugetlb */
> > +#define  ZAP_FLAG_UNMAP              ((__force zap_flags_t) BIT(1))
> 
> PeterZ wants to add ZAP_FLAG_FORCE_FLUSH that would be set on
> zap_pte_range(). Not sure you would want to combine them both together, but
> at least be aware of potential conflict.
> 
> https://lore.kernel.org/all/Y1f7YvKuwOl1XEwU@hirez.programming.kicks-ass.net/
> 

Ok, noted

> [snip]
> 
> > +#ifdef CONFIG_ADVISE_SYSCALLS
> > +/*
> > + * Similar setup as in zap_page_range().  madvise(MADV_DONTNEED) can not call
> > + * zap_page_range for hugetlb vmas as __unmap_hugepage_range_final will delete
> > + * the associated vma_lock.
> > + */
> > +void clear_hugetlb_page_range(struct vm_area_struct *vma, unsigned long start,
> > +				unsigned long end)
> > +{
> > +	struct mmu_notifier_range range;
> > +	struct mmu_gather tlb;
> > +
> > +	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm,
> > +				start, end);
> > +	adjust_range_if_pmd_sharing_possible(vma, &range.start, &range.end);
> > +	tlb_gather_mmu(&tlb, vma->vm_mm);
> > +	update_hiwater_rss(vma->vm_mm);
> > +	mmu_notifier_invalidate_range_start(&range);
> > +
> > +	__unmap_hugepage_range_locking(&tlb, vma, start, end, NULL, 0);
> > +
> > +	mmu_notifier_invalidate_range_end(&range);
> > +	tlb_finish_mmu(&tlb);
> > }
> > +#endif
> 
> I hate ifdef’s. And the second definition of clear_hugetlb_page_range() is
> confusing since it does not have an ifdef at all. . How about moving the
> ifdef’s into the function like being done in io_madvise_prep()? I think it
> is less confusing.

Dang!!!  I sent the wrong version of the patch.  clear_hugetlb_page_range
is not even used here.  I used Peter's idea to eliminate the need for
this separate routine.  And, this was a development version.

Really sorry about that.

Andrew, if you can please drop the version of the patch.
-- 
Mike Kravetz