Re: [PATCH] dax: fix potential overflow on 32bit machine

[Matthew Wilcox <willy@infradead.org>]

On Tue, Dec 05, 2017 at 12:19:28PM -0700, Ross Zwisler wrote:
> On Tue, Dec 05, 2017 at 09:37:13AM -0800, Matthew Wilcox wrote:
> > On Tue, Dec 05, 2017 at 10:07:09AM -0700, Ross Zwisler wrote:
> > > >  /* The 'colour' (ie low bits) within a PMD of a page offset.  */
> > > >  #define PG_PMD_COLOUR	((PMD_SIZE >> PAGE_SHIFT) - 1)
> > > > +#define PG_PMD_NR	(PMD_SIZE >> PAGE_SHIFT)
> > > 
> > > I wonder if it's confusing that PG_PMD_COLOUR is a mask, but PG_PMD_NR is a
> > > count?  Would "PAGES_PER_PMD" be clearer, in the spirit of
> > > PTRS_PER_{PGD,PMD,PTE}? 
> > 
> > Maybe.  I don't think that 'NR' can ever be confused with a mask.
> > I went with PG_PMD_NR because I didn't want to use HPAGE_PMD_NR, but
> > in retrospect I just needed to go to sleep and leave thinking about
> > hard problems like naming things for the morning.  I decided to call it
> > 'colour' rather than 'mask' originally because I got really confused with
> > PMD_MASK masking off the low bits.  If you ask 'What colour is this page
> > within the PMD', you know you're talking about the low bits.
> > 
> > I actually had cause to define PMD_ORDER in a separate unrelated patch
> > I was working on this morning.  How does this set of definitions grab you?
> > 
> > #define PMD_ORDER	(PMD_SHIFT - PAGE_SHIFT)
> > #define PMD_PAGES	(1UL << PMD_ORDER)
> > #define PMD_PAGE_COLOUR	(PMD_PAGES - 1)
> > 
> > and maybe put them in linux/mm.h so everybody can see them?
> 
> Yep, I personally like these better, and putting them in a global header seems
> like the right way to go.

Ugh.  ARM, MIPS and PARISC all define a rather interesting PMD_ORDER.
I'm going to have to rename them first.

> > > Also, can we use the same define both in fs/dax.c and in mm/truncate.c,
> > > instead of the latter using HPAGE_PMD_NR?
> > 
> > I'm OK with the latter using HPAGE_PMD_NR because it's explicitly "is
> > this a huge page?"  But I'd kind of like to get rid of a lot of the HPAGE_*
> > definitions, so 
> 
> I would also like to get rid of them if possible, but quick grep makes me
> think that unfortunately they may not be entirely equivalent to other defines
> we have?
> 
> i.e:
> 
> arch/metag/include/asm/page.h:# define HPAGE_SHIFT      13
> arch/metag/include/asm/page.h:# define HPAGE_SHIFT      14
> arch/metag/include/asm/page.h:# define HPAGE_SHIFT      15
> arch/metag/include/asm/page.h:# define HPAGE_SHIFT      16
> arch/metag/include/asm/page.h:# define HPAGE_SHIFT      17
> arch/metag/include/asm/page.h:# define HPAGE_SHIFT      18
> arch/metag/include/asm/page.h:# define HPAGE_SHIFT      19
> arch/metag/include/asm/page.h:# define HPAGE_SHIFT      20
> arch/metag/include/asm/page.h:# define HPAGE_SHIFT      21
> arch/metag/include/asm/page.h:# define HPAGE_SHIFT      22
> 
> this arch has no PMD_SHIFT definition...
> 
> I'm not really familiar with the HPAGE defines, though, so maybe it's not as
> complex as it seems.

I think it's more complex than it seems.  Some of the HPAGE definitions
(like the ones you've found) are for hugetlbfs hugepages which are a
bit different from the transparent hugepages.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>