On 8/25/20 10:32 PM, Andrew Morton wrote:
> On Wed, 26 Aug 2020 05:06:59 +0800 kernel test robot <lkp@xxxxxxxxx> wrote:
>
>> drivers/dax/bus.c:626 alloc_dev_dax_range() error: potential null dereference 'alloc'. (__request_region returns null)
>> drivers/dax/bus.c:626 alloc_dev_dax_range() error: we previously assumed 'alloc' could be null (see line 610)
>
> yes, looks fishy:
>
> alloc = __request_region(res, start, size, dev_name(dev), 0);
> if (!alloc && !dev_dax->nr_range) {
> /*
> * If we adjusted an existing @ranges leave it alone,
> * but if this was an empty set of ranges nothing else
> * will release @ranges, so do it now.
> */
> kfree(ranges);
> return -ENOMEM;
> }
>
> ...
>
> .start = alloc->start,
>
> Added by "[PATCH v4 18/23] device-dax: Add dis-contiguous resource
> support",
> https://lkml.kernel.org/r/159643104304.4062302.16561669534797528660.stgit@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>
This should have been:
diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
index 8dd82ea9d53d..06a789aba58a 100644
--- a/drivers/dax/bus.c
+++ b/drivers/dax/bus.c
@@ -607,13 +607,16 @@ static int alloc_dev_dax_range(struct dev_dax *dev_dax, u64 start,
return -ENOMEM;
alloc = __request_region(res, start, size, dev_name(dev), 0);
- if (!alloc && !dev_dax->nr_range) {
+ if (!alloc) {
/*
- * If we adjusted an existing @ranges leave it alone,
- * but if this was an empty set of ranges nothing else
+ * If this was an empty set of ranges nothing else
* will release @ranges, so do it now.
*/
- kfree(ranges);
+ if (!dev_dax->nr_range) {
+ kfree(ranges);
+ ranges = NULL;
+ }
+ dev_dax->ranges = ranges;
return -ENOMEM;
}
To kfree only with nr_range == 0, while also avoiding the leakage of @ranges (from
krealloc success case with nr_ranges > 0) without the null deref that this introduces.
>
>> drivers/dax/device.c:111:21: warning: variable 'dax_region' set but not used [-Wunused-but-set-variable]
>
> yup, "device-dax: make align a per-device property" needs
>
> --- a/drivers/dax/device.c~device-dax-make-align-a-per-device-property-fix
> +++ a/drivers/dax/device.c
> @@ -108,7 +108,6 @@ static vm_fault_t __dev_dax_pmd_fault(st
> {
> unsigned long pmd_addr = vmf->address & PMD_MASK;
> struct device *dev = &dev_dax->dev;
> - struct dax_region *dax_region;
> phys_addr_t phys;
> pgoff_t pgoff;
> unsigned int fault_size = PMD_SIZE;
> @@ -116,7 +115,6 @@ static vm_fault_t __dev_dax_pmd_fault(st
> if (check_vma(dev_dax, vmf->vma, __func__))
> return VM_FAULT_SIGBUS;
>
> - dax_region = dev_dax->region;
> if (dev_dax->align > PMD_SIZE) {
> dev_dbg(dev, "alignment (%#x) > fault size (%#x)\n",
> dev_dax->align, fault_size);
> @@ -151,7 +149,6 @@ static vm_fault_t __dev_dax_pud_fault(st
> {
> unsigned long pud_addr = vmf->address & PUD_MASK;
> struct device *dev = &dev_dax->dev;
> - struct dax_region *dax_region;
> phys_addr_t phys;
> pgoff_t pgoff;
> unsigned int fault_size = PUD_SIZE;
> @@ -160,7 +157,6 @@ static vm_fault_t __dev_dax_pud_fault(st
> if (check_vma(dev_dax, vmf->vma, __func__))
> return VM_FAULT_SIGBUS;
>
> - dax_region = dev_dax->region;
> if (dev_dax->align > PUD_SIZE) {
> dev_dbg(dev, "alignment (%#x) > fault size (%#x)\n",
> dev_dax->align, fault_size);
> _
>
Yes.
And also the MEMORY_HOTPLUG=n and ACPI=n build issues that Randy/you fixed which are
introduced by this series too. Just to confirm: I should add up all these fixes and respin
the series right? Or should I follow-up the first one on top of what you've already staged
in -mm tree?
Joao
