Dan Williams <dan.j.williams@xxxxxxxxx> writes:
[...]
> First up for the fix is release_mem_region_adjustable() needs to
> reliably delete the resource inserted by add_memory_driver_managed().
> That is thwarted by a check for IORESOURCE_SYSRAM that predates the
> dax/kmem driver, from commit:
>
> 65c78784135f ("kernel, resource: check for IORESOURCE_SYSRAM in release_mem_region_adjustable")
>
> That appears to be working around the behavior of HMM's
> "MEMORY_DEVICE_PUBLIC" facility that has since been deleted. With that
> check removed the "System RAM (kmem)" resource gets removed, but
> corruption still occurs occasionally because the "dax" resource is not
> reliably removed.
And re-added in the form of MEMORY_DEVICE_COHERENT, which is pretty
similar. However I can't understand why that commit was needed in the
first place. As mentioned in that commit message "we only call
release_mem_region_adjustable() in __remove_pages if the zone is not
ZONE_DEVICE" anyway, so I don't really understand how this code path
could ever have been exercised. I can't see why it's needed now either,
so for the change to resource.c feel free to add:
Reviewed-by: Alistair Popple <apopple@xxxxxxxxxx>
> The dax range information is freed before the device is unregistered, so
> the driver can not reliably recall (another use after free) what it is
> meant to release. Lastly if that use after free got lucky, the driver
> was covering up the leak of "System RAM (kmem)" due to its use of
> release_resource() which detaches, but does not free, child resources.
> The switch to remove_resource() forces remove_memory() to be responsible
> for the deletion of the resource added by add_memory_driver_managed().
>
> Fixes: c2f3011ee697 ("device-dax: add an allocation interface for device-dax instances")
> Cc: <stable@xxxxxxxxxxxxxxx>
> Cc: Oscar Salvador <osalvador@xxxxxxx>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Cc: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
> drivers/dax/bus.c | 2 +-
> drivers/dax/kmem.c | 4 ++--
> kernel/resource.c | 14 --------------
> 3 files changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index 012d576004e9..67a64f4c472d 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -441,8 +441,8 @@ static void unregister_dev_dax(void *dev)
> dev_dbg(dev, "%s\n", __func__);
>
> kill_dev_dax(dev_dax);
> - free_dev_dax_ranges(dev_dax);
> device_del(dev);
> + free_dev_dax_ranges(dev_dax);
> put_device(dev);
> }
>
> diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
> index 918d01d3fbaa..7b36db6f1cbd 100644
> --- a/drivers/dax/kmem.c
> +++ b/drivers/dax/kmem.c
> @@ -146,7 +146,7 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
> if (rc) {
> dev_warn(dev, "mapping%d: %#llx-%#llx memory add failed\n",
> i, range.start, range.end);
> - release_resource(res);
> + remove_resource(res);
> kfree(res);
> data->res[i] = NULL;
> if (mapped)
> @@ -195,7 +195,7 @@ static void dev_dax_kmem_remove(struct dev_dax *dev_dax)
>
> rc = remove_memory(range.start, range_len(&range));
> if (rc == 0) {
> - release_resource(data->res[i]);
> + remove_resource(data->res[i]);
> kfree(data->res[i]);
> data->res[i] = NULL;
> success++;
> diff --git a/kernel/resource.c b/kernel/resource.c
> index ddbbacb9fb50..b1763b2fd7ef 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1343,20 +1343,6 @@ void release_mem_region_adjustable(resource_size_t start, resource_size_t size)
> continue;
> }
>
> - /*
> - * All memory regions added from memory-hotplug path have the
> - * flag IORESOURCE_SYSTEM_RAM. If the resource does not have
> - * this flag, we know that we are dealing with a resource coming
> - * from HMM/devm. HMM/devm use another mechanism to add/release
> - * a resource. This goes via devm_request_mem_region and
> - * devm_release_mem_region.
> - * HMM/devm take care to release their resources when they want,
> - * so if we are dealing with them, let us just back off here.
> - */
> - if (!(res->flags & IORESOURCE_SYSRAM)) {
> - break;
> - }
> -
> if (!(res->flags & IORESOURCE_MEM))
> break;
>
