On Wed, May 04, 2022 at 02:44:25PM -0700, Zach O'Keefe wrote:
> +static int find_pmd_or_thp_or_none(struct mm_struct *mm,
> + unsigned long address,
> + pmd_t **pmd)
> +{
> + pmd_t pmde;
> +
> + *pmd = mm_find_pmd_raw(mm, address);
> + if (!*pmd)
> + return SCAN_PMD_NULL;
> +
> + pmde = pmd_read_atomic(*pmd);
It seems to be correct on using the atomic fetcher here. Though irrelevant
to this patchset.. does it also mean that we miss that on mm_find_pmd()? I
meant a separate fix like this one:
---8<---
diff --git a/mm/rmap.c b/mm/rmap.c
index 69416072b1a6..61309718640f 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -785,7 +785,7 @@ pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address)
* without holding anon_vma lock for write. So when looking for a
* genuine pmde (in which to find pte), test present and !THP together.
*/
- pmde = *pmd;
+ pmde = pmd_read_atomic(pmd);
barrier();
if (!pmd_present(pmde) || pmd_trans_huge(pmde))
pmd = NULL;
---8<---
As otherwise it seems it's also prone to PAE race conditions when reading
pmd out, but I could be missing something.
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> + /* See comments in pmd_none_or_trans_huge_or_clear_bad() */
> + barrier();
> +#endif
> + if (!pmd_present(pmde))
> + return SCAN_PMD_NULL;
> + if (pmd_trans_huge(pmde))
> + return SCAN_PMD_MAPPED;
Would it be safer to check pmd_bad()? I think not all mm pmd paths check
that, frankly I don't really know what's the major cause of a bad pmd
(either software bugs or corrupted mem), but just to check with you,
because potentially a bad pmd can be read as SCAN_SUCCEED and go through.
> + return SCAN_SUCCEED;
> +}
The rest looks good to me, thanks.
--
Peter Xu
