linux-kernel - Re: [PATCH -mmotm] [BUGFIX] pagemap: fix pfn calculation for hugepage

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: <20100325055501.GA3744@spritzerA.linux.bs1.fc.nec.co.jp>
Date:	Thu, 25 Mar 2010 14:55:01 +0900
From:	Naoya Horiguchi <n-horiguchi@...jp.nec.com>
To:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Cc:	LKML <linux-kernel@...r.kernel.org>, linux-mm <linux-mm@...ck.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andi Kleen <andi@...stfloor.org>,
	Wu Fengguang <fengguang.wu@...el.com>,
	Matt Mackall <mpm@...enic.com>,
	Daisuke Nishimura <nishimura@....nes.nec.co.jp>
Subject: Re: [PATCH -mmotm] [BUGFIX] pagemap: fix pfn calculation for
	hugepage

On Wed, Mar 24, 2010 at 02:57:25PM +0900, KAMEZAWA Hiroyuki wrote:
> On Wed, 24 Mar 2010 14:42:27 +0900
> Naoya Horiguchi <n-horiguchi@...jp.nec.com> wrote:
> 
> > When we look into pagemap using page-types with option -p, the value
> > of pfn for hugepages looks wrong (see below.)
> > This is because pte was evaluated only once for one vma
> > although it should be updated for each hugepage. This patch fixes it.
> > 
> >   $ page-types -p 3277 -Nl -b huge
> >   voffset   offset  len     flags
> >   7f21e8a00 11e400  1       ___U___________H_G________________
> >   7f21e8a01 11e401  1ff     ________________TG________________
> >                ^^^
> >   7f21e8c00 11e400  1       ___U___________H_G________________
> >   7f21e8c01 11e401  1ff     ________________TG________________
> >                ^^^
> > 
> > One hugepage contains 1 head page and 511 tail pages in x86_64 and
> > each two lines represent each hugepage. Voffset and offset mean
> > virtual address and physical address in the page unit, respectively.
> > The different hugepages should not have the same offset value.
> > 
> > With this patch applied:
> > 
> >   $ page-types -p 3386 -Nl -b huge
> >   voffset   offset   len    flags
> >   7fec7a600 112c00   1      ___UD__________H_G________________
> >   7fec7a601 112c01   1ff    ________________TG________________
> >                ^^^
> >   7fec7a800 113200   1      ___UD__________H_G________________
> >   7fec7a801 113201   1ff    ________________TG________________
> >                ^^^
> >                OK
> > 
> > Changelog:
> >  - add hugetlb entry walker in mm/pagewalk.c
> >    (the idea based on Kamezawa-san's patch)
> > 
> Seems good.
> 
> More info.

Thanks, I'll add this to patch description.

>  - This patch modifies walk_page_range()'s hugepage walker.
>    But the change only affects pagemap_read(), it's only caller of hugepage callback.
> 
>  - Before patch, hugetlb_entry() callback is called once per pgd. Then,
>    hugtlb_entry() has to walk pgd's contents by itself. 
>    This caused BUG.

I think there is misunderstanding on this part.
I would add this instead:
 - Without this patch, hugetlb_entry() callback is called once per vma,
   that doesn't match the natural expectation from its name.

> 
>  - After patch, hugetlb_entry() callback is called once per hugepte entry.
>    Then, callback will be much simpler.
> 
> 

...
> > +static int walk_hugetlb_range(struct vm_area_struct *vma,
> > +			      unsigned long addr, unsigned long end,
> > +			      struct mm_walk *walk)
> > +{
> > +	struct hstate *h = hstate_vma(vma);
> > +	unsigned long next;
> > +	unsigned long hmask = huge_page_mask(h);
> > +	pte_t *pte;
> > +	int err = 0;
> > +
> > +	do {
> > +		next = hugetlb_entry_end(h, addr, end);
> > +		pte = huge_pte_offset(walk->mm, addr & hmask);
> > +		if (pte && walk->hugetlb_entry)
> > +			err = walk->hugetlb_entry(pte, hmask, addr, next, walk);
> > +		if (err)
> > +			return err;
> > +	} while (addr = next, addr != end);
> > +
> > +	return err;
> > +}
> nitpick.
> 
> seems nicer than mine but "return 0" is ok if you add "return err" in the loop.
>
OK. I fixed it.

Thanks,
Naoya Horiguchi
--
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/