Re: [PATCH] 2.6.28, vmalloc.c, vmap_page_range

From: Johannes Weiner
Date: Fri Dec 26 2008 - 22:40:21 EST


On Fri, Dec 26, 2008 at 04:39:46PM -0800, Andrew Morton wrote:
> On Thu, 25 Dec 2008 22:02:35 +0100 Adam Lackorzynski <adam@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> > Hi,
> >
> > in 2.6.28, the flush_cache_vmap in vmap_page_range() is called with the end of
> > the range twice. The following patch fixes this for me.
> >
>
> Did this bug have any observeable runtime effects? If so, what were
> they?
>
> > ---
> > vmalloc.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > --- linux-2.6.28/mm/vmalloc.c 2008-12-25 00:26:37.000000000 +0100
> > +++ linux-2.6.28.a/mm/vmalloc.c 2008-12-25 21:45:43.118725744 +0100
> > @@ -155,7 +155,7 @@
> > pgprot_t prot, struct page **pages)
> > {
> > pgd_t *pgd;
> > - unsigned long next;
> > + unsigned long next, start = addr;
> > int err = 0;
> > int nr = 0;
> >
> > @@ -167,7 +167,7 @@
> > if (err)
> > break;
> > } while (pgd++, addr = next, addr != end);
> > - flush_cache_vmap(addr, end);
> > + flush_cache_vmap(start, end);
> >
> > if (unlikely(err))
> > return err;
>
> Well yeah. This is what happens when functions modify their incoming
> arguments. It's a bad programming practice which leads directly to
> exactly this sort of bug.
>
> How about we fix that?
>
>
> --- a/mm/vmalloc.c~vmallocc-fix-flushing-in-vmap_page_range
> +++ a/mm/vmalloc.c
> @@ -151,11 +151,12 @@ static int vmap_pud_range(pgd_t *pgd, un
> *
> * Ie. pte at addr+N*PAGE_SIZE shall point to pfn corresponding to pages[N]
> */
> -static int vmap_page_range(unsigned long addr, unsigned long end,
> +static int vmap_page_range(unsigned long start_addr, unsigned long end,
> pgprot_t prot, struct page **pages)
> {
> pgd_t *pgd;
> unsigned long next;
> + unsigned long addr = start_addr;

Ugh, start_addr is an awful name. How about start? I know it doesn't
hold the same amount of information but it's a local API, the
pgd_offset_k() should make the unit unambiguous, it goes better with
the end parameter and it's unique enough for this short function.

Hannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/