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 BF0E2C4345F for ; Wed, 17 Apr 2024 20:54:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A5C336B0082; Wed, 17 Apr 2024 16:54:18 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A0CC16B0083; Wed, 17 Apr 2024 16:54:18 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8AC1F6B0085; Wed, 17 Apr 2024 16:54:18 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 6A18B6B0082 for ; Wed, 17 Apr 2024 16:54:18 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 1465D140625 for ; Wed, 17 Apr 2024 20:54:18 +0000 (UTC) X-FDA: 82020226596.12.FF9CA9C Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf09.hostedemail.com (Postfix) with ESMTP id DD216140018 for ; Wed, 17 Apr 2024 20:54:15 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=iNmiafFb; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf09.hostedemail.com: domain of peterx@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=peterx@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713387256; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=WxVps9eo7fKiokjE5p2dXVAq1Ns0BOvxX7WHeY7GD0Y=; b=uStjn2pO/BBXwLiSJmR6MAykbpp9FeRV84dItjUF7i9s9W3MUs12MytrhiM13NVuUKr4LK jPCeUavGwVeL8tOlkWmJVKBD7FFfaYYl85ErN7w+uvkaidHdhOmNtInZenKIN161b/shks w7ZhYuK3ExyIBKyLlgkW+rIaf1mErTw= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=iNmiafFb; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf09.hostedemail.com: domain of peterx@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=peterx@redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713387256; a=rsa-sha256; cv=none; b=en87K/c+LnfyKwFSmDQhTBxf2D+K4DWRUMSTQrRPn8V3GyHEKphLCBh4g9tRjDztb6e46t WHGfybSPeOhrcjCPChqf0X1iu75BGOUO45wEuWPtEU5y53ly5+r1GlhlvsoacSZKTo1tRG uTuQ+4dRCAH2dDfzschyW6++Itm/VnI= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1713387255; h=from:from: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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=WxVps9eo7fKiokjE5p2dXVAq1Ns0BOvxX7WHeY7GD0Y=; b=iNmiafFb8FKdS41R/XcwHDrplibj8Yhs/qqC5bhsO/VNOXjcvYFBMTIZfWEaGPhiyiwIfG zdVCXMQpAV+9xbt0QePK4dkZmNTt7Gy14sASkhDHZzbZ/b6thMB+HZZb5BmK2lBryeUPJD mf8OwgVwRPRgmuQaZ5yDjNl8hwap0W8= Received: from mail-oo1-f70.google.com (mail-oo1-f70.google.com [209.85.161.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-248-9Iljt5uKOLWj8TGlppXvgQ-1; Wed, 17 Apr 2024 16:54:13 -0400 X-MC-Unique: 9Iljt5uKOLWj8TGlppXvgQ-1 Received: by mail-oo1-f70.google.com with SMTP id 006d021491bc7-5acdbf9d0c6so41726eaf.3 for ; Wed, 17 Apr 2024 13:54:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713387253; x=1713992053; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=WxVps9eo7fKiokjE5p2dXVAq1Ns0BOvxX7WHeY7GD0Y=; b=PfyKEV+acdUjm4uJMdeKLQHdQoVqWj4225nkk5oaD6HXjXnrJCbCG9nQQWU4QDtoTf xjiB+hQGX2Z/zj8lqaN+kvE6Vs4u/cRmUXF76y+H9Fib0hh0o9Tq3QXakBcaNG+he5FB AiA5gXENfcUAETnCU3k1nzV2VwwOklNARWIAGgxqntk2jSCiDzwP1LfoScXmFPyPFxWT USm6a6GWxJBMLKBp+fq12We14EqBzRmyCSDLo/7rzROj60bw5i22PQRn2eppcWfyagEZ +KcCs/g2FQX62fUh/lYabRD0MO6tFT6Xu0za4LFyDWYKCxT5agx8i/VA8NAA7I5avb6W +cSA== X-Forwarded-Encrypted: i=1; AJvYcCXwrPxSFgFdGM8hqM/tpBrR877nmg4/D2uN/+otNZID7cv6qCP0SnqYYmFBOldUhvMdU4KIZGc71ogq5l7SogCAmMI= X-Gm-Message-State: AOJu0Yyc0Raf9/jn43tRJE6njPRsK/1HDrufDOL1IDzONlNn/qfGigLo FDSptq0BCXbOBcIUPdMtCXBVYGI+NE4mZjprIHhJF50+2jdOfC8KgLYAeyKRLWJ7iyISlkOfWkP hYb1xVduPD1EPebiFZ33ua6b5syvMoAhVvbBBcS8O5RW4+xwq X-Received: by 2002:a4a:b605:0:b0:5aa:6a31:4f53 with SMTP id z5-20020a4ab605000000b005aa6a314f53mr967206oon.0.1713387252721; Wed, 17 Apr 2024 13:54:12 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEWGUZrTe7cf5mjN8f+tB8tf5bzpBYCB+ahmV2tQBDGKxmqdwX9Gk5b28HM+irN2ttqKfx4nQ== X-Received: by 2002:a4a:b605:0:b0:5aa:6a31:4f53 with SMTP id z5-20020a4ab605000000b005aa6a314f53mr967186oon.0.1713387252290; Wed, 17 Apr 2024 13:54:12 -0700 (PDT) Received: from x1n (pool-99-254-121-117.cpe.net.cable.rogers.com. [99.254.121.117]) by smtp.gmail.com with ESMTPSA id e15-20020ac8490f000000b00434d86fb403sm8417343qtq.86.2024.04.17.13.54.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 17 Apr 2024 13:54:11 -0700 (PDT) Date: Wed, 17 Apr 2024 16:54:09 -0400 From: Peter Xu To: Pasha Tatashin Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Andrew Morton , David Hildenbrand , Nadav Amit , Axel Rasmussen Subject: Re: [PATCH v3] mm/page_table_check: Support userfault wr-protect entries Message-ID: References: <20240417192643.2671335-1-peterx@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam11 X-Rspamd-Queue-Id: DD216140018 X-Stat-Signature: 6ff6rcwa1sszqkd8fuqd4hg4s9cu5s49 X-HE-Tag: 1713387255-873706 X-HE-Meta: U2FsdGVkX1+HeMt6YeTU9injlLzx4BBtUgU4e3Arz7aSkHd5mhQ/v03jpw0L9JVzzFakiTmBqMFBXVTwFRj85bg371nX6vl1T4cXKHo+z1unsBDeKUHl+vB2WlXXSxvkmvkdYKLNn5NPRcAF6B2LIzSGnIvGbnXTqjtyRWqW7RiRq4wPKqhk4qAk3aLURo6oc+RJZJ9dYB08vmeNtw0Ry1G70SpaBlmrsEhNC9f7N4e/g/RpGsCBs2DU6/InWuxnNLh0phCec1ev7JEULwk76kkYfdo2rdfTRazWvdADNNhZ8dTC6IW0qNE7/aA1mXyoJmNBQiHQdQ7C9QgitZqYzYqkQZvzjNahzYnT8upkTfDi+7ragGli5b5S87dMV+Jma71CDiqYLxbPNa7nin+uGadihGXqhefMF5WETyGXaaIOKP2G7HRf/PfIvF+IOaT+98MVHxWSz2/hYqyTkIpp9cmFuWk5eQfymMaxMaWVthUhxTk8hFt66HGjIFvo8O85EluIzA3kyQpxDfw6ygkqE+D0Y/l6+SitZ4BFqc4QTzRa01xadmZcX1sRTuOn+SnJQn21PdSCw/kM8vzI7/aDiCOK027CqibcHv38VVKzJxZzmK5x9B3VH4bv9lBxuZCFajFM6zvR8rpUGVLoOR6OFtHKq20HOsDSieQfcX+Km9DFOwfpOF9lP5gCLTYuSjKtnbe6+hO0iMt0gezGyf0UaEg44NY6yIYWDwvQYxPhbeFtdCckU9ZoeFvO2URLDSp0gwqy2h6AxYUHuoa92icoIXhJmolU+zGEeMLOH/notzUdPlYKiFM6zLoYzlROXnf/kMKTsQjz+igjvv9kC7Au0fYfJSVsGkncR+G4SOLrBFHQVzDTlRyQ17rmPboE4j15LpR7tNe8XF9bsaxtlamMjIjPGrgk9o2Y/7QEetFA09v9I37r+StkKImH4BsdOI1JDZVc+9MoxMvdgVZ17Xc bnkQeUE5 xalCuiXJQIuUEMzeZrV3DXIe83DovtD1JzDfB8Z5byCkiD61XefXpXxZZZrBpQD/gd/YH+vDrl3XZbAbaqwv9UZA5DRX+K0GJw+M8bRRMQq39zL4TnFOSYmodCFVl84C/k5xuxvxB2cPPrTF5Aa7o/4Clz5YlD5MZitUsk3Qfy+F2bk39yE4J2twing9lXNkDxrC3d3KS1udiwxc7qYlRRi7/avHoQiKe92DTKBKbzzGMLWC8jOOAzx4opRxbQHIYSvK+fU7kubrRONnykcI75hAU/cFwldcbZQHp+sTA/yHZQqJEB0yZXOtEAFDUhEnRc/dRb/6raC/wffqT5b3AiFBosEqPQalRaeaPJ5qePehfUemrN5RXTts/xMKkDLdSC8NgLc/klicz742MUVzVF9dmibNW1BY7iGau5wPRC2v24LmQu994n5qRNAENrdrXAcwwFDA2POLnn/1QI51eklfVuGnOS+0Xf467xGI+LnWzykxTmrtSyTecmsgDa9WBSvV3vy7iYeW+9+7nZ6wBdiD09Et5g9feqAeBXRksyVfqglQTZowvvw9SjO1JFq+QkwvnNllHzt8MXal/EhbJDmI0ILy2ONKlc7vEtsvjlCwiEziiNW3vnEEZfWzChW85vjf8ENH0s6WxEWSHZ63KOifZKjvzI/j5RFWF 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: List-Subscribe: List-Unsubscribe: On Wed, Apr 17, 2024 at 03:44:29PM -0400, Pasha Tatashin wrote: > On Wed, Apr 17, 2024 at 3:26 PM Peter Xu wrote: > > > > Allow page_table_check hooks to check over userfaultfd wr-protect criteria > > upon pgtable updates. The rule is no co-existance allowed for any writable > > flag against userfault wr-protect flag. > > > > This should be better than c2da319c2e, where we used to only sanitize such > > issues during a pgtable walk, but when hitting such issue we don't have a > > good chance to know where does that writable bit came from [1], so that > > even the pgtable walk exposes a kernel bug (which is still helpful on > > triaging) but not easy to track and debug. > > > > Now we switch to track the source. It's much easier too with the recent > > introduction of page table check. > > > > There are some limitations with using the page table check here for > > userfaultfd wr-protect purpose: > > > > - It is only enabled with explicit enablement of page table check configs > > and/or boot parameters, but should be good enough to track at least > > syzbot issues, as syzbot should enable PAGE_TABLE_CHECK[_ENFORCED] for > > x86 [1]. We used to have DEBUG_VM but it's now off for most distros, > > while distros also normally not enable PAGE_TABLE_CHECK[_ENFORCED], which > > is similar. > > > > - It conditionally works with the ptep_modify_prot API. It will be > > bypassed when e.g. XEN PV is enabled, however still work for most of the > > rest scenarios, which should be the common cases so should be good > > enough. > > > > - Hugetlb check is a bit hairy, as the page table check cannot identify > > hugetlb pte or normal pte via trapping at set_pte_at(), because of the > > current design where hugetlb maps every layers to pte_t... For example, > > the default set_huge_pte_at() can invoke set_pte_at() directly and lose > > the hugetlb context, treating it the same as a normal pte_t. So far it's > > fine because we have huge_pte_uffd_wp() always equals to pte_uffd_wp() as > > long as supported (x86 only). It'll be a bigger problem when we'll > > define _PAGE_UFFD_WP differently at various pgtable levels, because then > > one huge_pte_uffd_wp() per-arch will stop making sense first.. as of now > > we can leave this for later too. > > > > This patch also removes commit c2da319c2e altogether, as we have something > > better now. > > > > [1] https://lore.kernel.org/all/000000000000dce0530615c89210@google.com/ > > > > Cc: Pasha Tatashin > > Signed-off-by: Peter Xu > > --- > > v2: > > - Rename __page_table_check_pxx() to page_table_check_pxx_flags(), > > meanwhile move the pte check out of the loop [Pasha] > > - Fix build issues reported from the bot, also added SWP_DEVICE_WRITE which > > was overlooked before > > v3: > > - Add missing doc update [Pasha] > > --- > > Documentation/mm/page_table_check.rst | 9 ++++++- > > arch/x86/include/asm/pgtable.h | 18 +------------ > > mm/page_table_check.c | 39 +++++++++++++++++++++++++++ > > 3 files changed, 48 insertions(+), 18 deletions(-) > > > > diff --git a/Documentation/mm/page_table_check.rst b/Documentation/mm/page_table_check.rst > > index c12838ce6b8d..5bd1d987d76d 100644 > > --- a/Documentation/mm/page_table_check.rst > > +++ b/Documentation/mm/page_table_check.rst > > @@ -14,7 +14,7 @@ Page table check performs extra verifications at the time when new pages become > > accessible from the userspace by getting their page table entries (PTEs PMDs > > etc.) added into the table. > > > > -In case of detected corruption, the kernel is crashed. There is a small > > +In case of most detected corruption, the kernel is crashed. There is a small > > performance and memory overhead associated with the page table check. Therefore, > > it is disabled by default, but can be optionally enabled on systems where the > > extra hardening outweighs the performance costs. Also, because page table check > > @@ -22,6 +22,13 @@ is synchronous, it can help with debugging double map memory corruption issues, > > by crashing kernel at the time wrong mapping occurs instead of later which is > > often the case with memory corruptions bugs. > > > > +It can also be used to do page table entry checks over various flags, dump > > +warnings when illegal combinations of entry flags are detected. Currently, > > +userfaultfd is the only user of such to sanity check wr-protect bit against > > +any writable flags. Illegal flag combinations will not directly cause data > > +corruption in this case immediately, but that will cause read-only data to > > +be writable, causing data corrupt when the page content is later modified. > > I would replace: "causing data corrupt ..." to "leading to corruption ..." OK. > > > + > > Double mapping detection logic > > ============================== > > > > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h > > index 273f7557218c..65b8e5bb902c 100644 > > --- a/arch/x86/include/asm/pgtable.h > > +++ b/arch/x86/include/asm/pgtable.h > > @@ -388,23 +388,7 @@ static inline pte_t pte_wrprotect(pte_t pte) > > #ifdef CONFIG_HAVE_ARCH_USERFAULTFD_WP > > static inline int pte_uffd_wp(pte_t pte) > > { > > - bool wp = pte_flags(pte) & _PAGE_UFFD_WP; > > - > > -#ifdef CONFIG_DEBUG_VM > > - /* > > - * Having write bit for wr-protect-marked present ptes is fatal, > > - * because it means the uffd-wp bit will be ignored and write will > > - * just go through. > > - * > > - * Use any chance of pgtable walking to verify this (e.g., when > > - * page swapped out or being migrated for all purposes). It means > > - * something is already wrong. Tell the admin even before the > > - * process crashes. We also nail it with wrong pgtable setup. > > - */ > > - WARN_ON_ONCE(wp && pte_write(pte)); > > -#endif > > - > > - return wp; > > + return pte_flags(pte) & _PAGE_UFFD_WP; > > } > > > > static inline pte_t pte_mkuffd_wp(pte_t pte) > > diff --git a/mm/page_table_check.c b/mm/page_table_check.c > > index af69c3c8f7c2..388bcf60d8b5 100644 > > --- a/mm/page_table_check.c > > +++ b/mm/page_table_check.c > > @@ -7,6 +7,8 @@ > > #include > > #include > > #include > > +#include > > +#include > > > > #undef pr_fmt > > #define pr_fmt(fmt) "page_table_check: " fmt > > @@ -182,6 +184,31 @@ void __page_table_check_pud_clear(struct mm_struct *mm, pud_t pud) > > } > > EXPORT_SYMBOL(__page_table_check_pud_clear); > > > > +/* Whether the swap entry cached writable information */ > > +static inline bool swap_cached_writable(swp_entry_t entry) > > +{ > > + unsigned type = swp_type(entry); > > + > > +#ifdef CONFIG_DEVICE_PRIVATE > > + if (type == SWP_DEVICE_EXCLUSIVE_WRITE || type == SWP_DEVICE_WRITE) > > + return true; > > +#endif > > +#ifdef CONFIG_MIGRATION > > + if (type == SWP_MIGRATION_WRITE) > > + return true; > > +#endif > > + > > + return false; > > +} > > This should be re-written like this: > > static inline bool swap_cached_writable(swp_entry_t entry) > { > return is_writable_device_exclusive_entry(entry) || > is_writable_device_private_entry(entry) || > is_writable_migration_entry(entry); > } > > Otherwise the patch looks good. Oops, yes definitely.. I'll prepare one more. -- Peter Xu