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

["Zi Yan" <ziy@nvidia.com>]

On Mon Aug 31, 2026 at 3:25 PM EDT, Zi Yan wrote:
> After the changes of the prior commits, page/folio->private != NULL is now
> equivalent to checking PG_private.
>
> Stop checking PG_private on pages and folios and use page/folio->private
> instead, except swapcache and hugetlb folios, because the former uses a
> field (swp_entry_t swap) overlapping with ->private and the latter sets its
> flags in ->private. Exclude swapcache and hugetlb when the code is meant to
> check PG_private only.
>
> folio_expected_ref_count() can be called without folio lock, so annotate
> folio_test_private() with data_race() to avoid triggering race condition
> checks. While at it, annotate folio->mapping too.
>
> folio_set/clear_private() and Set/ClearPagePrivate() become no-ops.
> PG_private is no longer checked at page free time.
>
> Remove KPF_PRIVATE since PG_private is no longer used.
>
<snip>

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index dd09c438fa23e..786f8a47cea6d 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3022,9 +3022,9 @@ static inline bool folio_maybe_mapped_shared(struct folio *folio)
>   * @folio: the folio
>   *
>   * Calculate the expected folio refcount, taking references from the pagecache,
> - * swapcache, PG_private and page table mappings into account. Useful in
> - * combination with folio_ref_count() to detect unexpected references (e.g.,
> - * GUP or other temporary references).
> + * swapcache, private data (folio->private != NULL) and page table mappings into
> + * account. Useful in combination with folio_ref_count() to detect unexpected
> + * references (e.g., GUP or other temporary references).
>   *
>   * Does currently not consider references from the LRU cache. If the folio
>   * was isolated from the LRU (which is the case during migration or split),
> @@ -3062,10 +3062,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);

Sashiko said [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.

Answer:

Yes, it is a problem, since folio->swap.val and PG_swapcache cannot be
read as a whole, when __swap_cache_do_del_folio() (was
__delete_from_swap_cache()) clears folio->swap.val first then
PG_swapcache.

Fortunately, PG_swapbacked is stable during the process. So the code can
exclude swapcache folios by checking PG_swapbacked instead.

In the next patch, folio_test_fs_private() will be changed to replace the
new check: folio_test_private() && !folio_test_hugetlb(folio) &&
!folio_test_swapbacked().

<snip>

> 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) \

Sashiko said:
hould 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.

Answer:

Yes, will annotate with data_race() here.

>  	)
>  
>  TRACE_EVENT(mm_lru_insertion,
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index ced400f72d43a..546b37ccce37f 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -4810,7 +4810,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)) &&
>  		    folio_expected_ref_count(folio) != folio_ref_count(folio))
>  			goto next;

This is another lockless check and data_race() annotation is needed.


>  
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 15b45832bcfa7..f14e7bfee14bd 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);
>  			goto out;
>  		}

Sashiko said [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.


Answer:

Not an issue. With the help of gpt-5.6-sol, this issue only affects
buffer heads. Folios using iomap always clears folio->private before
folio->mapping is cleared.

> 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)));
>  

Another place needs data_race() annotation since filemap_dirty_folio()
can be called locklessly (e.g., zap_pte_range()).


-- 
Best Regards,
Yan, Zi