linux-kernel - Re: [PATCH v2 1/2] dma/iommu: Add pgsize_bitmap confirmation in __iommu_dma_alloc

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:	Fri, 8 Apr 2016 14:07:33 +0100
From:	Will Deacon <will.deacon@....com>
To:	Doug Anderson <dianders@...omium.org>
Cc:	Yong Wu <yong.wu@...iatek.com>, Joerg Roedel <joro@...tes.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Robin Murphy <robin.murphy@....com>,
	Daniel Kurtz <djkurtz@...gle.com>,
	Tomasz Figa <tfiga@...gle.com>, Arnd Bergmann <arnd@...db.de>,
	Lucas Stach <l.stach@...gutronix.de>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	"moderated list:ARM/Mediatek SoC support" 
	<linux-mediatek@...ts.infradead.org>, srv_heupstream@...iatek.com,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	iommu@...ts.linux-foundation.org
Subject: Re: [PATCH v2 1/2] dma/iommu: Add pgsize_bitmap confirmation in
 __iommu_dma_alloc_pages

On Tue, Apr 05, 2016 at 10:03:32AM -0700, Doug Anderson wrote:
> On Tue, Mar 29, 2016 at 10:02 AM, Will Deacon <will.deacon@....com> wrote:
> > On Mon, Mar 28, 2016 at 02:32:11PM +0800, Yong Wu wrote:
> >> @@ -213,13 +215,16 @@ static struct page **__iommu_dma_alloc_pages(unsigned int count, gfp_t gfp)
> >>               /*
> >>                * Higher-order allocations are a convenience rather
> >>                * than a necessity, hence using __GFP_NORETRY until
> >> -              * falling back to single-page allocations.
> >> +              * falling back to min size allocations.
> >>                */
> >> -             for (order = min_t(unsigned int, order, __fls(count));
> >> -                  order > 0; order--) {
> >> -                     page = alloc_pages(gfp | __GFP_NORETRY, order);
> >> +             for (order = min_t(int, order, __fls(count));
> >> +                  order >= min_order; order--) {
> >> +                     page = alloc_pages((order == min_order) ? gfp :
> >> +                                        gfp | __GFP_NORETRY, order);
> >>                       if (!page)
> >>                               continue;
> >> +                     if (!order)
> >> +                             break;
> >
> > Isn't this handled by the loop condition?
> 
> He changed the loop condition to be ">= min_order" instead of "> 0",
> so now we can get here with an order == 0.  This makes sense because
> when min_order is not 0 you still want to run the code to split the
> pages and it is sane not to duplicate that below.
> 
> Maybe I'm misunderstanding, though.  Perhaps you can explain how you
> think this code should look?

My reading of the code was that we require order >= min_order to enter
the loop. Given that order doesn't change between the loop header and the
if (!order) check, then that must mean we can enter the loop body with
order == 0 and order >= min_order, which means that min_order is allowed
to be negative. That feels weird.

Am I barking up the wrong tree?

Will