Re: [PATCH V5 3/9] dax/fsdev: clear vmemmap_shift when binding static pgmap

[sashiko-bot@kernel.org]

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

New issues:
- [High] Rebinding a static DAX device from fsdev_dax to device_dax (or unloading the fsdev_dax module) leaves a dangling `pgmap->ops` pointer on the persistent static `pgmap` structure, risking a use-after-free on hardware memory failure.

Pre-existing issues:
- [High] Probe functions in fsdev_dax re-initialize an embedded cdev via `cdev_init()`, clobbering its kobject state if there are open file descriptors from a previous bind.
--

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

dax/fsdev: clear vmemmap_shift when binding static pgmap

Clear pgmap->vmemmap_shift for static DAX devices. When rebinding a
static device from device_dax to fsdev_dax, the stale vmemmap_shift
persists on the shared pgmap. Explicitly zero it 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
> index 2c5de3d80a618..52f46b3e245ea 100644
> --- 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]
Since we are fixing the stale vmemmap_shift on the shared pgmap here, do we
also need to handle the pgmap->ops pointer?

If fsdev_dax unbinds or the module is unloaded, it doesn't appear that we
clear pgmap->ops, which is set to &fsdev_pagemap_ops later in
fsdev_dax_probe(). Additionally, if device_dax is bound to the same device
instead, it appears to update pgmap->type but leave pgmap->ops unchanged.

If a hardware memory error occurs on the region after unbind, will
memory_failure_dev_pagemap() attempt to call the dangling
pgmap->ops->memory_failure pointer:

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

and cause a use-after-free or panic?

[Severity: High]
This isn't a bug introduced by this patch, but does the embedded cdev
initialization in this probe function clobber the kobject state?

If there are open file descriptors from a previous bind, and
fsdev_dax_probe() re-initializes the embedded cdev later in the function:

drivers/dax/fsdev.c:fsdev_dax_probe()
    cdev_init(cdev, &fsdev_fops);

will cdev_init() destroy the active kobject's refcount and zero out the
cdev->list? Could this cause list corruption when the stale file descriptors
are later closed, or mismatched kobject refcounting that frees the cdev
while the new driver instance is using it?

>  	} else {
>  		size_t pgmap_size;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019eb7bcda4b-3f8edae9-d7a4-4bfa-aaea-fcef77fdbbc3-000000@email.amazonses.com?part=3