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 BA68DC4345F for ; Tue, 16 Apr 2024 21:35:30 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 16F7D6B0083; Tue, 16 Apr 2024 17:35:30 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 11FB36B0085; Tue, 16 Apr 2024 17:35:30 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F02516B0088; Tue, 16 Apr 2024 17:35:29 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id D20516B0083 for ; Tue, 16 Apr 2024 17:35:29 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 52421120B47 for ; Tue, 16 Apr 2024 21:35:29 +0000 (UTC) X-FDA: 82016701578.27.3D625FA Received: from mail-qt1-f179.google.com (mail-qt1-f179.google.com [209.85.160.179]) by imf29.hostedemail.com (Postfix) with ESMTP id 9E74D120010 for ; Tue, 16 Apr 2024 21:35:27 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=soleen-com.20230601.gappssmtp.com header.s=20230601 header.b=eRyqRYVf; dmarc=pass (policy=none) header.from=soleen.com; spf=pass (imf29.hostedemail.com: domain of pasha.tatashin@soleen.com designates 209.85.160.179 as permitted sender) smtp.mailfrom=pasha.tatashin@soleen.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713303327; 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=NsVvW9UN/T1Z6HI6qn67MvHY2qYpnw17QsJrjMaEsPI=; b=qS5zyurcNfcY02mGbMeb8KOehd+LFRi7iBCeYv3pCq2JDOQKZIi/wpRlcnpTAGdNF9gMgS dWLSgpLinRdBxeWFMGjEkc03eUdag2xwK2b4LOu7AQrlTyNYlUHj+YKNye1WDg5A1xwiya zol2cTWDoJecVKSyTWiwkNfwoOMAiEQ= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=soleen-com.20230601.gappssmtp.com header.s=20230601 header.b=eRyqRYVf; dmarc=pass (policy=none) header.from=soleen.com; spf=pass (imf29.hostedemail.com: domain of pasha.tatashin@soleen.com designates 209.85.160.179 as permitted sender) smtp.mailfrom=pasha.tatashin@soleen.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713303327; a=rsa-sha256; cv=none; b=H9KSEQdhQjHxtnqbqmLANCfMt8SMTkEvYeVQxWcLQ4tbHw4i6xSbA6HraDcAV3onzwrsXn 6VBtL+lfmQUQ7pPFhJfBKEVcCQnaJjcXCSSj8XR3nYy2sz6QmOfuESZmxbSOmESWxiS05O 5hAefa62UH1gCRlXW9dzQfVXWPBc2k4= Received: by mail-qt1-f179.google.com with SMTP id d75a77b69052e-4375b8b4997so3806791cf.2 for ; Tue, 16 Apr 2024 14:35:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=soleen-com.20230601.gappssmtp.com; s=20230601; t=1713303327; x=1713908127; 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=NsVvW9UN/T1Z6HI6qn67MvHY2qYpnw17QsJrjMaEsPI=; b=eRyqRYVfzY+aV5soQCHgEtYLRUaF4AS4c/yciF+7h1hFa/vslMoqZypVHqFH5TbKIf r0KhAc1LUPhnndSBsInSOl90N2N/pxg5MIA5/qTSaw9Fj3JE0lj5XzkVysZFWz5Ojgdn PKsU7x6uPJnE+Y+GmAtzh7INjt2RzjXKZbbLKDIOZTm94jXDmz9LW/JJHs3JNvX0uprV X94IVedvCq2iwUYUk85x2694Q1thC3bdh14wEqrCBdeXT3PRqU+xcVFCw63eewiqgT+o JwxKlg6cDrTHS5RtcMBcLel06LUOf358Z9M/2TAkJOtqh21fTRxMK5AshGqTUmetW3c9 0Buw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713303327; x=1713908127; 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=NsVvW9UN/T1Z6HI6qn67MvHY2qYpnw17QsJrjMaEsPI=; b=s7xpcSqI41wac8DjucuVirq0RWLikU1uE6AS6ehkpVe9/GxJcpOTsbXCG/UHimnbqV tf98tSIqoJepYs4jQuYLCJd6JW4dcDslqmC+hZXrofZoKxKrVD9WBS0Q2ZsKSdRAuBQE yLuyelwhI2L9NCPIndFLmvujyp8CSAgtXA/wyh+omdIJDS4fzoLxeAe//ZYC1mD9HJ32 qXEExLMCczv2rsGvjof/yDg8c+Lq3LRWlM5JH9V1f65Nx9+bjZM8bnywxFtUij7PVp3D GxysfQiM5OAqwx/s+fhpNWosiS5TzJx/1JvXrGHNaGHilHJpecV/5FamDJVg4kOrvI2d HAWg== X-Forwarded-Encrypted: i=1; AJvYcCUWNNcnyRAeyzXI8lJZgFYgd6ruDSbmcYwCRIQPBAXBL0gkq5AN2qlQg68BPKT+PYYcwp4ZcXmtmLeSlinnjyWu93g= X-Gm-Message-State: AOJu0YwksQCucjtZSdfBdaIlVZTmM+gnSJ/V6zh5fTU+p1LrC4udUcUZ EqC6mXT/PHXDx0kBeR3EOdsUCwU8sHdJVLLp+0Fkq3sY+GIwT9DYbRV2F/lwmAjdXeqr0ceu/XW ElzfQifG/4FxWn6rBt0VbOrvcRbWbPO+w1qGM3Q== X-Google-Smtp-Source: AGHT+IFuefJGGOxRGnvAFV36oj38O3TLAnqZtZNp7C+CXSwoxzFpU/Yla/VjxPkq7ft4YaBFjwQ0EwK4l8JA0kUOItw= X-Received: by 2002:ac8:59c1:0:b0:437:5da2:7537 with SMTP id f1-20020ac859c1000000b004375da27537mr1760441qtf.9.1713303326702; Tue, 16 Apr 2024 14:35:26 -0700 (PDT) MIME-Version: 1.0 References: <20240415205259.2535077-1-peterx@redhat.com> In-Reply-To: <20240415205259.2535077-1-peterx@redhat.com> From: Pasha Tatashin Date: Tue, 16 Apr 2024 17:34:50 -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-Rspam-User: X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 9E74D120010 X-Stat-Signature: 7tnb1nk5oq7rmytn76afagwrfdrrjhed X-HE-Tag: 1713303327-551315 X-HE-Meta: U2FsdGVkX1/EOvPV/NwCFvbn5aEqBs4PgUT9gBH66Tf1mjXzaesES4SXHHFUZcgXkoG7/RUNbeGmm4pRHl6iRYExsrcdJKkv2ULeeJAVmUX/JbfwRqC6r9gPT+63iYe9xZyPSzE+EJePHo36K4RYQPZqVWQ2p+c0tn5N0hf9bocy7NKzMTxffWLGQLKe3MjtIU7PH5UUzuhBwm/ldZU3UKUElN18lm2BtqRwiH2j/XA0uV/SQMj3f/gEossWKInj698KMnAbkeLBSxDYanEtFYpPlpnce0iXnB/u4amCMaj2rzUxpu7L5lFiKFVv18lWsxYR9uSSMqO4CHuBdEo2HSl/5vG73h7/4TZ1ExIgno+CVVy2u0kXmv7i15CWyPDG2vteQgn7iE9N4aJuvhWUA6EnvhqaZID9IfalzULZt4TZrGhGuddMAzQ1BwUm70MBxkoSeLxFtEKt4R0FGnRYWsiUwjCN5Yo4WudfMIe2EEepLUFiE8eM//hqW1rKvQg9zRIPdnZE9rZLLiENX1ISfvTgX+BskIZEH+62EsdxArc4FQ5RD9h2lloMf0uZMfmrGwQkKSlurQBjj04TgL5LzGFzSjXhU0jjD8BtkVE0tw2o5GdsQqBai2LHt0HowzrG7/y9LRPKIfOQWAbuAqUHGxndH7M40ELAP948aCsX9C6j4uADUomHOXvI6Ph7f7UTuUKsn16E0WNJ2b2EWFNr0ypbTbaJ1MSrrDbRfOgBLzrS4koTRV9Kh9IkyB5PDH+Vw728mOjBxCmOfGeC++jCKKbyP+V7oZGLocFrRd33lK9OloF2QvY83MiBSBIFela4iU9Sgn8wa5Wr6iKoxjFyedjBGaiPJboPddwdrDyYcUKXr/MwUvYXza9fxbes7/P2BSJlUz3Lqqllcimm3FYnMoY9gMEkcL298cuTexz6sQBszOeEbzYEHHbiGGUdCYupKsyTBaaCri374Zxc4h0 B4pvvZp0 xD1qYBRPnPE6TmqkE8WsPoHwtvkfLx3/Xh6PwcWs8K8FqhRni6laFmfobnBYSvh7vt7769uI319BZwGo+DLY+oGV61Zb5mCt/+2czBwBSc/qtZKDcnmqc6rYHeTi0TRStUkRc+MvEgUpeFraZinKgxi4BEWEj839ugCoqIhC+EyA+MtI= 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: Hi Peter, Thanks for this patch, I like this extra checking logic, my comments below: On Mon, Apr 15, 2024 at 4:53=E2=80=AFPM Peter Xu wrote: > > Allow page_table_check hooks to check over userfaultfd wr-protect criteri= a > upon pgtable updates. The rule is no co-existance allowed for any writab= le > flag against userfault wr-protect flag. > > This should be better than c2da319c2e, where we used to only sanitize suc= h > 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 confi= gs > 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], whi= ch > 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 th= e > 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 the= n > one huge_pte_uffd_wp() per-arch will stop making sense first.. as of no= w > we can leave this for later too. > > This patch also removes commit c2da319c2e altogether, as we have somethin= g > better now. > > [1] https://lore.kernel.org/all/000000000000dce0530615c89210@google.com/ > > 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 > 2 files changed, 32 insertions(+), 18 deletions(-) > > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtabl= e.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 fatal, > - * because it means the uffd-wp bit will be ignored and write wil= l > - * 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..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_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 =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. > +{ > + 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(pte)))= ; > +} > + > 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_struct *= 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. > __page_table_check_pte_clear(mm, ptep_get(ptep + i)); > + } > if (pte_user_accessible_page(pte)) > page_table_check_set(pte_pfn(pte), nr, pte_write(pte)); > } > EXPORT_SYMBOL(__page_table_check_ptes_set); > > +static inline void __page_table_check_pmd(pmd_t pmd) page_table_check_new_pmd() ? > +{ > + if (pmd_present(pmd) && pmd_uffd_wp(pmd)) > + WARN_ON_ONCE(pmd_write(pmd)); > + else if (is_swap_pmd(pmd) && pmd_swp_uffd_wp(pmd)) > + WARN_ON_ONCE(swap_cached_writable(pmd_to_swp_entry(pmd)))= ; > +} > + > void __page_table_check_pmd_set(struct mm_struct *mm, pmd_t *pmdp, pmd_t= pmd) > { > if (&init_mm =3D=3D mm) > return; > > + __page_table_check_pmd(pmd); > __page_table_check_pmd_clear(mm, *pmdp); > if (pmd_user_accessible_page(pmd)) { > page_table_check_set(pmd_pfn(pmd), PMD_SIZE >> PAGE_SHIFT= , > -- > 2.44.0 > Thanks, Pasha