[PATCH] 2.6.28, vmalloc.c, vmap_page_range

[PATCH] 2.6.28, vmalloc.c, vmap_page_range

[Adam Lackorzynski]

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.

Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
---
 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;




Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

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

[Andrew Morton]

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;
 	int err = 0;
 	int nr = 0;
 
@@ -167,7 +168,7 @@ static int vmap_page_range(unsigned long
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
-	flush_cache_vmap(addr, end);
+	flush_cache_vmap(start_addr, end);
 
 	if (unlikely(err))
 		return err;
_

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

[Johannes Weiner]

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.

	Hannes

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

[Ingo Molnar]

* 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

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

[Adam Lackorzynski]

On Fri Dec 26, 2008 at 16:39:46 -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?

Just in my own code, i.e. it doesn't have any effects to the kernel
itself.
 
> 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?

fine.

> --- 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;
>  	int err = 0;
>  	int nr = 0;
>  
> @@ -167,7 +168,7 @@ static int vmap_page_range(unsigned long
>  		if (err)
>  			break;
>  	} while (pgd++, addr = next, addr != end);
> -	flush_cache_vmap(addr, end);
> +	flush_cache_vmap(start_addr, end);
>  
>  	if (unlikely(err))
>  		return err;

Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

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

[Johannes Weiner]

On Sat, Dec 27, 2008 at 10:02:21AM +0100, Ingo Molnar wrote:
> > > --- 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.

It would surely not justify such a big change.

And at least in mm/* you have them paired with `end_addr' if they
denote range start and end.

	Hannes