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 <[email protected]>
---
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 [email protected]
Lackorzynski http://os.inf.tu-dresden.de/~adam/
On Thu, 25 Dec 2008 22:02:35 +0100 Adam Lackorzynski <[email protected]> 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;
_
On Fri, Dec 26, 2008 at 04:39:46PM -0800, Andrew Morton wrote:
> On Thu, 25 Dec 2008 22:02:35 +0100 Adam Lackorzynski <[email protected]> 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
* Johannes Weiner <[email protected]> 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 <[email protected]> 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
On Fri Dec 26, 2008 at 16:39:46 -0800, Andrew Morton wrote:
> On Thu, 25 Dec 2008 22:02:35 +0100 Adam Lackorzynski <[email protected]> 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 [email protected]
Lackorzynski http://os.inf.tu-dresden.de/~adam/
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