linux-kernel - Re: [PATCH v5 3/3] ARM: mm: use static_vm for managing static mapped areas

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 05 Feb 2013 11:28:20 -0600
From:	Rob Herring <robherring2@...il.com>
To:	Nicolas Pitre <nicolas.pitre@...aro.org>
CC:	Joonsoo Kim <iamjoonsoo.kim@....com>,
	Rob Herring <rob.herring@...xeda.com>,
	Russell King <linux@....linux.org.uk>, js1304@...il.com,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v5 3/3] ARM: mm: use static_vm for managing static mapped
 areas

On 02/04/2013 10:44 PM, Nicolas Pitre wrote:
> On Tue, 5 Feb 2013, Joonsoo Kim wrote:
> 
>> A static mapped area is ARM-specific, so it is better not to use
>> generic vmalloc data structure, that is, vmlist and vmlist_lock
>> for managing static mapped area. And it causes some needless overhead and
>> reducing this overhead is better idea.
>>
>> Now, we have newly introduced static_vm infrastructure.
>> With it, we don't need to iterate all mapped areas. Instead, we just
>> iterate static mapped areas. It helps to reduce an overhead of finding
>> matched area. And architecture dependency on vmalloc layer is removed,
>> so it will help to maintainability for vmalloc layer.
>>
>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@....com>

[snip]

>> @@ -859,17 +864,12 @@ static void __init pci_reserve_io(void)
>>  {
>>  	struct vm_struct *vm;
>>  	unsigned long addr;
>> +	struct static_vm *svm;
>>  
>> -	/* we're still single threaded hence no lock needed here */
>> -	for (vm = vmlist; vm; vm = vm->next) {
>> -		if (!(vm->flags & VM_ARM_STATIC_MAPPING))
>> -			continue;
>> -		addr = (unsigned long)vm->addr;
>> -		addr &= ~(SZ_2M - 1);
>> -		if (addr == PCI_IO_VIRT_BASE)
>> -			return;
>> +	svm = find_static_vm_vaddr((void *)PCI_IO_VIRT_BASE);
>> +	if (svm)
>> +		return;
>>
>> -	}
>>  
>>  	vm_reserve_area_early(PCI_IO_VIRT_BASE, SZ_2M, pci_reserve_io);
>>  }
> 
> The replacement code is not equivalent.  I can't recall why the original 
> is as it is, but it doesn't look right to me.  The 2MB round down 
> certainly looks suspicious.

The PCI mapping is at a fixed, aligned 2MB mapping. If we find any
virtual address within that region already mapped, it is an error. We
probably should have had a WARN here.

> 
> The replacement code should be better.  However I'd like you to get an 
> ACK from Rob Herring as well for this patch.

It doesn't appear to me the above case is handled. The virt addr is
checked whether it is within an existing mapping, but not whether the
new mapping would overlap an existing mapping. It would be good to check
for this generically rather than specifically for the PCI i/o mapping.

Rob

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/