[PATCH] linux: fix grant table bug

[PATCH] linux: fix grant table bug

[Michael Abd-El-Malek]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

A PV OS has two grant table data structures: the grant table itself and a free 
list.  The free list is composed of an array of pages, which grow dynamically as 
the guest OS requires more grants.  While the grant table contains 8-byte 
entries, the free list contains 4-byte entries.  So we have half as many pages 
in the free list than in the grant table.

There was a bug in the free list allocation code. The free list was indexed as 
if it was the same size as the grant table.  But it's only half as large.  So 
memory got corrupted, and I was seeing crashes in the slab allocator later on.

Signed-off-by: Michael Abd-El-Malek <mabdelmalek@cmu.edu>

[-- Attachment #2: a --]
[-- Type: text/plain, Size: 2087 bytes --]

diff -r 1721546b9277 drivers/xen/core/gnttab.c
--- a/drivers/xen/core/gnttab.c	Fri Mar 28 01:39:23 2008 -0400
+++ b/drivers/xen/core/gnttab.c	Sun Mar 30 23:24:42 2008 -0400
@@ -379,11 +379,15 @@ static int grow_gnttab_list(unsigned int
 static int grow_gnttab_list(unsigned int more_frames)
 {
 	unsigned int new_nr_grant_frames, extra_entries, i;
+	unsigned int nr_glist_frames, new_nr_glist_frames;
 
 	new_nr_grant_frames = nr_grant_frames + more_frames;
 	extra_entries       = more_frames * GREFS_PER_GRANT_FRAME;
 
-	for (i = nr_grant_frames; i < new_nr_grant_frames; i++)
+	nr_glist_frames = (nr_grant_frames * GREFS_PER_GRANT_FRAME + RPP - 1) / RPP;
+	new_nr_glist_frames =
+		(new_nr_grant_frames * GREFS_PER_GRANT_FRAME + RPP - 1) / RPP;
+	for (i = nr_glist_frames; i < new_nr_glist_frames; i++)
 	{
 		gnttab_list[i] = (grant_ref_t *)__get_free_page(GFP_ATOMIC);
 		if (!gnttab_list[i])
@@ -406,7 +410,7 @@ static int grow_gnttab_list(unsigned int
 	return 0;
 	
 grow_nomem:
-	for ( ; i >= nr_grant_frames; i--)
+	for ( ; i >= nr_glist_frames; i--)
 		free_page((unsigned long) gnttab_list[i]);
 	return -ENOMEM;
 }
@@ -720,7 +724,7 @@ int __devinit gnttab_init(void)
 int __devinit gnttab_init(void)
 {
 	int i;
-	unsigned int max_nr_glist_frames;
+	unsigned int max_nr_glist_frames, nr_glist_frames;
 	unsigned int nr_init_grefs;
 
 	if (!is_running_on_xen())
@@ -733,15 +737,15 @@ int __devinit gnttab_init(void)
 	 * grant reference free list on the current hypervisor.
 	 */
 	max_nr_glist_frames = (boot_max_nr_grant_frames *
-			       GREFS_PER_GRANT_FRAME /
-			       (PAGE_SIZE / sizeof(grant_ref_t)));
+			       GREFS_PER_GRANT_FRAME / RPP);
 
 	gnttab_list = kmalloc(max_nr_glist_frames * sizeof(grant_ref_t *),
 			      GFP_KERNEL);
 	if (gnttab_list == NULL)
 		return -ENOMEM;
 
-	for (i = 0; i < nr_grant_frames; i++) {
+	nr_glist_frames = (nr_grant_frames * GREFS_PER_GRANT_FRAME + RPP - 1) / RPP;
+	for (i = 0; i < nr_glist_frames; i++) {
 		gnttab_list[i] = (grant_ref_t *)__get_free_page(GFP_KERNEL);
 		if (gnttab_list[i] == NULL)
 			goto ini_nomem;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] linux: fix grant table bug

[Keir Fraser]

On 31/3/08 04:42, "Michael Abd-El-Malek" <mabdelmalek@cmu.edu> wrote:

> A PV OS has two grant table data structures: the grant table itself and a free
> list.  The free list is composed of an array of pages, which grow dynamically
> as 
> the guest OS requires more grants.  While the grant table contains 8-byte
> entries, the free list contains 4-byte entries.  So we have half as many pages
> in the free list than in the grant table.
> 
> There was a bug in the free list allocation code. The free list was indexed as
> if it was the same size as the grant table.  But it's only half as large.  So
> memory got corrupted, and I was seeing crashes in the slab allocator later on.

Nice catch. That code is a bit confusing!

 -- Keir

[PATCH] xen: Fix grant table bug

[Mark McLoughlin]

On Sun, 2008-03-30 at 23:42 -0400, Michael Abd-El-Malek wrote:

> There was a bug in the free list allocation code. The free list was indexed as 
> if it was the same size as the grant table.  But it's only half as large.  So 
> memory got corrupted, and I was seeing crashes in the slab allocator later on.

Looks like an obvious candidate for upstream linux too. Rebased version
below.

Cheers,
Mark.

From: Michael Abd-El-Malek <mabdelmalek@cmu.edu>

A PV OS has two grant table data structures: the grant table itself
and a free list.  The free list is composed of an array of pages,
which grow dynamically as the guest OS requires more grants.  While
the grant table contains 8-byte entries, the free list contains 4-byte
entries.  So we have half as many pages in the free list than in the
grant table.

There was a bug in the free list allocation code. The free list was
indexed as if it was the same size as the grant table.  But it's only
half as large.  So memory got corrupted, and I was seeing crashes in
the slab allocator later on.

Taken from:

  http://xenbits.xensource.com/linux-2.6.18-xen.hg?rev/4018c0da3360

Signed-off-by: Michael Abd-El-Malek <mabdelmalek@cmu.edu>
Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 drivers/xen/grant-table.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index ea94dba..d85dc6d 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -381,11 +381,15 @@ EXPORT_SYMBOL_GPL(gnttab_cancel_free_callback);
 static int grow_gnttab_list(unsigned int more_frames)
 {
 	unsigned int new_nr_grant_frames, extra_entries, i;
+	unsigned int nr_glist_frames, new_nr_glist_frames;
 
 	new_nr_grant_frames = nr_grant_frames + more_frames;
 	extra_entries       = more_frames * GREFS_PER_GRANT_FRAME;
 
-	for (i = nr_grant_frames; i < new_nr_grant_frames; i++) {
+	nr_glist_frames = (nr_grant_frames * GREFS_PER_GRANT_FRAME + RPP - 1) / RPP;
+	new_nr_glist_frames =
+		(new_nr_grant_frames * GREFS_PER_GRANT_FRAME + RPP - 1) / RPP;
+	for (i = nr_glist_frames; i < new_nr_glist_frames; i++) {
 		gnttab_list[i] = (grant_ref_t *)__get_free_page(GFP_ATOMIC);
 		if (!gnttab_list[i])
 			goto grow_nomem;
@@ -407,7 +411,7 @@ static int grow_gnttab_list(unsigned int more_frames)
 	return 0;
 
 grow_nomem:
-	for ( ; i >= nr_grant_frames; i--)
+	for ( ; i >= nr_glist_frames; i--)
 		free_page((unsigned long) gnttab_list[i]);
 	return -ENOMEM;
 }
@@ -530,7 +534,7 @@ static int gnttab_expand(unsigned int req_entries)
 static int __devinit gnttab_init(void)
 {
 	int i;
-	unsigned int max_nr_glist_frames;
+	unsigned int max_nr_glist_frames, nr_glist_frames;
 	unsigned int nr_init_grefs;
 
 	if (!is_running_on_xen())
@@ -543,15 +547,15 @@ static int __devinit gnttab_init(void)
 	 * grant reference free list on the current hypervisor.
 	 */
 	max_nr_glist_frames = (boot_max_nr_grant_frames *
-			       GREFS_PER_GRANT_FRAME /
-			       (PAGE_SIZE / sizeof(grant_ref_t)));
+			       GREFS_PER_GRANT_FRAME / RPP);
 
 	gnttab_list = kmalloc(max_nr_glist_frames * sizeof(grant_ref_t *),
 			      GFP_KERNEL);
 	if (gnttab_list == NULL)
 		return -ENOMEM;
 
-	for (i = 0; i < nr_grant_frames; i++) {
+	nr_glist_frames = (nr_grant_frames * GREFS_PER_GRANT_FRAME + RPP - 1) / RPP;
+	for (i = 0; i < nr_glist_frames; i++) {
 		gnttab_list[i] = (grant_ref_t *)__get_free_page(GFP_KERNEL);
 		if (gnttab_list[i] == NULL)
 			goto ini_nomem;
-- 

[PATCH] xen: Fix grant table bug

[Mark McLoughlin]

On Sun, 2008-03-30 at 23:42 -0400, Michael Abd-El-Malek wrote:

> There was a bug in the free list allocation code. The free list was indexed as 
> if it was the same size as the grant table.  But it's only half as large.  So 
> memory got corrupted, and I was seeing crashes in the slab allocator later on.

Looks like an obvious candidate for upstream linux too. Rebased version
below.

Cheers,
Mark.

From: Michael Abd-El-Malek <mabdelmalek@cmu.edu>

A PV OS has two grant table data structures: the grant table itself
and a free list.  The free list is composed of an array of pages,
which grow dynamically as the guest OS requires more grants.  While
the grant table contains 8-byte entries, the free list contains 4-byte
entries.  So we have half as many pages in the free list than in the
grant table.

There was a bug in the free list allocation code. The free list was
indexed as if it was the same size as the grant table.  But it's only
half as large.  So memory got corrupted, and I was seeing crashes in
the slab allocator later on.

Taken from:

  http://xenbits.xensource.com/linux-2.6.18-xen.hg?rev/4018c0da3360

Signed-off-by: Michael Abd-El-Malek <mabdelmalek@cmu.edu>
Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 drivers/xen/grant-table.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index ea94dba..d85dc6d 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -381,11 +381,15 @@ EXPORT_SYMBOL_GPL(gnttab_cancel_free_callback);
 static int grow_gnttab_list(unsigned int more_frames)
 {
 	unsigned int new_nr_grant_frames, extra_entries, i;
+	unsigned int nr_glist_frames, new_nr_glist_frames;
 
 	new_nr_grant_frames = nr_grant_frames + more_frames;
 	extra_entries       = more_frames * GREFS_PER_GRANT_FRAME;
 
-	for (i = nr_grant_frames; i < new_nr_grant_frames; i++) {
+	nr_glist_frames = (nr_grant_frames * GREFS_PER_GRANT_FRAME + RPP - 1) / RPP;
+	new_nr_glist_frames =
+		(new_nr_grant_frames * GREFS_PER_GRANT_FRAME + RPP - 1) / RPP;
+	for (i = nr_glist_frames; i < new_nr_glist_frames; i++) {
 		gnttab_list[i] = (grant_ref_t *)__get_free_page(GFP_ATOMIC);
 		if (!gnttab_list[i])
 			goto grow_nomem;
@@ -407,7 +411,7 @@ static int grow_gnttab_list(unsigned int more_frames)
 	return 0;
 
 grow_nomem:
-	for ( ; i >= nr_grant_frames; i--)
+	for ( ; i >= nr_glist_frames; i--)
 		free_page((unsigned long) gnttab_list[i]);
 	return -ENOMEM;
 }
@@ -530,7 +534,7 @@ static int gnttab_expand(unsigned int req_entries)
 static int __devinit gnttab_init(void)
 {
 	int i;
-	unsigned int max_nr_glist_frames;
+	unsigned int max_nr_glist_frames, nr_glist_frames;
 	unsigned int nr_init_grefs;
 
 	if (!is_running_on_xen())
@@ -543,15 +547,15 @@ static int __devinit gnttab_init(void)
 	 * grant reference free list on the current hypervisor.
 	 */
 	max_nr_glist_frames = (boot_max_nr_grant_frames *
-			       GREFS_PER_GRANT_FRAME /
-			       (PAGE_SIZE / sizeof(grant_ref_t)));
+			       GREFS_PER_GRANT_FRAME / RPP);
 
 	gnttab_list = kmalloc(max_nr_glist_frames * sizeof(grant_ref_t *),
 			      GFP_KERNEL);
 	if (gnttab_list == NULL)
 		return -ENOMEM;
 
-	for (i = 0; i < nr_grant_frames; i++) {
+	nr_glist_frames = (nr_grant_frames * GREFS_PER_GRANT_FRAME + RPP - 1) / RPP;
+	for (i = 0; i < nr_glist_frames; i++) {
 		gnttab_list[i] = (grant_ref_t *)__get_free_page(GFP_KERNEL);
 		if (gnttab_list[i] == NULL)
 			goto ini_nomem;
-- 

Re: [Xen-devel] [PATCH] xen: Fix grant table bug

[Jeremy Fitzhardinge]

Mark McLoughlin wrote:
> On Sun, 2008-03-30 at 23:42 -0400, Michael Abd-El-Malek wrote:
>
>   
>> There was a bug in the free list allocation code. The free list was indexed as 
>> if it was the same size as the grant table.  But it's only half as large.  So 
>> memory got corrupted, and I was seeing crashes in the slab allocator later on.
>>     
>
> Looks like an obvious candidate for upstream linux too. Rebased version
> below.
>   

Queued, thanks.

    J

Re: [PATCH] xen: Fix grant table bug

[Jeremy Fitzhardinge]

Mark McLoughlin wrote:
> On Sun, 2008-03-30 at 23:42 -0400, Michael Abd-El-Malek wrote:
>
>   
>> There was a bug in the free list allocation code. The free list was indexed as 
>> if it was the same size as the grant table.  But it's only half as large.  So 
>> memory got corrupted, and I was seeing crashes in the slab allocator later on.
>>     
>
> Looks like an obvious candidate for upstream linux too. Rebased version
> below.
>   

Queued, thanks.

    J