Re: [PATCH V6 03/10] dax/fsdev: clear vmemmap_shift when binding static pgmap

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Dangling pointer in `dev_dax->pgmap` on probe failure for dynamic DAX devices permanently breaks subsequent driver binds.
--

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

dax/fsdev: clear vmemmap_shift when binding static pgmap

This commit clears the vmemmap_shift on the shared pgmap for static DAX
devices when rebinding from device_dax to fsdev_dax. This explicitly
zeroes the shift before devm_memremap_pages() so the vmemmap is built
for order-0 folios.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -237,6 +237,7 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  		}
>  
>  		pgmap = dev_dax->pgmap;
> +		pgmap->vmemmap_shift = 0;

[Severity: High]
This is a pre-existing issue, but while reviewing the probe logic here,
could a dangling pointer occur during probe failure for dynamic DAX
devices in drivers/dax/device.c?

Looking at dev_dax_probe() in drivers/dax/device.c, dev_dax->pgmap is
assigned to a devm-managed allocation:

drivers/dax/device.c:dev_dax_probe() {
    ...
    dev_dax->pgmap = pgmap;
    ...
}

If a subsequent devm operation fails, such as:

drivers/dax/device.c:dev_dax_probe() {
    ...
    if (!devm_request_mem_region(dev, range->start,
                                 range_len(range), dev_name(dev))) {
        dev_warn(dev, "mapping%d: %#llx-%#llx could not reserve range\n",
                 i, range->start, range->end);
        return -EBUSY;
    }
    ...
}

The probe aborts and devres frees the pgmap. However, since dev_dax_kill
has not yet been registered as a devm action, dev_dax->pgmap is not reset
to NULL.

If a subsequent probe attempt occurs, would it hit this earlier check and
permanently fail by evaluating the dangling pointer?

drivers/dax/device.c:dev_dax_probe() {
    ...
    if (dev_dax->pgmap) {
        dev_warn(dev,
                 "dynamic-dax with pre-populated page map\n");
        return -EINVAL;
    }
    ...
}

Since the underlying dev_dax structure persists on the DAX bus, does this
effectively block the dynamic DAX device from being bound until it is
explicitly deleted and recreated?

>  	} else {
>  		size_t pgmap_size;
>

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