From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 03D72C433FE for ; Mon, 28 Nov 2022 19:49:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 569EA6B0071; Mon, 28 Nov 2022 14:49:07 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 4F1CC6B0072; Mon, 28 Nov 2022 14:49:07 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3BA646B0073; Mon, 28 Nov 2022 14:49:07 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 2561A6B0071 for ; Mon, 28 Nov 2022 14:49:07 -0500 (EST) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id E21BDC06F1 for ; Mon, 28 Nov 2022 19:49:06 +0000 (UTC) X-FDA: 80183889492.05.B942449 Received: from mail-pj1-f51.google.com (mail-pj1-f51.google.com [209.85.216.51]) by imf28.hostedemail.com (Postfix) with ESMTP id 60E27C000D for ; Mon, 28 Nov 2022 19:49:06 +0000 (UTC) Received: by mail-pj1-f51.google.com with SMTP id x13-20020a17090a46cd00b00218f611b6e9so11179357pjg.1 for ; Mon, 28 Nov 2022 11:49:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Bc0z+h1BKtiHUdHLorhWfKEhoNufKsDwhuSJEHGv5dY=; b=XcDBHom1MSyJ4el3ZwFGwFzRNWDG3PX2alHC0JF8NQ3+W5b55INiZ8NsED8B64hTPw K2OWxUJY0xfWrQ7oqKuJ19tHTVCjwyeZTzwSQb0d5Wg6SRCaNiR/t3xBWcEBquuzHrD8 QYvfu7Po/Jch6Jlg6CMvA3TKrfe+zwj9ySBb5hSb9pgVzdZVG/rxPZk3nz65UiFiYEW7 b9ro5A5LxQnHyOqWp/F8f1ekg7P4XnmvUTRLVZUsh9rRkfDS6UcUYFgRiX/9cmDvcOHA NPbDM/xE40/LxzzdXc11zQQazsIotigFxVlp76XwjEIPTtL78xkFjmQL5Sexxa2iSJlU nj8Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Bc0z+h1BKtiHUdHLorhWfKEhoNufKsDwhuSJEHGv5dY=; b=FLgeI8Ih7vostP+8eKx+3UFj8SFtsdaC2C33YcTNyNsLNt48c4R78VKLmItr6Oef9T ISg2F9VdvFz7pZSLFn6mK7NVq4QbW7IcrkYjodh4h/r12rFTCF5wq3Jr8B/dhysiJYtt LfSVN8Cjh0z25IGSx0DKU0bmHdchaw+LgLTxYJnyOUP+FkApl1CN5Vi5s8OZ7tQL/f6o ghhJoeF8E0G/sGg04OWfnb4UA0ZCHwj2QH47H9JaahiwCxAmpJESr51tUOxX9mEURbxH m5IK85FdBEmeubUZlQKsSZG9uAMJ7ZQGmkINxUgeATZrbiKaHS8kINlICBBVDOS/J+gT SA4g== X-Gm-Message-State: ANoB5pmOXiOzJ9WcZr03JQ84D/LXu1fMeDrfZ3BvHvledPMWqmMPjPpN wNOiEdx2m+6IeZBcC+xKmhPZF1x9WbQQP3WnpnY= X-Google-Smtp-Source: AA0mqf4D0QlIMfESDatZiMGRUq/zSsuUfXvRWIQCys/Ks8r38MEfvPbGvZ33bAxBgBdZIaOI4PFfBWZcSDJ3u/nJaa8= X-Received: by 2002:a17:90a:4302:b0:20a:e469:dc7d with SMTP id q2-20020a17090a430200b0020ae469dc7dmr54011155pjg.97.1669664945331; Mon, 28 Nov 2022 11:49:05 -0800 (PST) MIME-Version: 1.0 References: <20221128180252.1684965-1-jannh@google.com> In-Reply-To: <20221128180252.1684965-1-jannh@google.com> From: Yang Shi Date: Mon, 28 Nov 2022 11:48:53 -0800 Message-ID: Subject: Re: [PATCH v4 1/3] mm/khugepaged: Take the right locks for page table retraction To: Jann Horn Cc: security@kernel.org, Andrew Morton , David Hildenbrand , Peter Xu , John Hubbard , linux-kernel@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1669664946; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Bc0z+h1BKtiHUdHLorhWfKEhoNufKsDwhuSJEHGv5dY=; b=ioePm03Lc5kkVuMe8/jHkP/+KTLu9sDvFYMJCXR/jHPgC3AFJhRIB59BXNlVZ3FW+asL+K mDctelvpibQFRQYK2zZkL9LLh3V8vKTJphMyhKeD+geut4ASLUjg7G2gaTCrEuB6bQ4WkR m1dG5Fp07zJX7cV9Jbb7LncOPc4/XRk= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=XcDBHom1; spf=pass (imf28.hostedemail.com: domain of shy828301@gmail.com designates 209.85.216.51 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1669664946; a=rsa-sha256; cv=none; b=2d0QwtCHHA9lvCbOeoMu17rKCEugwue53khm911+pD9H2Vas/wjqm7XBphcYalg/8J0ZxE fj88POhCwJGJsruXX0iYOhoCqqdu0tSDs7sOI24fpPPfPvQIG0mI/tovNOcHB5QX/IQha5 VkVfSIaw1l53OKYY+fzB40KK6Z/9wbs= X-Stat-Signature: a7qmux73yck5t51cz6t9ta8c3is8eaff X-Rspamd-Queue-Id: 60E27C000D Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=XcDBHom1; spf=pass (imf28.hostedemail.com: domain of shy828301@gmail.com designates 209.85.216.51 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Rspamd-Server: rspam06 X-Rspam-User: X-HE-Tag: 1669664946-446097 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Nov 28, 2022 at 10:03 AM Jann Horn wrote: > > pagetable walks on address ranges mapped by VMAs can be done under the mmap > lock, the lock of an anon_vma attached to the VMA, or the lock of the VMA's > address_space. Only one of these needs to be held, and it does not need to > be held in exclusive mode. > > Under those circumstances, the rules for concurrent access to page table > entries are: > > - Terminal page table entries (entries that don't point to another page > table) can be arbitrarily changed under the page table lock, with the > exception that they always need to be consistent for > hardware page table walks and lockless_pages_from_mm(). > This includes that they can be changed into non-terminal entries. > - Non-terminal page table entries (which point to another page table) > can not be modified; readers are allowed to READ_ONCE() an entry, verify > that it is non-terminal, and then assume that its value will stay as-is. > > Retracting a page table involves modifying a non-terminal entry, so > page-table-level locks are insufficient to protect against concurrent > page table traversal; it requires taking all the higher-level locks under > which it is possible to start a page walk in the relevant range in > exclusive mode. > > The collapse_huge_page() path for anonymous THP already follows this rule, > but the shmem/file THP path was getting it wrong, making it possible for > concurrent rmap-based operations to cause corruption. > > Cc: stable@kernel.org > Fixes: 27e1f8273113 ("khugepaged: enable collapse pmd for pte-mapped THP") > Acked-by: David Hildenbrand > Signed-off-by: Jann Horn > --- > v4: added ack by David Hildenbrand Reviewed-by: Yang Shi > > mm/khugepaged.c | 55 +++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 51 insertions(+), 4 deletions(-) > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index 4734315f79407..674b111a24fa7 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -1384,16 +1384,37 @@ static int set_huge_pmd(struct vm_area_struct *vma, unsigned long addr, > return SCAN_SUCCEED; > } > > +/* > + * A note about locking: > + * Trying to take the page table spinlocks would be useless here because those > + * are only used to synchronize: > + * > + * - modifying terminal entries (ones that point to a data page, not to another > + * page table) > + * - installing *new* non-terminal entries > + * > + * Instead, we need roughly the same kind of protection as free_pgtables() or > + * mm_take_all_locks() (but only for a single VMA): > + * The mmap lock together with this VMA's rmap locks covers all paths towards > + * the page table entries we're messing with here, except for hardware page > + * table walks and lockless_pages_from_mm(). > + */ > static void collapse_and_free_pmd(struct mm_struct *mm, struct vm_area_struct *vma, > unsigned long addr, pmd_t *pmdp) > { > - spinlock_t *ptl; > pmd_t pmd; > > mmap_assert_write_locked(mm); > - ptl = pmd_lock(vma->vm_mm, pmdp); > + if (vma->vm_file) > + lockdep_assert_held_write(&vma->vm_file->f_mapping->i_mmap_rwsem); > + /* > + * All anon_vmas attached to the VMA have the same root and are > + * therefore locked by the same lock. > + */ > + if (vma->anon_vma) > + lockdep_assert_held_write(&vma->anon_vma->root->rwsem); > + > pmd = pmdp_collapse_flush(vma, addr, pmdp); > - spin_unlock(ptl); > mm_dec_nr_ptes(mm); > page_table_check_pte_clear_range(mm, addr, pmd); > pte_free(mm, pmd_pgtable(pmd)); > @@ -1444,6 +1465,14 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr, > if (!hugepage_vma_check(vma, vma->vm_flags, false, false, false)) > return SCAN_VMA_CHECK; > > + /* > + * Symmetry with retract_page_tables(): Exclude MAP_PRIVATE mappings > + * that got written to. Without this, we'd have to also lock the > + * anon_vma if one exists. > + */ > + if (vma->anon_vma) > + return SCAN_VMA_CHECK; > + > /* Keep pmd pgtable for uffd-wp; see comment in retract_page_tables() */ > if (userfaultfd_wp(vma)) > return SCAN_PTE_UFFD_WP; > @@ -1477,6 +1506,20 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr, > goto drop_hpage; > } > > + /* > + * We need to lock the mapping so that from here on, only GUP-fast and > + * hardware page walks can access the parts of the page tables that > + * we're operating on. > + * See collapse_and_free_pmd(). > + */ > + i_mmap_lock_write(vma->vm_file->f_mapping); > + > + /* > + * This spinlock should be unnecessary: Nobody else should be accessing > + * the page tables under spinlock protection here, only > + * lockless_pages_from_mm() and the hardware page walker can access page > + * tables while all the high-level locks are held in write mode. > + */ > start_pte = pte_offset_map_lock(mm, pmd, haddr, &ptl); > result = SCAN_FAIL; > > @@ -1531,6 +1574,8 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr, > /* step 4: remove pte entries */ > collapse_and_free_pmd(mm, vma, haddr, pmd); > > + i_mmap_unlock_write(vma->vm_file->f_mapping); > + > maybe_install_pmd: > /* step 5: install pmd entry */ > result = install_pmd > @@ -1544,6 +1589,7 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr, > > abort: > pte_unmap_unlock(start_pte, ptl); > + i_mmap_unlock_write(vma->vm_file->f_mapping); > goto drop_hpage; > } > > @@ -1600,7 +1646,8 @@ static int retract_page_tables(struct address_space *mapping, pgoff_t pgoff, > * An alternative would be drop the check, but check that page > * table is clear before calling pmdp_collapse_flush() under > * ptl. It has higher chance to recover THP for the VMA, but > - * has higher cost too. > + * has higher cost too. It would also probably require locking > + * the anon_vma. > */ > if (vma->anon_vma) { > result = SCAN_PAGE_ANON; > > base-commit: eb7081409f94a9a8608593d0fb63a1aa3d6f95d8 > -- > 2.38.1.584.g0f3c55d4c2-goog >