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

[Ingo Molnar <mingo@elte.hu>]

* Johannes Weiner <hannes@cmpxchg.org> wrote:

> 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@os.inf.tu-dresden.de> 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.

i'd like to observe that there's 449 start_addr instances in the kernel 
source, 17 of them in mm/*.c alone. So if it's 'ugly' (it isnt to me), 
this patch is not the place to start worrying about it. If you feel 
strongly about it then prepare a cleanup patch that eradicates them all, 
put your justification for why it's bad into the changelog and post it to 
lkml.

Anyway, happy bikeshed painting,

	Ingo