Re: [PATCH V4 1/9] dax: fix misleading comment about share/index union in dax_folio_reset_order()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect PFN advancement in `fsdev_clear_folio_state()` when starting from a tail page of a stale compound folio.
--

commit eb3e25245bdf433dc92d71266558b877ef6655c1
Author: John Groves <John@Groves.net>
dax: fix misleading comment about share/index union in dax_folio_reset_order()

This commit updates a misleading comment in dax_folio_reset_order() to
accurately describe the aliasing of folio->share and folio->index in the
union. It clarifies that the assignment safely clears whichever interpretation
is active, as both fields should be zeroed when the folio is released.

> diff --git a/fs/dax.c b/fs/dax.c
> index 6d175cd47a99b..df19c9317d10e 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -392,12 +392,12 @@ int dax_folio_reset_order(struct folio *folio)
>  	int order = folio_order(folio);
>  
>  	/*
> -	 * DAX maintains the invariant that folio->share != 0 only when
> -	 * folio->mapping == NULL (enforced by dax_folio_make_shared()).
> -	 * Equivalently: folio->mapping != NULL implies folio->share == 0.
> -	 * Callers ensure share has been decremented to zero before
> -	 * calling here, so unconditionally clearing both fields is
> -	 * correct.
> +	 * Clear the mapping and the index/share union word. folio->share
> +	 * and folio->index occupy the same union in struct folio. For
> +	 * non-shared folios (mapping != NULL), the union holds folio->index
> +	 * (file page offset); for shared folios (mapping == NULL), it holds
> +	 * folio->share (reference count). Either way, we are releasing the
> +	 * folio and both fields should be zeroed.
>  	 */
>  	folio->mapping = NULL;

[Severity: High]
This isn't a bug introduced by this patch, but looking at how callers use
dax_folio_reset_order():

drivers/dax/fsdev.c:fsdev_clear_folio_state() {
...
		while (pfn < end_pfn) {
			struct folio *folio = pfn_folio(pfn);
			int order = dax_folio_reset_order(folio);

			pfn += 1UL << order;
		}
...
}

Does this loop incorrectly advance the pfn if the range start falls within a
previously initialized compound page (a tail page)?

Since pfn_folio() returns the head folio, advancing by 1UL << order from the
current pfn might skip over the beginning of the next contiguous memory
region. This could leave stale compound metadata on those skipped pages.

Should the pfn advancement account for the page's index within the folio?

>  	folio->share = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019ea3929225-a0f8e6f7-30ae-4f8e-ae6f-19129666c4c3-000000@email.amazonses.com?part=1