Re: [PATCH v2 09/14] mm/page-flags: check page/folio->private instead of PG_private

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Lockless TOCTOU race in `folio_expected_ref_count()` miscalculates expected references for shmem folios during swap cache removal, allowing `F_SEAL_WRITE` to bypass GUP pins.
- [Low] Lockless reads of `folio->private` via `folio_test_fs_private()` lack `data_race()` or `READ_ONCE()` annotations in several paths, triggering KCSAN warnings.

Pre-existing issues:
- [High] In `migrate_folio_unmap()`, an orphaned folio with private data is blindly assumed to have `buffer_head`s, causing a type confusion crash for filesystems that use other structures.
--

commit cfacca810fa1b9878c94f6054258d5e3c3b6824f
Author: Zi Yan <ziy@nvidia.com>

mm/page-flags: check page/folio->private instead of PG_private

This patch changes the kernel to check page/folio->private instead of 
PG_private, except for swapcache and hugetlb folios. It annotates
folio_expected_ref_count to prevent data races and removes KPF_PRIVATE.

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a9fbe26536f45..8be6399a5e4a0 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
[ ... ]
> @@ -3050,10 +3050,18 @@ static inline int folio_expected_ref_count(const struct folio *folio)
>  	ref_count += folio_test_swapcache(folio) << order;
>  
>  	if (!folio_test_anon(folio)) {
> -		/* One reference per page from the pagecache. */
> -		ref_count += !!folio->mapping << order;
> -		/* One reference from PG_private. */
> -		ref_count += folio_test_private(folio);
> +		/*
> +		 * One reference per page from the pagecache.
> +		 * Use data_race() since folio might not be locked.
> +		 */
> +		ref_count += !!data_race(folio->mapping) << order;
> +		/*
> +		 * One reference from filesystem private data.
> +		 * Use data_race() since folio might not be locked.
> +		 */
> +		ref_count += data_race(folio_test_private(folio)) &&
> +			     !folio_test_hugetlb(folio) &&
> +			     !folio_test_swapcache(folio);

[Severity: High]
Could this lockless evaluation of folio->private and PG_swapcache lead to a
TOCTOU race for shmem folios during swap cache removal?

During __delete_from_swap_cache, folio->swap.val (which aliases
folio->private) is cleared before PG_swapcache. Without memory barriers, a
lockless reader like memfd_tag_pins calling folio_expected_ref_count could
observe the stale non-zero folio->private and the newly cleared PG_swapcache.

This would evaluate the condition above as true, falsely inflating the
expected refcount by 1. If the folio has exactly one extra GUP pin, the
inflated expected refcount would match the actual refcount, bypassing the
F_SEAL_WRITE protections.

>  	}
>  
>  	/* One reference per page table mapping. */
> diff --git a/include/trace/events/pagemap.h b/include/trace/events/pagemap.h
> index 36c3a90f0acca..fb9abec40ec79 100644
> --- a/include/trace/events/pagemap.h
> +++ b/include/trace/events/pagemap.h
> @@ -22,7 +22,9 @@
>  	(folio_test_swapcache(folio)	? PAGEMAP_SWAPCACHE  : 0) | \
>  	(folio_test_swapbacked(folio)	? PAGEMAP_SWAPBACKED : 0) | \
>  	(folio_test_mappedtodisk(folio)	? PAGEMAP_MAPPEDDISK : 0) | \
> -	(folio_test_private(folio)	? PAGEMAP_BUFFERS    : 0) \
> +	(folio_test_private(folio) && \
> +	 !folio_test_swapcache(folio) && \
> +	 !folio_test_hugetlb(folio)	? PAGEMAP_BUFFERS    : 0) \

[Severity: Low]
Should this lockless read of folio->private be annotated with data_race() or
READ_ONCE()?

Since tracepoints like TRACE_EVENT(mm_lru_insertion) are often executed
without the folio lock, concurrent modifications to folio->private could
trigger KCSAN data race warnings here.

>  	)
>  
>  TRACE_EVENT(mm_lru_insertion,
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 54494c3fa9835..5f2964a5fffbb 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -4858,7 +4858,9 @@ static int split_huge_pages_pid(int pid, unsigned long vaddr_start,
>  		 * will try to drop it before split and then check if the folio
>  		 * can be split or not. So skip the check here.
>  		 */
> -		if (!folio_test_private(folio) &&
> +		if (!(folio_test_private(folio) &&
> +		      !folio_test_swapcache(folio) &&
> +		      !folio_test_hugetlb(folio)) &&

[Severity: Low]
Can this similarly trigger a KCSAN data race warning?

This block in split_huge_pages_pid checks the private flags locklessly before
folio_trylock is called, so reading folio->private here without data_race()
could cause issues.

>  		    folio_expected_ref_count(folio) != folio_ref_count(folio))
>  			goto next;
>  
> diff --git a/mm/migrate.c b/mm/migrate.c
> index a369d0c95c386..9d6c8b6f57475 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1327,7 +1327,8 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>  	 * free the metadata, so the page can be freed.
>  	 */
>  	if (!src->mapping) {
> -		if (folio_test_private(src)) {
> +		if (folio_test_private(src) && !folio_test_swapcache(src) &&
> +		    !folio_test_hugetlb(src)) {
>  			try_to_free_buffers(src);

[Severity: High]
This is a pre-existing issue, but does this generic migration code blindly
assume all private data on an orphaned folio corresponds to buffer heads?

If a folio is orphaned after truncation (src->mapping is NULL) but retains
private data because an earlier release failed, try_to_free_buffers is
unconditionally called. For filesystems like iomap, folio->private contains
custom structures (like iomap_folio_state), not a linked list of buffer
heads. Calling try_to_free_buffers here could lead to type confusion and a
kernel panic when accessing what it expects to be bh->b_this_page.

>  			goto out;
>  		}
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index eeab25d6ce364..4022d6c381896 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2705,7 +2705,10 @@ bool filemap_dirty_folio(struct address_space *mapping, struct folio *folio)
>  	if (folio_test_set_dirty(folio))
>  		return false;
>  
> -	__folio_mark_dirty(folio, mapping, !folio_test_private(folio));
> +	__folio_mark_dirty(folio, mapping,
> +			   !(folio_test_private(folio) &&
> +			     !folio_test_swapcache(folio) &&
> +			     !folio_test_hugetlb(folio)));

[Severity: Low]
Is a data_race() annotation missing here as well?

Functions like filemap_dirty_folio can be called locklessly (for example,
from zap_pte_range with only the pte lock held), resulting in an unannotated
read of folio->private that might race with concurrent modifications.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-remove-pg_private-v2-0-3668159cd9e8@nvidia.com?part=9