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 B4E65C4345F for ; Wed, 17 Apr 2024 17:18:09 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 184496B007B; Wed, 17 Apr 2024 13:18:09 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 10D216B0083; Wed, 17 Apr 2024 13:18:09 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EEF9D6B0087; Wed, 17 Apr 2024 13:18:08 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id CFCDD6B007B for ; Wed, 17 Apr 2024 13:18:08 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 8DB12C09F8 for ; Wed, 17 Apr 2024 17:18:08 +0000 (UTC) X-FDA: 82019681856.27.E6A343D Received: from mail-qt1-f182.google.com (mail-qt1-f182.google.com [209.85.160.182]) by imf18.hostedemail.com (Postfix) with ESMTP id BD4301C003E for ; Wed, 17 Apr 2024 17:18:05 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=soleen-com.20230601.gappssmtp.com header.s=20230601 header.b=sD5m+t4y; spf=pass (imf18.hostedemail.com: domain of pasha.tatashin@soleen.com designates 209.85.160.182 as permitted sender) smtp.mailfrom=pasha.tatashin@soleen.com; dmarc=pass (policy=none) header.from=soleen.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713374285; 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=Ge6pGSPn0Uuw9Wl7VGZm6zy1FRDKrs+Xgw3FaaawFgk=; b=dO4Q17naYTOrKD4xTiPu+hDdwy73xM8/Ts8wJcq+RQX6lgculMS5GtwEYVKLEhdopjvvqP IXHXl2uBXYEFnU2Zc5kORRSac0MfJWd+srurdyXMcoB8+KL3mQNhHrVhX/pecHgVUSW43C 0KVOZOF3M9CNqiSBMHIkxmZmfnM7OB8= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=soleen-com.20230601.gappssmtp.com header.s=20230601 header.b=sD5m+t4y; spf=pass (imf18.hostedemail.com: domain of pasha.tatashin@soleen.com designates 209.85.160.182 as permitted sender) smtp.mailfrom=pasha.tatashin@soleen.com; dmarc=pass (policy=none) header.from=soleen.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713374285; a=rsa-sha256; cv=none; b=O6QuaeASjWk0teKV7CmxZehvzRgux13lKGOivVdfh7jHfoOaP/oIdbsb1pJQwvUdoRCwPS ELxUt5lJGU8PQikqqhuGd8U3ziVGStSL5SPvoF6H3PsOiBZfzB4oRQwE7rqN7nn3BSl8pH ec1VGdb+jy2tIrUBf1LzQXnX9Ol9/kg= Received: by mail-qt1-f182.google.com with SMTP id d75a77b69052e-4376e3fd7e4so4785821cf.2 for ; Wed, 17 Apr 2024 10:18:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=soleen-com.20230601.gappssmtp.com; s=20230601; t=1713374283; x=1713979083; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=Ge6pGSPn0Uuw9Wl7VGZm6zy1FRDKrs+Xgw3FaaawFgk=; b=sD5m+t4yHyRBs1PuTCj0BQgC3SjUV3jqfA8O+KMUH/yyjPLTo7UXc2p+UaQPr25wiG I+7Q3C8B09rSVi9OZ4LmzQopL3GgUAR7oEbNzcLk267JG4Oyfym7jQ/4bmH8Az8cREFq vsmk3chdPEzx4ozAbATdqhQ5Oi4rljtnnAZ6BCxuZOpxD3/iW9slYXIZ2E0lqlLBzygw HI3NSXjFPYMlfTN6BkPTqR6jtl8H39byPNagiyJub8All8HoWtZ2XHisggEjuxf+WuCA 96Df4190R8qUaFnCwFkgJtfBmvTbXS+DUNMWUEHpWp/WJSbJ1vU9Pf6z1vtUKxAqkuTm eEKw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713374283; x=1713979083; h=content-transfer-encoding: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=Ge6pGSPn0Uuw9Wl7VGZm6zy1FRDKrs+Xgw3FaaawFgk=; b=rSXajrQssxjxV5+0oisfOxPsEgCLIRfmvIaJN6ynhi2P4fmsyie5lwADze11PAmOWA HavN3mTkQcCBME+1U8VqueYzOK0cWz2fhqIarlslAxFGQKiPtC+V5LV7Myr6sLLX4L7e e54enpl5P9kRRNOFUlYWqQwxQkrGLVorjEr1txva5iZBkYRqFZdVACiQaK2ep1sskIw1 yss01oz2EFomjh0UtEDX2SlmKv1NlYoQlxFQ+sVh3SKZ727qgD+3/6iEcD9WTTWkk8Vt XNNYjMHmp8adLHKRlOqa4byHUsths94IbojRFtjM6GJWNWctRIDtB/qmOwOjEYA1iyRH NMPA== X-Forwarded-Encrypted: i=1; AJvYcCU5CXNTDKpN9qPXwCgTstvtYnw1unPBjZTzuurf0GkuokIfR5vhGMhNy6/7YjtDuObapb2PqKaLzVPsPk+37w0z3dg= X-Gm-Message-State: AOJu0Yy6u6vfO4NKAUJgjKVKYGESbi4V82dd4el61pqYKVICUx9yMMF3 blG7wyWSrQXhBvcwYF/9n2gxuAcHzojOV0gK2eM9jUXlJTzP5b4/5xIJVvvn+0ugg7a+/AQowfa TQ01sqXKhkcVpvCPA04S7CS1X4jpHI1Eui036oQ== X-Google-Smtp-Source: AGHT+IHjmc6rnhxHm5z1T/LOAQZV6fGKUJLqQpoEWSTZWCRmG16CaUcwe8GOES9wc6+B7vz0VYS79I6a9VIIeHWosBg= X-Received: by 2002:ac8:590d:0:b0:436:d9cf:7da0 with SMTP id 13-20020ac8590d000000b00436d9cf7da0mr104758qty.17.1713374282679; Wed, 17 Apr 2024 10:18:02 -0700 (PDT) MIME-Version: 1.0 References: <20240415205259.2535077-1-peterx@redhat.com> In-Reply-To: From: Pasha Tatashin Date: Wed, 17 Apr 2024 13:17:26 -0400 Message-ID: Subject: Re: [PATCH] mm/page_table_check: Support userfault wr-protect entries To: Peter Xu Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, David Hildenbrand , Andrew Morton , Nadav Amit , Axel Rasmussen Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: 9z1o8jyap7i4y77srz76kndp7j4juk4w X-Rspamd-Queue-Id: BD4301C003E X-Rspamd-Server: rspam10 X-Rspam-User: X-HE-Tag: 1713374285-642768 X-HE-Meta: U2FsdGVkX18++vDBB2s9ZWcCKrqa6RiEmjlTlYVhG2VbkIkksCbxVCmcsNW+46bontGT34YQ1gAzE2Jo2VZ3elPBAcm8AhSlttgJZKveuPXo5jvBpZ/YgHl05q4X1k8xd9A2SANF0tcpdefvpGSR8rU9cMpyC6UXiHGupc98yOC9ZJD3cV7/0jrBxbESAISHzJQ8gYS7Sx4n3U1dWRPBtEcsuW9qZnkpcn/35LtjiFAbOo+sfAFNvykH038zHtNVGIDALYLTWwjtrR+dNcVRESpIIL/7xD/CKGw6MCCPfP73BXH6Qh6fpdLcscY2k96UUy1FnsqmamYW4TlZRBSQ1lQ5LAZg4+Cg59vlHDbxiCnraCuBvUqQhIQ6PuCVeoeELs5xV+5BoEo91eWPZCSQtonG9bEgYl0tEsIy2V4PMXN2HFhYnHgvHaU8rLQuekXIyDwaP+GLfcWa4DEQKI/hLxkedMxJYU8nLsFM5QZcjeCAJu5WtYQNQETItDRloyww9hQr8tLF8ZFvsdhcgTZAqrzQWWpaV1AAo1hphw979517qTbgu4Y7dmSwKmgLhBadJbP9R8f3aw4Adicvd+mR1ahg+yutPJE4602BJf5vgtxXrHwFvRvQQKSD/zGhkHPGtAFiy3QBdbuo3AKLcBnvCgN9sUSdwFI/fQhjKkivyLLR1G2s62ytH0PmnFuSaMfSotzZnfSG0tZ3FrGYkrzeQ9E9J7eKbRPsoBs8I6uLWXrxuU7e9Z3z8PwNy8e8snvurq8VSX/GPkPSeuj5LBn2xrt8IKXK6+h4QmEiHmxl9/TiO+C5LrPhgvBp/fHRHNVmBBBDwhqssJ+g3omVuiDIeu8yZq550gdU/NE/zwuTuPD+Lp7lzzCFv8S3uXJ8zTt/bpoG2B0dncnUfw5wRhTtIJ3Iks1Fc+l1r/7Ly0p0fx60xery8K7y+dGavI7qFWUxDdulEAxRdvGRqBpa717 Rkr1Dmxl OAYFthQscd84NUTReE+JmDNgFnHC1wvQujEJbAhLL2Y4zEiJ9a3GANVyONE+6DhAvyhh+Pw0XMTHWzI0rB3CIeEf/aa7zm3o6apqx32DweYsXzbAOmBYN/go3YdNWHMENnR9NYmvxsN8jB5lLg7qJRxC6EVbCsZ6JhIA79Q5Hi6rIAZnTF01kS0650v/CH0mpncbP3UpYzOiiQCaVHYpiCqVTcdi/oLhXhLwPklbs5gni/sNlbXz1zNFqdvAIjFd8Ge4DXKAK/GAHbp3PgRfBAP3O5Ppp1xnVwkr7uDlHqYzHFFU4shTkHWkx/oshL7IUv+BVwovHYDDfG7UfOC/2JOnfqxK/U+M2kB/hOvGvi3bFE6d2p2RC46jW7C+KxGeQ+CHXgJwJQjGswMu4TwkKwkEROTMLstPFNpABiMb68bKvlJtMuNvYIks6+XLACxVagm69kZZVIPCeWiTKsDEIHy7mtQtj/tfd38R/UH474+QhAe8gEEPJygzF8AucD2qipeMXAkzqVxjEvnLLm5yqxzHX0kxFCDACoUFrIlz2Kn/dBf9yQyPx6wIIcesDPQlLA+TT 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 12:53=E2=80=AFPM Peter Xu wrote= : > > On Tue, Apr 16, 2024 at 05:34:50PM -0400, Pasha Tatashin wrote: > > Hi Peter, > > Pasha, > > > > > Thanks for this patch, I like this extra checking logic, my comments be= low: > > Thanks for taking a look. > > > > > On Mon, Apr 15, 2024 at 4:53=E2=80=AFPM Peter Xu wr= ote: > > > > > > Allow page_table_check hooks to check over userfaultfd wr-protect cri= teria > > > upon pgtable updates. The rule is no co-existance allowed for any wr= itable > > > 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 ha= ve a > > > good chance to know where does that writable bit came from [1], so th= at > > > 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 rec= ent > > > 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 c= onfigs > > > 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 distro= s, > > > 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 o= f 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 iden= tify > > > 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 exam= ple, > > > 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_w= p() 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 o= f now > > > we can leave this for later too. > > > > > > This patch also removes commit c2da319c2e altogether, as we have some= thing > > > better now. > > > > > > [1] https://lore.kernel.org/all/000000000000dce0530615c89210@google.c= om/ > > > > > > Cc: Pasha Tatashin > > > Signed-off-by: Peter Xu > > > --- > > > arch/x86/include/asm/pgtable.h | 18 +----------------- > > > mm/page_table_check.c | 32 +++++++++++++++++++++++++++++++- > > > > Please add the new logic to: Documentation/mm/page_table_check.rst > > Will do. > > > > > > 2 files changed, 32 insertions(+), 18 deletions(-) > > > > > > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pg= table.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 =3D pte_flags(pte) & _PAGE_UFFD_WP; > > > - > > > -#ifdef CONFIG_DEBUG_VM > > > - /* > > > - * Having write bit for wr-protect-marked present ptes is fat= al, > > > - * 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., wh= en > > > - * page swapped out or being migrated for all purposes). It m= eans > > > - * something is already wrong. Tell the admin even before th= e > > > - * 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..d4eb1212f0f5 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,23 @@ void __page_table_check_pud_clear(struct mm_stru= ct *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 =3D swp_type(entry); > > > + > > > + return type =3D=3D SWP_DEVICE_EXCLUSIVE_WRITE || > > > + type =3D=3D SWP_MIGRATION_WRITE; > > > +} > > > + > > > +static inline void __page_table_check_pte(pte_t pte) > > > > may be something like: > > page_table_check_new_pte() ? Naming is starting to get confusing. The > > idea for this function is to check the pte that is about to be set > > into the page table. > > But then we keep __page_table_check_ptes_set() as is? > > It feels more natural if we keep using those underscores if all the rest > does so. The "_new" is also not matching with what you used to have as In mm/page_table_check.c, function names with an underscore prefix are intended for global symbols with internal use only. All local functions, such as page_table_check_set() and page_table_check_clear(), do not have this prefix as we do not pollute the global namespace. > "_set". If you see that's how I carefully chose the current name, with t= he > hope to match everything.. > > No strong opinions on these, but let me know your final choice of such > name. I'm happy to align that to your preference. > > > > > > +{ > > > + if (pte_present(pte) && pte_uffd_wp(pte)) > > > + WARN_ON_ONCE(pte_write(pte)); > > > + else if (is_swap_pte(pte) && pte_swp_uffd_wp(pte)) > > > + WARN_ON_ONCE(swap_cached_writable(pte_to_swp_entry(pt= e))); > > > +} > > > + > > > void __page_table_check_ptes_set(struct mm_struct *mm, pte_t *ptep, = pte_t pte, > > > unsigned int nr) > > > { > > > @@ -190,18 +209,29 @@ void __page_table_check_ptes_set(struct mm_stru= ct *mm, pte_t *ptep, pte_t pte, > > > if (&init_mm =3D=3D mm) > > > return; > > > > > > - for (i =3D 0; i < nr; i++) > > > + for (i =3D 0; i < nr; i++) { > > > + __page_table_check_pte(pte); > > > > This should really be called only once after this loop. > > This is also my intention to keep it in the loop just to make it as gener= ic > e.g. to have no assumption of "ignoring PFNs", and I didn't worry on perf > much as we'll read/write these ptes anyway, also because it's only enable= d > for debugging kernels. > > But I made it at least inaccurate by checking pte not *ptep.. > How about I move it out, rename it to __page_table_check_pte_flags(pte)? Sounds good. I like: page_table_check_pte_flags() page_table_check_pmd_flags() Pasha