Re: [PATCH v2 1/4] arm64: export memblock_reserve()d regions via /proc/iomem

[James Morse]

Hi Dave,

On 19/06/18 14:37, Dave Kleikamp wrote:
> On 06/19/2018 01:44 AM, AKASHI Takahiro wrote:
>> +static int __init reserve_memblock_reserved_regions(void)
>> +{
>> +	phys_addr_t start, end, roundup_end = 0;
>> +	struct resource *mem, *res;
>> +	u64 i;
>> +
>> +	for_each_reserved_mem_region(i, &start, &end) {
>> +		if (end <= roundup_end)
>> +			continue; /* done already */
>> +
>> +		start = __pfn_to_phys(PFN_DOWN(start));
>> +		end = __pfn_to_phys(PFN_UP(end)) - 1;
>> +		roundup_end = end;
>> +
>> +		res = kzalloc(sizeof(*res), GFP_ATOMIC);
>> +		if (WARN_ON(!res))
>> +			return -ENOMEM;
>> +		res->start = start;
>> +		res->end = end;
>> +		res->name  = "reserved";
>> +		res->flags = IORESOURCE_MEM;
>> +
>> +		mem = request_resource_conflict(&iomem_resource, res);
>> +		/*
>> +		 * We expected memblock_reserve() regions to conflict with
>> +		 * memory created by request_standard_resources().
>> +		 */
>> +		if (WARN_ON_ONCE(!mem))
>> +			continue;
>> +		kfree(res);
> 
> Why is kfree() after the conditional continue? This is a memory leak.

request_resource_conflict() inserts res into the iomem_resource tree, or returns
the conflict if there is already something at this address.

We expect something at this address: a 'System RAM' section added by
request_resource(). This code wants the conflicting 'System RAM' entry so that
reserve_region_with_split() can fill in the gaps below it with 'reserved'. See
the commit-message for an example.

If there was no conflict, it means the memory map doesn't look like we expect,
we can't pass NULL to reserve_region_with_split(), and we just inserted the
'reserved' at the top level. Freeing res at this point would be a use-after-free
hanging from /proc/iomem.
This code generates a WARN_ON_ONCE() and leaves the 'reserved' description where
it is.

Trying to cleanup here is pointless, we can remove the resource entry and free
it ... but we still want to report it as reserved, which is what just happened,
just not quite how we we wanted it.

If you ever see this warning, it means some RAM stopped being nomap between
request_resources() and reserve_memblock_reserved_regions(). I can't find any
case where that ever happens.


If all that makes sense: how can I improve the comment above the WARN_ON_ONCE()
to make it clearer?


Thanks,

James


>> +
>> +		reserve_region_with_split(mem, start, end, "reserved");
>> +	}
>> +
>> +	return 0;
>> +}
>> +arch_initcall(reserve_memblock_reserved_regions);
>> +
>>  u64 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HWID };
>>  
>>  void __init setup_arch(char **cmdline_p)
>>