From patchwork Tue Nov 29 15:47:28 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jann Horn X-Patchwork-Id: 13058730 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 1AF16C4167B for ; Tue, 29 Nov 2022 15:47:44 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 563436B0071; Tue, 29 Nov 2022 10:47:43 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 512DF6B0072; Tue, 29 Nov 2022 10:47:43 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3DAA46B0074; Tue, 29 Nov 2022 10:47:43 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 2BA006B0071 for ; Tue, 29 Nov 2022 10:47:43 -0500 (EST) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id DD2E11206B4 for ; Tue, 29 Nov 2022 15:47:42 +0000 (UTC) X-FDA: 80186909964.13.FBDB350 Received: from mail-wr1-f47.google.com (mail-wr1-f47.google.com [209.85.221.47]) by imf03.hostedemail.com (Postfix) with ESMTP id 67E7B20015 for ; Tue, 29 Nov 2022 15:47:42 +0000 (UTC) Received: by mail-wr1-f47.google.com with SMTP id h11so15469725wrw.13 for ; Tue, 29 Nov 2022 07:47:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=zcNi41xvTMS4NGTUpyHD2vhE6PXJ5qCDMr6KFHcPQag=; b=Ymj/TX58LmBHMlJm2tvaAwNPklk0OGDZu/h2bpwb8PEHBxKhk4ajUV93di2Grz8TVV 9lMyxfkKDAp24o03K5+1FRcgoCKVXZjlBGbIytbuviIem7nF4rYyG6KooxotGqK+hP1E oOi+BBXCoaw4iA3xFq6+2BtTm31okvGXqpDLHXAVrSQF4e3INEQh3xc0r9WI6qWJs9vr cn8nSzY+otKe+LsoUuNa8GtXB+bJo3a/2ezw2yBL/OKQ89GnW5aHwVJlGaUwwQWVVlUi OfYz0qsbjPaz3Sq039jK3ZktX72U89/hNuW+ZLmHuz+gzgfB998b5wYXplTrOYLmT1UV HOGw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=zcNi41xvTMS4NGTUpyHD2vhE6PXJ5qCDMr6KFHcPQag=; b=VWc2LB9UTA1BvWQU8QGeWj8hOSF428kYeDyaADdnFmkqMjm7kgV7AheiOLPJh9pKWv L5VBh8BVnWe77NLYVH60dE4CsiEK/6OZPUlIZFv0L3MxqiV/lIG1YKRaw0Jwwfdgvuxt jRm8uIughw0SMr5VP8zNOW5hcGzTNcH05bz+fviaTswDvFsk9BocFH8bjEM2/L3le+KV VRuNX3yFUSeN6hqpfC7nm/CbamRP9CnHNI2NCjasYcf04gfI/kkqLGelHT4buHqTfafi N+LFRWvT2Hfd3wiQ/XGS2bkiY+SI9TvdOIkBqsqnfa8AE8V3wY5IjOs9N+6u2SoahP8h /K3Q== X-Gm-Message-State: ANoB5pnqBanZiJ3RKg2xfXxWRd31C0OjiQ2beKbgUtyqV253LGKZaU9W XUNu0iQ8DQGSjqrB2n4ZDTVj5A== X-Google-Smtp-Source: AA0mqf4VS2OE+nEDrDPjpzaYNf+mLsm5WKH9zV+k+70WKT3wLEzTcfYv6YO/CIf/HCa4ZWlt9jGz0w== X-Received: by 2002:a05:6000:70f:b0:22e:41c5:7ef7 with SMTP id bs15-20020a056000070f00b0022e41c57ef7mr35635050wrb.332.1669736860806; Tue, 29 Nov 2022 07:47:40 -0800 (PST) Received: from localhost ([2a00:79e0:9d:4:5011:adcc:fddd:accf]) by smtp.gmail.com with ESMTPSA id x9-20020a5d6b49000000b002366e3f1497sm14090293wrw.6.2022.11.29.07.47.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 29 Nov 2022 07:47:40 -0800 (PST) From: Jann Horn To: security@kernel.org, Andrew Morton Cc: Yang Shi , David Hildenbrand , Peter Xu , John Hubbard , linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: [PATCH v5 1/3] mm/khugepaged: Take the right locks for page table retraction Date: Tue, 29 Nov 2022 16:47:28 +0100 Message-Id: <20221129154730.2274278-1-jannh@google.com> X-Mailer: git-send-email 2.38.1.584.g0f3c55d4c2-goog MIME-Version: 1.0 ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1669736862; 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-transfer-encoding:content-transfer-encoding: in-reply-to:references:dkim-signature; bh=zcNi41xvTMS4NGTUpyHD2vhE6PXJ5qCDMr6KFHcPQag=; b=3j4Otpi5YzltAH5FMWtGY7QpzMxICPNbaxupmtaIHrBaHQKvIO+z2QdIUuW1vcD1jbABBm 4G11h85hRbSJu6W4rkjRsnX8CAQyztigmOvS17UYZ8wT6eYOUWgALoNOE0bLW/u6ks3tem Ky+aohCqL8qIfWTo4irBpWQ/VW3mbLI= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b="Ymj/TX58"; spf=pass (imf03.hostedemail.com: domain of jannh@google.com designates 209.85.221.47 as permitted sender) smtp.mailfrom=jannh@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1669736862; a=rsa-sha256; cv=none; b=F9rZDf5kTqHEf4goPnC7DItzjetjICfNyQ1C/aOv8OFeHGdS6RXhoMF9kw+aXXlXd6nO3J /C+wvaWblNDaFdOQV2mo/syS34CuH18IPFqm0MBvYOFbtP4u9D5Rw1mEND3uz4DQczTffD 3XSXVoL6Tw/g6lTl8itYKVrn2NfU6w0= X-Stat-Signature: db3wrcjpfd6ptuzndqcsredyhjp3rtz6 X-Rspamd-Queue-Id: 67E7B20015 Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b="Ymj/TX58"; spf=pass (imf03.hostedemail.com: domain of jannh@google.com designates 209.85.221.47 as permitted sender) smtp.mailfrom=jannh@google.com; dmarc=pass (policy=reject) header.from=google.com X-Rspamd-Server: rspam06 X-Rspam-User: X-HE-Tag: 1669736862-835814 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: 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 Reviewed-by: Yang Shi Signed-off-by: Jann Horn --- Notes: v4: added ack by David Hildenbrand v5: added reviewed-by by Yang Shi mm/khugepaged.c | 55 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 4 deletions(-) base-commit: eb7081409f94a9a8608593d0fb63a1aa3d6f95d8 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;