Re: [PATCH] dax/kmem: Fix leak of memory-hotplug resources

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Thu, 2023-02-16 at 00:36 -0800, Dan Williams wrote:
> While experimenting with CXL region removal the following corruption of
> /proc/iomem appeared.
> 
> Before:
> f010000000-f04fffffff : CXL Window 0
>   f010000000-f02fffffff : region4
>     f010000000-f02fffffff : dax4.0
>       f010000000-f02fffffff : System RAM (kmem)
> 
> After (modprobe -r cxl_test):
> f010000000-f02fffffff : **redacted binary garbage**
>   f010000000-f02fffffff : System RAM (kmem)
> 
> ...and testing further the same is visible with persistent memory
> assigned to kmem:
> 
> Before:
> 480000000-243fffffff : Persistent Memory
>   480000000-57e1fffff : namespace3.0
>   580000000-243fffffff : dax3.0
>     580000000-243fffffff : System RAM (kmem)
> 
> After (ndctl disable-region all):
> 480000000-243fffffff : Persistent Memory
>   580000000-243fffffff : ***redacted binary garbage***
>     580000000-243fffffff : System RAM (kmem)
> 
> The corrupted data is from a use-after-free of the "dax4.0" and "dax3.0"
> resources, and it also shows that the "System RAM (kmem)" resource is
> not being removed. The bug does not appear after "modprobe -r kmem", it
> requires the parent of "dax4.0" and "dax3.0" to be removed which
> re-parents the leaked "System RAM (kmem)" instances. Those in turn
> reference the freed resource as a parent.
> 
> 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.
> 
> 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@vger.kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> ---
>  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;
>  
>