Re: [PATCH V6 05/10] dax/fsdev: clear pgmap ops and owner on unbind

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incorrect devres registration order leaves an error path unprotected and introduces a concurrent NULL pointer dereference during unbind.

Pre-existing issues:
- [High] Prematurely clearing folio state on unbind can corrupt memory for concurrent users.
--

commit 7c895b5292c660c81442978247753fce9d42653f
Author: John Groves <John@Groves.net>
Subject: dax/fsdev: clear pgmap ops and owner on unbind

This commit registers a devm action to clear pgmap->ops and pgmap->owner
on unbind. This prevents stale handlers from being executed if the static
device is later rebound to another driver or if the module is unloaded.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index 0fd5e1293d725..68a4369562f70 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c

[ ... ]

> @@ -306,6 +323,11 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  	if (IS_ERR(addr))
>  		return PTR_ERR(addr);
>  
> +	/* Drop fsdev's pgmap->ops/owner on unbind so no stale ops survive. */
> +	rc = devm_add_action_or_reset(dev, fsdev_clear_pgmap_ops, pgmap);
> +	if (rc)
> +		return rc;
> +

[Severity: High]
Does registering this action after devm_memremap_pages() cause an issue with
the devres LIFO unbind order?

Since devres unwinds in reverse order, fsdev_clear_pgmap_ops() will run
before the memory is unmapped by the devm_memremap_pages() release action.
This means pgmap->ops and pgmap->owner are set to NULL while the memory map
is still active and its per-CPU refcount is alive.

If a hardware error occurs concurrently during unbind, could memory_failure()
evaluate pgmap_has_memory_failure() as true, but then dereference a newly
NULL pgmap->ops?

mm/memory-failure.c:memory_failure_dev_pagemap() {
    ...
    if (pgmap_has_memory_failure(pgmap)) {
        rc = pgmap->ops->memory_failure(pgmap, pfn, 1, flags);
    ...
}

Similarly, could fsdev_pagemap_memory_failure() dereference a NULL dev_dax
pointer since the owner was cleared early?

Also, if devm_memremap_pages() returns an error, the function returns early
before this devm action is registered. Will the shared static pgmap retain
the stale ops and owner pointers on this error path?

Would moving this registration before devm_memremap_pages() fix both issues?

>  	/*
>  	 * Clear any stale compound folio state left over from a previous
>  	 * driver (e.g., device_dax with vmemmap_shift). Also register this
>  	 * as a devm action so folio state is cleared on unbind, ensuring
>  	 * clean pages for subsequent drivers (e.g., kmem for system-ram).
>  	 */
>  	fsdev_clear_folio_state(dev_dax);
>  	rc = devm_add_action_or_reset(dev, fsdev_clear_folio_state_action,
>  				      dev_dax);

[Severity: High]
This isn't a bug introduced by this patch, but does the preexisting
fsdev_clear_folio_state_action registration suffer from the same LIFO
devres ordering issue?

Because it is registered after devm_memremap_pages(), this action will
also execute before the active page map references drop during unbind.

Since fsdev_clear_folio_state() calls dax_folio_reset_order() to zero out
folio->mapping and folio->share, could this corrupt memory if threads are
performing direct I/O pins (e.g., via get_user_pages) and actively hold
valid references to these pages as compound folios?

Modifying folio metadata while the pages are actively pinned could lead to
refcount imbalances when concurrent users eventually unpin the folios.

Should this action also be moved before devm_memremap_pages()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019ecc080a68-8dc0c99f-ab17-4aa9-83d9-490e9c97ac2e-000000@email.amazonses.com?part=5