From: Ryan Roberts <ryan.roberts@arm.com>
To: Andrew Morton <akpm@linux-foundation.org>,
David Hildenbrand <david@redhat.com>,
Muhammad Usama Anjum <usama.anjum@collabora.com>,
Peter Xu <peterx@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v1] fs/proc/task_mmu: Fix uffd-wp confusion in pagemap_scan_pmd_entry()
Date: Mon, 29 Apr 2024 16:55:27 +0100 [thread overview]
Message-ID: <5f193f0b-7f41-441e-8f98-09a7ddbf4166@arm.com> (raw)
In-Reply-To: <20240429114104.182890-1-ryan.roberts@arm.com>
On 29/04/2024 12:41, Ryan Roberts wrote:
> pagemap_scan_pmd_entry() checks if uffd-wp is set on each pte to avoid
> unnecessary if set. However it was previously checking with
> `pte_uffd_wp(ptep_get(pte))` without first confirming that the pte was
> present. It is only valid to call pte_uffd_wp() for present ptes. For
> swap ptes, pte_swp_uffd_wp() must be called because the uffd-wp bit may
> be kept in a different position, depending on the arch.
>
> This was leading to test failures in the pagemap_ioctl mm selftest, when
> bringing up uffd-wp support on arm64 due to incorrectly interpretting
> the uffd-wp status of migration entries.
>
> Let's fix this by using the correct check based on pte_present(). While
> we are at it, let's pass the pte to make_uffd_wp_pte() to avoid the
> pointless extra ptep_get() which can't be optimized out due to
> READ_ONCE() on many arches.
>
> Closes: https://lore.kernel.org/linux-arm-kernel/ZiuyGXt0XWwRgFh9@x1n/
> Fixes: 12f6b01a0bcb ("fs/proc/task_mmu: add fast paths to get/clear PAGE_IS_WRITTEN flag")
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
I guess this should have cc'ed stable but I forgot to add it. Are you able to
fix this up when you take it, Andrew, or do I need to repost?
> ---
> fs/proc/task_mmu.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index af4bc1da0c01..102f48668c35 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1817,10 +1817,8 @@ static unsigned long pagemap_page_category(struct pagemap_scan_private *p,
> }
>
> static void make_uffd_wp_pte(struct vm_area_struct *vma,
> - unsigned long addr, pte_t *pte)
> + unsigned long addr, pte_t *pte, pte_t ptent)
> {
> - pte_t ptent = ptep_get(pte);
> -
> if (pte_present(ptent)) {
> pte_t old_pte;
>
> @@ -2175,9 +2173,12 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
> if ((p->arg.flags & PM_SCAN_WP_MATCHING) && !p->vec_out) {
> /* Fast path for performing exclusive WP */
> for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
> - if (pte_uffd_wp(ptep_get(pte)))
> + pte_t ptent = ptep_get(pte);
> +
> + if ((pte_present(ptent) && pte_uffd_wp(ptent)) ||
> + pte_swp_uffd_wp_any(ptent))
> continue;
> - make_uffd_wp_pte(vma, addr, pte);
> + make_uffd_wp_pte(vma, addr, pte, ptent);
> if (!flush_end)
> start = addr;
> flush_end = addr + PAGE_SIZE;
> @@ -2190,8 +2191,10 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
> p->arg.return_mask == PAGE_IS_WRITTEN) {
> for (addr = start; addr < end; pte++, addr += PAGE_SIZE) {
> unsigned long next = addr + PAGE_SIZE;
> + pte_t ptent = ptep_get(pte);
>
> - if (pte_uffd_wp(ptep_get(pte)))
> + if ((pte_present(ptent) && pte_uffd_wp(ptent)) ||
> + pte_swp_uffd_wp_any(ptent))
> continue;
> ret = pagemap_scan_output(p->cur_vma_category | PAGE_IS_WRITTEN,
> p, addr, &next);
> @@ -2199,7 +2202,7 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
> break;
> if (~p->arg.flags & PM_SCAN_WP_MATCHING)
> continue;
> - make_uffd_wp_pte(vma, addr, pte);
> + make_uffd_wp_pte(vma, addr, pte, ptent);
> if (!flush_end)
> start = addr;
> flush_end = next;
> @@ -2208,8 +2211,9 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
> }
>
> for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
> + pte_t ptent = ptep_get(pte);
> unsigned long categories = p->cur_vma_category |
> - pagemap_page_category(p, vma, addr, ptep_get(pte));
> + pagemap_page_category(p, vma, addr, ptent);
> unsigned long next = addr + PAGE_SIZE;
>
> if (!pagemap_scan_is_interesting_page(categories, p))
> @@ -2224,7 +2228,7 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
> if (~categories & PAGE_IS_WRITTEN)
> continue;
>
> - make_uffd_wp_pte(vma, addr, pte);
> + make_uffd_wp_pte(vma, addr, pte, ptent);
> if (!flush_end)
> start = addr;
> flush_end = next;
> --
> 2.25.1
>
prev parent reply other threads:[~2024-04-29 15:55 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-29 11:41 [PATCH v1] fs/proc/task_mmu: Fix uffd-wp confusion in pagemap_scan_pmd_entry() Ryan Roberts
2024-04-29 12:22 ` David Hildenbrand
2024-04-29 13:28 ` Muhammad Usama Anjum
2024-04-29 15:55 ` Ryan Roberts [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5f193f0b-7f41-441e-8f98-09a7ddbf4166@arm.com \
--to=ryan.roberts@arm.com \
--cc=akpm@linux-foundation.org \
--cc=david@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=peterx@redhat.com \
--cc=usama.anjum@collabora.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).