| 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
| ||
|
Message-ID: <20130206050527.GA8249@lge.com>
Date: Wed, 6 Feb 2013 14:05:27 +0900
From: Joonsoo Kim <iamjoonsoo.kim@....com>
To: Rob Herring <robherring2@...il.com>
Cc: Nicolas Pitre <nicolas.pitre@...aro.org>,
Rob Herring <rob.herring@...xeda.com>,
Russell King <linux@....linux.org.uk>,
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 Wed, Feb 06, 2013 at 11:07:07AM +0900, Joonsoo Kim wrote:
> Hello, Rob.
>
> On Tue, Feb 05, 2013 at 01:12:51PM -0600, Rob Herring wrote:
> > On 02/05/2013 12:13 PM, Nicolas Pitre wrote:
> > > On Tue, 5 Feb 2013, Rob Herring wrote:
> > >
> > >> 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.
> > > Ah, OK. This wasn't clear looking at the code.
> > >> We probably should have had a WARN here.
> > >
> > > Indeed.
> > >
>
> Okay.
> I should fix it to find any mapping within PCI reserved region.
Ah...
Above comment is my mistake.
If there is a region already mapped within PCI reserved region and
it is not found by find_static_vm_vaddr(), vm_area_add_early() hit BUG_ON().
So, to leave find_static_vm_vaddr() is safe.
> But, I think that it is not an error.
> Now, I see your original commit 'c2794437091a4fda72c4a4f3567dd728dcc0c3c9'
> and find below message.
>
> "Platforms which need early i/o mapping (e.g. for vga console) can call
> pci_map_io_early in their .map_io function."
>
> Therfore, for some platform, it is possible that there is a mapping within
> PCI reserved range.
>
> So, I will not add WARN here.
>
> I will fix and re-send v6 with your ACK.
>
> Thanks for review.
>
> > >>>
> > >>> 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.
> > >
> > > Agreed. However that is checked already in vm_area_add_early().
> > > Therefore the overlap test here is redundant.
> >
> > Ah, right. In that case:
> >
> > Acked-by: Rob Herring <rob.herring@...xeda.com>
> >
> > 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/
> --
> 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/
--
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/
Powered by blists - more mailing lists