linux-kernel - Re: gen_pool_add broken with LPAE based systems

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] [day] [month] [year] [list]

Message-Id: <20130319160014.caabf567b11b5aef65c0ac1d@linux-foundation.org>
Date:	Tue, 19 Mar 2013 16:00:14 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Laura Abbott <lauraa@...eaurora.org>
Cc:	Benjamin Gaignard <benjamin.gaignard@...ricsson.com>,
	Jean-Christophe PLAGNIOL-VILLARD <plagnioj@...osoft.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: gen_pool_add broken with LPAE based systems

On Tue, 19 Mar 2013 15:49:24 -0700 Laura Abbott <lauraa@...eaurora.org> wrote:

> On 3/19/2013 2:54 PM, Andrew Morton wrote:
> > On Thu, 14 Mar 2013 16:05:27 -0700 Laura Abbott <lauraa@...eaurora.org> wrote:
> >
> >> Hi,
> >>
> >> We use genalloc for managing certain pools of physical memory. genalloc
> >> currently uses unsigned long for virtual addresses and phys_addr_t for
> >> physical addresses. Our ARM LPAE systems have 64-bit physical addresses
> >> but unsigned long is still 32 bits.  Using gen_pool_add breaks with
> >> addresses > 4G because gen_pool_add treats the address passed in as the
> >> virtual address. gen_pool allocates internally based on the 32 bit
> >> virtual address as well so everything is broken if we want to be able to
> >> manage the full address space after 4G. I see a couple of options:
> >
> > The above only makes sense if ARM LPAE has 64-bit (actually >= 33-bit)
> > virtual addresses.  If so, I don't understand how ARM LPAE can work at
> > all - the core MM assumes that addresses-fit-in-ulongs in eleventy
> > trillion places.
> >
> > I think we need a better description of the problem, please.
> >
> 
> Sorry, let me clarify. ARM LPAE still has 32 bit virtual addresses.
> 
> Change 3c8f370ded3483b27f1218ff0051fcf0c7a2facd (lib/genalloc.c: add 
> support for specifying the physical address) added support for using 
> genalloc to know about both physical addresses and virtual addresses. 
> Allocation in gen_pool is still based on the virtual address though.
> 
> The problem is we've been using genalloc to allocate physical addresses, 
> not virtual ones so allocating and returning an unsigned long breaks 
> with sizeof(phys_addr_t) > sizeof(unsigned long). It looks like genalloc 
> was added and extended with virtual addresses in mind but apart from the 
> address size limitation right now it should be able to work just fine 
> for physical addresses.
> 
> There seem to be a few other clients scattered about who are using 
> genalloc for physical addresses as well (although all are 32 bit systems 
> right now)

I see.  So genpool has never worked properly for this application?

> A better subject would be 'genalloc broken on LPAE systems when used to 
> allocate physical addresses instead of virtual addresses'

I'd say "extend genpool so we can use physical addresses instead of
virtual addresses" ;)

> >> 1) Change gen_pool_add to use physical addresses and allocate based on
> >> physical addresses instead of virtual addresses
> >> 2) Change the virtual address to be a 64 bit type or something
> >> selectable to a 64 bit type.
> >> 3) Allow a flag per pool to select whether the allocator is virtual or
> >> physical and switch between those.
> >> 4) Split the APIs into virtual <-> physical and physical only and have
> >> separate types for each.
> >>
> >> Any of these suggestions seem reasonable or is there another option to
> >> consider?
> >
> > 2) sounds least intrusive but I can't think with my head spinning so fast.

I suppose using a bare u64 for `addr' would fix things up.

I think it's rather regrettable that genpool.c contains terms like
"addr", "phys" and "virt" at all.  It's in lib/ and it's a
general-purpose container thing.  It should know whether it's operating
on addresses or bananas or whatever.  A better layering would be to
weed all that out of there, implement a truly general-purpose container
and then add convenience wrappers around that for each particular
application.

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