Re: [PATCH V6 07/10] dax/fsdev: fail probe on invalid pgmap offset

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The validation check for an impossible mapping state (`pgmap_phys > phys`) is executed after iterating and modifying folio states, leading to a potential kernel panic or memory corruption before the error can be gracefully caught.
- [High] The devm action `fsdev_clear_folio_state_action` is registered after `devm_memremap_pages`. On unbind, it executes before the release of memory, violently clearing compound folio states while the folios might still be pinned by active users.
--

commit e4852035a220057fe89d87ee2e638d326fb3c8e1
Author: John Groves <John@Groves.net>

dax/fsdev: fail probe on invalid pgmap offset

This commit converts a WARN_ON into a fatal error during device probe when
the remapped region starts after the device's data region. If this invalid
state is detected, the probe now returns -EINVAL instead of continuing with
a misaligned address.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index 57c589e19b539..d50891d6dc135 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -342,8 +342,12 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  		u64 phys = dev_dax->ranges[0].range.start;
>  		u64 pgmap_phys = pgmap[0].range.start;
>  
> -		if (!WARN_ON(pgmap_phys > phys))
> -			data_offset = phys - pgmap_phys;
> +		if (pgmap_phys > phys) {
> +			dev_err(dev, "pgmap start %#llx exceeds data start %#llx\n",
> +				pgmap_phys, phys);
> +			return -EINVAL;
> +		}
> +		data_offset = phys - pgmap_phys;

[Severity: High]
This isn't a bug introduced by this patch, but does this check happen too
late?

Looking at fsdev_dax_probe(), fsdev_clear_folio_state() is called before this
validation check occurs. fsdev_clear_folio_state() iterates over the ranges
starting from phys. If pgmap_phys > phys, devm_memremap_pages() mapped the
vmemmap starting from pgmap_phys.

Will accessing pfn_folio() for physical addresses before pgmap_phys access
unpopulated vmemmap or memory belonging to someone else, leading to a kernel
panic or memory corruption before we ever reach this return -EINVAL?

[Severity: High]
This is a pre-existing issue, but does the tear-down order of devm actions
cause a race condition on unbind?

In fsdev_dax_probe(), fsdev_clear_folio_state_action is registered via
devm_add_action_or_reset() after devm_memremap_pages() is called.

Since devm actions run in LIFO order, fsdev_clear_folio_state_action will
execute on unbind before memunmap_pages() has a chance to wait for active
folio references to drop. Can this violently clear compound folio states
(like setting folio->share = 0 and splitting compound folios) while external
subsystems might still be actively holding references, leading to refcount
underflows and memory corruption?

>  
>  		pr_debug("%s: offset detected phys=%llx pgmap_phys=%llx offset=%llx\n",
>  		       __func__, phys, pgmap_phys, data_offset);

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